diff mbox series

[v2,1/2] x86/IOMMU: address violations of MISRA C:2012 Rule 14.4

Message ID 746a33fff1386b2e76657b5f7cfb31f3b117a1fe.1702310368.git.maria.celeste.cesario@bugseng.com (mailing list archive)
State New, archived
Headers show
Series xen: address violations of MISRA C:2012 Rule 14.4 | expand

Commit Message

Simone Ballarin Dec. 13, 2023, 4:10 p.m. UTC
From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>

The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
headline states:
"The controlling expression of an if statement and the controlling
expression of an iteration-statement shall have essentially Boolean type".

Add comparisons to avoid using enum constants as controlling expressions
to comply with Rule 14.4.
No functional change.

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
---
Changes in v2
- rename prefix from AMD/IOMMU to x86/IOMMU
- move changes on msi.c and hpet.c in this patch.
---
 xen/arch/x86/hpet.c                      | 6 +++---
 xen/arch/x86/msi.c                       | 4 ++--
 xen/drivers/passthrough/amd/iommu_init.c | 4 ++--
 xen/drivers/passthrough/vtd/iommu.c      | 4 ++--
 xen/drivers/passthrough/vtd/quirks.c     | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

Comments

Nicola Vetrini Feb. 5, 2024, 3:36 p.m. UTC | #1
On 2023-12-13 17:10, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
> headline states:
> "The controlling expression of an if statement and the controlling
> expression of an iteration-statement shall have essentially Boolean 
> type".
> 
> Add comparisons to avoid using enum constants as controlling 
> expressions
> to comply with Rule 14.4.
> No functional change.
> 
> Signed-off-by: Maria Celeste Cesario  
> <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
> ---
> Changes in v2
> - rename prefix from AMD/IOMMU to x86/IOMMU
> - move changes on msi.c and hpet.c in this patch.
> ---
>  xen/arch/x86/hpet.c                      | 6 +++---
>  xen/arch/x86/msi.c                       | 4 ++--
>  xen/drivers/passthrough/amd/iommu_init.c | 4 ++--
>  xen/drivers/passthrough/vtd/iommu.c      | 4 ++--
>  xen/drivers/passthrough/vtd/quirks.c     | 2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 

+Stefano

Hi all,

this patch seems not to have been committed into staging, unlike the 
other patch from this series. Since these are the only remaining 
violations for Rule 14.4, then I think these changes could be reviewed.

This should apply cleanly on staging, but I can send a rebased version 
if there are problems.

Additionally, despite having patches addressing R14.4 already in 
staging, the rule itself is not in docs/misra/rules.rst due to concerns 
on the patch that aimed to add it [1], so it's probably best to send a 
new version of that as well, if Stefano agrees.

[1] 
https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2308301840520.6458@ubuntu-linux-20-04-desktop/
Jan Beulich Feb. 5, 2024, 3:47 p.m. UTC | #2
On 05.02.2024 16:36, Nicola Vetrini wrote:
> On 2023-12-13 17:10, Simone Ballarin wrote:
>> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
>>
>> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
>> headline states:
>> "The controlling expression of an if statement and the controlling
>> expression of an iteration-statement shall have essentially Boolean 
>> type".
>>
>> Add comparisons to avoid using enum constants as controlling 
>> expressions
>> to comply with Rule 14.4.
>> No functional change.
>>
>> Signed-off-by: Maria Celeste Cesario  
>> <maria.celeste.cesario@bugseng.com>
>> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
>> ---
>> Changes in v2
>> - rename prefix from AMD/IOMMU to x86/IOMMU
>> - move changes on msi.c and hpet.c in this patch.
>> ---
>>  xen/arch/x86/hpet.c                      | 6 +++---
>>  xen/arch/x86/msi.c                       | 4 ++--
>>  xen/drivers/passthrough/amd/iommu_init.c | 4 ++--
>>  xen/drivers/passthrough/vtd/iommu.c      | 4 ++--
>>  xen/drivers/passthrough/vtd/quirks.c     | 2 +-
>>  5 files changed, 10 insertions(+), 10 deletions(-)
>>
> 
> +Stefano
> 
> Hi all,
> 
> this patch seems not to have been committed into staging, unlike the 
> other patch from this series. Since these are the only remaining 
> violations for Rule 14.4, then I think these changes could be reviewed.

