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

From: Jan Beulich <jbeulich@suse.com>
To: Nicola Vetrini <nicola.vetrini@bugseng.com>
Cc: sstabellini@kernel.org, michal.orzel@amd.com,
	xenia.ragiadakou@amd.com, ayan.kumar.halder@amd.com,
	consulting@bugseng.com, bertrand.marquis@arm.com, julien@xen.org,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH] amd/iommu: add fixed size to function parameter of array type
Date: Thu, 14 Mar 2024 09:32:27 +0100	[thread overview]
Message-ID: <6186b676-660c-4bfa-a825-18ff7f0d7f62@suse.com> (raw)
In-Reply-To: <533a2d4f0c92d7fe92aa200b64434389de546f69.1710343652.git.nicola.vetrini@bugseng.com>

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@bugseng.com>
> ---
> 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[].

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