Re: [PATCH] do_multicall and MISRA Rule 8.3\

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Fri, Mar 15, 2024 at 6:54 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 15.03.2024 01:21, Stefano Stabellini wrote:
> > On Mon, 11 Mar 2024, Julien Grall wrote:
> >> On 11/03/2024 11:32, George Dunlap wrote:
> >>> 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.
> >>
> >> FWIW +1. We at least need some consistency.
> >
> > Consistency is my top concern. Let's put the "unsigned int" vs
> > "uint32_t" argument aside.
> >
> > do_multicall is not consistent with itself. We need
> > hypercall-defs.c:do_multicall and multicall.c:do_multicall to match.
> >
> > Option1) We can change hypercall-defs.c:do_multicall to use uint32_t.
> >
> > Option2) Or we can change multicall.c:do_multicall to use unsigned int.
> >
> > I went with Option1. Andrew expressed his strong preference toward
> > Option1 in the past. George seems to prefer Option1.
> >
> > Jan, can you accept Option1 and move on?
>
> Counter question: Why do we have the opposite of what you all want stated
> in ./CODING_STYLE?

Indeed, that's what I wanted to ask at the committer's meeting on
Wednesday, but we ran out of time.

> Looking at the commit, it was actually George who ack-ed
> it. I can accept option 1 if ./CODING_STYLE is first changed / amended.

That change was added in 2019, but I certainly remember discussions
along these lines going on long before then.  Presumably there was a
long unwritten tradition of avoiding explicitly-sized types unless
necessary, and someone said, "that's not in the CODING_STYLE", and so
you added it.  Having the expectation written down is certainly worth
having, even if I don't personally care that much about it.

I will note that when I gave my Ack, I said that it probably wanted an
Ack from then ARM maintainers as well [1]; that doesn't seem to have
happened, so there's an argument that it was checked in improperly.

The coding style says, "Fixed width types should only be used when a
fixed width quantity is meant".  In the discussion on v2 of the patch,
I went through some uses of uint32_t, and regarding instances "Inside
headers for public interfaces", you said [2]:

> Here fixed width types are definitely the right choice.

It sounds like Andy and Stefano feel like this is a situation where "a
fixed width quantity is meant"; absent any further guidance from the
CODING_STYLE about when fixed widths should or should not be used, I
don't think this change would be a violation of CODING_STYLE.

 -George

[1] 
https://lore.kernel.org/xen-devel/0a8031c0-b668-eeb1-a9a2-659b52aaf98d@xxxxxxxxxx/
[2] 
https://lore.kernel.org/xen-devel/72580391-d34e-aaf9-2e41-ab1df5967408@xxxxxxxx/