Re: [XEN PATCH 2/5] iommu: address violation of MISRA C Rule 5.5

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2025 23:21, Stefano Stabellini wrote:
> On Mon, 7 Jul 2025, Jan Beulich wrote:
>> On 04.07.2025 22:39, Dmytro Prokopchuk1 wrote:
>>> Address a violation of MISRA C:2012 Rule 5.5:
>>> "Identifiers shall be distinct from macro names".
>>>
>>> Reports for service MC3A2.R5.5:
>>> xen/include/xen/iommu.h: non-compliant struct 'page_list_head'
>>> xen/include/xen/mm.h: non-compliant macro 'page_list_head'
>>
>> What is this about? There's no code change that I could see which would
>> alter the situation here.
>>
>>> xen/drivers/passthrough/iommu.c: non-compliant macro 'iommu_quarantine'
>>> xen/include/xen/iommu.h: non-compliant variable 'iommu_quarantine'
>>>
>>> These external variables ('iommu_pt_cleanup_lock'
>>> and 'iommu_pt_cleanup_list') are no longer used
>>> in the codebase. Removing them eliminates dead
>>> code and ensures compliance with coding standards.
>>> Fixes: b5622eb627 (iommu: remove unused iommu_ops method and tasklet, 
>>> 2020-09-22)
>>
>> That's a different Misra rule, so may better be put in a separate patch?
>> Otherwise please at least mention the rule number as well.
>>
>>> The variable 'iommu_quarantine' makes sence to use
>>> only if 'CONFIG_HAS_PCI=y', so place it inside '#ifdef'.
>>
>> Hmm, yes - not nice, but what do you do. I question "makes sense" though:
>> Quarantining is possible also without PCI, aiui. Just we don't that right
>> now.
> 
> As far as I can tell the change to #ifdef iommu_quarantine is necessary
> to resolve a R5.5 violation here:
> 
> #ifdef CONFIG_HAS_PCI
> uint8_t __read_mostly iommu_quarantine =
> # if defined(CONFIG_IOMMU_QUARANTINE_NONE)
>     IOMMU_quarantine_none;
> # elif defined(CONFIG_IOMMU_QUARANTINE_BASIC)
>     IOMMU_quarantine_basic;
> # elif defined(CONFIG_IOMMU_QUARANTINE_SCRATCH_PAGE)
>     IOMMU_quarantine_scratch_page;
> # endif
> #else
> # define iommu_quarantine IOMMU_quarantine_none       <<< violation
> #endif /* CONFIG_HAS_PCI */

Yes. And I expressed that I accept the need to this change. I merely
questioned the wording used in the description.

What I can't derive is why page_list_head is mentioned in the
description, but then there's no related code change.

> As you can see from the patch series, often it is not nice to find a
> resoltution for these R5.5 violations. This is the reason why I
> originally suggested to deviate R5.5 entirely.
> 
> https://lore.kernel.org/xen-devel/139aa595-8b41-44e7-b205-415443c8c357@xxxxxxxx/](https://lore.kernel.org/xen-devel/139aa595-8b41-44e7-b205-415443c8c357@xxxxxxxx/
> 
> That said, this patch is one of the nicer changes in this series, I
> think it is OK.

With some adjustment(s) to the description, perhaps.

Jan