Re: [PATCH] do_multicall and MISRA Rule 8.3\

[Jan Beulich <jbeulich@xxxxxxxx>]

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? Looking at the commit, it was actually George who ack-ed
it. I can accept option 1 if ./CODING_STYLE is first changed / amended.

Jan