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 13:23:35 +0100	[thread overview]
Message-ID: <a10bf170-da69-45c2-aa7a-97d94995d4db@suse.com> (raw)
In-Reply-To: <503aa63b5204e9b9eab5a21235df7c6c@bugseng.com>

On 14.03.2024 10:25, Nicola Vetrini wrote:
> 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@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[].
>>
> 
> I'm not understanding what you consider dead code.

iommu_guest.c:guest_iommu_{init,destroy,set_base,add_event_log}() have
no callers; guest_iommu_add_ppr_log() having one is suspicious, but the
function will still bail early due to domain_iommu() never returning
non-NULL in practice. With that I'm pretty sure nothing else in the file
is actually reachable.

> I see three users of amd_iommu_send_guest_cmd

All in said file.

> and seven for send_iommu_command.

Well, this one of course isn't dead.

> I can adjust u32 for sure. There are also other u32/uint32_t 
> incosistencies in that header.

Which we're going to take care of over time.

Jan