do_multicall and MISRA Rule 8.3

Commit Message

Stefano Stabellini Nov. 22, 2023, 9:46 p.m. UTC

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/7e3abd4c0[e-mail]/
Link: https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
Signed-off-by: Stefano Stabellini <[e-mail]>
---
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
- 2/3 of the do_multicall definition/declaration are using uint32_t

Comments

Jan Beulich Nov. 23, 2023, 8:30 a.m. UTC | #1

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/7e3abd4c0[e-mail]/
> Link: https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
> Signed-off-by: Stefano Stabellini <[e-mail]>
> ---
> 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).

Jan

Stefano Stabellini March 9, 2024, 1:59 a.m. UTC | #2

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/7e3abd4c0[e-mail]/
> > Link: https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
> > Signed-off-by: Stefano Stabellini <[e-mail]>
> > ---
> > 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).
> 
> Jan
>

George Dunlap March 11, 2024, 11:32 a.m. UTC | #3

On Sat, Mar 9, 2024 at 1:59 AM Stefano Stabellini
<[e-mail]> 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/7e3abd4c0[e-mail]/
> > > Link: https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
> > > Signed-off-by: Stefano Stabellini <[e-mail]>
> > > ---
> > > 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

Julien Grall March 11, 2024, 11:50 a.m. UTC | #4

Hi,

On 11/03/2024 11:32, George Dunlap wrote:
> On Sat, Mar 9, 2024 at 1:59 AM Stefano Stabellini
> <[e-mail]> 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/7e3abd4c0[e-mail]/
>>>> Link: https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
>>>> Signed-off-by: Stefano Stabellini <[e-mail]>
>>>> ---
>>>> 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.

Cheers,

Stefano Stabellini March 15, 2024, 12:21 a.m. UTC | #5

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
> > <[e-mail]> 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/7e3abd4c0[e-mail]/
> > > > > Link:
> > > > > https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
> > > > > Signed-off-by: Stefano Stabellini <[e-mail]>
> > > > > ---
> > > > > 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?

Jan Beulich March 15, 2024, 6:54 a.m. UTC | #6

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
>>> <[e-mail]> 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/7e3abd4c0[e-mail]/
>>>>>> Link:
>>>>>> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
>>>>>> Signed-off-by: Stefano Stabellini <[e-mail]>
>>>>>> ---
>>>>>> 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

George Dunlap March 15, 2024, 10:59 a.m. UTC | #7

On Fri, Mar 15, 2024 at 6:54 AM Jan Beulich <[e-mail]> 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
> >>> <[e-mail]> 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/7e3abd4c0[e-mail]/
> >>>>>> Link:
> >>>>>> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
> >>>>>> Signed-off-by: Stefano Stabellini <[e-mail]>
> >>>>>> ---
> >>>>>> 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/[e-mail]/
[2] https://lore.kernel.org/xen-devel/[e-mail]/

Jan Beulich March 15, 2024, 11:57 a.m. UTC | #8

On 15.03.2024 11:59, George Dunlap wrote:
> On Fri, Mar 15, 2024 at 6:54 AM Jan Beulich <[e-mail]> 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
>>>>> <[e-mail]> 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/7e3abd4c0[e-mail]/
>>>>>>>> Link:
>>>>>>>> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308251502430.6458@ubuntu-linux-20-04-desktop/
>>>>>>>> Signed-off-by: Stefano Stabellini <[e-mail]>
>>>>>>>> ---
>>>>>>>> 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

George Dunlap March 15, 2024, 12:17 p.m. UTC | #9

On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <[e-mail]> wrote:
> > 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.

Come, now.  There are lots of situations where we just need some sort
of number, and there's no real need to worry about the exact size.
There are other situations, where we mean "whatever covers the whole
address space" or the like, where it makes sense to have something
like "unsigned long", which changes size, but in predictable and
useful ways.  There are other situations, like when talking over an
API to code which may be compiled by a different compiler, or may be
running in a different processor mode, where we want to be more
specific, and set an exact number of bits.

