[PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long

Andrew Cooper posted 2 patches 5 months ago
[PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Andrew Cooper 5 months ago
Right now, the non-compat declaration and definition of do_multicall()
differing types for the nr_calls parameter.

This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
first 128bit architecture (RISC-V looks as if it might get there first).

Worse, the type chosen here has a side effect of truncating the guest
parameter, because Xen still doesn't have a clean hypercall ABI definition.

Switch uniformly to using unsigned long.

This addresses the MISRA violation, and while it is a guest-visible ABI
change, it's only in the corner case where the guest kernel passed a
bogus-but-correct-when-truncated value.  I can't find any any users of
mutilcall which pass a bad size to begin with, so this should have no
practical effect on guests.

In fact, this brings the behaviour of multicalls more in line with the header
description of how it behaves.

With this fix, Xen is now fully clean to Rule 8.3, so mark it so.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
CC: consulting@bugseng.com <consulting@bugseng.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>

I know this isn't going to be universally liked, but we need to do something,
and this is my very strong vote for the least bad way out of the current mess.
---
 automation/eclair_analysis/ECLAIR/tagging.ecl | 1 +
 xen/common/multicall.c                        | 4 ++--
 xen/include/hypercall-defs.c                  | 4 ++--
 xen/include/public/xen.h                      | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
index b829655ca0bc..3d06a1aad410 100644
--- a/automation/eclair_analysis/ECLAIR/tagging.ecl
+++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
@@ -45,6 +45,7 @@ MC3R1.R7.2||
 MC3R1.R7.4||
 MC3R1.R8.1||
 MC3R1.R8.2||
+MC3R1.R8.3||
 MC3R1.R8.5||
 MC3R1.R8.6||
 MC3R1.R8.8||
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index 1f0cc4cb267c..ce394c5efcfe 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -34,11 +34,11 @@ static void trace_multicall_call(multicall_entry_t *call)
 }
 
 ret_t do_multicall(
-    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, uint32_t nr_calls)
+    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, unsigned long nr_calls)
 {
     struct vcpu *curr = current;
     struct mc_state *mcs = &curr->mc_state;
-    uint32_t         i;
+    unsigned long    i;
     int              rc = 0;
     enum mc_disposition disp = mc_continue;
 
diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
index 47c093acc84d..7720a29ade0b 100644
--- a/xen/include/hypercall-defs.c
+++ b/xen/include/hypercall-defs.c
@@ -135,7 +135,7 @@ xenoprof_op(int op, void *arg)
 #ifdef CONFIG_COMPAT
 prefix: compat
 set_timer_op(uint32_t lo, uint32_t hi)
-multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls)
+multicall(multicall_entry_compat_t *call_list, unsigned long nr_calls)
 memory_op(unsigned int cmd, void *arg)
 #ifdef CONFIG_IOREQ_SERVER
 dm_op(domid_t domid, unsigned int nr_bufs, void *bufs)
@@ -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, unsigned long 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)
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index b47d48d0e2d6..e051f989a5ca 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -623,7 +623,7 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
- * `                      uint32_t nr_calls);
+ * `                      unsigned long nr_calls);
  *
  * NB. The fields are logically the natural register size for this
  * architecture. In cases where xen_ulong_t is larger than this then
-- 
2.39.2


Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Jan Beulich 5 months ago
On 21.06.2024 22:58, Andrew Cooper wrote:
> Right now, the non-compat declaration and definition of do_multicall()
> differing types for the nr_calls parameter.
> 
> This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
> first 128bit architecture (RISC-V looks as if it might get there first).
> 
> Worse, the type chosen here has a side effect of truncating the guest
> parameter, because Xen still doesn't have a clean hypercall ABI definition.
> 
> Switch uniformly to using unsigned long.

And re-raising all the same question again: Why not uniformly unsigned int?
Or uint32_t?

> This addresses the MISRA violation, and while it is a guest-visible ABI
> change, it's only in the corner case where the guest kernel passed a
> bogus-but-correct-when-truncated value.  I can't find any any users of
> mutilcall which pass a bad size to begin with, so this should have no
> practical effect on guests.
> 
> In fact, this brings the behaviour of multicalls more in line with the header
> description of how it behaves.

Which description? If you mean ...

> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -623,7 +623,7 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
>  /*
>   * ` enum neg_errnoval
>   * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
> - * `                      uint32_t nr_calls);
> + * `                      unsigned long nr_calls);
>   *
>   * NB. The fields are logically the natural register size for this
>   * architecture. In cases where xen_ulong_t is larger than this then

... this comment here, note how is says "fields", i.e. talks about the
subsequent struct.

What you're doing is effectively an ABI change: All of the sudden the
upper bits of the incoming argument would be respected. Yes, it is
overwhelmingly likely that no-one would ever pass such a value. Yet
iirc on other similar hypercall handler adjustments in the past you
did raise a similar concern.

Jan
Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Stefano Stabellini 1 week, 2 days ago
Hi Jan,

It is challenging to create a solution that satisfies everyone for this
patch. However, we should add R8.3 to the clean list as soon as possible
to enable rule blocking in GitLab-CI. Failing to do so risks introducing
regressions, as recently occurred, undoing the significant efforts made
by Bugseng and the community over the past year.

Unless there is a specific counterproposal, let us proceed with
committing this patch.

Cheers,
Stefano

On Mon, 24 Jun 2024, Jan Beulich wrote:
> On 21.06.2024 22:58, Andrew Cooper wrote:
> > Right now, the non-compat declaration and definition of do_multicall()
> > differing types for the nr_calls parameter.
> > 
> > This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
> > first 128bit architecture (RISC-V looks as if it might get there first).
> > 
> > Worse, the type chosen here has a side effect of truncating the guest
> > parameter, because Xen still doesn't have a clean hypercall ABI definition.
> > 
> > Switch uniformly to using unsigned long.
> 
> And re-raising all the same question again: Why not uniformly unsigned int?
> Or uint32_t?
> 
> > This addresses the MISRA violation, and while it is a guest-visible ABI
> > change, it's only in the corner case where the guest kernel passed a
> > bogus-but-correct-when-truncated value.  I can't find any any users of
> > mutilcall which pass a bad size to begin with, so this should have no
> > practical effect on guests.
> > 
> > In fact, this brings the behaviour of multicalls more in line with the header
> > description of how it behaves.
> 
> Which description? If you mean ...
> 
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -623,7 +623,7 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
> >  /*
> >   * ` enum neg_errnoval
> >   * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
> > - * `                      uint32_t nr_calls);
> > + * `                      unsigned long nr_calls);
> >   *
> >   * NB. The fields are logically the natural register size for this
> >   * architecture. In cases where xen_ulong_t is larger than this then
> 
> ... this comment here, note how is says "fields", i.e. talks about the
> subsequent struct.
> 
> What you're doing is effectively an ABI change: All of the sudden the
> upper bits of the incoming argument would be respected. Yes, it is
> overwhelmingly likely that no-one would ever pass such a value. Yet
> iirc on other similar hypercall handler adjustments in the past you
> did raise a similar concern.
> 
> Jan
>
Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Jan Beulich 1 week, 1 day ago
On 13.11.2024 04:15, Stefano Stabellini wrote:
> It is challenging to create a solution that satisfies everyone for this
> patch. However, we should add R8.3 to the clean list as soon as possible
> to enable rule blocking in GitLab-CI. Failing to do so risks introducing
> regressions, as recently occurred, undoing the significant efforts made
> by Bugseng and the community over the past year.
> 
> Unless there is a specific counterproposal, let us proceed with
> committing this patch.

Well, I find this odd. We leave things sit in limbo for months and then
want to go ahead with a controversial solution? Rather than actually
(and finally) sorting out the underlying disagreement (of which there
are actually two sufficiently separate parts)? Plus ...

> On Mon, 24 Jun 2024, Jan Beulich wrote:
>> On 21.06.2024 22:58, Andrew Cooper wrote:
>>> Right now, the non-compat declaration and definition of do_multicall()
>>> differing types for the nr_calls parameter.
>>>
>>> This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
>>> first 128bit architecture (RISC-V looks as if it might get there first).
>>>
>>> Worse, the type chosen here has a side effect of truncating the guest
>>> parameter, because Xen still doesn't have a clean hypercall ABI definition.
>>>
>>> Switch uniformly to using unsigned long.
>>
>> And re-raising all the same question again: Why not uniformly unsigned int?
>> Or uint32_t?

... this question of mine effectively represents a concrete alternative
proposal (or even two, if you like).

The two parts where there appears to be disagreement are:
1) When to (not) use fixed width types, as presently outlined in
   ./CODING_STYLE.
2) How to type C function parameters called solely from assembly code (of
   which the hypercall handlers are a subset).

And maybe
2b) How to best express such function parameters when they're (sometimes)
    shared between native and compat handlers.

Of course 2) is affected by, as Andrew validly says, there not being a
formally clean ABI definition.

My fear is that if this gets committed as is, it'll be used as a handle to
force in further similarly questionable / controversial changes to other
hypercall handlers. Which is why I think the controversy needs sorting out
first (which admittedly is hard when the ABI is fuzzy).

Jan
Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Stefano Stabellini 1 week, 1 day ago
On Wed, 13 Nov 2024, Jan Beulich wrote:
> On 13.11.2024 04:15, Stefano Stabellini wrote:
> > It is challenging to create a solution that satisfies everyone for this
> > patch. However, we should add R8.3 to the clean list as soon as possible
> > to enable rule blocking in GitLab-CI. Failing to do so risks introducing
> > regressions, as recently occurred, undoing the significant efforts made
> > by Bugseng and the community over the past year.
> > 
> > Unless there is a specific counterproposal, let us proceed with
> > committing this patch.
> 
> Well, I find this odd. We leave things sit in limbo for months and then
> want to go ahead with a controversial solution? Rather than actually
> (and finally) sorting out the underlying disagreement (of which there
> are actually two sufficiently separate parts)? Plus ...

The reason is that several MISRA regressions were recently introduced.
These regressions could have been easily detected by GitLab CI if we had
marked the rules as clean. I believe we should expedite accepting the
fixes and marking the rules as clean. We can always adjust the fixes or
deviations later to better suit our preferences. In my opinion, we
should prioritize marking the rules as clean.


> > On Mon, 24 Jun 2024, Jan Beulich wrote:
> >> On 21.06.2024 22:58, Andrew Cooper wrote:
> >>> Right now, the non-compat declaration and definition of do_multicall()
> >>> differing types for the nr_calls parameter.
> >>>
> >>> This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
> >>> first 128bit architecture (RISC-V looks as if it might get there first).
> >>>
> >>> Worse, the type chosen here has a side effect of truncating the guest
> >>> parameter, because Xen still doesn't have a clean hypercall ABI definition.
> >>>
> >>> Switch uniformly to using unsigned long.
> >>
> >> And re-raising all the same question again: Why not uniformly unsigned int?
> >> Or uint32_t?
> 
> ... this question of mine effectively represents a concrete alternative
> proposal (or even two, if you like).
> 
> The two parts where there appears to be disagreement are:
> 1) When to (not) use fixed width types, as presently outlined in
>    ./CODING_STYLE.
> 2) How to type C function parameters called solely from assembly code (of
>    which the hypercall handlers are a subset).
> 
> And maybe
> 2b) How to best express such function parameters when they're (sometimes)
>     shared between native and compat handlers.
> 
> Of course 2) is affected by, as Andrew validly says, there not being a
> formally clean ABI definition.
> 
> My fear is that if this gets committed as is, it'll be used as a handle to
> force in further similarly questionable / controversial changes to other
> hypercall handlers. Which is why I think the controversy needs sorting out
> first (which admittedly is hard when the ABI is fuzzy).

While I appreciate your concern, as you know, aligning on the topics
above takes time. I do not believe it is in the interest of the
community, both contributors and reviewers, to delay marking this rule
as clean. Honestly, I do not mind how it gets marked as clean, as long
as we do it soon.

Additionally, please keep in mind that the Xen Project tends to have a
long memory. As a result, there is usually little risk of the so-called
"slippery slope" problem.

If you prefer a deviation I am OK with that too. I just want 8.3 as
clean :-)
Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Jan Beulich 1 week ago
On 14.11.2024 03:34, Stefano Stabellini wrote:
> On Wed, 13 Nov 2024, Jan Beulich wrote:
>> On 13.11.2024 04:15, Stefano Stabellini wrote:
>>> It is challenging to create a solution that satisfies everyone for this
>>> patch. However, we should add R8.3 to the clean list as soon as possible
>>> to enable rule blocking in GitLab-CI. Failing to do so risks introducing
>>> regressions, as recently occurred, undoing the significant efforts made
>>> by Bugseng and the community over the past year.
>>>
>>> Unless there is a specific counterproposal, let us proceed with
>>> committing this patch.
>>
>> Well, I find this odd. We leave things sit in limbo for months and then
>> want to go ahead with a controversial solution? Rather than actually
>> (and finally) sorting out the underlying disagreement (of which there
>> are actually two sufficiently separate parts)? Plus ...
> 
> The reason is that several MISRA regressions were recently introduced.
> These regressions could have been easily detected by GitLab CI if we had
> marked the rules as clean. I believe we should expedite accepting the
> fixes and marking the rules as clean. We can always adjust the fixes or
> deviations later to better suit our preferences. In my opinion, we
> should prioritize marking the rules as clean.
> 
> 
>>> On Mon, 24 Jun 2024, Jan Beulich wrote:
>>>> On 21.06.2024 22:58, Andrew Cooper wrote:
>>>>> Right now, the non-compat declaration and definition of do_multicall()
>>>>> differing types for the nr_calls parameter.
>>>>>
>>>>> This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
>>>>> first 128bit architecture (RISC-V looks as if it might get there first).
>>>>>
>>>>> Worse, the type chosen here has a side effect of truncating the guest
>>>>> parameter, because Xen still doesn't have a clean hypercall ABI definition.
>>>>>
>>>>> Switch uniformly to using unsigned long.
>>>>
>>>> And re-raising all the same question again: Why not uniformly unsigned int?
>>>> Or uint32_t?
>>
>> ... this question of mine effectively represents a concrete alternative
>> proposal (or even two, if you like).
>>
>> The two parts where there appears to be disagreement are:
>> 1) When to (not) use fixed width types, as presently outlined in
>>    ./CODING_STYLE.
>> 2) How to type C function parameters called solely from assembly code (of
>>    which the hypercall handlers are a subset).
>>
>> And maybe
>> 2b) How to best express such function parameters when they're (sometimes)
>>     shared between native and compat handlers.
>>
>> Of course 2) is affected by, as Andrew validly says, there not being a
>> formally clean ABI definition.
>>
>> My fear is that if this gets committed as is, it'll be used as a handle to
>> force in further similarly questionable / controversial changes to other
>> hypercall handlers. Which is why I think the controversy needs sorting out
>> first (which admittedly is hard when the ABI is fuzzy).
> 
> While I appreciate your concern, as you know, aligning on the topics
> above takes time. I do not believe it is in the interest of the
> community, both contributors and reviewers, to delay marking this rule
> as clean. Honestly, I do not mind how it gets marked as clean, as long
> as we do it soon.
> 
> Additionally, please keep in mind that the Xen Project tends to have a
> long memory. As a result, there is usually little risk of the so-called
> "slippery slope" problem.
> 
> If you prefer a deviation I am OK with that too. I just want 8.3 as
> clean :-) 

No, please no deviations when we can avoid them. Since it feels like it's
always (going to be?) me to give in when there is such disagreement, why
don't I do so here as well: Go ahead.

Jan
Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Stefano Stabellini 1 week ago
On Thu, 14 Nov 2024, Jan Beulich wrote:
> On 14.11.2024 03:34, Stefano Stabellini wrote:
> > On Wed, 13 Nov 2024, Jan Beulich wrote:
> >> On 13.11.2024 04:15, Stefano Stabellini wrote:
> >>> It is challenging to create a solution that satisfies everyone for this
> >>> patch. However, we should add R8.3 to the clean list as soon as possible
> >>> to enable rule blocking in GitLab-CI. Failing to do so risks introducing
> >>> regressions, as recently occurred, undoing the significant efforts made
> >>> by Bugseng and the community over the past year.
> >>>
> >>> Unless there is a specific counterproposal, let us proceed with
> >>> committing this patch.
> >>
> >> Well, I find this odd. We leave things sit in limbo for months and then
> >> want to go ahead with a controversial solution? Rather than actually
> >> (and finally) sorting out the underlying disagreement (of which there
> >> are actually two sufficiently separate parts)? Plus ...
> > 
> > The reason is that several MISRA regressions were recently introduced.
> > These regressions could have been easily detected by GitLab CI if we had
> > marked the rules as clean. I believe we should expedite accepting the
> > fixes and marking the rules as clean. We can always adjust the fixes or
> > deviations later to better suit our preferences. In my opinion, we
> > should prioritize marking the rules as clean.
> > 
> > 
> >>> On Mon, 24 Jun 2024, Jan Beulich wrote:
> >>>> On 21.06.2024 22:58, Andrew Cooper wrote:
> >>>>> Right now, the non-compat declaration and definition of do_multicall()
> >>>>> differing types for the nr_calls parameter.
> >>>>>
> >>>>> This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
> >>>>> first 128bit architecture (RISC-V looks as if it might get there first).
> >>>>>
> >>>>> Worse, the type chosen here has a side effect of truncating the guest
> >>>>> parameter, because Xen still doesn't have a clean hypercall ABI definition.
> >>>>>
> >>>>> Switch uniformly to using unsigned long.
> >>>>
> >>>> And re-raising all the same question again: Why not uniformly unsigned int?
> >>>> Or uint32_t?
> >>
> >> ... this question of mine effectively represents a concrete alternative
> >> proposal (or even two, if you like).
> >>
> >> The two parts where there appears to be disagreement are:
> >> 1) When to (not) use fixed width types, as presently outlined in
> >>    ./CODING_STYLE.
> >> 2) How to type C function parameters called solely from assembly code (of
> >>    which the hypercall handlers are a subset).
> >>
> >> And maybe
> >> 2b) How to best express such function parameters when they're (sometimes)
> >>     shared between native and compat handlers.
> >>
> >> Of course 2) is affected by, as Andrew validly says, there not being a
> >> formally clean ABI definition.
> >>
> >> My fear is that if this gets committed as is, it'll be used as a handle to
> >> force in further similarly questionable / controversial changes to other
> >> hypercall handlers. Which is why I think the controversy needs sorting out
> >> first (which admittedly is hard when the ABI is fuzzy).
> > 
> > While I appreciate your concern, as you know, aligning on the topics
> > above takes time. I do not believe it is in the interest of the
> > community, both contributors and reviewers, to delay marking this rule
> > as clean. Honestly, I do not mind how it gets marked as clean, as long
> > as we do it soon.
> > 
> > Additionally, please keep in mind that the Xen Project tends to have a
> > long memory. As a result, there is usually little risk of the so-called
> > "slippery slope" problem.
> > 
> > If you prefer a deviation I am OK with that too. I just want 8.3 as
> > clean :-) 
> 
> No, please no deviations when we can avoid them. Since it feels like it's
> always (going to be?) me to give in when there is such disagreement, why
> don't I do so here as well: Go ahead.

Thanks Jan, I really appreciate this
Re: [PATCH 2/2] xen/multicall: Change nr_calls to uniformly be unsigned long
Posted by Stefano Stabellini 5 months ago
On Fri, 21 Jun 2024, Andrew Cooper wrote:
> Right now, the non-compat declaration and definition of do_multicall()
> differing types for the nr_calls parameter.
> 
> This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
> first 128bit architecture (RISC-V looks as if it might get there first).
> 
> Worse, the type chosen here has a side effect of truncating the guest
> parameter, because Xen still doesn't have a clean hypercall ABI definition.
> 
> Switch uniformly to using unsigned long.
> 
> This addresses the MISRA violation, and while it is a guest-visible ABI
> change, it's only in the corner case where the guest kernel passed a
> bogus-but-correct-when-truncated value.  I can't find any any users of
> mutilcall which pass a bad size to begin with, so this should have no
> practical effect on guests.
> 
> In fact, this brings the behaviour of multicalls more in line with the header
> description of how it behaves.
> 
> With this fix, Xen is now fully clean to Rule 8.3, so mark it so.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I am in favor of this approach. I have 2 suggestions which are
nice-to-have, not must-have.

We all know that "unsigned long" is register size. However, the C
standard doesn't define it that way. I tried to make it clear in
docs/misra/C-language-toolchain.rst. However, nowhere in the public
Xen headers we say that by "unsigned long" we mean register size. I
think we should say that somewhere in the headers, but I can see it
doesn't have to be part of this patch. It would be nice to add a comment
in xen/include/public/xen.h saying "unsigned long is register size" or
"refer to docs/misra/C-language-toolchain.rst for type sizes and
definitions", but it is not a hard request.

The second observation, if we are concerned about invalid nr_calls
values, we could add a check at the beginning of do_multicall:

  if ( nr_calls >= UINT32_MAX )

I am not sure it is an improvement, but maybe it is.


Given that both the above suggestions are nice-to-have:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>





> ---
> CC: George Dunlap <George.Dunlap@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> CC: consulting@bugseng.com <consulting@bugseng.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> I know this isn't going to be universally liked, but we need to do something,
> and this is my very strong vote for the least bad way out of the current mess.
> ---
>  automation/eclair_analysis/ECLAIR/tagging.ecl | 1 +
>  xen/common/multicall.c                        | 4 ++--
>  xen/include/hypercall-defs.c                  | 4 ++--
>  xen/include/public/xen.h                      | 2 +-
>  4 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl b/automation/eclair_analysis/ECLAIR/tagging.ecl
> index b829655ca0bc..3d06a1aad410 100644
> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> @@ -45,6 +45,7 @@ MC3R1.R7.2||
>  MC3R1.R7.4||
>  MC3R1.R8.1||
>  MC3R1.R8.2||
> +MC3R1.R8.3||
>  MC3R1.R8.5||
>  MC3R1.R8.6||
>  MC3R1.R8.8||
> diff --git a/xen/common/multicall.c b/xen/common/multicall.c
> index 1f0cc4cb267c..ce394c5efcfe 100644
> --- a/xen/common/multicall.c
> +++ b/xen/common/multicall.c
> @@ -34,11 +34,11 @@ static void trace_multicall_call(multicall_entry_t *call)
>  }
>  
>  ret_t do_multicall(
> -    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, uint32_t nr_calls)
> +    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, unsigned long nr_calls)
>  {
>      struct vcpu *curr = current;
>      struct mc_state *mcs = &curr->mc_state;
> -    uint32_t         i;
> +    unsigned long    i;
>      int              rc = 0;
>      enum mc_disposition disp = mc_continue;
>  
> diff --git a/xen/include/hypercall-defs.c b/xen/include/hypercall-defs.c
> index 47c093acc84d..7720a29ade0b 100644
> --- a/xen/include/hypercall-defs.c
> +++ b/xen/include/hypercall-defs.c
> @@ -135,7 +135,7 @@ xenoprof_op(int op, void *arg)
>  #ifdef CONFIG_COMPAT
>  prefix: compat
>  set_timer_op(uint32_t lo, uint32_t hi)
> -multicall(multicall_entry_compat_t *call_list, uint32_t nr_calls)
> +multicall(multicall_entry_compat_t *call_list, unsigned long nr_calls)
>  memory_op(unsigned int cmd, void *arg)
>  #ifdef CONFIG_IOREQ_SERVER
>  dm_op(domid_t domid, unsigned int nr_bufs, void *bufs)
> @@ -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, unsigned long 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)
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index b47d48d0e2d6..e051f989a5ca 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -623,7 +623,7 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
>  /*
>   * ` enum neg_errnoval
>   * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
> - * `                      uint32_t nr_calls);
> + * `                      unsigned long nr_calls);
>   *
>   * NB. The fields are logically the natural register size for this
>   * architecture. In cases where xen_ulong_t is larger than this then
> -- 
> 2.39.2
>