[XEN PATCH v2 0/4] x86: address some violations of MISRA C:2012 Rule 5.3

[XEN PATCH v2 0/4] x86: address some violations of MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Some variables are renamed or deleted in this series to avoid shadowing, which
violates MISRA C:2012 Rule 5.3, whose headline is:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope".

---
Changes in v2:
- Addressed comments on patches 1 and 2. Patches 3 and 4 are unchanged.

Nicola Vetrini (4):
  x86/mce: address MISRA C:2012 Rule 5.3
  x86/mtrr: address MISRA C:2012 Rule 5.3
  x86/irq: rename variable to address MISRA C:2012 Rule 5.3
  x86/setup: address MISRA C:2012 Rule 5.3

 xen/arch/x86/cpu/mcheck/barrier.c |  8 ++++----
 xen/arch/x86/cpu/mcheck/barrier.h | 14 +++++++-------
 xen/arch/x86/hvm/hvm.c            |  2 +-
 xen/arch/x86/hvm/mtrr.c           | 32 +++++++++++++++----------------
 xen/arch/x86/include/asm/irq.h    |  2 +-
 xen/arch/x86/include/asm/setup.h  |  2 +-
 xen/arch/x86/io_apic.c            | 10 +++++-----
 xen/arch/x86/irq.c                | 12 ++++++------
 xen/arch/x86/msi.c                |  4 ++--
 xen/arch/x86/setup.c              |  3 +--
 xen/include/xen/irq.h             |  2 +-
 11 files changed, 45 insertions(+), 46 deletions(-)

--
2.34.1

[XEN PATCH v2 1/4] x86/mce: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Suitable mechanical renames are made to avoid shadowing
the function identifier 'wait' declared in 'xen/include/xen/wait.h',
thus addressing violations of MISRA C:2012 Rule 5.3:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

The parameter name 'bar' is added as well to comply with MISRA C:2012
Rules 8.2 and 8.3.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Added parameter name 'bar' where missing.
- Amended commit message.
---
 xen/arch/x86/cpu/mcheck/barrier.c |  8 ++++----
 xen/arch/x86/cpu/mcheck/barrier.h | 14 +++++++-------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/cpu/mcheck/barrier.c b/xen/arch/x86/cpu/mcheck/barrier.c
index a7e5b19a44..51a1d37a76 100644
--- a/xen/arch/x86/cpu/mcheck/barrier.c
+++ b/xen/arch/x86/cpu/mcheck/barrier.c
@@ -16,11 +16,11 @@ void mce_barrier_dec(struct mce_softirq_barrier *bar)
     atomic_dec(&bar->val);
 }

-void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
+void mce_barrier_enter(struct mce_softirq_barrier *bar, bool do_wait)
 {
     int gen;

-    if ( !wait )
+    if ( !do_wait )
         return;
     atomic_inc(&bar->ingen);
     gen = atomic_read(&bar->outgen);
@@ -34,11 +34,11 @@ void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
     }
 }