Should we use uint32_t for random loop variables?  Pretty clearly
"No".  Should we use uint32_t for the C entry of a hypercall, even
though the assembly code allegedly makes that unnecessary?  At least
two core maintainers think "maybe just to be safe".  That's hardly a
slippery slope of "anyone can say anything".

Other than "it's in CODING_STYLE", and "it's not really necessary
because it's ensured in the assembly code", you haven't advanced a
single reason why "uint32_t" is problematic.

 -George

Jan Beulich March 15, 2024, 1:24 p.m. UTC | #10

On 15.03.2024 13:17, George Dunlap wrote:
> On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <[e-mail]> wrote:
>>> 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.
> 
> Come, now.  There are lots of situations where we just need some sort
> of number, and there's no real need to worry about the exact size.
> There are other situations, where we mean "whatever covers the whole
> address space" or the like, where it makes sense to have something
> like "unsigned long", which changes size, but in predictable and
> useful ways.  There are other situations, like when talking over an
> API to code which may be compiled by a different compiler, or may be
> running in a different processor mode, where we want to be more
> specific, and set an exact number of bits.
> 
> Should we use uint32_t for random loop variables?  Pretty clearly
> "No".  Should we use uint32_t for the C entry of a hypercall, even
> though the assembly code allegedly makes that unnecessary?  At least
> two core maintainers think "maybe just to be safe".  That's hardly a
> slippery slope of "anyone can say anything".
> 
> Other than "it's in CODING_STYLE", and "it's not really necessary
> because it's ensured in the assembly code", you haven't advanced a
> single reason why "uint32_t" is problematic.

And it isn't, I never said it would be. But if we set rules for
ourselves, why would we take the first opportunity to not respect them?

Jan

Julien Grall March 15, 2024, 1:55 p.m. UTC | #11

Hi Jan,

On 15/03/2024 13:24, Jan Beulich wrote:
> On 15.03.2024 13:17, George Dunlap wrote:
>> On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <[e-mail]> wrote:
>>>> 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.
>>
>> Come, now.  There are lots of situations where we just need some sort
>> of number, and there's no real need to worry about the exact size.
>> There are other situations, where we mean "whatever covers the whole
>> address space" or the like, where it makes sense to have something
>> like "unsigned long", which changes size, but in predictable and
>> useful ways.  There are other situations, like when talking over an
>> API to code which may be compiled by a different compiler, or may be
>> running in a different processor mode, where we want to be more
>> specific, and set an exact number of bits.
>>
>> Should we use uint32_t for random loop variables?  Pretty clearly
>> "No".  Should we use uint32_t for the C entry of a hypercall, even
>> though the assembly code allegedly makes that unnecessary?  At least
>> two core maintainers think "maybe just to be safe".  That's hardly a
>> slippery slope of "anyone can say anything".
>>
>> Other than "it's in CODING_STYLE", and "it's not really necessary
>> because it's ensured in the assembly code", you haven't advanced a
>> single reason why "uint32_t" is problematic.
> 
> And it isn't, I never said it would be. But if we set rules for
> ourselves, why would we take the first opportunity to not respect them?

I am a bit confused. Reading through the thread you seem to agree that
the written rules are respected here. So what rules are you talking about?

Or are we just disagreeing on the interpretation? In which case, then 
that's a call for clarification of CODING_STYLE.

Cheers,

Jan Beulich March 15, 2024, 2:13 p.m. UTC | #12

On 15.03.2024 14:55, Julien Grall wrote:
> Hi Jan,
> 
> On 15/03/2024 13:24, Jan Beulich wrote:
>> On 15.03.2024 13:17, George Dunlap wrote:
>>> On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <[e-mail]> wrote:
>>>>> 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.
>>>
>>> Come, now.  There are lots of situations where we just need some sort
>>> of number, and there's no real need to worry about the exact size.
>>> There are other situations, where we mean "whatever covers the whole
>>> address space" or the like, where it makes sense to have something
>>> like "unsigned long", which changes size, but in predictable and
>>> useful ways.  There are other situations, like when talking over an
>>> API to code which may be compiled by a different compiler, or may be
>>> running in a different processor mode, where we want to be more
>>> specific, and set an exact number of bits.
>>>
>>> Should we use uint32_t for random loop variables?  Pretty clearly
>>> "No".  Should we use uint32_t for the C entry of a hypercall, even
>>> though the assembly code allegedly makes that unnecessary?  At least
>>> two core maintainers think "maybe just to be safe".  That's hardly a
>>> slippery slope of "anyone can say anything".
>>>
>>> Other than "it's in CODING_STYLE", and "it's not really necessary
>>> because it's ensured in the assembly code", you haven't advanced a
>>> single reason why "uint32_t" is problematic.
>>
>> And it isn't, I never said it would be. But if we set rules for
>> ourselves, why would we take the first opportunity to not respect them?
> 
> I am a bit confused. Reading through the thread you seem to agree that
> the written rules are respected here. So what rules are you talking about?

