| Message ID | 20211015125152.25198-9-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: drop hypercall function tables | expand |
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
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
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(-)