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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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³².

> --- 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.

> --- 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.

Jan