What was proposed is use of a fixed width type where according to my
reading ./CODING_STYLE says it shouldn't be used.

Jan

George Dunlap March 15, 2024, 2:45 p.m. UTC | #13

On Fri, Mar 15, 2024 at 2:13 PM Jan Beulich <[e-mail]> wrote:
>
> On 15.03.2024 14:55, Julien Grall wrote:
> > Hi Jan,
> >
> > On 15/03/2024 13:24, Jan Beulich wrote:
> >> On 15.03.2024 13:17, George Dunlap wrote:
> >>> On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <[e-mail]> wrote:
> >>>>> 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.
[snip]
> >>> Other than "it's in CODING_STYLE", and "it's not really necessary
> >>> because it's ensured in the assembly code", you haven't advanced a
> >>> single reason why "uint32_t" is problematic.
> >>
> >> And it isn't, I never said it would be. But if we set rules for
> >> ourselves, why would we take the first opportunity to not respect them?
> >
> > I am a bit confused. Reading through the thread you seem to agree that
> > the written rules are respected here. So what rules are you talking about?
>
> What was proposed is use of a fixed width type where according to my
> reading ./CODING_STYLE says it shouldn't be used.

This conversation is starting to get frustrating.  That's simply not
what it says, and I pointed that out just a few messages ago.

To reiterate:The text says fixed-width types are OK when a fixed-width
quantity is "meant"; and that in this case, Stefano and Andy "mean" to
use a fixed-width quantity.  The implied subtext of that sentence
could be, "Don't use fixed width types unless there's a good reason to
use a fixed width", and both Andy and Stefano think there's a good
reason.  This argument you haven't really addressed at all, except
with a specious "slippery slope" argument meant to nullify the
exception; and now you attempt to simply ignore.

I venture to assert that for most people, the rules are a means to an
end: That end being code which is correct, robust, fast, easy to
write, maintain, debug, and review patches for.  What I agreed to,
when I accepted this patch, was that *in general* we would avoid using
fixed-width types; but that there were cases where doing so made
sense.  Some of those were discussed in the thread above.

Andy and Stefano have already put forward reasons why they think a
fixed-width type would be better here, which are related to "end
goals": namely, more robust and easy to maintain code.  When I asked
what "end goals" would be negatively impacted by using a fixed-width
type, you explicitly said that there were none!  That the *only*
reason you're continuing to argue is that we have a document somewhere
that says we should -- a document which explicitly acknowledges that
there will be exceptions!

The ideal response would have been to lay out a reasonably
comprehensive set of criteria for when to use basic types, when to use
fixed-width types, and how each criteria advanced the end goals of a
better codebase.  A sufficient response would have been to lay out
reasons why "better codebase", in this instance, tilts towards using a
basic type rather than a fixed-width type.

Saying "That's what the rules say", without motivating it by
explaining how it connects to a better codebase, is just not a helpful
response; and making that argument after it's been pointed out that
that is *not* what the rules say is even worse.

 -George

Stefano Stabellini March 15, 2024, 11:58 p.m. UTC | #14

