Re: [PATCH] do_multicall and MISRA Rule 8.3\

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2024 11:59, George Dunlap wrote:
> 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.

Hmm. "Would be good" doesn't sound like a strict requirement to me.
Don't forget that there was over 8 months between submission and your
ack. Anyone caring to object had their chance. I also didn't commit
this the same day you gave your ack.

> 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.

As with any not sufficiently clear statement, that's certainly true here,
too. Yet if we try to give as wide meaning as possible to "a fixed width
quantity is meant", there's basically no restriction on use of fixed width
types because everyone can just say "but I mean a fixed width quantity
here". I think the earlier sentence needs taking with higher priority,
i.e. if a basic type does for the purpose, that's what should be used. The
2nd sentence then only tries to further clarify what the 1st means.

Jan