| Message ID | 16fa23ecb465442c566a18af0a569092075eef26.1690969271.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: address some violations of MISRA C:2012 Rule 5.3 | expand |
On Wed, 2 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" > > No functional changes. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> This one clashes with xen/arch/x86/include/asm/mtrr.h:struct mtrr_state right? > --- > 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..d504d1e43b 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 *mtrr = &v->arch.hvm.mtrr; I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places that we use: const struct mtrr_state *m maybe that's better? I'll let Jan comment. Anyway, the patch looks correct to me. > struct hvm_hw_mtrr hw_mtrr = { > - .msr_mtrr_def_type = mtrr_state->def_type | > - MASK_INSR(mtrr_state->fixed_enabled, > + .msr_mtrr_def_type = mtrr->def_type | > + MASK_INSR(mtrr->fixed_enabled, > MTRRdefType_FE) | > - MASK_INSR(mtrr_state->enabled, MTRRdefType_E), > - .msr_mtrr_cap = mtrr_state->mtrr_cap, > + MASK_INSR(mtrr->enabled, MTRRdefType_E), > + .msr_mtrr_cap = mtrr->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] = mtrr->var_ranges->base; > + hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr->var_ranges->mask; > } > > BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) != > - sizeof(mtrr_state->fixed_ranges)); > + sizeof(mtrr->fixed_ranges)); > > - memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges, > + memcpy(hw_mtrr.msr_mtrr_fixed, mtrr->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 *mtrr; > 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; > + mtrr = &v->arch.hvm.mtrr; > > hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr); > > - mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap; > + mtrr->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, mtrr, 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, mtrr, > 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, mtrr, > 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, mtrr, hw_mtrr.msr_mtrr_def_type); > > return 0; > } > -- > 2.34.1 >
On 03/08/2023 03:51, Stefano Stabellini wrote: > On Wed, 2 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" >> >> No functional changes. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > This one clashes with xen/arch/x86/include/asm/mtrr.h:struct mtrr_state > right? > Yes, indeed. You can also check this when in doubt at https://saas.eclairit.com, but I do agree that mentioning it directly in the commit would make review easier. > >> --- >> 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..d504d1e43b 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 *mtrr = &v->arch.hvm.mtrr; > > I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places > that we use: > > const struct mtrr_state *m > > maybe that's better? I'll let Jan comment. > > Anyway, the patch looks correct to me. > > I though about it, and it seemed too generic, so in the end I decided for mtrr, but as always with names, I'm open to alternative suggestions.
On 03.08.2023 03:51, Stefano Stabellini wrote: > On Wed, 2 Aug 2023, Nicola Vetrini wrote: >> --- 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 *mtrr = &v->arch.hvm.mtrr; > > I can see in hvm_vcpu_cacheattr_init, mtrr_get_type and other places > that we use: > > const struct mtrr_state *m > > maybe that's better? I'll let Jan comment. When unambiguous, personally I prefer shorter names (and as you say existing code supports me in that). Obviously the longer a function, the more potential future ambiguities also need taking into consideration. Jan
diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c index 29f3fb1607..d504d1e43b 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 *mtrr = &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 = mtrr->def_type | + MASK_INSR(mtrr->fixed_enabled, MTRRdefType_FE) | - MASK_INSR(mtrr_state->enabled, MTRRdefType_E), - .msr_mtrr_cap = mtrr_state->mtrr_cap, + MASK_INSR(mtrr->enabled, MTRRdefType_E), + .msr_mtrr_cap = mtrr->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] = mtrr->var_ranges->base; + hw_mtrr.msr_mtrr_var[i * 2 + 1] = mtrr->var_ranges->mask; } BUILD_BUG_ON(sizeof(hw_mtrr.msr_mtrr_fixed) != - sizeof(mtrr_state->fixed_ranges)); + sizeof(mtrr->fixed_ranges)); - memcpy(hw_mtrr.msr_mtrr_fixed, mtrr_state->fixed_ranges, + memcpy(hw_mtrr.msr_mtrr_fixed, mtrr->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 *mtrr; 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; + mtrr = &v->arch.hvm.mtrr; hvm_set_guest_pat(v, hw_mtrr.msr_pat_cr); - mtrr_state->mtrr_cap = hw_mtrr.msr_mtrr_cap; + mtrr->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, mtrr, 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, mtrr, 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, mtrr, 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, mtrr, hw_mtrr.msr_mtrr_def_type); return 0; }
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" No functional changes. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/x86/hvm/mtrr.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)