On Fri, 15 Mar 2024, George Dunlap wrote:
> On Fri, Mar 15, 2024 at 2:13 PM Jan Beulich <[e-mail]> wrote:
> >
> > On 15.03.2024 14:55, Julien Grall wrote:
> > > Hi Jan,
> > >
> > > On 15/03/2024 13:24, Jan Beulich wrote:
> > >> On 15.03.2024 13:17, George Dunlap wrote:
> > >>> On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <[e-mail]> wrote:
> > >>>>> 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.
> [snip]
> > >>> Other than "it's in CODING_STYLE", and "it's not really necessary
> > >>> because it's ensured in the assembly code", you haven't advanced a
> > >>> single reason why "uint32_t" is problematic.
> > >>
> > >> And it isn't, I never said it would be. But if we set rules for
> > >> ourselves, why would we take the first opportunity to not respect them?
> > >
> > > I am a bit confused. Reading through the thread you seem to agree that
> > > the written rules are respected here. So what rules are you talking about?
> >
> > What was proposed is use of a fixed width type where according to my
> > reading ./CODING_STYLE says it shouldn't be used.
> 
> This conversation is starting to get frustrating.  That's simply not
> what it says, and I pointed that out just a few messages ago.
> 
> To reiterate:The text says fixed-width types are OK when a fixed-width
> quantity is "meant"; and that in this case, Stefano and Andy "mean" to
> use a fixed-width quantity.  The implied subtext of that sentence
> could be, "Don't use fixed width types unless there's a good reason to
> use a fixed width", and both Andy and Stefano think there's a good
> reason.  This argument you haven't really addressed at all, except
> with a specious "slippery slope" argument meant to nullify the
> exception; and now you attempt to simply ignore.
> 
> I venture to assert that for most people, the rules are a means to an
> end: That end being code which is correct, robust, fast, easy to
> write, maintain, debug, and review patches for.  What I agreed to,
> when I accepted this patch, was that *in general* we would avoid using
> fixed-width types; but that there were cases where doing so made
> sense.  Some of those were discussed in the thread above.
> 
> Andy and Stefano have already put forward reasons why they think a
> fixed-width type would be better here, which are related to "end
> goals": namely, more robust and easy to maintain code.  When I asked
> what "end goals" would be negatively impacted by using a fixed-width
> type, you explicitly said that there were none!  That the *only*
> reason you're continuing to argue is that we have a document somewhere
> that says we should -- a document which explicitly acknowledges that
> there will be exceptions!
> 
> The ideal response would have been to lay out a reasonably
> comprehensive set of criteria for when to use basic types, when to use
> fixed-width types, and how each criteria advanced the end goals of a
> better codebase.  A sufficient response would have been to lay out
> reasons why "better codebase", in this instance, tilts towards using a
> basic type rather than a fixed-width type.
> 
> Saying "That's what the rules say", without motivating it by
> explaining how it connects to a better codebase, is just not a helpful
> response; and making that argument after it's been pointed out that
> that is *not* what the rules say is even worse.

Thanks George for taking the time to write all of the above.

Let's please move forward with this patch.

Jan Beulich March 18, 2024, 7:46 a.m. UTC | #15

On 15.03.2024 15:45, George Dunlap wrote:
> On Fri, Mar 15, 2024 at 2:13 PM Jan Beulich <[e-mail]> wrote:
>> On 15.03.2024 14:55, Julien Grall wrote:
>>> On 15/03/2024 13:24, Jan Beulich wrote:
>>>> On 15.03.2024 13:17, George Dunlap wrote:
>>>>> On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <[e-mail]> wrote:
>>>>>>> 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.
> [snip]
>>>>> Other than "it's in CODING_STYLE", and "it's not really necessary
>>>>> because it's ensured in the assembly code", you haven't advanced a
>>>>> single reason why "uint32_t" is problematic.
>>>>
>>>> And it isn't, I never said it would be. But if we set rules for
>>>> ourselves, why would we take the first opportunity to not respect them?
>>>
>>> I am a bit confused. Reading through the thread you seem to agree that
>>> the written rules are respected here. So what rules are you talking about?
>>
>> What was proposed is use of a fixed width type where according to my
>> reading ./CODING_STYLE says it shouldn't be used.
> 
> This conversation is starting to get frustrating.

Same here.

