Re: [PATCH 04/12] xen/x86: modify hvm_memory_op() prototype

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.10.2021 15:27, Juergen Gross wrote:
> On 18.10.21 14:31, Jan Beulich wrote:
>> On 15.10.2021 14:51, Juergen Gross wrote:
>>> hvm_memory_op() should take an unsigned long as cmd, like
>>> do_memory_op().
>>>
>>> As hvm_memory_op() is basically just calling do_memory_op() (or
>>> compat_memory_op()) passing through the parameters the cmd parameter
>>> should have no smaller size than that of the called functions.
>>>
>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> Nevertheless ...
>>
>>> --- a/xen/arch/x86/hvm/hypercall.c
>>> +++ b/xen/arch/x86/hvm/hypercall.c
>>> @@ -31,7 +31,7 @@
>>>   #include <public/hvm/hvm_op.h>
>>>   #include <public/hvm/params.h>
>>>   
>>> -static long hvm_memory_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>> +static long hvm_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) 
>>> arg)
>>>   {
>>>       long rc;
>>
>> ... I think this would even better be dealt with by splitting the
>> function into a native one (using unsigned long) and a compat one
>> (using unsigned int).
> 
> Why? In 32-bit case the value is naturally limited to 32 bits width
> zero-extending perfectly fine to unsigned long.

It all ends up working fine, yes. Else I wouldn't have given R-b.
But the .compat slot of the hypercall table really should use a
prototype without unsigned long, and then the calls wouldn't
zero-extend the arguments anymore. And then the declaration would
be wrong, as then it would need to be the callee to zero-extend if
it wants to use 64-bit values.

> Otherwise I couldn't use the same definition later.

Right. And this will be less of a problem once the function pointer
tables are gone, as then the compiler sees the real parameter types
for the individual functions.

Jan