When running as pv-shim the hypercall is modified today in order to
replace the functions for __HYPERVISOR_event_channel_op and
__HYPERVISOR_grant_table_op hypercalls.

Change this to call the related functions from the normal handlers
instead when running as shim. The performance implications are not
really relevant, as a normal production hypervisor will not be
configured to support shim mode, so the related calls will be dropped
due to optimization of the compiler.

Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy
wrapper do_grant_table_op() needed, as in this case grant_table.c
isn't being built.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/pv/shim.c        | 48 +++++++++++++++--------------------
 xen/common/event_channel.c    |  9 +++++++
 xen/common/grant_table.c      |  9 +++++++
 xen/include/asm-x86/pv/shim.h |  3 +++
 4 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index d9704121a7..8f66aa3957 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -56,11 +56,6 @@ static DEFINE_SPINLOCK(balloon_lock);
 
 static struct platform_bad_page __initdata reserved_pages[2];
 
-static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg);
-static long pv_shim_grant_table_op(unsigned int cmd,
-                                   XEN_GUEST_HANDLE_PARAM(void) uop,
-                                   unsigned int count);
-
 /*
  * By default give the shim 1MB of free memory slack. Some users may wish to
  * tune this constants for better memory utilization. This can be achieved
@@ -203,7 +198,6 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
                               start_info_t *si)
 {
     bool compat = is_pv_32bit_domain(d);
-    pv_hypercall_table_t *rw_pv_hypercall_table;
     uint64_t param = 0;
     long rc;
 
@@ -249,23 +243,6 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
         consoled_set_ring_addr(page);
     }
 
-    /*
-     * Locate pv_hypercall_table[] (usually .rodata) in the directmap (which
-     * is writeable) and insert some shim-specific hypercall handlers.
-     */
-    rw_pv_hypercall_table = __va(__pa(pv_hypercall_table));
-    rw_pv_hypercall_table[__HYPERVISOR_event_channel_op].native =
-        (hypercall_fn_t *)pv_shim_event_channel_op;
-    rw_pv_hypercall_table[__HYPERVISOR_grant_table_op].native =
-        (hypercall_fn_t *)pv_shim_grant_table_op;
-
-#ifdef CONFIG_PV32
-    rw_pv_hypercall_table[__HYPERVISOR_event_channel_op].compat =
-        (hypercall_fn_t *)pv_shim_event_channel_op;
-    rw_pv_hypercall_table[__HYPERVISOR_grant_table_op].compat =
-        (hypercall_fn_t *)pv_shim_grant_table_op;
-#endif
-
     guest = d;
 
     /*
@@ -435,7 +412,7 @@ int pv_shim_shutdown(uint8_t reason)
     return 0;
 }
 
-static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     struct domain *d = current->domain;
     struct evtchn_close close;
@@ -683,9 +660,9 @@ void pv_shim_inject_evtchn(unsigned int port)
 # define compat_handle_okay guest_handle_okay
 #endif
 
-static long pv_shim_grant_table_op(unsigned int cmd,
-                                   XEN_GUEST_HANDLE_PARAM(void) uop,
-                                   unsigned int count)
+long pv_shim_grant_table_op(unsigned int cmd,
+                            XEN_GUEST_HANDLE_PARAM(void) uop,
+                            unsigned int count)
 {
     struct domain *d = current->domain;
     long rc = 0;
@@ -845,6 +822,23 @@ static long pv_shim_grant_table_op(unsigned int cmd,
     return rc;
 }
 
+#ifndef CONFIG_GRANT_TABLE
+/* Thin wrapper(s) needed. */
+long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
+                       unsigned int count)
+{
+    return pv_shim_grant_table_op(cmd, uop, count);
+}
+
+#ifdef CONFIG_PV32
+int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
+                          unsigned int count)
+{
+    return pv_shim_grant_table_op(cmd, uop, count);
+}
+#endif
+#endif
+
 long pv_shim_cpu_up(void *data)
 {
     struct vcpu *v = data;
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 12006f592e..a15f986b32 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -32,6 +32,10 @@
 #include <public/event_channel.h>
 #include <xsm/xsm.h>
 
+#ifdef CONFIG_PV_SHIM
+#include <asm/guest.h>
+#endif
+
 #define ERROR_EXIT(_errno)                                          \
     do {                                                            \
         gdprintk(XENLOG_WARNING,                                    \
@@ -1190,6 +1194,11 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     int rc;
 
+#ifdef CONFIG_PV_SHIM
+    if ( pv_shim )
+        return pv_shim_event_channel_op(cmd, arg);
+#endif
+
     switch ( cmd )
     {
     case EVTCHNOP_alloc_unbound: {
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 49e12621ac..de097c0eda 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -45,6 +45,10 @@
 #include <asm/flushtlb.h>
 #include <asm/guest_atomics.h>
 
+#ifdef CONFIG_PV_SHIM
+#include <asm/guest.h>
+#endif
+
 /* Per-domain grant information. */
 struct grant_table {
     /*
@@ -3526,6 +3530,11 @@ do_grant_table_op(
     long rc;
     unsigned int opaque_in = cmd & GNTTABOP_ARG_MASK, opaque_out = 0;
 
+#ifdef CONFIG_PV_SHIM
+    if ( pv_shim )
+        return pv_shim_grant_table_op(cmd, uop, count);
+#endif
+
     if ( (int)count < 0 )
         return -EINVAL;
 
diff --git a/xen/include/asm-x86/pv/shim.h b/xen/include/asm-x86/pv/shim.h
index 8a91f4f9df..6415f8068e 100644
--- a/xen/include/asm-x86/pv/shim.h
+++ b/xen/include/asm-x86/pv/shim.h
@@ -19,6 +19,7 @@
 #ifndef __X86_PV_SHIM_H__
 #define __X86_PV_SHIM_H__
 
+#include <xen/hypercall.h>
 #include <xen/types.h>
 
 #if defined(CONFIG_PV_SHIM_EXCLUSIVE)
@@ -45,6 +46,8 @@ domid_t get_initial_domain_id(void);
 uint64_t pv_shim_mem(uint64_t avail);
 void pv_shim_fixup_e820(struct e820map *e820);
 const struct platform_bad_page *pv_shim_reserved_pages(unsigned int *size);
+typeof(do_event_channel_op) pv_shim_event_channel_op;
+typeof(do_grant_table_op) pv_shim_grant_table_op;
 
 #else
 
-- 
2.26.2

On 15.10.2021 14:51, Juergen Gross wrote:
> When running as pv-shim the hypercall is modified today in order to
> replace the functions for __HYPERVISOR_event_channel_op and
> __HYPERVISOR_grant_table_op hypercalls.
> 
> Change this to call the related functions from the normal handlers
> instead when running as shim. The performance implications are not
> really relevant, as a normal production hypervisor will not be
> configured to support shim mode, so the related calls will be dropped
> due to optimization of the compiler.
> 
> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy
> wrapper do_grant_table_op() needed, as in this case grant_table.c
> isn't being built.

While you say CONFIG_PV_SHIM_EXCLUSIVE here, ...

> @@ -845,6 +822,23 @@ static long pv_shim_grant_table_op(unsigned int cmd,
>      return rc;
>  }
>  
> +#ifndef CONFIG_GRANT_TABLE

... you don't actually enforce this here. I also don't see why it would
be needed in the "exclusive" case only. A binary usable both ways would
still need these, wouldn't it?

> +/* Thin wrapper(s) needed. */
> +long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
> +                       unsigned int count)
> +{
> +    return pv_shim_grant_table_op(cmd, uop, count);
> +}
> +
> +#ifdef CONFIG_PV32
> +int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
> +                          unsigned int count)
> +{
> +    return pv_shim_grant_table_op(cmd, uop, count);
> +}
> +#endif
> +#endif

Don't you also need to adjust the respective #ifdef in
pv_hypercall_table[]? Otherwise I don't see how, at this point of the
series, the functions would actually get hooked up. In a bi-modal
binary further guarding will then be needed inside the wrappers, I
think. (While the table is going to go away, that guarding is going
to be needed even at the end of this series, I believe.)

Talking of wrappers - do you need actual code to be emitted for this?
Can't you simply put in place an alias each, which is permitted now
that pv_shim_grant_table_op() isn't static anymore? (Albeit that's
partly moot if guarding code gets added to the functions - in that
case only compat_grant_table_op() could become an alias of
do_grant_table_op().)

Jan

On 15.10.21 15:57, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
>> When running as pv-shim the hypercall is modified today in order to
>> replace the functions for __HYPERVISOR_event_channel_op and
>> __HYPERVISOR_grant_table_op hypercalls.
>>
>> Change this to call the related functions from the normal handlers
>> instead when running as shim. The performance implications are not
>> really relevant, as a normal production hypervisor will not be
>> configured to support shim mode, so the related calls will be dropped
>> due to optimization of the compiler.
>>
>> Note that for the CONFIG_PV_SHIM_EXCLUSIVE case there is a dummy
>> wrapper do_grant_table_op() needed, as in this case grant_table.c
>> isn't being built.
> 
> While you say CONFIG_PV_SHIM_EXCLUSIVE here, ...
> 
>> @@ -845,6 +822,23 @@ static long pv_shim_grant_table_op(unsigned int cmd,
>>       return rc;
>>   }
>>   
>> +#ifndef CONFIG_GRANT_TABLE
> 
> ... you don't actually enforce this here. I also don't see why it would
> be needed in the "exclusive" case only. A binary usable both ways would
> still need these, wouldn't it?

The "exclusive" case is normally the one where CONFIG_GRANT_TABLE is not
set. I highlighted this as it is a common situation.

> 
>> +/* Thin wrapper(s) needed. */
>> +long do_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
>> +                       unsigned int count)
>> +{
>> +    return pv_shim_grant_table_op(cmd, uop, count);
>> +}
>> +
>> +#ifdef CONFIG_PV32
>> +int compat_grant_table_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop,
>> +                          unsigned int count)
>> +{
>> +    return pv_shim_grant_table_op(cmd, uop, count);
>> +}
>> +#endif
>> +#endif
> 
> Don't you also need to adjust the respective #ifdef in
> pv_hypercall_table[]? Otherwise I don't see how, at this point of the
> series, the functions would actually get hooked up.

Ah, right.

> In a bi-modal
> binary further guarding will then be needed inside the wrappers, I
> think. (While the table is going to go away, that guarding is going
> to be needed even at the end of this series, I believe.)

Oh, you mean for the weird case of !pv_shim? Yes, I need to return
-ENOSYS in this case.

> Talking of wrappers - do you need actual code to be emitted for this?
> Can't you simply put in place an alias each, which is permitted now
> that pv_shim_grant_table_op() isn't static anymore? (Albeit that's
> partly moot if guarding code gets added to the functions - in that
> case only compat_grant_table_op() could become an alias of
> do_grant_table_op().)

I didn't think of an alias. But I don't think I can make
compat_grant_table_op() an alias of do_grant_table_op(), as they have
different return types.


Juergen