Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Jan,

It is challenging to create a solution that satisfies everyone for this
patch. However, we should add R8.3 to the clean list as soon as possible
to enable rule blocking in GitLab-CI. Failing to do so risks introducing
regressions, as recently occurred, undoing the significant efforts made
by Bugseng and the community over the past year.

Unless there is a specific counterproposal, let us proceed with
committing this patch.

Cheers,
Stefano

On Mon, 24 Jun 2024, Jan Beulich wrote:
> On 21.06.2024 22:58, Andrew Cooper wrote:
> > Right now, the non-compat declaration and definition of do_multicall()
> > differing types for the nr_calls parameter.
> > 
> > This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
> > first 128bit architecture (RISC-V looks as if it might get there first).
> > 
> > Worse, the type chosen here has a side effect of truncating the guest
> > parameter, because Xen still doesn't have a clean hypercall ABI definition.
> > 
> > Switch uniformly to using unsigned long.
> 
> And re-raising all the same question again: Why not uniformly unsigned int?
> Or uint32_t?
> 
> > This addresses the MISRA violation, and while it is a guest-visible ABI
> > change, it's only in the corner case where the guest kernel passed a
> > bogus-but-correct-when-truncated value.  I can't find any any users of
> > mutilcall which pass a bad size to begin with, so this should have no
> > practical effect on guests.
> > 
> > In fact, this brings the behaviour of multicalls more in line with the 
> > header
> > description of how it behaves.
> 
> Which description? If you mean ...
> 
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -623,7 +623,7 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
> >  /*
> >   * ` enum neg_errnoval
> >   * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
> > - * `                      uint32_t nr_calls);
> > + * `                      unsigned long nr_calls);
> >   *
> >   * NB. The fields are logically the natural register size for this
> >   * architecture. In cases where xen_ulong_t is larger than this then
> 
> ... this comment here, note how is says "fields", i.e. talks about the
> subsequent struct.
> 
> What you're doing is effectively an ABI change: All of the sudden the
> upper bits of the incoming argument would be respected. Yes, it is
> overwhelmingly likely that no-one would ever pass such a value. Yet
> iirc on other similar hypercall handler adjustments in the past you
> did raise a similar concern.
> 
> Jan
>