-void mce_barrier_exit(struct mce_softirq_barrier *bar, bool wait)
+void mce_barrier_exit(struct mce_softirq_barrier *bar, bool do_wait)
 {
     int gen;

-    if ( !wait )
+    if ( !do_wait )
         return;
     atomic_inc(&bar->outgen);
     gen = atomic_read(&bar->ingen);
diff --git a/xen/arch/x86/cpu/mcheck/barrier.h b/xen/arch/x86/cpu/mcheck/barrier.h
index c4d52b6192..7ec483226f 100644
--- a/xen/arch/x86/cpu/mcheck/barrier.h
+++ b/xen/arch/x86/cpu/mcheck/barrier.h
@@ -20,7 +20,7 @@ struct mce_softirq_barrier {
 /*
  * Initialize a barrier. Just set it to 0.
  */
-void mce_barrier_init(struct mce_softirq_barrier *);
+void mce_barrier_init(struct mce_softirq_barrier *bar);

 /*
  * This function will need to be used when offlining a CPU in the
@@ -29,17 +29,17 @@ void mce_barrier_init(struct mce_softirq_barrier *);
  * Decrement a barrier only. Needed for cases where the CPU
  * in question can't do it itself (e.g. it is being offlined).
  */
-void mce_barrier_dec(struct mce_softirq_barrier *);
+void mce_barrier_dec(struct mce_softirq_barrier *bar);

 /*
- * If @wait is false, mce_barrier_enter/exit() will return immediately
+ * If @do_wait is false, mce_barrier_enter/exit() will return immediately
  * without touching the barrier. It's used when handling a
  * non-broadcasting MCE (e.g. MCE on some old Intel CPU, MCE on AMD
  * CPU and LMCE on Intel Skylake-server CPU) which is received on only
  * one CPU and thus does not invoke mce_barrier_enter/exit() calls on
  * all CPUs.
  *
- * If @wait is true, mce_barrier_enter/exit() will handle the given
+ * If @do_wait is true, mce_barrier_enter/exit() will handle the given
  * barrier as below.
  *
  * Increment the generation number and the value. The generation number
@@ -53,9 +53,9 @@ void mce_barrier_dec(struct mce_softirq_barrier *);
  * These barrier functions should always be paired, so that the
  * counter value will reach 0 again after all CPUs have exited.
  */
-void mce_barrier_enter(struct mce_softirq_barrier *, bool wait);
-void mce_barrier_exit(struct mce_softirq_barrier *, bool wait);
+void mce_barrier_enter(struct mce_softirq_barrier *bar, bool do_wait);
+void mce_barrier_exit(struct mce_softirq_barrier *bar, bool do_wait);

-void mce_barrier(struct mce_softirq_barrier *);
+void mce_barrier(struct mce_softirq_barrier *bar);

 #endif /* _MCHECK_BARRIER_H */
--
2.34.1

Re: [XEN PATCH v2 1/4] x86/mce: address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> Suitable mechanical renames are made to avoid shadowing
> the function identifier 'wait' declared in 'xen/include/xen/wait.h',
> thus addressing violations of MISRA C:2012 Rule 5.3:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> The parameter name 'bar' is added as well to comply with MISRA C:2012
> Rules 8.2 and 8.3.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
> Changes in v2:
> - Added parameter name 'bar' where missing.
> - Amended commit message.
> ---
>  xen/arch/x86/cpu/mcheck/barrier.c |  8 ++++----
>  xen/arch/x86/cpu/mcheck/barrier.h | 14 +++++++-------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/barrier.c b/xen/arch/x86/cpu/mcheck/barrier.c
> index a7e5b19a44..51a1d37a76 100644
> --- a/xen/arch/x86/cpu/mcheck/barrier.c
> +++ b/xen/arch/x86/cpu/mcheck/barrier.c
> @@ -16,11 +16,11 @@ void mce_barrier_dec(struct mce_softirq_barrier *bar)
>      atomic_dec(&bar->val);
>  }
> 
> -void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
> +void mce_barrier_enter(struct mce_softirq_barrier *bar, bool do_wait)
>  {
>      int gen;
> 
> -    if ( !wait )
> +    if ( !do_wait )
>          return;
>      atomic_inc(&bar->ingen);
>      gen = atomic_read(&bar->outgen);
> @@ -34,11 +34,11 @@ void mce_barrier_enter(struct mce_softirq_barrier *bar, bool wait)
>      }
>  }
> 
> -void mce_barrier_exit(struct mce_softirq_barrier *bar, bool wait)
> +void mce_barrier_exit(struct mce_softirq_barrier *bar, bool do_wait)
>  {
>      int gen;
> 
> -    if ( !wait )
> +    if ( !do_wait )
>          return;
>      atomic_inc(&bar->outgen);
>      gen = atomic_read(&bar->ingen);
> diff --git a/xen/arch/x86/cpu/mcheck/barrier.h b/xen/arch/x86/cpu/mcheck/barrier.h
> index c4d52b6192..7ec483226f 100644
> --- a/xen/arch/x86/cpu/mcheck/barrier.h
> +++ b/xen/arch/x86/cpu/mcheck/barrier.h
> @@ -20,7 +20,7 @@ struct mce_softirq_barrier {
>  /*
>   * Initialize a barrier. Just set it to 0.
>   */
> -void mce_barrier_init(struct mce_softirq_barrier *);
> +void mce_barrier_init(struct mce_softirq_barrier *bar);
> 
>  /*
>   * This function will need to be used when offlining a CPU in the
> @@ -29,17 +29,17 @@ void mce_barrier_init(struct mce_softirq_barrier *);
>   * Decrement a barrier only. Needed for cases where the CPU
>   * in question can't do it itself (e.g. it is being offlined).
>   */
> -void mce_barrier_dec(struct mce_softirq_barrier *);
> +void mce_barrier_dec(struct mce_softirq_barrier *bar);
> 
>  /*
> - * If @wait is false, mce_barrier_enter/exit() will return immediately
> + * If @do_wait is false, mce_barrier_enter/exit() will return immediately
>   * without touching the barrier. It's used when handling a
>   * non-broadcasting MCE (e.g. MCE on some old Intel CPU, MCE on AMD
>   * CPU and LMCE on Intel Skylake-server CPU) which is received on only
>   * one CPU and thus does not invoke mce_barrier_enter/exit() calls on
>   * all CPUs.
>   *
> - * If @wait is true, mce_barrier_enter/exit() will handle the given
> + * If @do_wait is true, mce_barrier_enter/exit() will handle the given
>   * barrier as below.
>   *
>   * Increment the generation number and the value. The generation number
> @@ -53,9 +53,9 @@ void mce_barrier_dec(struct mce_softirq_barrier *);
>   * These barrier functions should always be paired, so that the
>   * counter value will reach 0 again after all CPUs have exited.
>   */
> -void mce_barrier_enter(struct mce_softirq_barrier *, bool wait);
> -void mce_barrier_exit(struct mce_softirq_barrier *, bool wait);
> +void mce_barrier_enter(struct mce_softirq_barrier *bar, bool do_wait);
> +void mce_barrier_exit(struct mce_softirq_barrier *bar, bool do_wait);
> 
> -void mce_barrier(struct mce_softirq_barrier *);
> +void mce_barrier(struct mce_softirq_barrier *bar);
> 
>  #endif /* _MCHECK_BARRIER_H */
> --
> 2.34.1
> 

Re: [XEN PATCH v2 1/4] x86/mce: address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 04.08.2023 22:43, Stefano Stabellini wrote:
> On Fri, 4 Aug 2023, Nicola Vetrini wrote:
>> Suitable mechanical renames are made to avoid shadowing
>> the function identifier 'wait' declared in 'xen/include/xen/wait.h',
>> thus addressing violations of MISRA C:2012 Rule 5.3:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>>
>> The parameter name 'bar' is added as well to comply with MISRA C:2012
>> Rules 8.2 and 8.3.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Acked-by: Jan Beulich <jbeulich@suse.com>

[XEN PATCH v2 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Rename variables to avoid shadowing and thus address
MISRA C:2012 Rule 5.3:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope".

The shadowing happens between the struct declaration 'mtrr_state' in
'xen/arch/x86/include/asm/mtrr.h' and local variable names.
The latter are renamed to 'm', which is used elsewhere in
'xen/arch/x86/hvm/mtrr.c' for the same purpose.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Renamed 'mtrr' local variables to 'm'.
- Added references in the commit message.
---
 xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 29f3fb1607..7f486358b1 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,

 static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
 {
-    const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
+    const struct mtrr_state *m = &v->arch.hvm.mtrr;
     struct hvm_hw_mtrr hw_mtrr = {
-        .msr_mtrr_def_type = mtrr_state->def_type |
-                             MASK_INSR(mtrr_state->fixed_enabled,
+        .msr_mtrr_def_type = m->def_type |
+                             MASK_INSR(m->fixed_enabled,
                                        MTRRdefType_FE) |
-                            MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
-        .msr_mtrr_cap      = mtrr_state->mtrr_cap,
+                            MASK_INSR(m->enabled, MTRRdefType_E),
+        .msr_mtrr_cap      = m->mtrr_cap,
     };
     unsigned int i;

@@ -710,14 +710,14 @@ static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)

     for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
     {
-        hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
-        hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
+        hw_mtrr.msr_mtrr_var[i * 2] = m->var_ranges->base;
+        hw_mtrr.msr_mtrr_var[i * 2 + 1] = m->var_ranges->mask;
     }

     BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) !=
-                 sizeof(mtrr_state->fixed_ranges));
+                 sizeof(m->fixed_ranges));

-    memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
+    memcpy(hw_mtrr.msr_mtrr_fixed, m->fixed_ranges,
            sizeof(hw_mtrr.msr_mtrr_fixed));

     return hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr);
