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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 23 Aug 2023, Jan Beulich wrote:
> 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.

There are two points that Andrew made:
1) uint32_t should have been register-width (AKA register_t on ARM)
2) unsigned int is worse than uint32_t because is not fixed-width

As a general rule I would say that it is better not to introduce any ABI
changes as part of a clean-up patch for MISRA. (ABI changes could be
done but separately.)

In regard to 1) I don't think it is a good idea to change nr_calls to a
type of different size. Even if it is the right thing to do. If it is,
let's discuss it as a separate patch: you wouldn't want this type of
change to be hidden inside a innocuous MISRA C patch anyway.

In regard to 2), assuming "unsigned int" is exactly equivalent to
uint32_t on all supported arches, then I am in two minds here. Just one
year ago I would have agreed with Andrew and strongly requested an
explicitly-sized type as argument. Then I discovered that all the RISC-V
APIs are based on long, int and similar, and it seems to be working
quite well for them.  See for instance:
https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/riscv-sbi.pdf

Looking at Xen public headers, we have many instances of unsigned long
and unsigned int in use. So now I think both explicitly-sized and
non-explicitly-sized types can work well as long as we are clear on what
they mean, their sizes on all supported arches, and we use them
consistently. (For sure all of the above could be improved in Xen.)

Coming to unsigned int, it should be 32-bit on all supported arches, so
it is down to consistency, which is a bit arbitrary. We have quite a
good mix of unsigned int and uint32_t in hypercall-defs.c, specifically:
10 uint32_t
32 unsigned int

By popular vote, I would go with unsigned int. So, I would keep the
patch as is.