* [PATCH 0/5] XSM: cleanups
@ 2021-11-05 13:55 Andrew Cooper
2021-11-05 13:55 ` [PATCH 1/5] x86/altcall: allow compound types to be passed Andrew Cooper
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Andrew Cooper @ 2021-11-05 13:55 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Daniel De Graaf, Daniel Smith, Jan Beulich
Various XSM cleanups from observations based on Daniel's previous cleanup
series.
Andrew Cooper (4):
xen/xsm: Complete altcall conversion of xsm interface
xen/xsm: Drop xsm_hvm_control() hook
xen/xsm: Improve fallback handling in xsm_fixup_ops()
xen/xsm: Address hypercall ABI problems
Jan Beulich (1):
x86/altcall: allow compound types to be passed
xen/include/Makefile | 2 +-
xen/include/asm-x86/alternative.h | 4 +-
xen/include/xsm/dummy.h | 19 ---
xen/include/xsm/xsm.h | 39 ++----
xen/xsm/Makefile | 1 +
xen/xsm/dummy.c | 254 ++++++++++++++++++++------------------
xen/xsm/flask/flask_op.c | 60 ++++-----
xen/xsm/flask/hooks.c | 9 --
xen/xsm/private.h | 16 +++
xen/xsm/xsm_core.c | 12 --
xen/xsm/xsm_op.c | 51 ++++++++
11 files changed, 245 insertions(+), 222 deletions(-)
create mode 100644 xen/xsm/private.h
create mode 100644 xen/xsm/xsm_op.c
--
2.11.0
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/5] x86/altcall: allow compound types to be passed 2021-11-05 13:55 [PATCH 0/5] XSM: cleanups Andrew Cooper @ 2021-11-05 13:55 ` Andrew Cooper 2021-11-05 14:09 ` Daniel P. Smith 2021-11-05 13:55 ` [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface Andrew Cooper ` (3 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Andrew Cooper @ 2021-11-05 13:55 UTC (permalink / raw) To: Xen-devel Cc: Jan Beulich, Andrew Cooper, Daniel De Graaf, Daniel Smith, Jan Beulich From: Jan Beulich <jbeulich@suse.com> Replace the conditional operator in ALT_CALL_ARG(), which was intended to limit usable types to scalar ones, by a size check. Some restriction here is necessary to make sure we don't violate the ABI's calling conventions, but limiting to scalar types was both too restrictive (disallowing e.g. guest handles) and too permissive (allowing e.g. __int128_t). Note that there was some anomaly with that conditional operator anyway: Something - I don't recall what - made it impossible to omit the middle operand. Code-generation-wise this has the effect of removing certain zero- or sign-extending in some altcall invocations. This ought to be fine as the ABI doesn't require sub-sizeof(int) values to be extended, except when passed through an ellipsis. No functions subject to altcall patching has a variable number of arguments, though. Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Unfortunately this triggers [-Werror=sizeof-array-argument] on some versions of GCC, so alter xsm_{alloc,free}_security_evtchns() to use a pointer rather than array parameter. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> CC: Daniel Smith <dpsmith@apertussolutions.com> CC: Jan Beulich <JBeulich@suse.com> --- xen/include/asm-x86/alternative.h | 4 ++-- xen/include/xsm/xsm.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h index 8e78cc91c35b..8bc59b02afd7 100644 --- a/xen/include/asm-x86/alternative.h +++ b/xen/include/asm-x86/alternative.h @@ -168,8 +168,8 @@ extern void alternative_branches(void); #define ALT_CALL_arg6 "r9" #define ALT_CALL_ARG(arg, n) \ - register typeof((arg) ? (arg) : 0) a ## n ## _ \ - asm ( ALT_CALL_arg ## n ) = (arg) + register typeof(arg) a ## n ## _ asm ( ALT_CALL_arg ## n ) = \ + ({ BUILD_BUG_ON(sizeof(arg) > sizeof(void *)); (arg); }) #define ALT_CALL_NO_ARG(n) \ register unsigned long a ## n ## _ asm ( ALT_CALL_arg ## n ) diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index c101e653f6d7..0b360e1a3553 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -332,13 +332,13 @@ static inline void xsm_free_security_domain(struct domain *d) } static inline int xsm_alloc_security_evtchns( - struct evtchn chn[], unsigned int nr) + struct evtchn *chn, unsigned int nr) { return alternative_call(xsm_ops.alloc_security_evtchns, chn, nr); } static inline void xsm_free_security_evtchns( - struct evtchn chn[], unsigned int nr) + struct evtchn *chn, unsigned int nr) { alternative_vcall(xsm_ops.free_security_evtchns, chn, nr); } -- 2.11.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/5] x86/altcall: allow compound types to be passed 2021-11-05 13:55 ` [PATCH 1/5] x86/altcall: allow compound types to be passed Andrew Cooper @ 2021-11-05 14:09 ` Daniel P. Smith 0 siblings, 0 replies; 21+ messages in thread From: Daniel P. Smith @ 2021-11-05 14:09 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Jan Beulich, Daniel De Graaf On 11/5/21 9:55 AM, Andrew Cooper wrote: > From: Jan Beulich <jbeulich@suse.com> > > Replace the conditional operator in ALT_CALL_ARG(), which was intended > to limit usable types to scalar ones, by a size check. Some restriction > here is necessary to make sure we don't violate the ABI's calling > conventions, but limiting to scalar types was both too restrictive > (disallowing e.g. guest handles) and too permissive (allowing e.g. > __int128_t). > > Note that there was some anomaly with that conditional operator anyway: > Something - I don't recall what - made it impossible to omit the middle > operand. > > Code-generation-wise this has the effect of removing certain zero- or > sign-extending in some altcall invocations. This ought to be fine as the > ABI doesn't require sub-sizeof(int) values to be extended, except when > passed through an ellipsis. No functions subject to altcall patching has > a variable number of arguments, though. > > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface 2021-11-05 13:55 [PATCH 0/5] XSM: cleanups Andrew Cooper 2021-11-05 13:55 ` [PATCH 1/5] x86/altcall: allow compound types to be passed Andrew Cooper @ 2021-11-05 13:55 ` Andrew Cooper 2021-11-05 14:00 ` Jan Beulich ` (2 more replies) 2021-11-05 13:55 ` [PATCH 3/5] xen/xsm: Drop xsm_hvm_control() hook Andrew Cooper ` (2 subsequent siblings) 4 siblings, 3 replies; 21+ messages in thread From: Andrew Cooper @ 2021-11-05 13:55 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Daniel De Graaf, Daniel Smith With alternative_call() capable of handling compound types, the three remaining hooks can be optimised at boot time too. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> CC: Daniel Smith <dpsmith@apertussolutions.com> I'm on the fence as to whether to declare this as fixing "xsm: convert xsm_ops hook calls to alternative call" --- xen/include/xsm/xsm.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 0b360e1a3553..82458066f625 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -579,13 +579,13 @@ static inline int xsm_hypfs_op(xsm_default_t def) static inline long xsm_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) { - return xsm_ops.do_xsm_op(op); + return alternative_call(xsm_ops.do_xsm_op, op); } #ifdef CONFIG_COMPAT static inline int xsm_do_compat_op(XEN_GUEST_HANDLE_PARAM(void) op) { - return xsm_ops.do_compat_op(op); + return alternative_call(xsm_ops.do_compat_op, op); } #endif @@ -698,7 +698,7 @@ static inline int xsm_mmuext_op( static inline int xsm_update_va_mapping( xsm_default_t def, struct domain *d, struct domain *f, l1_pgentry_t pte) { - return xsm_ops.update_va_mapping(d, f, pte); + return alternative_call(xsm_ops.update_va_mapping, d, f, pte); } static inline int xsm_priv_mapping( -- 2.11.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface 2021-11-05 13:55 ` [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface Andrew Cooper @ 2021-11-05 14:00 ` Jan Beulich 2021-11-05 14:11 ` Daniel P. Smith 2021-11-08 9:11 ` Jan Beulich 2 siblings, 0 replies; 21+ messages in thread From: Jan Beulich @ 2021-11-05 14:00 UTC (permalink / raw) To: Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 05.11.2021 14:55, Andrew Cooper wrote: > With alternative_call() capable of handling compound types, the three > remaining hooks can be optimised at boot time too. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface 2021-11-05 13:55 ` [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface Andrew Cooper 2021-11-05 14:00 ` Jan Beulich @ 2021-11-05 14:11 ` Daniel P. Smith 2021-11-08 9:11 ` Jan Beulich 2 siblings, 0 replies; 21+ messages in thread From: Daniel P. Smith @ 2021-11-05 14:11 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Daniel De Graaf On 11/5/21 9:55 AM, Andrew Cooper wrote: > With alternative_call() capable of handling compound types, the three > remaining hooks can be optimised at boot time too. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> > > I'm on the fence as to whether to declare this as fixing "xsm: convert xsm_ops > hook calls to alternative call" I don't know about fixing but completing was accurate. I am fairly certain in one of the incarnations of the patch set there was a comment identifying that these were left remaining since compound types were not supported by altcall. Regardless, Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> v/r, dps ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface 2021-11-05 13:55 ` [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface Andrew Cooper 2021-11-05 14:00 ` Jan Beulich 2021-11-05 14:11 ` Daniel P. Smith @ 2021-11-08 9:11 ` Jan Beulich 2021-11-17 22:22 ` Andrew Cooper 2 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2021-11-08 9:11 UTC (permalink / raw) To: Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 05.11.2021 14:55, Andrew Cooper wrote: > With alternative_call() capable of handling compound types, the three > remaining hooks can be optimised at boot time too. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> > > I'm on the fence as to whether to declare this as fixing "xsm: convert xsm_ops > hook calls to alternative call" Forgot to say a word on this: I'd consider Fixes: appropriate, as the commit's description says nothing about these having been left out, nor why. Jan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface 2021-11-08 9:11 ` Jan Beulich @ 2021-11-17 22:22 ` Andrew Cooper 0 siblings, 0 replies; 21+ messages in thread From: Andrew Cooper @ 2021-11-17 22:22 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 08/11/2021 09:11, Jan Beulich wrote: > On 05.11.2021 14:55, Andrew Cooper wrote: >> With alternative_call() capable of handling compound types, the three >> remaining hooks can be optimised at boot time too. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> CC: Daniel Smith <dpsmith@apertussolutions.com> >> >> I'm on the fence as to whether to declare this as fixing "xsm: convert xsm_ops >> hook calls to alternative call" > Forgot to say a word on this: I'd consider Fixes: appropriate, as the > commit's description says nothing about these having been left out, > nor why. Ok. I'll insert one. ~Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/5] xen/xsm: Drop xsm_hvm_control() hook 2021-11-05 13:55 [PATCH 0/5] XSM: cleanups Andrew Cooper 2021-11-05 13:55 ` [PATCH 1/5] x86/altcall: allow compound types to be passed Andrew Cooper 2021-11-05 13:55 ` [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface Andrew Cooper @ 2021-11-05 13:55 ` Andrew Cooper 2021-11-05 14:20 ` Daniel P. Smith 2021-11-05 13:55 ` [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() Andrew Cooper 2021-11-05 13:55 ` [PATCH 5/5] xen/xsm: Address hypercall ABI problems Andrew Cooper 4 siblings, 1 reply; 21+ messages in thread From: Andrew Cooper @ 2021-11-05 13:55 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Daniel De Graaf, Daniel Smith The final caller was dropped by c/s 58cbc034dc62 "dm_op: convert HVMOP_inject_trap and HVMOP_inject_msi" Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> CC: Daniel Smith <dpsmith@apertussolutions.com> --- xen/include/xsm/dummy.h | 7 ------- xen/include/xsm/xsm.h | 7 ------- xen/xsm/dummy.c | 1 - xen/xsm/flask/hooks.c | 1 - 4 files changed, 16 deletions(-) diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 3b1b378b5899..b024119896e6 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -580,13 +580,6 @@ static XSM_INLINE int xsm_hvm_param( return xsm_default_action(action, current->domain, d); } -static XSM_INLINE int xsm_hvm_control( - XSM_DEFAULT_ARG struct domain *d, unsigned long op) -{ - XSM_ASSERT_ACTION(XSM_DM_PRIV); - return xsm_default_action(action, current->domain, d); -} - static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) { XSM_ASSERT_ACTION(XSM_PRIV); diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 82458066f625..c5bd4213490a 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -138,7 +138,6 @@ struct xsm_ops { #endif int (*hvm_param)(struct domain *d, unsigned long op); - int (*hvm_control)(struct domain *d, unsigned long op); int (*hvm_param_altp2mhvm)(struct domain *d); int (*hvm_altp2mhvm_op)(struct domain *d, uint64_t mode, uint32_t op); int (*get_vnumainfo)(struct domain *d); @@ -595,12 +594,6 @@ static inline int xsm_hvm_param( return alternative_call(xsm_ops.hvm_param, d, op); } -static inline int xsm_hvm_control( - xsm_default_t def, struct domain *d, unsigned long op) -{ - return alternative_call(xsm_ops.hvm_control, d, op); -} - static inline int xsm_hvm_param_altp2mhvm(xsm_default_t def, struct domain *d) { return alternative_call(xsm_ops.hvm_param_altp2mhvm, d); diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index d8c935328e67..041f59fdf4ad 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -103,7 +103,6 @@ void __init xsm_fixup_ops (struct xsm_ops *ops) set_to_dummy_if_null(ops, page_offline); set_to_dummy_if_null(ops, hypfs_op); set_to_dummy_if_null(ops, hvm_param); - set_to_dummy_if_null(ops, hvm_control); set_to_dummy_if_null(ops, hvm_param_altp2mhvm); set_to_dummy_if_null(ops, hvm_altp2mhvm_op); diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index ea9a12bd7121..3b29f7fde372 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1816,7 +1816,6 @@ static const struct xsm_ops __initconstrel flask_ops = { .page_offline = flask_page_offline, .hypfs_op = flask_hypfs_op, .hvm_param = flask_hvm_param, - .hvm_control = flask_hvm_param, .hvm_param_altp2mhvm = flask_hvm_param_altp2mhvm, .hvm_altp2mhvm_op = flask_hvm_altp2mhvm_op, -- 2.11.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/5] xen/xsm: Drop xsm_hvm_control() hook 2021-11-05 13:55 ` [PATCH 3/5] xen/xsm: Drop xsm_hvm_control() hook Andrew Cooper @ 2021-11-05 14:20 ` Daniel P. Smith 0 siblings, 0 replies; 21+ messages in thread From: Daniel P. Smith @ 2021-11-05 14:20 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Daniel De Graaf On 11/5/21 9:55 AM, Andrew Cooper wrote: > The final caller was dropped by c/s 58cbc034dc62 "dm_op: convert > HVMOP_inject_trap and HVMOP_inject_msi" > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() 2021-11-05 13:55 [PATCH 0/5] XSM: cleanups Andrew Cooper ` (2 preceding siblings ...) 2021-11-05 13:55 ` [PATCH 3/5] xen/xsm: Drop xsm_hvm_control() hook Andrew Cooper @ 2021-11-05 13:55 ` Andrew Cooper 2021-11-05 14:22 ` Daniel P. Smith 2021-11-08 9:04 ` Jan Beulich 2021-11-05 13:55 ` [PATCH 5/5] xen/xsm: Address hypercall ABI problems Andrew Cooper 4 siblings, 2 replies; 21+ messages in thread From: Andrew Cooper @ 2021-11-05 13:55 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Daniel De Graaf, Daniel Smith The current xsm_fixup_ops() is just shy of a full page when compiled, and very fragile to NULL function pointer errors. Address both of these issues with a minor piece of structure (ab)use. Introduce dummy_ops, and fixup the provided xsm_ops pointer by treating both as an array of unsigned longs. The compiled size improvement speaks for itself: $ ../scripts/bloat-o-meter xen-syms-before xen-syms-after add/remove: 1/0 grow/shrink: 0/1 up/down: 712/-3897 (-3185) Function old new delta dummy_ops - 712 +712 xsm_fixup_ops 3987 90 -3897 and there is an additional safety check that will make it obvious during development if there is an issue with the fallback handling. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> CC: Daniel Smith <dpsmith@apertussolutions.com> --- xen/include/xsm/xsm.h | 9 ++ xen/xsm/dummy.c | 254 +++++++++++++++++++++++++++----------------------- 2 files changed, 147 insertions(+), 116 deletions(-) diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index c5bd4213490a..5aa4dd588d17 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -42,6 +42,15 @@ enum xsm_default { }; typedef enum xsm_default xsm_default_t; +/* + * !!! WARNING !!! + * + * For simplicity, xsm_fixup_ops() expects that this structure is made + * exclusively of function pointers to non-init functions. Think carefully + * before deviating from the pattern. + * + * !!! WARNING !!! + */ struct xsm_ops { void (*security_domaininfo)(struct domain *d, struct xen_domctl_getdomaininfo *info); diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index 041f59fdf4ad..c111fa05968d 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -13,145 +13,167 @@ #define XSM_NO_WRAPPERS #include <xsm/dummy.h> -#define set_to_dummy_if_null(ops, function) \ - do { \ - if ( !ops->function ) \ - ops->function = xsm_##function; \ - } while (0) - -void __init xsm_fixup_ops (struct xsm_ops *ops) -{ - set_to_dummy_if_null(ops, security_domaininfo); - set_to_dummy_if_null(ops, domain_create); - set_to_dummy_if_null(ops, getdomaininfo); - set_to_dummy_if_null(ops, domctl_scheduler_op); - set_to_dummy_if_null(ops, sysctl_scheduler_op); - set_to_dummy_if_null(ops, set_target); - set_to_dummy_if_null(ops, domctl); - set_to_dummy_if_null(ops, sysctl); - set_to_dummy_if_null(ops, readconsole); - - set_to_dummy_if_null(ops, evtchn_unbound); - set_to_dummy_if_null(ops, evtchn_interdomain); - set_to_dummy_if_null(ops, evtchn_close_post); - set_to_dummy_if_null(ops, evtchn_send); - set_to_dummy_if_null(ops, evtchn_status); - set_to_dummy_if_null(ops, evtchn_reset); - - set_to_dummy_if_null(ops, grant_mapref); - set_to_dummy_if_null(ops, grant_unmapref); - set_to_dummy_if_null(ops, grant_setup); - set_to_dummy_if_null(ops, grant_transfer); - set_to_dummy_if_null(ops, grant_copy); - set_to_dummy_if_null(ops, grant_query_size); - - set_to_dummy_if_null(ops, alloc_security_domain); - set_to_dummy_if_null(ops, free_security_domain); - set_to_dummy_if_null(ops, alloc_security_evtchns); - set_to_dummy_if_null(ops, free_security_evtchns); - set_to_dummy_if_null(ops, show_security_evtchn); - set_to_dummy_if_null(ops, init_hardware_domain); - - set_to_dummy_if_null(ops, get_pod_target); - set_to_dummy_if_null(ops, set_pod_target); - - set_to_dummy_if_null(ops, memory_exchange); - set_to_dummy_if_null(ops, memory_adjust_reservation); - set_to_dummy_if_null(ops, memory_stat_reservation); - set_to_dummy_if_null(ops, memory_pin_page); - set_to_dummy_if_null(ops, claim_pages); - - set_to_dummy_if_null(ops, console_io); - - set_to_dummy_if_null(ops, profile); - - set_to_dummy_if_null(ops, kexec); - set_to_dummy_if_null(ops, schedop_shutdown); - - set_to_dummy_if_null(ops, show_irq_sid); - set_to_dummy_if_null(ops, map_domain_pirq); - set_to_dummy_if_null(ops, map_domain_irq); - set_to_dummy_if_null(ops, unmap_domain_pirq); - set_to_dummy_if_null(ops, unmap_domain_irq); - set_to_dummy_if_null(ops, bind_pt_irq); - set_to_dummy_if_null(ops, unbind_pt_irq); - set_to_dummy_if_null(ops, irq_permission); - set_to_dummy_if_null(ops, iomem_permission); - set_to_dummy_if_null(ops, iomem_mapping); - set_to_dummy_if_null(ops, pci_config_permission); - set_to_dummy_if_null(ops, get_vnumainfo); +static const struct xsm_ops __initconstrel dummy_ops = { + .security_domaininfo = xsm_security_domaininfo, + .domain_create = xsm_domain_create, + .getdomaininfo = xsm_getdomaininfo, + .domctl_scheduler_op = xsm_domctl_scheduler_op, + .sysctl_scheduler_op = xsm_sysctl_scheduler_op, + .set_target = xsm_set_target, + .domctl = xsm_domctl, + .sysctl = xsm_sysctl, + .readconsole = xsm_readconsole, + + .evtchn_unbound = xsm_evtchn_unbound, + .evtchn_interdomain = xsm_evtchn_interdomain, + .evtchn_close_post = xsm_evtchn_close_post, + .evtchn_send = xsm_evtchn_send, + .evtchn_status = xsm_evtchn_status, + .evtchn_reset = xsm_evtchn_reset, + + .grant_mapref = xsm_grant_mapref, + .grant_unmapref = xsm_grant_unmapref, + .grant_setup = xsm_grant_setup, + .grant_transfer = xsm_grant_transfer, + .grant_copy = xsm_grant_copy, + .grant_query_size = xsm_grant_query_size, + + .alloc_security_domain = xsm_alloc_security_domain, + .free_security_domain = xsm_free_security_domain, + .alloc_security_evtchns = xsm_alloc_security_evtchns, + .free_security_evtchns = xsm_free_security_evtchns, + .show_security_evtchn = xsm_show_security_evtchn, + .init_hardware_domain = xsm_init_hardware_domain, + + .get_pod_target = xsm_get_pod_target, + .set_pod_target = xsm_set_pod_target, + + .memory_exchange = xsm_memory_exchange, + .memory_adjust_reservation = xsm_memory_adjust_reservation, + .memory_stat_reservation = xsm_memory_stat_reservation, + .memory_pin_page = xsm_memory_pin_page, + .claim_pages = xsm_claim_pages, + + .console_io = xsm_console_io, + + .profile = xsm_profile, + + .kexec = xsm_kexec, + .schedop_shutdown = xsm_schedop_shutdown, + + .show_irq_sid = xsm_show_irq_sid, + .map_domain_pirq = xsm_map_domain_pirq, + .map_domain_irq = xsm_map_domain_irq, + .unmap_domain_pirq = xsm_unmap_domain_pirq, + .unmap_domain_irq = xsm_unmap_domain_irq, + .bind_pt_irq = xsm_bind_pt_irq, + .unbind_pt_irq = xsm_unbind_pt_irq, + .irq_permission = xsm_irq_permission, + .iomem_permission = xsm_iomem_permission, + .iomem_mapping = xsm_iomem_mapping, + .pci_config_permission = xsm_pci_config_permission, + .get_vnumainfo = xsm_get_vnumainfo, #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI) - set_to_dummy_if_null(ops, get_device_group); - set_to_dummy_if_null(ops, assign_device); - set_to_dummy_if_null(ops, deassign_device); + .get_device_group = xsm_get_device_group, + .assign_device = xsm_assign_device, + .deassign_device = xsm_deassign_device, #endif #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE) - set_to_dummy_if_null(ops, assign_dtdevice); - set_to_dummy_if_null(ops, deassign_dtdevice); + .assign_dtdevice = xsm_assign_dtdevice, + .deassign_dtdevice = xsm_deassign_dtdevice, #endif - set_to_dummy_if_null(ops, resource_plug_core); - set_to_dummy_if_null(ops, resource_unplug_core); - set_to_dummy_if_null(ops, resource_plug_pci); - set_to_dummy_if_null(ops, resource_unplug_pci); - set_to_dummy_if_null(ops, resource_setup_pci); - set_to_dummy_if_null(ops, resource_setup_gsi); - set_to_dummy_if_null(ops, resource_setup_misc); - - set_to_dummy_if_null(ops, page_offline); - set_to_dummy_if_null(ops, hypfs_op); - set_to_dummy_if_null(ops, hvm_param); - set_to_dummy_if_null(ops, hvm_param_altp2mhvm); - set_to_dummy_if_null(ops, hvm_altp2mhvm_op); - - set_to_dummy_if_null(ops, do_xsm_op); + .resource_plug_core = xsm_resource_plug_core, + .resource_unplug_core = xsm_resource_unplug_core, + .resource_plug_pci = xsm_resource_plug_pci, + .resource_unplug_pci = xsm_resource_unplug_pci, + .resource_setup_pci = xsm_resource_setup_pci, + .resource_setup_gsi = xsm_resource_setup_gsi, + .resource_setup_misc = xsm_resource_setup_misc, + + .page_offline = xsm_page_offline, + .hypfs_op = xsm_hypfs_op, + .hvm_param = xsm_hvm_param, + .hvm_param_altp2mhvm = xsm_hvm_param_altp2mhvm, + .hvm_altp2mhvm_op = xsm_hvm_altp2mhvm_op, + + .do_xsm_op = xsm_do_xsm_op, #ifdef CONFIG_COMPAT - set_to_dummy_if_null(ops, do_compat_op); + .do_compat_op = xsm_do_compat_op, #endif - set_to_dummy_if_null(ops, add_to_physmap); - set_to_dummy_if_null(ops, remove_from_physmap); - set_to_dummy_if_null(ops, map_gmfn_foreign); + .add_to_physmap = xsm_add_to_physmap, + .remove_from_physmap = xsm_remove_from_physmap, + .map_gmfn_foreign = xsm_map_gmfn_foreign, - set_to_dummy_if_null(ops, vm_event_control); + .vm_event_control = xsm_vm_event_control, #ifdef CONFIG_MEM_ACCESS - set_to_dummy_if_null(ops, mem_access); + .mem_access = xsm_mem_access, #endif #ifdef CONFIG_MEM_PAGING - set_to_dummy_if_null(ops, mem_paging); + .mem_paging = xsm_mem_paging, #endif #ifdef CONFIG_MEM_SHARING - set_to_dummy_if_null(ops, mem_sharing); + .mem_sharing = xsm_mem_sharing, #endif - set_to_dummy_if_null(ops, platform_op); + .platform_op = xsm_platform_op, #ifdef CONFIG_X86 - set_to_dummy_if_null(ops, do_mca); - set_to_dummy_if_null(ops, shadow_control); - set_to_dummy_if_null(ops, mem_sharing_op); - set_to_dummy_if_null(ops, apic); - set_to_dummy_if_null(ops, machine_memory_map); - set_to_dummy_if_null(ops, domain_memory_map); - set_to_dummy_if_null(ops, mmu_update); - set_to_dummy_if_null(ops, mmuext_op); - set_to_dummy_if_null(ops, update_va_mapping); - set_to_dummy_if_null(ops, priv_mapping); - set_to_dummy_if_null(ops, ioport_permission); - set_to_dummy_if_null(ops, ioport_mapping); - set_to_dummy_if_null(ops, pmu_op); + .do_mca = xsm_do_mca, + .shadow_control = xsm_shadow_control, + .mem_sharing_op = xsm_mem_sharing_op, + .apic = xsm_apic, + .machine_memory_map = xsm_machine_memory_map, + .domain_memory_map = xsm_domain_memory_map, + .mmu_update = xsm_mmu_update, + .mmuext_op = xsm_mmuext_op, + .update_va_mapping = xsm_update_va_mapping, + .priv_mapping = xsm_priv_mapping, + .ioport_permission = xsm_ioport_permission, + .ioport_mapping = xsm_ioport_mapping, + .pmu_op = xsm_pmu_op, #endif - set_to_dummy_if_null(ops, dm_op); - set_to_dummy_if_null(ops, xen_version); - set_to_dummy_if_null(ops, domain_resource_map); + .dm_op = xsm_dm_op, + .xen_version = xsm_xen_version, + .domain_resource_map = xsm_domain_resource_map, #ifdef CONFIG_ARGO - set_to_dummy_if_null(ops, argo_enable); - set_to_dummy_if_null(ops, argo_register_single_source); - set_to_dummy_if_null(ops, argo_register_any_source); - set_to_dummy_if_null(ops, argo_send); + .argo_enable = xsm_argo_enable, + .argo_register_single_source = xsm_argo_register_single_source, + .argo_register_any_source = xsm_argo_register_any_source, + .argo_send = xsm_argo_send, #endif +}; + +void __init xsm_fixup_ops(struct xsm_ops *ops) +{ + /* + * We make some simplifying assumptions about struct xsm_ops; that it is + * made exclusively of function pointers to non-init text. + * + * This allows us to walk over struct xsm_ops as if it were an array of + * unsigned longs. + */ + unsigned long *dst = _p(ops); + unsigned long *src = _p(&dummy_ops); + + for ( ; dst < (unsigned long *)(ops + 1); src++, dst++ ) + { + /* + * If you encounter this BUG(), then you've most likely added a new + * XSM hook but failed to provide the default implementation in + * dummy_ops. + * + * If not, then perhaps a function pointer to an init function, or + * something which isn't a function pointer at all. + */ + BUG_ON(!is_kernel_text(*src)); + + if ( !*dst ) + *dst = *src; + } } -- 2.11.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() 2021-11-05 13:55 ` [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() Andrew Cooper @ 2021-11-05 14:22 ` Daniel P. Smith 2021-11-08 9:04 ` Jan Beulich 1 sibling, 0 replies; 21+ messages in thread From: Daniel P. Smith @ 2021-11-05 14:22 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: Daniel De Graaf On 11/5/21 9:55 AM, Andrew Cooper wrote: > The current xsm_fixup_ops() is just shy of a full page when compiled, and very > fragile to NULL function pointer errors. > > Address both of these issues with a minor piece of structure (ab)use. > Introduce dummy_ops, and fixup the provided xsm_ops pointer by treating both > as an array of unsigned longs. > > The compiled size improvement speaks for itself: > > $ ../scripts/bloat-o-meter xen-syms-before xen-syms-after > add/remove: 1/0 grow/shrink: 0/1 up/down: 712/-3897 (-3185) > Function old new delta > dummy_ops - 712 +712 > xsm_fixup_ops 3987 90 -3897 > > and there is an additional safety check that will make it obvious during > development if there is an issue with the fallback handling. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() 2021-11-05 13:55 ` [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() Andrew Cooper 2021-11-05 14:22 ` Daniel P. Smith @ 2021-11-08 9:04 ` Jan Beulich 2021-11-17 22:37 ` Andrew Cooper 1 sibling, 1 reply; 21+ messages in thread From: Jan Beulich @ 2021-11-08 9:04 UTC (permalink / raw) To: Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 05.11.2021 14:55, Andrew Cooper wrote: > +void __init xsm_fixup_ops(struct xsm_ops *ops) > +{ > + /* > + * We make some simplifying assumptions about struct xsm_ops; that it is > + * made exclusively of function pointers to non-init text. > + * > + * This allows us to walk over struct xsm_ops as if it were an array of > + * unsigned longs. > + */ > + unsigned long *dst = _p(ops); > + unsigned long *src = _p(&dummy_ops); I'm afraid I consider this an abuse of _p(): It hides casting when that would better not be hidden (and there's then also a pointless step through "unsigned long" in the casting). I suppose this is also why "src" didn't end up "const unsigned long *" - with spelled out casts the casting away of const might have been more noticable. > + for ( ; dst < (unsigned long *)(ops + 1); src++, dst++ ) > + { > + /* > + * If you encounter this BUG(), then you've most likely added a new > + * XSM hook but failed to provide the default implementation in > + * dummy_ops. > + * > + * If not, then perhaps a function pointer to an init function, or > + * something which isn't a function pointer at all. > + */ > + BUG_ON(!is_kernel_text(*src)); Just as a remark, not a request to change anything: A cause of this triggering may also be is_kernel_text() not covering all text sections. Some of what recently we've been talking about informally may lead to new text section variants appearing, and whether those would sensibly end up inside [_stext,_etext) is uncertain. Jan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() 2021-11-08 9:04 ` Jan Beulich @ 2021-11-17 22:37 ` Andrew Cooper 2021-11-18 11:59 ` Jan Beulich 0 siblings, 1 reply; 21+ messages in thread From: Andrew Cooper @ 2021-11-17 22:37 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 08/11/2021 09:04, Jan Beulich wrote: > On 05.11.2021 14:55, Andrew Cooper wrote: >> +void __init xsm_fixup_ops(struct xsm_ops *ops) >> +{ >> + /* >> + * We make some simplifying assumptions about struct xsm_ops; that it is >> + * made exclusively of function pointers to non-init text. >> + * >> + * This allows us to walk over struct xsm_ops as if it were an array of >> + * unsigned longs. >> + */ >> + unsigned long *dst = _p(ops); >> + unsigned long *src = _p(&dummy_ops); > I'm afraid I consider this an abuse of _p(): It hides casting when > that would better not be hidden (and there's then also a pointless > step through "unsigned long" in the casting). I suppose this is > also why "src" didn't end up "const unsigned long *" - with spelled > out casts the casting away of const might have been more noticable. I've changed to a const pointer, but opencoding _p() wouldn't make it any more likely for me to have spotted that it ought to have been const to begin with. But ultimately it comes down to neatness/clarity. This: unsigned long *dst = _p(ops); const unsigned long *src = _p(&dummy_ops); is easier to read than this: unsigned long *dst = (unsigned long *)ops; const unsigned long *src = (const unsigned long *)&dummy_ops; Fundamentally, I can do either, but I have a preference for the one which is easier to follow. >> + for ( ; dst < (unsigned long *)(ops + 1); src++, dst++ ) >> + { >> + /* >> + * If you encounter this BUG(), then you've most likely added a new >> + * XSM hook but failed to provide the default implementation in >> + * dummy_ops. >> + * >> + * If not, then perhaps a function pointer to an init function, or >> + * something which isn't a function pointer at all. >> + */ >> + BUG_ON(!is_kernel_text(*src)); > Just as a remark, not a request to change anything: A cause of this > triggering may also be is_kernel_text() not covering all text > sections. Some of what recently we've been talking about informally > may lead to new text section variants appearing, and whether those > would sensibly end up inside [_stext,_etext) is uncertain. I'm afraid that I'm not aware of what you're referring to here. But I don't think any good will come from having is_kernel_text() not covering suitable things. ~Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() 2021-11-17 22:37 ` Andrew Cooper @ 2021-11-18 11:59 ` Jan Beulich 0 siblings, 0 replies; 21+ messages in thread From: Jan Beulich @ 2021-11-18 11:59 UTC (permalink / raw) To: Andrew Cooper, Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 17.11.2021 23:37, Andrew Cooper wrote: > On 08/11/2021 09:04, Jan Beulich wrote: >> On 05.11.2021 14:55, Andrew Cooper wrote: >>> +void __init xsm_fixup_ops(struct xsm_ops *ops) >>> +{ >>> + /* >>> + * We make some simplifying assumptions about struct xsm_ops; that it is >>> + * made exclusively of function pointers to non-init text. >>> + * >>> + * This allows us to walk over struct xsm_ops as if it were an array of >>> + * unsigned longs. >>> + */ >>> + unsigned long *dst = _p(ops); >>> + unsigned long *src = _p(&dummy_ops); >> I'm afraid I consider this an abuse of _p(): It hides casting when >> that would better not be hidden (and there's then also a pointless >> step through "unsigned long" in the casting). I suppose this is >> also why "src" didn't end up "const unsigned long *" - with spelled >> out casts the casting away of const might have been more noticable. > > I've changed to a const pointer, but opencoding _p() wouldn't make it > any more likely for me to have spotted that it ought to have been const > to begin with. > > But ultimately it comes down to neatness/clarity. This: > > unsigned long *dst = _p(ops); > const unsigned long *src = _p(&dummy_ops); > > is easier to read than this: > > unsigned long *dst = (unsigned long *)ops; > const unsigned long *src = (const unsigned long *)&dummy_ops; > > Fundamentally, I can do either, but I have a preference for the one > which is easier to follow. One option would be to at least make _p() cast to const void *. Jan ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/5] xen/xsm: Address hypercall ABI problems 2021-11-05 13:55 [PATCH 0/5] XSM: cleanups Andrew Cooper ` (3 preceding siblings ...) 2021-11-05 13:55 ` [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() Andrew Cooper @ 2021-11-05 13:55 ` Andrew Cooper 2021-11-08 9:50 ` Jan Beulich 4 siblings, 1 reply; 21+ messages in thread From: Andrew Cooper @ 2021-11-05 13:55 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Daniel De Graaf, Daniel Smith Currently, __HYPERVISOR_xsm_op enters xsm_ops.do_{xsm,compat}_op() which means that if any other XSM module registers a handler, we'll break the hypercall ABI totally by having the behaviour depend on the selected XSM module. There are 2 options: 1) Rename __HYPERVISOR_xsm_op to __HYPERVISOR_flask_op. If another XSM module wants hypercalls, they'll have to introduce a new top-level hypercall. 2) Make the current flask_op() be common, and require new hypercalls to fit compatibly with xen_flask_op_t. This can be done fairly easily by subdividing the .cmd space. In this patch, we implement option 2. Move the stub {do,compat}_xsm_op() implementation into a new xsm_op.c so we can more easily do multi-inclusion compat magic. Also add a new private.h, because flask/hook.c's local declaration of {do,compat}_flask_op() wasn't remotely safe. The top level now dispatches into {do,compat}_flask_op() based on op.cmd, and handles the primary copy in/out. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> CC: Daniel Smith <dpsmith@apertussolutions.com> Only lightly tested. Slightly RFC. There are several things which aren't great, but probably want addressing in due course. 1) The public headers probably want to lose the flask name (in a compatible way), now that the hypercall is common. This probably wants to be combined with no longer taking a void handle. 2) {do,compat}_xsm_op() are currently identical other than the dispatched-to functions because the size of xsm_flask_op_t is invariant with COMPAT-ness. We could simplfy things by only having one, and dispatching to {do,compat}_*_op() directly, but I'm not sure whether the complexity is worth it. 3) Bloat-o-meter says these changes add 16 extra bytes to dm_op() and I can't figure out what could possibly be causing this. --- xen/include/Makefile | 2 +- xen/include/xsm/dummy.h | 12 ---------- xen/include/xsm/xsm.h | 17 -------------- xen/xsm/Makefile | 1 + xen/xsm/dummy.c | 5 ---- xen/xsm/flask/flask_op.c | 60 ++++++++++++++++++++++-------------------------- xen/xsm/flask/hooks.c | 8 ------- xen/xsm/private.h | 16 +++++++++++++ xen/xsm/xsm_core.c | 12 ---------- xen/xsm/xsm_op.c | 51 ++++++++++++++++++++++++++++++++++++++++ 10 files changed, 96 insertions(+), 88 deletions(-) create mode 100644 xen/xsm/private.h create mode 100644 xen/xsm/xsm_op.c diff --git a/xen/include/Makefile b/xen/include/Makefile index 95daa8a28975..9547f848635c 100644 --- a/xen/include/Makefile +++ b/xen/include/Makefile @@ -32,7 +32,7 @@ headers-$(CONFIG_HYPFS) += compat/hypfs.h headers-$(CONFIG_KEXEC) += compat/kexec.h headers-$(CONFIG_TRACEBUFFER) += compat/trace.h headers-$(CONFIG_XENOPROF) += compat/xenoprof.h -headers-$(CONFIG_XSM_FLASK) += compat/xsm/flask_op.h +headers-$(CONFIG_XSM) += compat/xsm/flask_op.h cppflags-y := -include public/xen-compat.h -DXEN_GENERATING_COMPAT_HEADERS cppflags-$(CONFIG_X86) += -m32 diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index b024119896e6..5448e9970bc1 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -466,18 +466,6 @@ static XSM_INLINE int xsm_hypfs_op(XSM_DEFAULT_VOID) return xsm_default_action(action, current->domain, NULL); } -static XSM_INLINE long xsm_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) -{ - return -ENOSYS; -} - -#ifdef CONFIG_COMPAT -static XSM_INLINE int xsm_do_compat_op(XEN_GUEST_HANDLE_PARAM(void) op) -{ - return -ENOSYS; -} -#endif - static XSM_INLINE char *xsm_show_irq_sid(int irq) { return NULL; diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 5aa4dd588d17..4856b589dd86 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -141,11 +141,6 @@ struct xsm_ops { int (*page_offline)(uint32_t cmd); int (*hypfs_op)(void); - long (*do_xsm_op)(XEN_GUEST_HANDLE_PARAM(void) op); -#ifdef CONFIG_COMPAT - int (*do_compat_op)(XEN_GUEST_HANDLE_PARAM(void) op); -#endif - int (*hvm_param)(struct domain *d, unsigned long op); int (*hvm_param_altp2mhvm)(struct domain *d); int (*hvm_altp2mhvm_op)(struct domain *d, uint64_t mode, uint32_t op); @@ -585,18 +580,6 @@ static inline int xsm_hypfs_op(xsm_default_t def) return alternative_call(xsm_ops.hypfs_op); } -static inline long xsm_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) -{ - return alternative_call(xsm_ops.do_xsm_op, op); -} - -#ifdef CONFIG_COMPAT -static inline int xsm_do_compat_op(XEN_GUEST_HANDLE_PARAM(void) op) -{ - return alternative_call(xsm_ops.do_compat_op, op); -} -#endif - static inline int xsm_hvm_param( xsm_default_t def, struct domain *d, unsigned long op) { diff --git a/xen/xsm/Makefile b/xen/xsm/Makefile index cf0a728f1c96..93ba20256f6e 100644 --- a/xen/xsm/Makefile +++ b/xen/xsm/Makefile @@ -1,4 +1,5 @@ obj-y += xsm_core.o +obj-y += xsm_op.o obj-$(CONFIG_XSM) += xsm_policy.o obj-$(CONFIG_XSM) += dummy.o obj-$(CONFIG_XSM_SILO) += silo.o diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index c111fa05968d..4fb7b0d04f7b 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -99,11 +99,6 @@ static const struct xsm_ops __initconstrel dummy_ops = { .hvm_param_altp2mhvm = xsm_hvm_param_altp2mhvm, .hvm_altp2mhvm_op = xsm_hvm_altp2mhvm_op, - .do_xsm_op = xsm_do_xsm_op, -#ifdef CONFIG_COMPAT - .do_compat_op = xsm_do_compat_op, -#endif - .add_to_physmap = xsm_add_to_physmap, .remove_from_physmap = xsm_remove_from_physmap, .map_gmfn_foreign = xsm_map_gmfn_foreign, diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c index 221ff00fd3cc..e25664711019 100644 --- a/xen/xsm/flask/flask_op.c +++ b/xen/xsm/flask/flask_op.c @@ -22,6 +22,8 @@ #include <objsec.h> #include <conditional.h> +#include "../private.h" + #define ret_t long #define _copy_to_guest copy_to_guest #define _copy_from_guest copy_from_guest @@ -607,21 +609,17 @@ static int flask_relabel_domain(struct xen_flask_relabel *arg) #endif /* !COMPAT */ -ret_t do_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op) +ret_t do_flask_op(xen_flask_op_t *op, bool *copyback) { - xen_flask_op_t op; int rv; - if ( copy_from_guest(&op, u_flask_op, 1) ) - return -EFAULT; - - if ( op.interface_version != XEN_FLASK_INTERFACE_VERSION ) + if ( op->interface_version != XEN_FLASK_INTERFACE_VERSION ) return -ENOSYS; - switch ( op.cmd ) + switch ( op->cmd ) { case FLASK_LOAD: - rv = flask_security_load(&op.u.load); + rv = flask_security_load(&op->u.load); break; case FLASK_GETENFORCE: @@ -629,27 +627,27 @@ ret_t do_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op) break; case FLASK_SETENFORCE: - rv = flask_security_setenforce(&op.u.enforce); + rv = flask_security_setenforce(&op->u.enforce); break; case FLASK_CONTEXT_TO_SID: - rv = flask_security_context(&op.u.sid_context); + rv = flask_security_context(&op->u.sid_context); break; case FLASK_SID_TO_CONTEXT: - rv = flask_security_sid(&op.u.sid_context); + rv = flask_security_sid(&op->u.sid_context); break; case FLASK_ACCESS: - rv = flask_security_access(&op.u.access); + rv = flask_security_access(&op->u.access); break; case FLASK_CREATE: - rv = flask_security_create(&op.u.transition); + rv = flask_security_create(&op->u.transition); break; case FLASK_RELABEL: - rv = flask_security_relabel(&op.u.transition); + rv = flask_security_relabel(&op->u.transition); break; case FLASK_POLICYVERS: @@ -657,11 +655,11 @@ ret_t do_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op) break; case FLASK_GETBOOL: - rv = flask_security_get_bool(&op.u.boolean); + rv = flask_security_get_bool(&op->u.boolean); break; case FLASK_SETBOOL: - rv = flask_security_set_bool(&op.u.boolean); + rv = flask_security_set_bool(&op->u.boolean); break; case FLASK_COMMITBOOLS: @@ -677,41 +675,41 @@ ret_t do_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op) break; case FLASK_SETAVC_THRESHOLD: - rv = flask_security_setavc_threshold(&op.u.setavc_threshold); + rv = flask_security_setavc_threshold(&op->u.setavc_threshold); break; case FLASK_AVC_HASHSTATS: - rv = avc_get_hash_stats(&op.u.hash_stats); + rv = avc_get_hash_stats(&op->u.hash_stats); break; #ifdef CONFIG_XSM_FLASK_AVC_STATS case FLASK_AVC_CACHESTATS: - rv = flask_security_avc_cachestats(&op.u.cache_stats); + rv = flask_security_avc_cachestats(&op->u.cache_stats); break; #endif case FLASK_MEMBER: - rv = flask_security_member(&op.u.transition); + rv = flask_security_member(&op->u.transition); break; case FLASK_ADD_OCONTEXT: - rv = flask_ocontext_add(&op.u.ocontext); + rv = flask_ocontext_add(&op->u.ocontext); break; case FLASK_DEL_OCONTEXT: - rv = flask_ocontext_del(&op.u.ocontext); + rv = flask_ocontext_del(&op->u.ocontext); break; case FLASK_GET_PEER_SID: - rv = flask_get_peer_sid(&op.u.peersid); + rv = flask_get_peer_sid(&op->u.peersid); break; case FLASK_RELABEL_DOMAIN: - rv = flask_relabel_domain(&op.u.relabel); + rv = flask_relabel_domain(&op->u.relabel); break; case FLASK_DEVICETREE_LABEL: - rv = flask_devicetree_label(&op.u.devicetree_label); + rv = flask_devicetree_label(&op->u.devicetree_label); break; default: @@ -719,16 +717,12 @@ ret_t do_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op) } if ( rv < 0 ) - goto out; + return rv; - if ( (FLASK_COPY_OUT&(1UL<<op.cmd)) ) - { - if ( copy_to_guest(u_flask_op, &op, 1) ) - rv = -EFAULT; - } + if ( (1ul << op->cmd) & FLASK_COPY_OUT ) + *copyback = true; - out: - return rv; + return 0; } #if defined(CONFIG_COMPAT) && !defined(COMPAT) diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 3b29f7fde372..8f4fc7458b8f 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1742,9 +1742,6 @@ static int flask_argo_send(const struct domain *d, const struct domain *t) #endif -long do_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op); -int compat_flask_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op); - static const struct xsm_ops __initconstrel flask_ops = { .security_domaininfo = flask_security_domaininfo, .domain_create = flask_domain_create, @@ -1819,7 +1816,6 @@ static const struct xsm_ops __initconstrel flask_ops = { .hvm_param_altp2mhvm = flask_hvm_param_altp2mhvm, .hvm_altp2mhvm_op = flask_hvm_altp2mhvm_op, - .do_xsm_op = do_flask_op, .get_vnumainfo = flask_get_vnumainfo, .vm_event_control = flask_vm_event_control, @@ -1836,10 +1832,6 @@ static const struct xsm_ops __initconstrel flask_ops = { .mem_sharing = flask_mem_sharing, #endif -#ifdef CONFIG_COMPAT - .do_compat_op = compat_flask_op, -#endif - .add_to_physmap = flask_add_to_physmap, .remove_from_physmap = flask_remove_from_physmap, .map_gmfn_foreign = flask_map_gmfn_foreign, diff --git a/xen/xsm/private.h b/xen/xsm/private.h new file mode 100644 index 000000000000..19ade2eed6b7 --- /dev/null +++ b/xen/xsm/private.h @@ -0,0 +1,16 @@ +#ifndef XSM_PRIVATE_H +#define XSM_PRIVATE_H + +#include <public/xsm/flask_op.h> + +long do_flask_op(xen_flask_op_t *op, bool *copyback); + +#ifdef CONFIG_COMPAT + +#include <compat/xsm/flask_op.h> + +int compat_flask_op(compat_flask_op_t *op, bool *copyback); + +#endif /* CONFIG_COMPAT */ + +#endif /* XSM_PRIVATE_H */ diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c index 21fffbcb41d3..819a6ccd54cc 100644 --- a/xen/xsm/xsm_core.c +++ b/xen/xsm/xsm_core.c @@ -218,15 +218,3 @@ bool __init has_xsm_magic(paddr_t start) #endif #endif - -long do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) -{ - return xsm_do_xsm_op(op); -} - -#ifdef CONFIG_COMPAT -int compat_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) -{ - return xsm_do_compat_op(op); -} -#endif diff --git a/xen/xsm/xsm_op.c b/xen/xsm/xsm_op.c new file mode 100644 index 000000000000..5922299fc4dc --- /dev/null +++ b/xen/xsm/xsm_op.c @@ -0,0 +1,51 @@ +#ifndef COMPAT + +#include <xen/guest_access.h> + +#include "private.h" + +#define ret_t long +#define _copy_to_guest copy_to_guest +#define _copy_from_guest copy_from_guest + +#endif /* COMPAT */ + +ret_t do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) u_flask_op) +{ + xen_flask_op_t op; + bool copyback = false; + ret_t rc = -ENOSYS; + + if ( copy_from_guest(&op, u_flask_op, 1) ) + return -EFAULT; + + switch ( op.cmd ) + { + case FLASK_LOAD ... FLASK_DEVICETREE_LABEL: + if ( IS_ENABLED(CONFIG_XSM_FLASK) ) + rc = do_flask_op(&op, ©back); + break; + } + + if ( copyback && copy_to_guest(u_flask_op, &op, 1) ) + rc = -EFAULT; + + return rc; +} + +#if defined(CONFIG_COMPAT) && !defined(COMPAT) +#define COMPAT + +#undef _copy_to_guest +#define _copy_to_guest copy_to_compat +#undef _copy_from_guest +#define _copy_from_guest copy_from_compat + +#define xen_flask_op_t compat_flask_op_t +#undef ret_t +#define ret_t int +#define do_flask_op compat_flask_op +#define do_xsm_op compat_xsm_op + +#include "xsm_op.c" +#endif -- 2.11.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] xen/xsm: Address hypercall ABI problems 2021-11-05 13:55 ` [PATCH 5/5] xen/xsm: Address hypercall ABI problems Andrew Cooper @ 2021-11-08 9:50 ` Jan Beulich 2021-11-17 23:14 ` Andrew Cooper 0 siblings, 1 reply; 21+ messages in thread From: Jan Beulich @ 2021-11-08 9:50 UTC (permalink / raw) To: Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 05.11.2021 14:55, Andrew Cooper wrote: > Currently, __HYPERVISOR_xsm_op enters xsm_ops.do_{xsm,compat}_op() which means > that if any other XSM module registers a handler, we'll break the hypercall > ABI totally by having the behaviour depend on the selected XSM module. > > There are 2 options: > 1) Rename __HYPERVISOR_xsm_op to __HYPERVISOR_flask_op. If another XSM > module wants hypercalls, they'll have to introduce a new top-level > hypercall. > 2) Make the current flask_op() be common, and require new hypercalls to fit > compatibly with xen_flask_op_t. This can be done fairly easily by > subdividing the .cmd space. > > In this patch, we implement option 2. > > Move the stub {do,compat}_xsm_op() implementation into a new xsm_op.c so we > can more easily do multi-inclusion compat magic. Also add a new private.h, > because flask/hook.c's local declaration of {do,compat}_flask_op() wasn't > remotely safe. > > The top level now dispatches into {do,compat}_flask_op() based on op.cmd, and > handles the primary copy in/out. I'm not convinced this is the only reasonable way of implementing 2). I could also see number space to be separated in different ways, ... > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> > CC: Daniel Smith <dpsmith@apertussolutions.com> > > Only lightly tested. Slightly RFC. There are several things which aren't > great, but probably want addressing in due course. > > 1) The public headers probably want to lose the flask name (in a compatible > way), now that the hypercall is common. This probably wants to be > combined with no longer taking a void handle. ... leaving per-module public headers and perhaps merely adding an abstracting struct xen_xsm_op_t { uint32_t op; ... /* placeholder */ }; or (making explicit one possible variant of number space splitting) union xen_xsm_op_t { uint32_t op; struct { uint16_t cmd; uint16_t mod; } u; ... /* placeholder */ }; in, say, a new public/xsm.h. As a result I consider this change either going too far (because of not knowing future needs) or not far enough (by e.g. leaving do_xsm_op() to use xen_flask_op_t. > 2) {do,compat}_xsm_op() are currently identical other than the dispatched-to > functions because the size of xsm_flask_op_t is invariant with > COMPAT-ness. We could simplfy things by only having one, and dispatching > to {do,compat}_*_op() directly, but I'm not sure whether the complexity is > worth it. Perhaps not, I would say, not the least because (as said above) I think we shouldn't put in place restrictions which may get in the way of adding some future module. Extending struct xen_flask_op to become a generic XSM interface structure (or even just for Flask's own purposes) also is not as straightforward as it might seem: There's currently no provision for sub-structs which would grow the overall size of the structure: The copy_{to,from}_guest() invocations for existing sub-ops may not copy more that the present worth of sizeof(struct xen_flask_op). Yet your implementation of do_xsm_op() moves this deficiency from Flask to XSM. > 3) Bloat-o-meter says these changes add 16 extra bytes to dm_op() and I can't > figure out what could possibly be causing this. Without comparing the object files in closer detail it's guesswork, but might this be register scheduling differences resulting from the changed sizeof(struct xsm_ops)? I've been observing similar seemingly unmotivated changes to generated code ... > --- a/xen/xsm/flask/flask_op.c > +++ b/xen/xsm/flask/flask_op.c > @@ -22,6 +22,8 @@ > #include <objsec.h> > #include <conditional.h> > > +#include "../private.h" Kind of odd: I'd expect a file named such to not get included across directory levels, unless a single component was split in such a way (to me Flask and XSM core are separate, yet still related components). Jan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] xen/xsm: Address hypercall ABI problems 2021-11-08 9:50 ` Jan Beulich @ 2021-11-17 23:14 ` Andrew Cooper 2021-11-17 23:20 ` Andrew Cooper 2021-11-18 12:59 ` Jan Beulich 0 siblings, 2 replies; 21+ messages in thread From: Andrew Cooper @ 2021-11-17 23:14 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 08/11/2021 09:50, Jan Beulich wrote: > On 05.11.2021 14:55, Andrew Cooper wrote: >> Currently, __HYPERVISOR_xsm_op enters xsm_ops.do_{xsm,compat}_op() which means >> that if any other XSM module registers a handler, we'll break the hypercall >> ABI totally by having the behaviour depend on the selected XSM module. >> >> There are 2 options: >> 1) Rename __HYPERVISOR_xsm_op to __HYPERVISOR_flask_op. If another XSM >> module wants hypercalls, they'll have to introduce a new top-level >> hypercall. >> 2) Make the current flask_op() be common, and require new hypercalls to fit >> compatibly with xen_flask_op_t. This can be done fairly easily by >> subdividing the .cmd space. >> >> In this patch, we implement option 2. >> >> Move the stub {do,compat}_xsm_op() implementation into a new xsm_op.c so we >> can more easily do multi-inclusion compat magic. Also add a new private.h, >> because flask/hook.c's local declaration of {do,compat}_flask_op() wasn't >> remotely safe. >> >> The top level now dispatches into {do,compat}_flask_op() based on op.cmd, and >> handles the primary copy in/out. > I'm not convinced this is the only reasonable way of implementing 2). > I could also see number space to be separated in different ways, ... > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> >> CC: Daniel Smith <dpsmith@apertussolutions.com> >> >> Only lightly tested. Slightly RFC. There are several things which aren't >> great, but probably want addressing in due course. >> >> 1) The public headers probably want to lose the flask name (in a compatible >> way), now that the hypercall is common. This probably wants to be >> combined with no longer taking a void handle. > ... leaving per-module public headers and perhaps merely adding an > abstracting > > struct xen_xsm_op_t { > uint32_t op; > ... /* placeholder */ > }; > > or (making explicit one possible variant of number space splitting) > > union xen_xsm_op_t { > uint32_t op; > struct { > uint16_t cmd; > uint16_t mod; > } u; > ... /* placeholder */ > }; > > in, say, a new public/xsm.h. That doesn't work. The ABI is fixed at sizeof(xen_flask_op_t) for all other modules, because a caller which doesn't know which module is in use must not have Xen over-read/write the object passed while it's trying to figure things out. > As a result I consider this change either going too far (because of > not knowing future needs) or not far enough (by e.g. leaving > do_xsm_op() to use xen_flask_op_t. Well - what it does is prevent someone from breaking the ABI with literally this patch diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c index 3550dded7b4e..36b82fd3bd3e 100644 --- a/xen/xsm/silo.c +++ b/xen/xsm/silo.c @@ -100,6 +100,11 @@ static int silo_argo_send(const struct domain *d1, const struct domain *d2) #endif +static long silo_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) +{ + /* ... */ +} + static const struct xsm_ops __initconstrel silo_xsm_ops = { .evtchn_unbound = silo_evtchn_unbound, .evtchn_interdomain = silo_evtchn_interdomain, @@ -110,6 +115,7 @@ static const struct xsm_ops __initconstrel silo_xsm_ops = { .argo_register_single_source = silo_argo_register_single_source, .argo_send = silo_argo_send, #endif + .do_xsm_op = silo_do_xsm_op, }; const struct xsm_ops *__init silo_init(void) This is a damage limitation patch, but without knowing how the next module is going to want to do hypercalls differently, it is rather hard to judge what is appropriate. I certainly didn't waste much time thinking about it. > >> 2) {do,compat}_xsm_op() are currently identical other than the dispatched-to >> functions because the size of xsm_flask_op_t is invariant with >> COMPAT-ness. We could simplfy things by only having one, and dispatching >> to {do,compat}_*_op() directly, but I'm not sure whether the complexity is >> worth it. > Perhaps not, I would say, not the least because (as said above) I > think we shouldn't put in place restrictions which may get in the > way of adding some future module. > > Extending struct xen_flask_op to become a generic XSM interface > structure (or even just for Flask's own purposes) also is not as > straightforward as it might seem: There's currently no provision > for sub-structs which would grow the overall size of the structure: > The copy_{to,from}_guest() invocations for existing sub-ops may not > copy more that the present worth of sizeof(struct xen_flask_op). > Yet your implementation of do_xsm_op() moves this deficiency from > Flask to XSM. Deficiency yes, but necessary to avoid breaking the ABI for caller which doesn't know which module is in use. This cannot be fixed without swapping to approach 1. >> 3) Bloat-o-meter says these changes add 16 extra bytes to dm_op() and I can't >> figure out what could possibly be causing this. > Without comparing the object files in closer detail it's guesswork, > but might this be register scheduling differences resulting from > the changed sizeof(struct xsm_ops)? I've been observing similar > seemingly unmotivated changes to generated code ... The only thing it can plausibly be is a knock-on effect from the structure of xsm_ops changing. Normally, I wouldn't be to surprised to see some displacement fields dropping from 4 to 1 byte as xsm_ops is getting smaller, but everything is alt_call()'d up so should be a 7-byte `call *disp32(%rip)`. >> --- a/xen/xsm/flask/flask_op.c >> +++ b/xen/xsm/flask/flask_op.c >> @@ -22,6 +22,8 @@ >> #include <objsec.h> >> #include <conditional.h> >> >> +#include "../private.h" > Kind of odd: I'd expect a file named such to not get included > across directory levels, unless a single component was split in > such a way (to me Flask and XSM core are separate, yet still > related components). Its all a tangled mess because logically separating XSM and Flask was a task done when SILO was introduced. There is not an appropriately located file (under xen/xsm/ ) where the prototypes could reasonably live, and this felt like the lesser of the available evils. ~Andrew ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] xen/xsm: Address hypercall ABI problems 2021-11-17 23:14 ` Andrew Cooper @ 2021-11-17 23:20 ` Andrew Cooper 2021-11-18 13:01 ` Jan Beulich 2021-11-18 12:59 ` Jan Beulich 1 sibling, 1 reply; 21+ messages in thread From: Andrew Cooper @ 2021-11-17 23:20 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 17/11/2021 23:14, Andrew Cooper wrote: > On 08/11/2021 09:50, Jan Beulich wrote: >> On 05.11.2021 14:55, Andrew Cooper wrote: >>> Currently, __HYPERVISOR_xsm_op enters xsm_ops.do_{xsm,compat}_op() >>> which means >>> that if any other XSM module registers a handler, we'll break the >>> hypercall >>> ABI totally by having the behaviour depend on the selected XSM module. >>> >>> There are 2 options: >>> 1) Rename __HYPERVISOR_xsm_op to __HYPERVISOR_flask_op. If >>> another XSM >>> module wants hypercalls, they'll have to introduce a new top-level >>> hypercall. >>> 2) Make the current flask_op() be common, and require new >>> hypercalls to fit >>> compatibly with xen_flask_op_t. This can be done fairly easily by >>> subdividing the .cmd space. >>> >>> In this patch, we implement option 2. >>> >>> Move the stub {do,compat}_xsm_op() implementation into a new >>> xsm_op.c so we >>> can more easily do multi-inclusion compat magic. Also add a new >>> private.h, >>> because flask/hook.c's local declaration of {do,compat}_flask_op() >>> wasn't >>> remotely safe. >>> >>> The top level now dispatches into {do,compat}_flask_op() based on >>> op.cmd, and >>> handles the primary copy in/out. >> I'm not convinced this is the only reasonable way of implementing 2). >> I could also see number space to be separated in different ways, ... >> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> >>> CC: Daniel Smith <dpsmith@apertussolutions.com> >>> >>> Only lightly tested. Slightly RFC. There are several things which >>> aren't >>> great, but probably want addressing in due course. >>> >>> 1) The public headers probably want to lose the flask name (in a >>> compatible >>> way), now that the hypercall is common. This probably wants to be >>> combined with no longer taking a void handle. >> ... leaving per-module public headers and perhaps merely adding an >> abstracting >> >> struct xen_xsm_op_t { >> uint32_t op; >> ... /* placeholder */ >> }; >> >> or (making explicit one possible variant of number space splitting) >> >> union xen_xsm_op_t { >> uint32_t op; >> struct { >> uint16_t cmd; >> uint16_t mod; >> } u; >> ... /* placeholder */ >> }; >> >> in, say, a new public/xsm.h. > > That doesn't work. The ABI is fixed at sizeof(xen_flask_op_t) for all > other modules, because a caller which doesn't know which module is in > use must not have Xen over-read/write the object passed while it's > trying to figure things out. > >> As a result I consider this change either going too far (because of >> not knowing future needs) or not far enough (by e.g. leaving >> do_xsm_op() to use xen_flask_op_t. > > Well - what it does is prevent someone from breaking the ABI with > literally this patch > > diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c > index 3550dded7b4e..36b82fd3bd3e 100644 > --- a/xen/xsm/silo.c > +++ b/xen/xsm/silo.c > @@ -100,6 +100,11 @@ static int silo_argo_send(const struct domain > *d1, const struct domain *d2) > > #endif > > +static long silo_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) > +{ > + /* ... */ > +} > + > static const struct xsm_ops __initconstrel silo_xsm_ops = { > .evtchn_unbound = silo_evtchn_unbound, > .evtchn_interdomain = silo_evtchn_interdomain, > @@ -110,6 +115,7 @@ static const struct xsm_ops __initconstrel > silo_xsm_ops = { > .argo_register_single_source = silo_argo_register_single_source, > .argo_send = silo_argo_send, > #endif > + .do_xsm_op = silo_do_xsm_op, > }; > > const struct xsm_ops *__init silo_init(void) > > > This is a damage limitation patch, but without knowing how the next > module is going to want to do hypercalls differently, it is rather > hard to judge what is appropriate. I certainly didn't waste much time > thinking about it. > >> >>> 2) {do,compat}_xsm_op() are currently identical other than the >>> dispatched-to >>> functions because the size of xsm_flask_op_t is invariant with >>> COMPAT-ness. We could simplfy things by only having one, and >>> dispatching >>> to {do,compat}_*_op() directly, but I'm not sure whether the >>> complexity is >>> worth it. >> Perhaps not, I would say, not the least because (as said above) I >> think we shouldn't put in place restrictions which may get in the >> way of adding some future module. >> >> Extending struct xen_flask_op to become a generic XSM interface >> structure (or even just for Flask's own purposes) also is not as >> straightforward as it might seem: There's currently no provision >> for sub-structs which would grow the overall size of the structure: >> The copy_{to,from}_guest() invocations for existing sub-ops may not >> copy more that the present worth of sizeof(struct xen_flask_op). >> Yet your implementation of do_xsm_op() moves this deficiency from >> Flask to XSM. > > Deficiency yes, but necessary to avoid breaking the ABI for caller > which doesn't know which module is in use. This cannot be fixed > without swapping to approach 1. > >>> 3) Bloat-o-meter says these changes add 16 extra bytes to dm_op() >>> and I can't >>> figure out what could possibly be causing this. >> Without comparing the object files in closer detail it's guesswork, >> but might this be register scheduling differences resulting from >> the changed sizeof(struct xsm_ops)? I've been observing similar >> seemingly unmotivated changes to generated code ... > > The only thing it can plausibly be is a knock-on effect from the > structure of xsm_ops changing. > > Normally, I wouldn't be to surprised to see some displacement fields > dropping from 4 to 1 byte as xsm_ops is getting smaller, but > everything is alt_call()'d up so should be a 7-byte `call *disp32(%rip)`. > >>> --- a/xen/xsm/flask/flask_op.c >>> +++ b/xen/xsm/flask/flask_op.c >>> @@ -22,6 +22,8 @@ >>> #include <objsec.h> >>> #include <conditional.h> >>> +#include "../private.h" >> Kind of odd: I'd expect a file named such to not get included >> across directory levels, unless a single component was split in >> such a way (to me Flask and XSM core are separate, yet still >> related components). > > Its all a tangled mess because logically separating XSM and Flask was > a task done when SILO was introduced. > > There is not an appropriately located file (under xen/xsm/ ) where the > prototypes could reasonably live, and this felt like the lesser of the > available evils. I guess it is worth adding, so we're all on the same page. The thing I actually need to do is fix the fact that the prototypes for {do,compat}_flask_op() are local in xen/xsm/flask/hooks.c and not in a header shared with xen/xsm/flask/flask_op.c. The thing I wanted to do is stop it being so trivially easy to break the ABI with a 5-line patch. Any bikeshedding beyond that can happen in due course. ~Andrew ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] xen/xsm: Address hypercall ABI problems 2021-11-17 23:20 ` Andrew Cooper @ 2021-11-18 13:01 ` Jan Beulich 0 siblings, 0 replies; 21+ messages in thread From: Jan Beulich @ 2021-11-18 13:01 UTC (permalink / raw) To: Andrew Cooper, Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 18.11.2021 00:20, Andrew Cooper wrote: > On 17/11/2021 23:14, Andrew Cooper wrote: >> On 08/11/2021 09:50, Jan Beulich wrote: >>> On 05.11.2021 14:55, Andrew Cooper wrote: >>>> --- a/xen/xsm/flask/flask_op.c >>>> +++ b/xen/xsm/flask/flask_op.c >>>> @@ -22,6 +22,8 @@ >>>> #include <objsec.h> >>>> #include <conditional.h> >>>> +#include "../private.h" >>> Kind of odd: I'd expect a file named such to not get included >>> across directory levels, unless a single component was split in >>> such a way (to me Flask and XSM core are separate, yet still >>> related components). >> >> Its all a tangled mess because logically separating XSM and Flask was >> a task done when SILO was introduced. >> >> There is not an appropriately located file (under xen/xsm/ ) where the >> prototypes could reasonably live, and this felt like the lesser of the >> available evils. > > I guess it is worth adding, so we're all on the same page. > > The thing I actually need to do is fix the fact that the prototypes for > {do,compat}_flask_op() are local in xen/xsm/flask/hooks.c and not in a > header shared with xen/xsm/flask/flask_op.c. And I fully agree we need to do so. Me saying "kind of odd" also wasn't meant as an objection, just to express my surprise to see something like this. Jan ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/5] xen/xsm: Address hypercall ABI problems 2021-11-17 23:14 ` Andrew Cooper 2021-11-17 23:20 ` Andrew Cooper @ 2021-11-18 12:59 ` Jan Beulich 1 sibling, 0 replies; 21+ messages in thread From: Jan Beulich @ 2021-11-18 12:59 UTC (permalink / raw) To: Andrew Cooper, Andrew Cooper; +Cc: Daniel De Graaf, Daniel Smith, Xen-devel On 18.11.2021 00:14, Andrew Cooper wrote: > On 08/11/2021 09:50, Jan Beulich wrote: >> On 05.11.2021 14:55, Andrew Cooper wrote: >>> Currently, __HYPERVISOR_xsm_op enters xsm_ops.do_{xsm,compat}_op() which means >>> that if any other XSM module registers a handler, we'll break the hypercall >>> ABI totally by having the behaviour depend on the selected XSM module. >>> >>> There are 2 options: >>> 1) Rename __HYPERVISOR_xsm_op to __HYPERVISOR_flask_op. If another XSM >>> module wants hypercalls, they'll have to introduce a new top-level >>> hypercall. >>> 2) Make the current flask_op() be common, and require new hypercalls to fit >>> compatibly with xen_flask_op_t. This can be done fairly easily by >>> subdividing the .cmd space. >>> >>> In this patch, we implement option 2. >>> >>> Move the stub {do,compat}_xsm_op() implementation into a new xsm_op.c so we >>> can more easily do multi-inclusion compat magic. Also add a new private.h, >>> because flask/hook.c's local declaration of {do,compat}_flask_op() wasn't >>> remotely safe. >>> >>> The top level now dispatches into {do,compat}_flask_op() based on op.cmd, and >>> handles the primary copy in/out. >> I'm not convinced this is the only reasonable way of implementing 2). >> I could also see number space to be separated in different ways, ... >> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov> >>> CC: Daniel Smith <dpsmith@apertussolutions.com> >>> >>> Only lightly tested. Slightly RFC. There are several things which aren't >>> great, but probably want addressing in due course. >>> >>> 1) The public headers probably want to lose the flask name (in a compatible >>> way), now that the hypercall is common. This probably wants to be >>> combined with no longer taking a void handle. >> ... leaving per-module public headers and perhaps merely adding an >> abstracting >> >> struct xen_xsm_op_t { >> uint32_t op; >> ... /* placeholder */ >> }; >> >> or (making explicit one possible variant of number space splitting) >> >> union xen_xsm_op_t { >> uint32_t op; >> struct { >> uint16_t cmd; >> uint16_t mod; >> } u; >> ... /* placeholder */ >> }; >> >> in, say, a new public/xsm.h. > > That doesn't work. The ABI is fixed at sizeof(xen_flask_op_t) for all > other modules, because a caller which doesn't know which module is in > use must not have Xen over-read/write the object passed while it's > trying to figure things out. Well, imo figuring out which module is in use should be via a sysctl. Then there would be no restrictions by one module's interface definitions on other modules. >> As a result I consider this change either going too far (because of >> not knowing future needs) or not far enough (by e.g. leaving >> do_xsm_op() to use xen_flask_op_t. > > Well - what it does is prevent someone from breaking the ABI with > literally this patch > > diff --git a/xen/xsm/silo.c b/xen/xsm/silo.c > index 3550dded7b4e..36b82fd3bd3e 100644 > --- a/xen/xsm/silo.c > +++ b/xen/xsm/silo.c > @@ -100,6 +100,11 @@ static int silo_argo_send(const struct domain *d1, > const struct domain *d2) > > #endif > > +static long silo_do_xsm_op(XEN_GUEST_HANDLE_PARAM(void) op) > +{ > + /* ... */ > +} > + > static const struct xsm_ops __initconstrel silo_xsm_ops = { > .evtchn_unbound = silo_evtchn_unbound, > .evtchn_interdomain = silo_evtchn_interdomain, > @@ -110,6 +115,7 @@ static const struct xsm_ops __initconstrel > silo_xsm_ops = { > .argo_register_single_source = silo_argo_register_single_source, > .argo_send = silo_argo_send, > #endif > + .do_xsm_op = silo_do_xsm_op, > }; > > const struct xsm_ops *__init silo_init(void) So I'm afraid I don't see any ABI breakage here. Provided of course silo_do_xsm_op() avoids the FLASK_* number space and uses a struct layout compatible with the head of struct xen_flask_op. Jan ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-11-18 13:01 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-05 13:55 [PATCH 0/5] XSM: cleanups Andrew Cooper 2021-11-05 13:55 ` [PATCH 1/5] x86/altcall: allow compound types to be passed Andrew Cooper 2021-11-05 14:09 ` Daniel P. Smith 2021-11-05 13:55 ` [PATCH 2/5] xen/xsm: Complete altcall conversion of xsm interface Andrew Cooper 2021-11-05 14:00 ` Jan Beulich 2021-11-05 14:11 ` Daniel P. Smith 2021-11-08 9:11 ` Jan Beulich 2021-11-17 22:22 ` Andrew Cooper 2021-11-05 13:55 ` [PATCH 3/5] xen/xsm: Drop xsm_hvm_control() hook Andrew Cooper 2021-11-05 14:20 ` Daniel P. Smith 2021-11-05 13:55 ` [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops() Andrew Cooper 2021-11-05 14:22 ` Daniel P. Smith 2021-11-08 9:04 ` Jan Beulich 2021-11-17 22:37 ` Andrew Cooper 2021-11-18 11:59 ` Jan Beulich 2021-11-05 13:55 ` [PATCH 5/5] xen/xsm: Address hypercall ABI problems Andrew Cooper 2021-11-08 9:50 ` Jan Beulich 2021-11-17 23:14 ` Andrew Cooper 2021-11-17 23:20 ` Andrew Cooper 2021-11-18 13:01 ` Jan Beulich 2021-11-18 12:59 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).