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