[v1] address violation of MISRA C Rule 5.5

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

Posted by Dmytro Prokopchuk1 3 months, 4 weeks ago

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'
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)

The variable 'iommu_quarantine' makes sence to use
only if 'CONFIG_HAS_PCI=y', so place it inside '#ifdef'.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
 xen/include/xen/iommu.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index ebfada1d88..57f338e2a0 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -53,7 +53,9 @@ static inline bool dfn_eq(dfn_t x, dfn_t y)
 extern bool iommu_enable, iommu_enabled;
 extern bool force_iommu, iommu_verbose;
 /* Boolean except for the specific purposes of drivers/passthrough/iommu.c. */
+#ifdef CONFIG_HAS_PCI
 extern uint8_t iommu_quarantine;
+#endif /* CONFIG_HAS_PCI */
 #else
 #define iommu_enabled false
 #endif
@@ -500,9 +502,6 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
  */
 DECLARE_PER_CPU(bool, iommu_dont_flush_iotlb);
 
-extern struct spinlock iommu_pt_cleanup_lock;
-extern struct page_list_head iommu_pt_cleanup_list;
-
 bool arch_iommu_use_permitted(const struct domain *d);
 
 #ifdef CONFIG_X86
-- 
2.43.0

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

Posted by Jan Beulich 3 months, 3 weeks ago

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.

Jan

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

Posted by Stefano Stabellini 3 months, 3 weeks ago

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.

Hi Jan,

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 */

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@suse.com/](https://lore.kernel.org/xen-devel/139aa595-8b41-44e7-b205-415443c8c357@suse.com/

That said, this patch is one of the nicer changes in this series, I
think it is OK.

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

Posted by Jan Beulich 3 months, 3 weeks ago

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@suse.com/](https://lore.kernel.org/xen-devel/139aa595-8b41-44e7-b205-415443c8c357@suse.com/
> 
> 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