@@ -727,7 +727,7 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
 {
     unsigned int vcpuid, i;
     struct vcpu *v;
-    struct mtrr_state *mtrr_state;
+    struct mtrr_state *m;
     struct hvm_hw_mtrr hw_mtrr;

     vcpuid = hvm_load_instance(h);
@@ -749,26 +749,26 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
         return -EINVAL;
     }

-    mtrr_state = &v->arch.hvm.mtrr;
+    m = &v->arch.hvm.mtrr;

     hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);

-    mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
+    m->mtrr_cap = hw_mtrr.msr_mtrr_cap;

     for ( i = 0; i < NUM_FIXED_MSR; i++ )
-        mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
+        mtrr_fix_range_msr_set(d, m, i, hw_mtrr.msr_mtrr_fixed[i]);

     for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
     {
-        mtrr_var_range_msr_set(d, mtrr_state,
+        mtrr_var_range_msr_set(d, m,
                                MSR_IA32_MTRR_PHYSBASE(i),
                                hw_mtrr.msr_mtrr_var[i * 2]);
-        mtrr_var_range_msr_set(d, mtrr_state,
+        mtrr_var_range_msr_set(d, m,
                                MSR_IA32_MTRR_PHYSMASK(i),
                                hw_mtrr.msr_mtrr_var[i * 2 + 1]);
     }

-    mtrr_def_type_msr_set(d, mtrr_state, hw_mtrr.msr_mtrr_def_type);
+    mtrr_def_type_msr_set(d, m, hw_mtrr.msr_mtrr_def_type);

     return 0;
 }
--
2.34.1

Re: [XEN PATCH v2 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> Rename variables to avoid shadowing and thus address
> MISRA C:2012 Rule 5.3:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope".
> 
> The shadowing happens between the struct declaration 'mtrr_state' in
> 'xen/arch/x86/include/asm/mtrr.h' and local variable names.
> The latter are renamed to 'm', which is used elsewhere in
> 'xen/arch/x86/hvm/mtrr.c' for the same purpose.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
> Changes in v2:
> - Renamed 'mtrr' local variables to 'm'.
> - Added references in the commit message.
> ---
>  xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
> index 29f3fb1607..7f486358b1 100644
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -687,13 +687,13 @@ int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
> 
>  static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
>  {
> -    const struct mtrr_state *mtrr_state = &v->arch.hvm.mtrr;
> +    const struct mtrr_state *m = &v->arch.hvm.mtrr;
>      struct hvm_hw_mtrr hw_mtrr = {
> -        .msr_mtrr_def_type = mtrr_state->def_type |
> -                             MASK_INSR(mtrr_state->fixed_enabled,
> +        .msr_mtrr_def_type = m->def_type |
> +                             MASK_INSR(m->fixed_enabled,
>                                         MTRRdefType_FE) |
> -                            MASK_INSR(mtrr_state->enabled, MTRRdefType_E),
> -        .msr_mtrr_cap      = mtrr_state->mtrr_cap,
> +                            MASK_INSR(m->enabled, MTRRdefType_E),
> +        .msr_mtrr_cap      = m->mtrr_cap,
>      };
>      unsigned int i;
> 
> @@ -710,14 +710,14 @@ static int cf_check hvm_save_mtrr_msr(struct vcpu *v, hvm_domain_context_t *h)
> 
>      for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
>      {
> -        hw_mtrr.msr_mtrr_var[i * 2] = mtrr_state->var_ranges->base;
> -        hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr_state->var_ranges->mask;
> +        hw_mtrr.msr_mtrr_var[i * 2] = m->var_ranges->base;
> +        hw_mtrr.msr_mtrr_var[i * 2 + 1] = m->var_ranges->mask;
>      }
> 
>      BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) !=
> -                 sizeof(mtrr_state->fixed_ranges));
> +                 sizeof(m->fixed_ranges));
> 
> -    memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges,
> +    memcpy(hw_mtrr.msr_mtrr_fixed, m->fixed_ranges,
>             sizeof(hw_mtrr.msr_mtrr_fixed));
> 
>      return hvm_save_entry(MTRR, v->vcpu_id, h, &hw_mtrr);
> @@ -727,7 +727,7 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
>  {
>      unsigned int vcpuid, i;
>      struct vcpu *v;
> -    struct mtrr_state *mtrr_state;
> +    struct mtrr_state *m;
>      struct hvm_hw_mtrr hw_mtrr;
> 
>      vcpuid = hvm_load_instance(h);
> @@ -749,26 +749,26 @@ static int cf_check hvm_load_mtrr_msr(struct domain *d, hvm_domain_context_t *h)
>          return -EINVAL;
>      }
> 
> -    mtrr_state = &v->arch.hvm.mtrr;
> +    m = &v->arch.hvm.mtrr;
> 
>      hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr);
> 
> -    mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap;
> +    m->mtrr_cap = hw_mtrr.msr_mtrr_cap;
> 
>      for ( i = 0; i < NUM_FIXED_MSR; i++ )
> -        mtrr_fix_range_msr_set(d, mtrr_state, i, hw_mtrr.msr_mtrr_fixed[i]);
> +        mtrr_fix_range_msr_set(d, m, i, hw_mtrr.msr_mtrr_fixed[i]);
> 
>      for ( i = 0; i < MASK_EXTR(hw_mtrr.msr_mtrr_cap, MTRRcap_VCNT); i++ )
>      {
> -        mtrr_var_range_msr_set(d, mtrr_state,
> +        mtrr_var_range_msr_set(d, m,
>                                 MSR_IA32_MTRR_PHYSBASE(i),
>                                 hw_mtrr.msr_mtrr_var[i * 2]);
> -        mtrr_var_range_msr_set(d, mtrr_state,
> +        mtrr_var_range_msr_set(d, m,
>                                 MSR_IA32_MTRR_PHYSMASK(i),
>                                 hw_mtrr.msr_mtrr_var[i * 2 + 1]);
>      }
> 
> -    mtrr_def_type_msr_set(d, mtrr_state, hw_mtrr.msr_mtrr_def_type);
> +    mtrr_def_type_msr_set(d, m, hw_mtrr.msr_mtrr_def_type);
> 
>      return 0;
>  }
> --
> 2.34.1
> 

Re: [XEN PATCH v2 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 04.08.2023 22:45, Stefano Stabellini wrote:
> On Fri, 4 Aug 2023, Nicola Vetrini wrote:
>> Rename variables to avoid shadowing and thus address
>> MISRA C:2012 Rule 5.3:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope".
>>
>> The shadowing happens between the struct declaration 'mtrr_state' in
>> 'xen/arch/x86/include/asm/mtrr.h' and local variable names.

Let's try to be precise: The issue isn't with the struct declaration,
but with the declaration of the global variable of that name a few
lines later. Afaict - please clarify.

>> The latter are renamed to 'm', which is used elsewhere in
>> 'xen/arch/x86/hvm/mtrr.c' for the same purpose.
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

With - as necessary - a suitable adjustment to the description:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN PATCH v2 2/4] x86/mtrr: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 07/08/2023 14:48, Jan Beulich wrote:
> On 04.08.2023 22:45, Stefano Stabellini wrote:
>> On Fri, 4 Aug 2023, Nicola Vetrini wrote:
>>> Rename variables to avoid shadowing and thus address
>>> MISRA C:2012 Rule 5.3:
>>> "An identifier declared in an inner scope shall not hide an
>>> identifier declared in an outer scope".
>>> 
>>> The shadowing happens between the struct declaration 'mtrr_state' in
>>> 'xen/arch/x86/include/asm/mtrr.h' and local variable names.
> 
> Let's try to be precise: The issue isn't with the struct declaration,
> but with the declaration of the global variable of that name a few
> lines later. Afaict - please clarify.

Yes, my wording is wrong.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

[XEN PATCH v2 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
local variables in the changed file. To avoid this, the variable is
renamed to 'irq_description'.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/hvm.c         |  2 +-
 xen/arch/x86/include/asm/irq.h |  2 +-
 xen/arch/x86/io_apic.c         | 10 +++++-----
 xen/arch/x86/irq.c             | 12 ++++++------
 xen/arch/x86/msi.c             |  4 ++--
 xen/include/xen/irq.h          |  2 +-
 6 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2180abeb33..ca5bb96388 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -474,7 +474,7 @@ void hvm_migrate_pirq(struct hvm_pirq_dpci *pirq_dpci, const struct vcpu *v)
 
         if ( !desc )
             return;
-        ASSERT(MSI_IRQ(desc - irq_desc));
+        ASSERT(MSI_IRQ(desc - irq_descriptor));
         irq_set_affinity(desc, cpumask_of(v->processor));
         spin_unlock_irq(&desc->lock);
     }
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index ad907fc97f..f6df977170 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -172,7 +172,7 @@ int assign_irq_vector(int irq, const cpumask_t *mask);
 
 void cf_check irq_complete_move(struct irq_desc *desc);
 
-extern struct irq_desc *irq_desc;
+extern struct irq_desc *irq_descriptor;
 
 void lock_vector_lock(void);
 void unlock_vector_lock(void);
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b3afef8933..b59d6cfb9e 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -990,9 +990,9 @@ static inline void ioapic_register_intr(int irq, unsigned long trigger)
 {
     if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
         trigger == IOAPIC_LEVEL)
-        irq_desc[irq].handler = &ioapic_level_type;
+        irq_descriptor[irq].handler = &ioapic_level_type;
     else
-        irq_desc[irq].handler = &ioapic_edge_type;
+        irq_descriptor[irq].handler = &ioapic_edge_type;
 }
 
 static void __init setup_IO_APIC_irqs(void)
