Re: [XEN PATCH v2] xen/hypercalls: address violations of MISRA C:2012 Rule 8.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.08.2023 09:28, Andrew Cooper wrote:
> On 23/08/2023 8:04 am, Federico Serafini wrote:
>> Make function declarations and definitions consistent to address
>> violations of MISRA C:2012 Rule 8.3 ("All declarations of an object or
>> function shall use the same names and type qualifiers").
>>
>> No functional change.
>>
>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>> ---
>> Changes in v2:
>> - change compat_grant_table_op() definition instead of the declaration;
>> - use unsigned int for multicall()'s parameter in accordance with XEN coding
>>   style.
> 
> Nack.

Oh, I'm sorry, I committed this just before seeing your reply. I'll
revert right away, until we can settle the discussion.

> You were correct in v1, and the request to change it was erroneous.
> 
>> diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
>> index 6d361ddfce..d1892fd14f 100644
>> --- a/xen/include/hypercall-defs.c
>> +++ b/xen/include/hypercall-defs.c
>> @@ -135,8 +135,8 @@ xenoprof_op(int op, void *arg)
>>  #ifdef CONFIG_COMPAT
>>  prefix: compat
>>  set_timer_op(uint32_t lo, int32_t hi)
>> -multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls)
>> -memory_op(unsigned int cmd, void *arg)
>> +multicall(multicall_entry_compat_t *call_list, unsigned int nr_calls)
> 
> This, unfortunately for many reasons, is an ABI with guests.
> 
> It is buggy that the entire file doesn't use unsigned long (i.e. one
> full GPR width) to begin with.  It these types alone which cause
> explicit truncation of the guest-provided hypercall parameter values.
> 
> But it is even more buggy to take something that at least truncates to a
> fixed width, and replace it with something that explicitly does not have
> a fixed width.

I disagree on all the points you make. Handling compat guests is quite
fine to use unsigned int in hypercall prototypes. Fixed width isn't
needed (just like you don't demand uint64_t, but suggest unsigned long),
and wider than 32 bits (i.e. long) isn't needed either because can't
pass in wider values anyway.

Jan