>  That's simply not
> what it says, and I pointed that out just a few messages ago.
> 
> To reiterate:The text says fixed-width types are OK when a fixed-width
> quantity is "meant"; and that in this case, Stefano and Andy "mean" to
> use a fixed-width quantity.  The implied subtext of that sentence
> could be, "Don't use fixed width types unless there's a good reason to
> use a fixed width", and both Andy and Stefano think there's a good
> reason.  This argument you haven't really addressed at all, except
> with a specious "slippery slope" argument meant to nullify the
> exception; and now you attempt to simply ignore.
> 
> I venture to assert that for most people, the rules are a means to an
> end: That end being code which is correct, robust, fast, easy to
> write, maintain, debug, and review patches for.  What I agreed to,
> when I accepted this patch, was that *in general* we would avoid using
> fixed-width types; but that there were cases where doing so made
> sense.  Some of those were discussed in the thread above.
> 
> Andy and Stefano have already put forward reasons why they think a
> fixed-width type would be better here, which are related to "end
> goals": namely, more robust and easy to maintain code.  When I asked
> what "end goals" would be negatively impacted by using a fixed-width
> type, you explicitly said that there were none!  That the *only*
> reason you're continuing to argue is that we have a document somewhere
> that says we should -- a document which explicitly acknowledges that
> there will be exceptions!

The named reasons simply aren't convincing to me, at all. There's no
loss towards the "end goals" when "unsigned int" is used instead of
"uint32_t". Plus I recall Andrew putting under question use of
"unsigned int" for various hypercall parameter declarations, indicating
his belief that "unsigned long" ought to be used. That's, to me, quite
the opposite of using fixed-width types here, as there's no uniformly
correct fixed-width type to use in that case.

So to me, besides there not having been technical arguments towards
the need to use fixed width types here, there's actually added
confusion about what's actually wanted. Imo if we started using fixed-
width types for hypercall handler parameter declarations, (almost?) all
non-handle ones would likely want to be converted. Consistency, after
all, is part of the "easy to maintain code" goal. Plus without
consistency how would one determine when to use what kind of types.

Bottom line: My take is that here we're discussing a matter of taste
(preferring fixed width types) vs a matter of a supposedly agreed upon
rule (preferring to avoid them). Hence my "there not having been
technical arguments". I therefore view your accuse as simply unfair.

> The ideal response would have been to lay out a reasonably
> comprehensive set of criteria for when to use basic types, when to use
> fixed-width types, and how each criteria advanced the end goals of a
> better codebase.  A sufficient response would have been to lay out
> reasons why "better codebase", in this instance, tilts towards using a
> basic type rather than a fixed-width type.

The main use of fixed width types, to me, is in interface structure
definitions - between Xen and hardware / firmware, or in hypercall
structures. I'm afraid I have a hard time seeing good uses outside of
that. Even in inline assembly, types of variables used for inputs or
outputs don't strictly need to be fixed-width; I can somewhat accept
their use there for documentation purposes.

> Saying "That's what the rules say", without motivating it by
> explaining how it connects to a better codebase, is just not a helpful
> response; and making that argument after it's been pointed out that
> that is *not* what the rules say is even worse.

Well, as above, I view this as unfair as well. The rule's there. It
should be followed. If there really were downsides to following it,
the rule should be amended or dropped. If it was dropped, we'd end
up where we were before: People randomly using fixed width types even
in places we agree they're not suitable for use. And there being no
real handle in reviews to ask for them to change.

In this context, may I remind you that I'm doing a lot more reviews
than you. Having been accused of (and in various cases also guilty of)
bike shedding, it is quite helpful when one can back change requests
by references to (more or less) clearly spelled out rules. If too much
room for interpretation remains (and if there's disagreement about
interpretation), what's written down needs tightening imo. Hence why a
goal of mine has been to get ./CODING_STYLE into less ambiguous shape.
With rather little success, sadly.

Jan

Stefano Stabellini March 19, 2024, 3:39 a.m. UTC | #16