@@ -1098,7 +1098,7 @@ static void __init setup_ExtINT_IRQ0_pin(unsigned int apic, unsigned int pin, in
      * The timer IRQ doesn't have to know that behind the
      * scene we have a 8259A-master in AEOI mode ...
      */
-    irq_desc[0].handler = &ioapic_edge_type;
+    irq_descriptor[0].handler = &ioapic_edge_type;
 
     /*
      * Add it to the IO-APIC irq-routing table:
@@ -1912,7 +1912,7 @@ static void __init check_timer(void)
     if ((ret = bind_irq_vector(0, vector, &cpumask_all)))
         printk(KERN_ERR"..IRQ0 is not set correctly with ioapic!!!, err:%d\n", ret);
     
-    irq_desc[0].status &= ~IRQ_DISABLED;
+    irq_descriptor[0].status &= ~IRQ_DISABLED;
 
     /*
      * Subtle, code in do_timer_interrupt() expects an AEOI
@@ -2009,7 +2009,7 @@ static void __init check_timer(void)
     printk(KERN_INFO "...trying to set up timer as Virtual Wire IRQ...");
 
     disable_8259A_irq(irq_to_desc(0));
-    irq_desc[0].handler = &lapic_irq_type;
+    irq_descriptor[0].handler = &lapic_irq_type;
     apic_write(APIC_LVT0, APIC_DM_FIXED | vector);	/* Fixed mode */
     enable_8259A_irq(irq_to_desc(0));
 
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 6abfd81621..ed95896bce 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -45,7 +45,7 @@ integer_param("irq-max-guests", irq_max_guests);
 
 vmask_t global_used_vector_map;
 
-struct irq_desc __read_mostly *irq_desc = NULL;
+struct irq_desc __read_mostly *irq_descriptor = NULL;
 
 static DECLARE_BITMAP(used_vectors, X86_NR_VECTORS);
 
@@ -424,9 +424,9 @@ int __init init_irq_data(void)
     for ( vector = 0; vector < X86_NR_VECTORS; ++vector )
         this_cpu(vector_irq)[vector] = INT_MIN;
 
-    irq_desc = xzalloc_array(struct irq_desc, nr_irqs);
-    
-    if ( !irq_desc )
+    irq_descriptor = xzalloc_array(struct irq_desc, nr_irqs);
+
+    if ( !irq_descriptor )
         return -ENOMEM;
 
     for ( irq = 0; irq < nr_irqs_gsi; irq++ )
@@ -1133,7 +1133,7 @@ static void cf_check set_eoi_ready(void *data);
 static void cf_check irq_guest_eoi_timer_fn(void *data)
 {
     struct irq_desc *desc = data;
-    unsigned int i, irq = desc - irq_desc;
+    unsigned int i, irq = desc - irq_descriptor;
     irq_guest_action_t *action;
 
     spin_lock_irq(&desc->lock);
@@ -1382,7 +1382,7 @@ static void __set_eoi_ready(const struct irq_desc *desc)
     struct pending_eoi *peoi = this_cpu(pending_eoi);
     int                 irq, sp;
 
-    irq = desc - irq_desc;
+    irq = desc - irq_descriptor;
 
     if ( !action || action->in_flight ||
          !cpumask_test_and_clear_cpu(smp_processor_id(),
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index d0bf63df1d..35d417c63a 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1322,7 +1322,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
         unsigned int i = 0, nr = 1;
 
         irq = entry->irq;
-        desc = &irq_desc[irq];
+        desc = &irq_descriptor[irq];
 
         spin_lock_irqsave(&desc->lock, flags);
 
@@ -1377,7 +1377,7 @@ int pci_restore_msi_state(struct pci_dev *pdev)
                 break;
 
             spin_unlock_irqrestore(&desc->lock, flags);
-            desc = &irq_desc[entry[++i].irq];
+            desc = &irq_descriptor[entry[++i].irq];
             spin_lock_irqsave(&desc->lock, flags);
             if ( desc->msi_desc != entry + i )
                 goto bogus;
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 9747e818f7..56a3aa6a29 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -107,7 +107,7 @@ typedef struct irq_desc {
 } __cacheline_aligned irq_desc_t;
 
 #ifndef irq_to_desc
-#define irq_to_desc(irq)    (&irq_desc[irq])
+#define irq_to_desc(irq)    (&irq_descriptor[irq])
 #endif
 
 int init_one_irq_desc(struct irq_desc *desc);
-- 
2.34.1

Re: [XEN PATCH v2 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
> local variables in the changed file. To avoid this, the variable is
> renamed to 'irq_description'.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

You missed me reviewed tag from the previous version:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Also Jan, sorry I am already losing my new habit of trimming my replies
:-)

Re: [XEN PATCH v2 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 04/08/2023 22:47, Stefano Stabellini wrote:
> On Fri, 4 Aug 2023, Nicola Vetrini wrote:
>> The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
>> local variables in the changed file. To avoid this, the variable is
>> renamed to 'irq_description'.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> You missed me reviewed tag from the previous version:
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

You're right, sorry.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 3/4] x86/irq: rename variable to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 04.08.2023 10:03, Nicola Vetrini wrote:
> The extern variable 'irq_desc' defined in 'irq.h' is shadowed by
> local variables in the changed file. To avoid this, the variable is
> renamed to 'irq_description'.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/hvm/hvm.c         |  2 +-
>  xen/arch/x86/include/asm/irq.h |  2 +-
>  xen/arch/x86/io_apic.c         | 10 +++++-----
>  xen/arch/x86/irq.c             | 12 ++++++------
>  xen/arch/x86/msi.c             |  4 ++--
>  xen/include/xen/irq.h          |  2 +-
>  6 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 2180abeb33..ca5bb96388 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -474,7 +474,7 @@ void hvm_migrate_pirq(struct hvm_pirq_dpci *pirq_dpci, const struct vcpu *v)
>  
>          if ( !desc )
>              return;
> -        ASSERT(MSI_IRQ(desc - irq_desc));
> +        ASSERT(MSI_IRQ(desc - irq_descriptor));
>          irq_set_affinity(desc, cpumask_of(v->processor));
>          spin_unlock_irq(&desc->lock);
>      }
> diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
> index ad907fc97f..f6df977170 100644
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -172,7 +172,7 @@ int assign_irq_vector(int irq, const cpumask_t *mask);
>  
>  void cf_check irq_complete_move(struct irq_desc *desc);
>  
> -extern struct irq_desc *irq_desc;
> +extern struct irq_desc *irq_descriptor;

In Arm code you'll find

static irq_desc_t irq_desc[NR_IRQS];

It's static there right now, yes, but we don't want to introduce arch
differences here. Therefore the global wants leaving alone, and "desc"
wants using where function parameters collide. (Arm uses either "desc"
or "irqd" everywhere afaics.)

Jan

[XEN PATCH v2 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The parameters renamed in the function declaration caused shadowing
with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
them also addresses Rule 8.3:
"All declarations of an object or function shall use the same names
and type qualifiers".

The local variable 'mask' is removed because it shadows the homonymous
variable defined in an outer scope, with no change to the semantics.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/include/asm/setup.h | 2 +-
 xen/arch/x86/setup.c             | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 51fce66607..b0e6a39e23 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -33,7 +33,7 @@ static inline void vesa_init(void) {};
 
 int construct_dom0(
     struct domain *d,
-    const module_t *kernel, unsigned long kernel_headroom,
+    const module_t *image, unsigned long image_headroom,
     module_t *initrd,
     const char *cmdline);
 void setup_io_bitmap(struct domain *d);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 2dbe9857aa..80ae973d64 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1577,8 +1577,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         s = map_s;
         if ( s < map_e )
         {
-            uint64_t mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
-
+            mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
             map_s = (s + mask) & ~mask;
             map_e &= ~mask;
             init_boot_pages(map_s, map_e);
-- 
2.34.1

Re: [XEN PATCH v2 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> The parameters renamed in the function declaration caused shadowing
> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
> them also addresses Rule 8.3:
> "All declarations of an object or function shall use the same names
> and type qualifiers".
> 
> The local variable 'mask' is removed because it shadows the homonymous
> variable defined in an outer scope, with no change to the semantics.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Same here

Re: [XEN PATCH v2 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 04.08.2023 10:03, Nicola Vetrini wrote:
> The parameters renamed in the function declaration caused shadowing
> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
> them also addresses Rule 8.3:
> "All declarations of an object or function shall use the same names
> and type qualifiers".

Can you explain to me how shadowing can happen in a declaration? I
would focus on 8.3 here, and only mention the other name collision.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1577,8 +1577,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          s = map_s;
>          if ( s < map_e )
>          {
> -            uint64_t mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
> -
> +            mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
>              map_s = (s + mask) & ~mask;
>              map_e &= ~mask;
>              init_boot_pages(map_s, map_e);

Re-using the outer scope variable is a little risky, don't you agree?
It just so happens that below here there's no further use requiring
the earlier value (PAGE_SIZE - 1). This isn't to say I'm opposed, but
it certainly needs evaluating with this in mind (mentioning of which
in the description would have demonstrated that you did consider this
aspect).

Jan

Re: [XEN PATCH v2 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 07/08/2023 15:05, Jan Beulich wrote:
> On 04.08.2023 10:03, Nicola Vetrini wrote:
>> The parameters renamed in the function declaration caused shadowing
>> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
>> them also addresses Rule 8.3:
>> "All declarations of an object or function shall use the same names
>> and type qualifiers".
> 
> Can you explain to me how shadowing can happen in a declaration? I
> would focus on 8.3 here, and only mention the other name collision.

There's "static struct file __initdata kernel;" in 
xen/common/efi/boot.c, which
is visible when the function is declared. Since renaming these parameter 
names would
have been addressed by Federico for R8.3 anyway, my intention was to 
address them both.

> 
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1577,8 +1577,7 @@ void __init noreturn __start_xen(unsigned long 
>> mbi_p)
>>          s = map_s;
>>          if ( s < map_e )
>>          {
>> -            uint64_t mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
>> -
>> +            mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
>>              map_s = (s + mask) & ~mask;
>>              map_e &= ~mask;
>>              init_boot_pages(map_s, map_e);
> 
> Re-using the outer scope variable is a little risky, don't you agree?
> It just so happens that below here there's no further use requiring
> the earlier value (PAGE_SIZE - 1). This isn't to say I'm opposed, but
> it certainly needs evaluating with this in mind (mentioning of which
> in the description would have demonstrated that you did consider this
> aspect).
> 
> Jan

I guess I should have mentioned it

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 07.08.2023 15:18, Nicola Vetrini wrote:
> On 07/08/2023 15:05, Jan Beulich wrote:
>> On 04.08.2023 10:03, Nicola Vetrini wrote:
>>> The parameters renamed in the function declaration caused shadowing
>>> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
>>> them also addresses Rule 8.3:
>>> "All declarations of an object or function shall use the same names
>>> and type qualifiers".
>>
>> Can you explain to me how shadowing can happen in a declaration? I
>> would focus on 8.3 here, and only mention the other name collision.
> 
> There's "static struct file __initdata kernel;" in 
> xen/common/efi/boot.c, which
> is visible when the function is declared. Since renaming these parameter 
> names would
> have been addressed by Federico for R8.3 anyway, my intention was to 
> address them both.

I understand what you say, but your reply doesn't answer my question.
Just to emphasize the important aspect: I could see the shadowing
aspect if the _definition_ of construct_dom0() used "kernel". But I'm
asking about declarations (the one here as well as in general): I
can't see how any shadowing can occur without there being any code in
the position of using any such variable / parameter. IOW if Eclair
spits out 5.3 violations on declarations, I'm inclined to think it's 
wrong. (Because of 8.3 a violation there would then need dealing with
anyway, but _only_ because of 8.3, if the definition is already okay.)

Jan

Re: [XEN PATCH v2 4/4] x86/setup: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 07/08/2023 15:42, Jan Beulich wrote:
> On 07.08.2023 15:18, Nicola Vetrini wrote:
>> On 07/08/2023 15:05, Jan Beulich wrote:
>>> On 04.08.2023 10:03, Nicola Vetrini wrote:
>>>> The parameters renamed in the function declaration caused shadowing
>>>> with the homonymous variable in 'xen/common/efi/boot.c'. Renaming
>>>> them also addresses Rule 8.3:
>>>> "All declarations of an object or function shall use the same names
>>>> and type qualifiers".
>>> 
>>> Can you explain to me how shadowing can happen in a declaration? I
>>> would focus on 8.3 here, and only mention the other name collision.
>> 
>> There's "static struct file __initdata kernel;" in
>> xen/common/efi/boot.c, which
>> is visible when the function is declared. Since renaming these 
>> parameter
>> names would
>> have been addressed by Federico for R8.3 anyway, my intention was to
>> address them both.
> 
> I understand what you say, but your reply doesn't answer my question.
> Just to emphasize the important aspect: I could see the shadowing
> aspect if the _definition_ of construct_dom0() used "kernel". But I'm
> asking about declarations (the one here as well as in general): I
> can't see how any shadowing can occur without there being any code in
> the position of using any such variable / parameter. IOW if Eclair
> spits out 5.3 violations on declarations, I'm inclined to think it's
> wrong. (Because of 8.3 a violation there would then need dealing with
> anyway, but _only_ because of 8.3, if the definition is already okay.)
> 
> Jan

The declaration itself is a scope and shadowing can happen, as in:

int x;
void f(int x, int arr[x]);

Now, the example is a bit contrived, but the fact that the rule does not 
list any
exception motivates this behaviour. In any case, I'll try to rephrase 
the commit message
to be less ambiguous.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)