It's no surprise the change isn't committed yet, when it hasn't had any
of the necessary tags. As far as I'm concerned, I seem to recall
indicating clearly that I'm not happy with this change, and hence acks
would need to come from elsewhere.

Jan
Stefano Stabellini Feb. 7, 2024, 12:51 a.m. UTC | #3
On Wed, 13 Dec 2023, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
> headline states:
> "The controlling expression of an if statement and the controlling
> expression of an iteration-statement shall have essentially Boolean type".
> 
> Add comparisons to avoid using enum constants as controlling expressions
> to comply with Rule 14.4.
> No functional change.
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

However it needs an ack from an x86 maintainer


> ---
> Changes in v2
> - rename prefix from AMD/IOMMU to x86/IOMMU
> - move changes on msi.c and hpet.c in this patch.
> ---
>  xen/arch/x86/hpet.c                      | 6 +++---
>  xen/arch/x86/msi.c                       | 4 ++--
>  xen/drivers/passthrough/amd/iommu_init.c | 4 ++--
>  xen/drivers/passthrough/vtd/iommu.c      | 4 ++--
>  xen/drivers/passthrough/vtd/quirks.c     | 2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 7be26c6a9b..d1ddc8ddf6 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -279,7 +279,7 @@ static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
>  {
>      ch->msi.msg = *msg;
>  
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>      {
>          int rc = iommu_update_ire_from_msi(&ch->msi, msg);
>  
> @@ -353,7 +353,7 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
>      u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
>      irq_desc_t *desc = irq_to_desc(ch->msi.irq);
>  
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>      {
>          ch->msi.hpet_id = hpet_blockid;
>          ret = iommu_setup_hpet_msi(&ch->msi);
> @@ -372,7 +372,7 @@ static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
>          ret = __hpet_setup_msi_irq(desc);
>      if ( ret < 0 )
>      {
> -        if ( iommu_intremap )
> +        if ( iommu_intremap != iommu_intremap_off )
>              iommu_update_ire_from_msi(&ch->msi, NULL);
>          return ret;
>      }
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 7f8e794254..72dce2e4ab 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -189,7 +189,7 @@ static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  {
>      entry->msg = *msg;
>  
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>      {
>          int rc;
>  
> @@ -555,7 +555,7 @@ int msi_free_irq(struct msi_desc *entry)
>              destroy_irq(entry[nr].irq);
>  
>          /* Free the unused IRTE if intr remap enabled */
> -        if ( iommu_intremap )
> +        if ( iommu_intremap != iommu_intremap_off )
>              iommu_update_ire_from_msi(entry + nr, NULL);
>      }
>  
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index 5515cb70fd..e02a09a9a7 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1480,7 +1480,7 @@ int __init amd_iommu_init(bool xt)
>              goto error_out;
>      }
>  
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>          register_keyhandler('V', &amd_iommu_dump_intremap_tables,
>                              "dump IOMMU intremap tables", 0);
>  
> @@ -1498,7 +1498,7 @@ int __init amd_iommu_init_late(void)
>  
>      /* Further initialize the device table(s). */
>      pci_init = true;
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>          rc = iterate_ivrs_mappings(amd_iommu_setup_device_table);
>  
>      for_each_amd_iommu ( iommu )
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index e13b7d99db..bd6d69a6f5 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2543,7 +2543,7 @@ static int __must_check init_vtd_hw(bool resume)
>      /*
>       * Enable interrupt remapping
>       */  
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>      {
>          int apic;
>          for ( apic = 0; apic < nr_ioapics; apic++ )
> @@ -2559,7 +2559,7 @@ static int __must_check init_vtd_hw(bool resume)
>              }
>          }
>      }
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>      {
>          for_each_drhd_unit ( drhd )
>          {
> diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
> index 5a56565ea8..950dcd56ef 100644
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -392,7 +392,7 @@ void __init platform_quirks_init(void)
>      map_igd_reg();
>  
>      /* Tylersburg interrupt remap quirk */
> -    if ( iommu_intremap )
> +    if ( iommu_intremap != iommu_intremap_off )
>          tylersburg_intremap_quirk();
>  }
>  
> -- 
> 2.40.0
> 
>
Stefano Stabellini Feb. 7, 2024, 12:52 a.m. UTC | #4
On Mon, 5 Feb 2024, Jan Beulich wrote:
> On 05.02.2024 16:36, Nicola Vetrini wrote:
> > On 2023-12-13 17:10, Simone Ballarin wrote:
> >> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> >>
> >> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
> >> headline states:
> >> "The controlling expression of an if statement and the controlling
> >> expression of an iteration-statement shall have essentially Boolean 
> >> type".
> >>
> >> Add comparisons to avoid using enum constants as controlling 
> >> expressions
> >> to comply with Rule 14.4.
> >> No functional change.
> >>
> >> Signed-off-by: Maria Celeste Cesario  
> >> <maria.celeste.cesario@bugseng.com>
> >> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
> >> ---
> >> Changes in v2
> >> - rename prefix from AMD/IOMMU to x86/IOMMU
> >> - move changes on msi.c and hpet.c in this patch.
> >> ---
> >>  xen/arch/x86/hpet.c                      | 6 +++---
> >>  xen/arch/x86/msi.c                       | 4 ++--
> >>  xen/drivers/passthrough/amd/iommu_init.c | 4 ++--
> >>  xen/drivers/passthrough/vtd/iommu.c      | 4 ++--
> >>  xen/drivers/passthrough/vtd/quirks.c     | 2 +-
> >>  5 files changed, 10 insertions(+), 10 deletions(-)
> >>
> > 
> > +Stefano
> > 
> > Hi all,
> > 
> > this patch seems not to have been committed into staging, unlike the 
> > other patch from this series. Since these are the only remaining 
> > violations for Rule 14.4, then I think these changes could be reviewed.
> 
> It's no surprise the change isn't committed yet, when it hasn't had any
> of the necessary tags. As far as I'm concerned, I seem to recall
> indicating clearly that I'm not happy with this change, and hence acks
> would need to come from elsewhere.

Thanks Jan for not blocking it. I gave my reviewed-by, let's see if one
of the other x86 maintainers is OK to ack it.
Jan Beulich Feb. 7, 2024, 7:20 a.m. UTC | #5
On 07.02.2024 01:51, Stefano Stabellini wrote:
> On Wed, 13 Dec 2023, Simone Ballarin wrote:
>> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
>>
>> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
>> headline states:
>> "The controlling expression of an if statement and the controlling
>> expression of an iteration-statement shall have essentially Boolean type".
>>
>> Add comparisons to avoid using enum constants as controlling expressions
>> to comply with Rule 14.4.
>> No functional change.
>>
>> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
>> Signed-off-by: Simone Ballarin  <simone.ballarin@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> However it needs an ack from an x86 maintainer

And from respective IOMMU ones.

Jan
Roger Pau Monné March 12, 2024, 11:01 a.m. UTC | #6
On Wed, Dec 13, 2023 at 05:10:50PM +0100, Simone Ballarin wrote:
> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
> 
> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
> headline states:
> "The controlling expression of an if statement and the controlling
> expression of an iteration-statement shall have essentially Boolean type".
> 
> Add comparisons to avoid using enum constants as controlling expressions
> to comply with Rule 14.4.

If we really want to go this route, we also need to amend the comment
in iommu_intremap definition, as it's no longer valid:

extern enum __packed iommu_intremap {
   /*
    * In order to allow traditional boolean uses of the iommu_intremap
    * variable, the "off" value has to come first (yielding a value of zero).
    */
   iommu_intremap_off,

We no longer allow traditional boolean uses of iommu_intremap.

Thanks, Roger.
Nicola Vetrini March 15, 2024, 11:20 a.m. UTC | #7
On 2024-03-12 12:01, Roger Pau Monné wrote:
> On Wed, Dec 13, 2023 at 05:10:50PM +0100, Simone Ballarin wrote:
>> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com>
>> 
>> The xen sources contain violations of MISRA C:2012 Rule 14.4 whose
>> headline states:
>> "The controlling expression of an if statement and the controlling
>> expression of an iteration-statement shall have essentially Boolean 
>> type".
>> 
>> Add comparisons to avoid using enum constants as controlling 
>> expressions
>> to comply with Rule 14.4.
> 
> If we really want to go this route, we also need to amend the comment
> in iommu_intremap definition, as it's no longer valid:
> 
> extern enum __packed iommu_intremap {
>    /*
>     * In order to allow traditional boolean uses of the iommu_intremap
>     * variable, the "off" value has to come first (yielding a value of 
> zero).
>     */
>    iommu_intremap_off,
> 
> We no longer allow traditional boolean uses of iommu_intremap.
> 
> Thanks, Roger.

Noted, thanks.
diff mbox series

Patch

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 7be26c6a9b..d1ddc8ddf6 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -279,7 +279,7 @@  static int hpet_msi_write(struct hpet_event_channel *ch, struct msi_msg *msg)
 {
     ch->msi.msg = *msg;
 
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
     {
         int rc = iommu_update_ire_from_msi(&ch->msi, msg);
 
@@ -353,7 +353,7 @@  static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
     u32 cfg = hpet_read32(HPET_Tn_CFG(ch->idx));
     irq_desc_t *desc = irq_to_desc(ch->msi.irq);
 
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
     {
         ch->msi.hpet_id = hpet_blockid;
         ret = iommu_setup_hpet_msi(&ch->msi);
@@ -372,7 +372,7 @@  static int __init hpet_setup_msi_irq(struct hpet_event_channel *ch)
         ret = __hpet_setup_msi_irq(desc);
     if ( ret < 0 )
     {
-        if ( iommu_intremap )
+        if ( iommu_intremap != iommu_intremap_off )
             iommu_update_ire_from_msi(&ch->msi, NULL);
         return ret;
     }
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 7f8e794254..72dce2e4ab 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -189,7 +189,7 @@  static int write_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
 {
     entry->msg = *msg;
 
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
     {
         int rc;
 
@@ -555,7 +555,7 @@  int msi_free_irq(struct msi_desc *entry)
             destroy_irq(entry[nr].irq);
 
         /* Free the unused IRTE if intr remap enabled */
-        if ( iommu_intremap )
+        if ( iommu_intremap != iommu_intremap_off )
             iommu_update_ire_from_msi(entry + nr, NULL);
     }
 
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 5515cb70fd..e02a09a9a7 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1480,7 +1480,7 @@  int __init amd_iommu_init(bool xt)
             goto error_out;
     }
 
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
         register_keyhandler('V', &amd_iommu_dump_intremap_tables,
                             "dump IOMMU intremap tables", 0);
 
@@ -1498,7 +1498,7 @@  int __init amd_iommu_init_late(void)
 
     /* Further initialize the device table(s). */
     pci_init = true;
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
         rc = iterate_ivrs_mappings(amd_iommu_setup_device_table);
 
     for_each_amd_iommu ( iommu )
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index e13b7d99db..bd6d69a6f5 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2543,7 +2543,7 @@  static int __must_check init_vtd_hw(bool resume)
     /*
      * Enable interrupt remapping
      */  
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
     {
         int apic;
         for ( apic = 0; apic < nr_ioapics; apic++ )
@@ -2559,7 +2559,7 @@  static int __must_check init_vtd_hw(bool resume)
             }
         }
     }
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
     {
         for_each_drhd_unit ( drhd )
         {
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 5a56565ea8..950dcd56ef 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -392,7 +392,7 @@  void __init platform_quirks_init(void)
     map_igd_reg();
 
     /* Tylersburg interrupt remap quirk */
-    if ( iommu_intremap )
+    if ( iommu_intremap != iommu_intremap_off )
         tylersburg_intremap_quirk();
 }