On Mon, 18 Mar 2024, Jan Beulich wrote:
> The named reasons simply aren't convincing to me, at all. There's no
> loss towards the "end goals" when "unsigned int" is used instead of
> "uint32_t". Plus I recall Andrew putting under question use of
> "unsigned int" for various hypercall parameter declarations, indicating
> his belief that "unsigned long" ought to be used. That's, to me, quite
> the opposite of using fixed-width types here, as there's no uniformly
> correct fixed-width type to use in that case.
> 
> So to me, besides there not having been technical arguments towards
> the need to use fixed width types here, there's actually added
> confusion about what's actually wanted. Imo if we started using fixed-
> width types for hypercall handler parameter declarations, (almost?) all
> non-handle ones would likely want to be converted. Consistency, after
> all, is part of the "easy to maintain code" goal. Plus without
> consistency how would one determine when to use what kind of types.

[...]

> The main use of fixed width types, to me, is in interface structure
> definitions - between Xen and hardware / firmware, or in hypercall
> structures. I'm afraid I have a hard time seeing good uses outside of
> that. Even in inline assembly, types of variables used for inputs or
> outputs don't strictly need to be fixed-width; I can somewhat accept
> their use there for documentation purposes.


Non-ABI interfaces are OK with native types.

Our ABI interfaces are, for better or for worse, described using C
header files. Looking at these header files, it should be clear the size
and alignment of all integer parameters.

To that end, I think we should use fixed-width types in all ABIs,
including hypercall entry points. In my opinion, C hypercall entry
points are part of the ABI and should match the integer types used in
the public header files. I don't consider the little assembly code on
hypercall entry as important. I think we should avoid having one
description of the hypercall types in sched.h and different types used
in do_sched_op.

Sometimes, we might have parameters that vary in size depending on the
architecture. For instance, a parameter could be 32-bit for 32-bit
architectures and 64-bit for 64-bit architectures.

For these cases, using "unsigned long", together with a document like
the one I submitted recently to xen-devel, is a good way forward: it
is semantically correct on all architectures, and it still comes with a
precise size and alignment as described in the document. In this
context, "unsigned long" means "register size" (on ARM we have
register_t). This is the one case where it makes sense not to use a
fixed-width type. Alternatively we would have to use #ifdefs.

George Dunlap April 3, 2024, 3:11 p.m. UTC | #17

On Tue, Mar 19, 2024 at 3:39 AM Stefano Stabellini
<[e-mail]> wrote:
> > The main use of fixed width types, to me, is in interface structure
> > definitions - between Xen and hardware / firmware, or in hypercall
> > structures. I'm afraid I have a hard time seeing good uses outside of
> > that. Even in inline assembly, types of variables used for inputs or
> > outputs don't strictly need to be fixed-width; I can somewhat accept
> > their use there for documentation purposes.
>
>
> Non-ABI interfaces are OK with native types.
>
> Our ABI interfaces are, for better or for worse, described using C
> header files. Looking at these header files, it should be clear the size
> and alignment of all integer parameters.
>
> To that end, I think we should use fixed-width types in all ABIs,
> including hypercall entry points. In my opinion, C hypercall entry
> points are part of the ABI and should match the integer types used in
> the public header files. I don't consider the little assembly code on
> hypercall entry as important.

So as Jan pointed out in the recent call, the "actual" ABI is
"register X, Y, Z are arguments 1-3".  The key thing for me then is
whether it's clear what values in register X are acceptable.  If the C
function implementing the hypercall has an argument of type "unsigned
int", is it clear what values the guest is allowed to be put into the
corresponding register, and how they'll be interpreted, as opposed to
"unsigned long"?

If we can document the expectations, for each architecture, for how
the "native types" map to sizes, then I think that should be
sufficient.

 -George

Patch

diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 6d361ddfce..8b8ee16074 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -172,7 +172,7 @@  console_io(unsigned int cmd, unsigned int count, char *buffer)
 vm_assist(unsigned int cmd, unsigned int type)
 event_channel_op(int cmd, void *arg)
 mmuext_op(mmuext_op_t *uops, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
-multicall(multicall_entry_t *call_list, unsigned int nr_calls)
+multicall(multicall_entry_t *call_list, uint32_t nr_calls)
 #ifdef CONFIG_PV
 mmu_update(mmu_update_t *ureqs, unsigned int count, unsigned int *pdone, unsigned int foreigndom)
 stack_switch(unsigned long ss, unsigned long esp)