Re: [PATCH] do_multicall and MISRA Rule 8.3

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Sat, Mar 9, 2024 at 1:59 AM Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
>
> I would like to resurrect this thread and ask other opinions.
>
>
> On Thu, 23 Nov 2023, Jan Beulich wrote:
> > On 22.11.2023 22:46, Stefano Stabellini wrote:
> > > Two out of three do_multicall definitions/declarations use uint32_t as
> > > type for the "nr_calls" parameters. Change the third one to be
> > > consistent with the other two.
> > >
> > > Link: 
> > > https://lore.kernel.org/xen-devel/7e3abd4c0ef5127a07a60de1bf090a8aefac8e5c.1692717906.git.federico.serafini@xxxxxxxxxxx/
> > > Link: 
> > > https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > > ---
> > > Note that a previous discussion showed disagreement between maintainers
> > > on this topic. The source of disagreements are that we don't want to
> > > change a guest-visible ABI and we haven't properly documented how to use
> > > types for guest ABIs.
> > >
> > > As an example, fixed-width types have the advantage of being explicit
> > > about their size but sometimes register-size types are required (e.g.
> > > unsigned long). The C specification says little about the size of
> > > unsigned long and today, and we even use unsigned int in guest ABIs
> > > without specifying the expected width of unsigned int on the various
> > > arches. As Jan pointed out, in Xen we assume sizeof(int) >= 4, but
> > > that's not written anywhere as far as I can tell.
> > >
> > > I think the appropriate solution would be to document properly our
> > > expectations of both fixed-width and non-fixed-width types, and how to
> > > use them for guest-visible ABIs.
> > >
> > > In this patch I used uint32_t for a couple of reasons:
> > > - until we have better documentation, I feel more confident in using
> > >   explicitly-sized integers in guest-visible ABIs
> >
> > I disagree with this way of looking at it. Guests don't invoke these
> > functions directly, and our assembly code sitting in between already is
> > expected to (and does) guarantee that (in the case here) unsigned int
> > would be okay to use (as would be unsigned long, but at least on x86
> > that's slightly less efficient), in line with what ./CODING_STYLE says.
> >
> > Otoh structure definitions in the public interface of course need to
> > use fixed with types (and still doesn't properly do so in a few cases).

You didn't address the other argument, which was that all the other
definitions have uint32_t; in particular,
common/multicall.c:do_multicall() takes uint32_t.  Surely that should
match the non-compat definition in include/hypercall-defs.c?

Whether they should both be `unsigned int` or `uint32_t` I don't
really feel like I have a good enough grasp of the situation to form a
strong opinion.

 -George