Re: [XEN PATCH] amd/iommu: add fixed size to function parameter of array type

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-03-14 09:32, Jan Beulich wrote:
> On 14.03.2024 08:42, Nicola Vetrini wrote:
> > The 'cmd' parameter of amd_iommu_send_guest_cmd is passed
> > to a function that expects arrays of size 4, therefore
> > specifying explicitly the size also in amd_iommu_send_guest_cmd
> > allows not to accidentally pass a smaller array or assume that
> > send_iommu_command handles array sizes >4 correctly.
> >
> > No functional change.
> >
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> > All current users pass an array of size 4, hence this patch is 
> > addressing
> > a potential issue noticed by the analyzer in the context of Rule 17.5
> > ("The function argument corresponding to a parameter declared to have 
> > an array
> > type shall have an appropriate number of elements"), not an actual 
> > problem in
> > the sources.
> While true, I think we want to consider alternatives. First one being 
> to rip
> out this dead code (thus addressing other Misra concerns as well). 
> Second,
> if we meant to keep it, to properly do away with the (ab)use of u32[].
I'm not understanding what you consider dead code.
I see three users of amd_iommu_send_guest_cmd and seven for 
send_iommu_command.
I can adjust u32 for sure. There are also other u32/uint32_t 
incosistencies in that header.
> Finally, if to be taken in this least-effort shape, ...
>
> > --- a/xen/drivers/passthrough/amd/iommu.h
> > +++ b/xen/drivers/passthrough/amd/iommu.h
> > @@ -346,7 +346,7 @@ void cf_check amd_iommu_crash_shutdown(void);
> >
> >  /* guest iommu support */
> >  #ifdef CONFIG_HVM
> > -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[]);
> > +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4]);
> ... u32 here and ...
>
> > --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> > +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> > @@ -390,7 +390,7 @@ void amd_iommu_flush_all_caches(struct amd_iommu 
> > *iommu)
> >      flush_command_buffer(iommu, 0);
> >  }
> >
> > -void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[])
> > +void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[4])
> ... here would better be replaced by uint32_t at the same time, not the
> least because that's what ...
>
> >  {
> >      send_iommu_command(iommu, cmd);
> ... this function already takes afaics.
>
> Jan
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)