Re: [PATCH 03/12] xen: harmonize return types of hypercall handlers

[Juergen Gross <jgross@xxxxxxxx>]

On 18.10.21 13:55, Jan Beulich wrote:
> On 15.10.2021 14:51, Juergen Gross wrote:
> > Today most hypercall handlers have a return type of long, while the
> > compat ones return an int. There are a few exceptions from that rule,
> > however.
> >
> > Get rid of the exceptions by letting compat handlers always return int
> > and others always return long.
> >
> > For the compat hvm case use eax instead of rax for the stored result as
> > it should have been from the beginning.
> >
> > Additionally move some prototypes to include/asm-x86/hypercall.h
> > as they are x86 specific. Move the do_physdev_op() prototype from both
> > architecture dependant headers to the common one. Move the
> > compat_platform_op() prototype to the common header.
> >
> > Switch some non style compliant types (u32, s32, s64) to style compliant
> > ones.
> >
> > Rename paging_domctl_continuation() to do_paging_domctl_cont() and add
> > a matching define for the associated hypercall.
> >
> > Make do_callback_op() and compat_callback_op() more similar by adding
> > the const attribute to compat_callback_op()'s 2nd parameter.
> >
> > The do_platform_op() prototype needs to be modified in order to better
> > match its compat variant.
> "Better" in what direction? So far both have been using typed handles,
> which I consider better than void ones. You also don't seem to have
> had a reason to switch e.g. multicall or dm_op, where (different)
> typed handles are also in use. So I wonder what the reason for this
> change is.
I had some problems making this build. Before my patches
platform_hypercall.c didn't include the header with the prototype,
so there was no mismatch detected.

Thanks for the pointers above. dm_op is no good example, as the
compat variant is explicitly a different implementation compared
to the normal one. But with the multicall example I can have
another try converting platform_op to a type safe variant using
non-void handles.

> > Change the type of the cmd parameter for [do|compat]_kexec_op() to
> > unsigned int, as this is more appropriate for the compat case.
> The change for the compat case is fine, but for native you change
> behavior for callers passing values equaling valid KEXEC_CMD_*
> modulo 2³².
TBH, I don't think this is really a problem. Or do you think there
really is a user of this interface relying on a -ENOSYS in this
case?

> > --- a/xen/arch/x86/pv/misc-hypercalls.c
> > +++ b/xen/arch/x86/pv/misc-hypercalls.c
> > @@ -28,12 +28,16 @@ long do_set_debugreg(int reg, unsigned long value)
> >       return set_debugreg(current, reg, value);
> >   }
> >   
> > -unsigned long do_get_debugreg(int reg)
> > +long do_get_debugreg(int reg)
> >   {
> > -    unsigned long val;
> > -    int res = x86emul_read_dr(reg, &val, NULL);
> > -
> > -    return res == X86EMUL_OKAY ? val : -ENODEV;
> > +    /* Avoid undefined behavior due to casting an unsigned long to long. */
> Nit: unsigned -> signed conversion is implementation-defined, not
> undefined.
Okay, will change the comment.

> > --- a/xen/common/argo.c
> > +++ b/xen/common/argo.c
> > @@ -2207,13 +2207,13 @@ do_argo_op(unsigned int cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg1,
> >   }
> >   
> >   #ifdef CONFIG_COMPAT
> > -long
> > -compat_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> > -               XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> > -               unsigned long arg4)
> > +int compat_argo_op(unsigned int cmd,
> > +                   XEN_GUEST_HANDLE_PARAM(void) arg1,
> > +                   XEN_GUEST_HANDLE_PARAM(void) arg2,
> > +                   unsigned long arg3, unsigned long arg4)
> Strictly speaking arg3 and arg4 also ought to be unsigned int here.
> But that's perhaps for a separate patch at another time.
I'd rather leave it as is, as this way I can use the same definition for
both cases in patch 6. And I don't see how anything could go wrong this
way, as expanding a 32-bit unsigned value to 64 bits is in no way
ambiguous.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature