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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>

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@xxxxxxxxxx>





> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Roberto Bagnara <roberto.bagnara@xxxxxxxxxxx>
> CC: consulting@xxxxxxxxxxx <consulting@xxxxxxxxxxx>
> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> 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
>