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.coop...@citrix.com>
>>> ---
>>> CC: Daniel De Graaf <dgde...@tycho.nsa.gov>
>>> CC: Daniel Smith <dpsm...@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
