diff mbox series

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

Message ID 2f7380396ee146132738dd5c6b2a80d62a8035d5.1691135862.git.nicola.vetrini@bugseng.com (mailing list archive)
State New, archived
Headers show
Series x86: address some violations of MISRA C:2012 Rule 5.3 | expand

Commit Message

Nicola Vetrini Aug. 4, 2023, 8:03 a.m. UTC 
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(-)

--
2.34.1

Comments

Stefano Stabellini Aug. 4, 2023, 8:45 p.m. UTC | #1 
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
>
Jan Beulich Aug. 7, 2023, 12:48 p.m. UTC | #2 
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
Nicola Vetrini Aug. 7, 2023, 4:43 p.m. UTC | #3 
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.
diff mbox series

Patch

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;
 }