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

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

[Nicola Vetrini]

This series aims to address some 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"

To resolve such violations two strategies have been applied:

1. rename several function parameters or local variables;
2. remove a local variable declaration in an inner scope, if
   using the outer variable does not change the semantics of the function.

Nicola Vetrini (4):
  x86: mechanically rename to address MISRA C:2012 Rule 5.3
  x86/emulate: move a variable declaration to address MISRA C:2012 Rule
    5.3
  x86/mm: mechanically rename variable to avoid shadowing
  x86: avoid shadowing to address MISRA C:2012 Rule 5.3

 xen/arch/x86/apic.c                    |  8 ++++----
 xen/arch/x86/cpu/mcheck/mce.c          | 12 ++++++------
 xen/arch/x86/cpu/mwait-idle.c          | 24 ++++++++++++------------
 xen/arch/x86/domain.c                  |  2 +-
 xen/arch/x86/e820.c                    |  6 +++---
 xen/arch/x86/emul-i8254.c              |  2 +-
 xen/arch/x86/hvm/emulate.c             |  6 +++---
 xen/arch/x86/include/asm/e820.h        |  2 +-
 xen/arch/x86/include/asm/hvm/save.h    |  8 ++++----
 xen/arch/x86/include/asm/hvm/vpt.h     |  2 +-
 xen/arch/x86/mm/p2m.c                  |  6 +++---
 xen/arch/x86/psr.c                     |  4 ++--
 xen/arch/x86/traps.c                   |  4 ++--
 xen/arch/x86/x86_emulate/x86_emulate.c | 14 ++++++--------
 14 files changed, 49 insertions(+), 51 deletions(-)

--
2.34.1

[XEN PATCH 3/4] x86/mm: mechanically rename variable to avoid shadowing

[Nicola Vetrini]

The rename s/p2mt/t/ is done to avoid shadowing the same declaration
in the enclosing scope.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Is the semantics of the function altered if the inner declaration
is removed entirely?
---
 xen/arch/x86/mm/p2m.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 714358f953..fa9f7616e8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2439,12 +2439,12 @@ int xenmem_add_to_physmap_one(
 
     case XENMAPSPACE_gmfn:
     {
-        p2m_type_t p2mt;
+        p2m_type_t t;
 
         gfn = idx;
-        mfn = get_gfn_unshare(d, gfn, &p2mt);
+        mfn = get_gfn_unshare(d, gfn, &t);
         /* If the page is still shared, exit early */
-        if ( p2m_is_shared(p2mt) )
+        if ( p2m_is_shared(t) )
         {
             put_gfn(d, gfn);
             return -ENOMEM;
-- 
2.34.1

[XEN PATCH 3/4] x86/mm: mechanically rename variable to avoid shadowing

[Nicola Vetrini]

The rename s/p2mt/t/ is done to avoid shadowing the same declaration
in the enclosing scope.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Is the semantics of the function altered if the inner declaration
is removed entirely?
---
 xen/arch/x86/mm/p2m.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 714358f953..fa9f7616e8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2439,12 +2439,12 @@ int xenmem_add_to_physmap_one(
 
     case XENMAPSPACE_gmfn:
     {
-        p2m_type_t p2mt;
+        p2m_type_t t;
 
         gfn = idx;
-        mfn = get_gfn_unshare(d, gfn, &p2mt);
+        mfn = get_gfn_unshare(d, gfn, &t);
         /* If the page is still shared, exit early */
-        if ( p2m_is_shared(p2mt) )
+        if ( p2m_is_shared(t) )
         {
             put_gfn(d, gfn);
             return -ENOMEM;
-- 
2.34.1

Re: [XEN PATCH 3/4] x86/mm: mechanically rename variable to avoid shadowing

[Jan Beulich]

On 27.07.2023 12:48, Nicola Vetrini wrote:
> The rename s/p2mt/t/ is done to avoid shadowing the same declaration
> in the enclosing scope.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Is the semantics of the function altered if the inner declaration
> is removed entirely?

No, that's what should be done. It's an output only for get_gfn().

Jan

[XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

The renames done by this patch avoid shadowing from happening.
They are as follows:
- s/str/s/ in 'lapic_disable'
- s/str/level/ in '(apic|mce)_set_verbosity'
- s/str/state_str/ in 'mwait_idle_probe'
- s/str/memmap_name/ in 'init_e820'
- s/i/j/ in 'mce_action' (the shadowing here is due to macro
  'x86_mcinfo_lookup' that defines 'i' as a loop counter)
- s/desc/descriptor/ in '_hvm_load_entry'
- s/socket_info/sock_info/ in 'do_write_psr_msrs'
- s/debug_stack_lines/dbg_stack_lines/ in 'compat_show_guest_stack'

The parameter 'cpu_khz' that causes a violation in 'pit_init'
is unused, and hence can be removed.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Function 'str' in 'xen/arch/x86/include/asm/desc.h'
causes the shadowing.
---
 xen/arch/x86/apic.c                 |  8 ++++----
 xen/arch/x86/cpu/mcheck/mce.c       | 12 ++++++------
 xen/arch/x86/cpu/mwait-idle.c       | 24 ++++++++++++------------
 xen/arch/x86/domain.c               |  2 +-
 xen/arch/x86/e820.c                 |  6 +++---
 xen/arch/x86/emul-i8254.c           |  2 +-
 xen/arch/x86/include/asm/e820.h     |  2 +-
 xen/arch/x86/include/asm/hvm/save.h |  8 ++++----
 xen/arch/x86/include/asm/hvm/vpt.h  |  2 +-
 xen/arch/x86/psr.c                  |  4 ++--
 xen/arch/x86/traps.c                |  4 ++--
 11 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 41879230ec..57ec500408 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -760,7 +760,7 @@ int lapic_resume(void)
  * Original code written by Keir Fraser.
  */
 
-static int __init cf_check lapic_disable(const char *str)
+static int __init cf_check lapic_disable(const char *s)
 {
     enable_local_apic = -1;
     setup_clear_cpu_cap(X86_FEATURE_APIC);
@@ -769,11 +769,11 @@ static int __init cf_check lapic_disable(const char *str)
 custom_param("nolapic", lapic_disable);
 boolean_param("lapic", enable_local_apic);
 
-static int __init cf_check apic_set_verbosity(const char *str)
+static int __init cf_check apic_set_verbosity(const char *level)
 {
-    if (strcmp("debug", str) == 0)
+    if (strcmp("debug", level) == 0)
         apic_verbosity = APIC_DEBUG;
-    else if (strcmp("verbose", str) == 0)
+    else if (strcmp("verbose", level) == 0)
         apic_verbosity = APIC_VERBOSE;
     else
         return -EINVAL;
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index c2173cfc78..9d76a462a7 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -63,9 +63,9 @@ struct mca_banks *mca_allbanks;
 #endif
 
 int mce_verbosity;
-static int __init cf_check mce_set_verbosity(const char *str)
+static int __init cf_check mce_set_verbosity(const char *level)
 {
-    if ( strcmp("verbose", str) == 0 )
+    if ( strcmp("verbose", level) == 0 )
         mce_verbosity = MCE_VERBOSE;
     else
         return -EINVAL;
@@ -1746,7 +1746,7 @@ static enum mce_result mce_action(const struct cpu_user_regs *regs,
     struct mcinfo_common *mic = NULL;
     struct mca_binfo binfo;
     const struct mca_error_handler *handlers = mce_dhandlers;
-    unsigned int i, handler_num = mce_dhandler_num;
+    unsigned int j, handler_num = mce_dhandler_num;
 
     /* When in mce context, regs is valid */
     if ( regs )
@@ -1780,11 +1780,11 @@ static enum mce_result mce_action(const struct cpu_user_regs *regs,
         binfo.mib = (struct mcinfo_bank *)mic;
         binfo.bank = binfo.mib->mc_bank;
         bank_result = MCER_NOERROR;
-        for ( i = 0; i < handler_num; i++ )
+        for ( j = 0; j < handler_num; j++ )
         {
-            if ( handlers[i].owned_error(binfo.mib->mc_status) )
+            if ( handlers[j].owned_error(binfo.mib->mc_status) )
             {
-                handlers[i].recovery_handler(&binfo, &bank_result, regs);
+                handlers[j].recovery_handler(&binfo, &bank_result, regs);
                 if ( worst_result < bank_result )
                     worst_result = bank_result;
                 break;
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index ff5c808bc9..8abe14773d 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -1429,7 +1429,7 @@ static int __init mwait_idle_probe(void)
 {
 	unsigned int eax, ebx, ecx;
 	const struct x86_cpu_id *id = x86_match_cpu(intel_idle_ids);
-	const char *str;
+	const char *state_str;
 
 	if (!id) {
 		pr_debug(PREFIX "does not run on family %d model %d\n",
@@ -1471,10 +1471,10 @@ static int __init mwait_idle_probe(void)
 	pr_debug(PREFIX "lapic_timer_reliable_states %#x\n",
 		 lapic_timer_reliable_states);
 
-	str = preferred_states;
-	if (isdigit(str[0]))
-		preferred_states_mask = simple_strtoul(str, &str, 0);
-	else if (str[0])
+	state_str = preferred_states;
+	if (isdigit(state_str[0]))
+		preferred_states_mask = simple_strtoul(state_str, &state_str, 0);
+	else if (state_str[0])
 	{
 		const char *ss;
 
@@ -1482,13 +1482,13 @@ static int __init mwait_idle_probe(void)
 			const struct cpuidle_state *state = icpu->state_table;
 			unsigned int bit = 1;
 
-			ss = strchr(str, ',');
+			ss = strchr(state_str, ',');
 			if (!ss)
-				ss = strchr(str, '\0');
+				ss = strchr(state_str, '\0');
 
 			for (; state->name[0]; ++state) {
 				bit <<= 1;
-				if (!cmdline_strcmp(str, state->name)) {
+				if (!cmdline_strcmp(state_str, state->name)) {
 					preferred_states_mask |= bit;
 					break;
 				}
@@ -1496,13 +1496,13 @@ static int __init mwait_idle_probe(void)
 			if (!state->name[0])
 				break;
 
-			str = ss + 1;
+			state_str = ss + 1;
 		} while (*ss);
 
-		str -= str == ss + 1;
+		state_str -= state_str == ss + 1;
 	}
-	if (str[0])
-		printk("unrecognized \"preferred-cstates=%s\"\n", str);
+	if (state_str[0])
+		printk("unrecognized \"preferred-cstates=%s\"\n", state_str);
 
 	mwait_idle_state_table_update();
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5f66c2ae33..fe86a7f853 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -858,7 +858,7 @@ int arch_domain_create(struct domain *d,
     }
 
     /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
-    pit_init(d, cpu_khz);
+    pit_init(d);
 
     /*
      * If the FPU does not save FCS/FDS then we can always
diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 0b89935510..5bff35c4cf 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -683,11 +683,11 @@ int __init reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e)
     return e820_change_range_type(e820, s, e, E820_RAM, E820_RESERVED);
 }
 
-unsigned long __init init_e820(const char *str, struct e820map *raw)
+unsigned long __init init_e820(const char *memmap_name, struct e820map *raw)
 {
     if ( e820_verbose )
     {
-        printk("Initial %s RAM map:\n", str);
+        printk("Initial %s RAM map:\n", memmap_name);
         print_e820_memory_map(raw->map, raw->nr_map);
     }
 
@@ -696,7 +696,7 @@ unsigned long __init init_e820(const char *str, struct e820map *raw)
     if ( cpu_has_hypervisor )
         hypervisor_e820_fixup(&e820);
 
-    printk("%s RAM map:\n", str);
+    printk("%s RAM map:\n", memmap_name);
     print_e820_memory_map(e820.map, e820.nr_map);
 
     return find_max_pfn();
diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c
index a81232fc55..41ec4a1ef1 100644
--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -495,7 +495,7 @@ void pit_reset(struct domain *d)
     spin_unlock(&pit->lock);
 }
 
-void pit_init(struct domain *d, unsigned long cpu_khz)
+void pit_init(struct domain *d)
 {
     PITState *pit = domain_vpit(d);
 
diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
index 92f5efa4f5..5cb1915a6e 100644
--- a/xen/arch/x86/include/asm/e820.h
+++ b/xen/arch/x86/include/asm/e820.h
@@ -31,7 +31,7 @@ extern int e820_change_range_type(
     uint32_t orig_type, uint32_t new_type);
 extern int e820_add_range(
     struct e820map *, uint64_t s, uint64_t e, uint32_t type);
-extern unsigned long init_e820(const char *, struct e820map *);
+extern unsigned long init_e820(const char *memmap_name, struct e820map *raw);
 extern void print_e820_memory_map(const struct e820entry *map,
     unsigned int entries);
 extern struct e820map e820;
diff --git a/xen/arch/x86/include/asm/hvm/save.h b/xen/arch/x86/include/asm/hvm/save.h
index 535cf68ed2..4d0a062b2c 100644
--- a/xen/arch/x86/include/asm/hvm/save.h
+++ b/xen/arch/x86/include/asm/hvm/save.h
@@ -49,21 +49,21 @@ void _hvm_read_entry(struct hvm_domain_context *h,
  */
 #define _hvm_load_entry(_x, _h, _dst, _strict) ({                       \
     int r;                                                              \
-    struct hvm_save_descriptor *desc                                    \
+    struct hvm_save_descriptor *descriptor                              \
         = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];         \
     if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),                 \
                HVM_SAVE_LENGTH(_x), (_strict))) == 0 )                  \
     {                                                                   \
         _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));             \
         if ( HVM_SAVE_HAS_COMPAT(_x) &&                                 \
-             desc->length != HVM_SAVE_LENGTH(_x) )                      \
-            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);          \
+             descriptor->length != HVM_SAVE_LENGTH(_x) )                \
+            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length);    \
     }                                                                   \
     else if (HVM_SAVE_HAS_COMPAT(_x)                                    \
              && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),          \
                        HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 ) { \
         _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));      \
-        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);              \
+        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length);        \
     }                                                                   \
     r; })
 
diff --git a/xen/arch/x86/include/asm/hvm/vpt.h b/xen/arch/x86/include/asm/hvm/vpt.h
index 2af76ca8dc..feb0bf43f1 100644
--- a/xen/arch/x86/include/asm/hvm/vpt.h
+++ b/xen/arch/x86/include/asm/hvm/vpt.h
@@ -179,7 +179,7 @@ void destroy_periodic_time(struct periodic_time *pt);
 int pv_pit_handler(int port, int data, int write);
 void pit_reset(struct domain *d);
 
-void pit_init(struct domain *d, unsigned long cpu_khz);
+void pit_init(struct domain *d);
 void pit_stop_channel0_irq(PITState * pit);
 void pit_deinit(struct domain *d);
 void rtc_init(struct domain *d);
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c
index a1e0af27c5..90bcb25e60 100644
--- a/xen/arch/x86/psr.c
+++ b/xen/arch/x86/psr.c
@@ -1252,7 +1252,7 @@ static void cf_check do_write_psr_msrs(void *data)
 {
     const struct cos_write_info *info = data;
     unsigned int i, index, cos = info->cos;
-    const struct psr_socket_info *socket_info =
+    const struct psr_socket_info *sock_info =
         get_socket_info(cpu_to_socket(smp_processor_id()));
 
     /*
@@ -1261,7 +1261,7 @@ static void cf_check do_write_psr_msrs(void *data)
      */
     for ( index = i = 0; i < ARRAY_SIZE(feat_props); i++ )
     {
-        struct feat_node *feat = socket_info->features[i];
+        struct feat_node *feat = sock_info->features[i];
         const struct feat_props *props = feat_props[i];
         unsigned int cos_num, j;
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4229bda159..f32cd2126d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -196,7 +196,7 @@ void show_code(const struct cpu_user_regs *regs)
 
 static void compat_show_guest_stack(struct vcpu *v,
                                     const struct cpu_user_regs *regs,
-                                    int debug_stack_lines)
+                                    int dbg_stack_lines)
 {
     unsigned int i, *stack, addr, mask = STACK_SIZE;
     void *stack_page = NULL;
@@ -233,7 +233,7 @@ static void compat_show_guest_stack(struct vcpu *v,
             mask = PAGE_SIZE;
     }
 
-    for ( i = 0; i < debug_stack_lines * 8; i++ )
+    for ( i = 0; i < dbg_stack_lines * 8; i++ )
     {
         if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
             break;
-- 
2.34.1

Re: [XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 27.07.2023 12:47, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> The renames done by this patch avoid shadowing from happening.
> They are as follows:
> - s/str/s/ in 'lapic_disable'
> - s/str/level/ in '(apic|mce)_set_verbosity'
> - s/str/state_str/ in 'mwait_idle_probe'
> - s/str/memmap_name/ in 'init_e820'

I'm sorry to say that, but I'm not willing to go and figure out where
that "str" is that there's supposedly a collision with. Please can you
state such right here, ...

> - s/i/j/ in 'mce_action' (the shadowing here is due to macro
>   'x86_mcinfo_lookup' that defines 'i' as a loop counter)

... much like you do in this case?

> - s/desc/descriptor/ in '_hvm_load_entry'
> - s/socket_info/sock_info/ in 'do_write_psr_msrs'

(I didn't look at any of these in any detail, partly because again
I hope for additional context before doing so.)

> - s/debug_stack_lines/dbg_stack_lines/ in 'compat_show_guest_stack'

This wants doing differently: The two functions originally lived in
different source files, so passing the static variable as argument
was preferred over making the variable non-static. When the function
was moved, that aspect was overlooked. The function argument simply
wants dropping.

Jan

Re: [XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3

[Andrew Cooper]

On 27/07/2023 3:50 pm, Jan Beulich wrote:
> On 27.07.2023 12:47, Nicola Vetrini wrote:
>> Rule 5.3 has the following headline:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>>
>> The renames done by this patch avoid shadowing from happening.
>> They are as follows:
>> - s/str/s/ in 'lapic_disable'
>> - s/str/level/ in '(apic|mce)_set_verbosity'
>> - s/str/state_str/ in 'mwait_idle_probe'
>> - s/str/memmap_name/ in 'init_e820'
> I'm sorry to say that, but I'm not willing to go and figure out where
> that "str" is that there's supposedly a collision with. Please can you
> state such right here, ...
>
>> - s/i/j/ in 'mce_action' (the shadowing here is due to macro
>>   'x86_mcinfo_lookup' that defines 'i' as a loop counter)
> ... much like you do in this case?

In fairness to Nicola, that was given.

> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Function 'str' in 'xen/arch/x86/include/asm/desc.h'
> causes the shadowing.

which is the wrapper for the STR instruction.

It's used in a single assertion, and I'd be happy getting rid of it
entirely.  Alternatively, it could be renamed to read_tr() (or
read_tr_sel() ?) if we want to keep the assertion.

We're not renaming every other use of 'str' to mean string just for this...

~Andrew

Re: [XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 27/07/23 17:00, Andrew Cooper wrote:
> On 27/07/2023 3:50 pm, Jan Beulich wrote:
>> On 27.07.2023 12:47, Nicola Vetrini wrote:
>>> Rule 5.3 has the following headline:
>>> "An identifier declared in an inner scope shall not hide an
>>> identifier declared in an outer scope"
>>>
>>> The renames done by this patch avoid shadowing from happening.
>>> They are as follows:
>>> - s/str/s/ in 'lapic_disable'
>>> - s/str/level/ in '(apic|mce)_set_verbosity'
>>> - s/str/state_str/ in 'mwait_idle_probe'
>>> - s/str/memmap_name/ in 'init_e820'
>> I'm sorry to say that, but I'm not willing to go and figure out where
>> that "str" is that there's supposedly a collision with. Please can you
>> state such right here, ...
>>
>>> - s/i/j/ in 'mce_action' (the shadowing here is due to macro
>>>    'x86_mcinfo_lookup' that defines 'i' as a loop counter)
>> ... much like you do in this case?
> 
> In fairness to Nicola, that was given.
> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Function 'str' in 'xen/arch/x86/include/asm/desc.h'
>> causes the shadowing.
> 
> which is the wrapper for the STR instruction.
> 
> It's used in a single assertion, and I'd be happy getting rid of it
> entirely.  Alternatively, it could be renamed to read_tr() (or
> read_tr_sel() ?) if we want to keep the assertion.
> 
> We're not renaming every other use of 'str' to mean string just for this...
> 
> ~Andrew

Seems reasonable to remove it, though there aren't that many instances 
of shadowing on 'str'.

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

[XEN PATCH 2/4] x86/emulate: move a variable declaration to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The declaration of local variable 'bytes' in 'hvmemul_rep_stos' causes
the shadowing of the same variable defined in the enclosing scope,
hence the declaration has been moved inside the scope where it's used,
with a different name.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/emulate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 75ee98a73b..0d41928ff3 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos(
 
     switch ( p2mt )
     {
-        unsigned long bytes;
         char *buf;
 
     default:
         /* Allocate temporary buffer. */
         for ( ; ; )
         {
-            bytes = *reps * bytes_per_rep;
-            buf = xmalloc_bytes(bytes);
+            unsigned long bytes_tmp;
+            bytes_tmp = *reps * bytes_per_rep;
+            buf = xmalloc_bytes(bytes_tmp);
             if ( buf || *reps <= 1 )
                 break;
             *reps >>= 1;
-- 
2.34.1

Re: [XEN PATCH 2/4] x86/emulate: move a variable declaration to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 27.07.2023 12:48, Nicola Vetrini wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos(
>  
>      switch ( p2mt )
>      {
> -        unsigned long bytes;
>          char *buf;
>  
>      default:
>          /* Allocate temporary buffer. */
>          for ( ; ; )
>          {
> -            bytes = *reps * bytes_per_rep;
> -            buf = xmalloc_bytes(bytes);
> +            unsigned long bytes_tmp;
> +            bytes_tmp = *reps * bytes_per_rep;
> +            buf = xmalloc_bytes(bytes_tmp);
>              if ( buf || *reps <= 1 )
>                  break;
>              *reps >>= 1;

This wants dealing with differently - the outer scope variable is unused
(only written to) afaics. Eliminating it will, aiui, address another
violation at the same time. And then the same in hvmemul_rep_movs(), just
that there the variable itself needs to survive. I guess I'll make a
patch ...

Jan

Re: [XEN PATCH 2/4] x86/emulate: move a variable declaration to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 27/07/23 17:06, Jan Beulich wrote:
> On 27.07.2023 12:48, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos(
>>   
>>       switch ( p2mt )
>>       {
>> -        unsigned long bytes;
>>           char *buf;
>>   
>>       default:
>>           /* Allocate temporary buffer. */
>>           for ( ; ; )
>>           {
>> -            bytes = *reps * bytes_per_rep;
>> -            buf = xmalloc_bytes(bytes);
>> +            unsigned long bytes_tmp;
>> +            bytes_tmp = *reps * bytes_per_rep;
>> +            buf = xmalloc_bytes(bytes_tmp);
>>               if ( buf || *reps <= 1 )
>>                   break;
>>               *reps >>= 1;
> 
> This wants dealing with differently - the outer scope variable is unused
> (only written to) afaics. Eliminating it will, aiui, address another
> violation at the same time. And then the same in hvmemul_rep_movs(), just
> that there the variable itself needs to survive. I guess I'll make a
> patch ...
> 
> Jan

Wouldn't this code at line ~2068 be possibly affected by writing to 
bytes, if the outer variable is used?

/* Adjust address for reverse store. */
if ( df )
   gpa -= bytes - bytes_per_rep;

rc = hvm_copy_to_guest_phys(gpa, buf, bytes, curr);

You're right about the other violation (R2.1)

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

Re: [XEN PATCH 2/4] x86/emulate: move a variable declaration to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 27.07.2023 17:22, Nicola Vetrini wrote:
> 
> 
> On 27/07/23 17:06, Jan Beulich wrote:
>> On 27.07.2023 12:48, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos(
>>>   
>>>       switch ( p2mt )
>>>       {
>>> -        unsigned long bytes;
>>>           char *buf;
>>>   
>>>       default:
>>>           /* Allocate temporary buffer. */
>>>           for ( ; ; )
>>>           {
>>> -            bytes = *reps * bytes_per_rep;
>>> -            buf = xmalloc_bytes(bytes);
>>> +            unsigned long bytes_tmp;
>>> +            bytes_tmp = *reps * bytes_per_rep;
>>> +            buf = xmalloc_bytes(bytes_tmp);
>>>               if ( buf || *reps <= 1 )
>>>                   break;
>>>               *reps >>= 1;
>>
>> This wants dealing with differently - the outer scope variable is unused
>> (only written to) afaics. Eliminating it will, aiui, address another
>> violation at the same time. And then the same in hvmemul_rep_movs(), just
>> that there the variable itself needs to survive. I guess I'll make a
>> patch ...
> 
> Wouldn't this code at line ~2068 be possibly affected by writing to 
> bytes, if the outer variable is used?

Which outer variable? I'm suggesting to drop that (see the patch that
I've sent already).

Jan

> /* Adjust address for reverse store. */
> if ( df )
>    gpa -= bytes - bytes_per_rep;
> 
> rc = hvm_copy_to_guest_phys(gpa, buf, bytes, curr);
> 
> You're right about the other violation (R2.1)
> 

Re: [XEN PATCH 2/4] x86/emulate: move a variable declaration to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 27/07/23 17:31, Jan Beulich wrote:
> On 27.07.2023 17:22, Nicola Vetrini wrote:
>>
>>
>> On 27/07/23 17:06, Jan Beulich wrote:
>>> On 27.07.2023 12:48, Nicola Vetrini wrote:
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -2024,15 +2024,15 @@ static int cf_check hvmemul_rep_stos(
>>>>    
>>>>        switch ( p2mt )
>>>>        {
>>>> -        unsigned long bytes;
>>>>            char *buf;
>>>>    
>>>>        default:
>>>>            /* Allocate temporary buffer. */
>>>>            for ( ; ; )
>>>>            {
>>>> -            bytes = *reps * bytes_per_rep;
>>>> -            buf = xmalloc_bytes(bytes);
>>>> +            unsigned long bytes_tmp;
>>>> +            bytes_tmp = *reps * bytes_per_rep;
>>>> +            buf = xmalloc_bytes(bytes_tmp);
>>>>                if ( buf || *reps <= 1 )
>>>>                    break;
>>>>                *reps >>= 1;
>>>
>>> This wants dealing with differently - the outer scope variable is unused
>>> (only written to) afaics. Eliminating it will, aiui, address another
>>> violation at the same time. And then the same in hvmemul_rep_movs(), just
>>> that there the variable itself needs to survive. I guess I'll make a
>>> patch ...
>>
>> Wouldn't this code at line ~2068 be possibly affected by writing to
>> bytes, if the outer variable is used?
> 
> Which outer variable? I'm suggesting to drop that (see the patch that
> I've sent already).
> 
> Jan
> 
>> /* Adjust address for reverse store. */
>> if ( df )
>>     gpa -= bytes - bytes_per_rep;
>>
>> rc = hvm_copy_to_guest_phys(gpa, buf, bytes, curr);
>>
>> You're right about the other violation (R2.1)
>>
> 

I see, sorry for the noise.

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

[XEN PATCH 4/4] x86: avoid shadowing to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"

The declaration of local variable 'cpuid_leaf' causes
shadowing with the homonymous function to happen, therefore
the variable is renamed to avoid this.

Local variable 'cr4' that shadows a previous declaration is
removed, as it is unnecessary and doing so does not alter the
semantics.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 2de1be7996..9403beb20f 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1483,7 +1483,7 @@ x86_emulate(
     {
         enum x86_segment seg;
         struct segment_register cs, sreg;
-        struct cpuid_leaf cpuid_leaf;
+        struct cpuid_leaf res;
         uint64_t msr_val;
         unsigned int i, n;
         unsigned long dummy;
@@ -5024,13 +5024,13 @@ x86_emulate(
         generate_exception_if((msr_val & MSR_MISC_FEATURES_CPUID_FAULTING),
                               X86_EXC_GP, 0); /* Faulting active? (Inc. CPL test) */
 
-        rc = ops->cpuid(_regs.eax, _regs.ecx, &cpuid_leaf, ctxt);
+        rc = ops->cpuid(_regs.eax, _regs.ecx, &res, ctxt);
         if ( rc != X86EMUL_OKAY )
             goto done;
-        _regs.r(ax) = cpuid_leaf.a;
-        _regs.r(bx) = cpuid_leaf.b;
-        _regs.r(cx) = cpuid_leaf.c;
-        _regs.r(dx) = cpuid_leaf.d;
+        _regs.r(ax) = res.a;
+        _regs.r(bx) = res.b;
+        _regs.r(cx) = res.c;
+        _regs.r(dx) = res.d;
         break;
 
     case X86EMUL_OPC(0x0f, 0xa3): bt: /* bt */
@@ -8408,8 +8408,6 @@ x86_emulate(
         generate_exception(X86_EXC_MF);
     if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
     {
-        unsigned long cr4;
-
         if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
             cr4 = X86_CR4_OSXMMEXCPT;
         generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);
-- 
2.34.1

Re: [XEN PATCH 4/4] x86: avoid shadowing to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 27.07.2023 12:48, Nicola Vetrini wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1483,7 +1483,7 @@ x86_emulate(
>      {
>          enum x86_segment seg;
>          struct segment_register cs, sreg;
> -        struct cpuid_leaf cpuid_leaf;
> +        struct cpuid_leaf res;

This is too generic a name for a variable with a scope of several
thousand lines. Perhaps just "leaf"?

> @@ -8408,8 +8408,6 @@ x86_emulate(
>          generate_exception(X86_EXC_MF);
>      if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
>      {
> -        unsigned long cr4;
> -
>          if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
>              cr4 = X86_CR4_OSXMMEXCPT;
>          generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);

This change looks okay to me, but I'd like to strongly encourage
you to split both changes. They're of different nature, and for
the latter it may even be worthwhile pointing out when exactly
this duplication of variables was introduced (it clearly would
better have been avoided).

Jan

Re: [XEN PATCH 4/4] x86: avoid shadowing to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 27/07/23 17:41, Jan Beulich wrote:
> On 27.07.2023 12:48, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -1483,7 +1483,7 @@ x86_emulate(
>>       {
>>           enum x86_segment seg;
>>           struct segment_register cs, sreg;
>> -        struct cpuid_leaf cpuid_leaf;
>> +        struct cpuid_leaf res;
> 
> This is too generic a name for a variable with a scope of several
> thousand lines. Perhaps just "leaf"?

It can also be defined inside the switch clause, since it has no other 
purpose than store a result.

> 
>> @@ -8408,8 +8408,6 @@ x86_emulate(
>>           generate_exception(X86_EXC_MF);
>>       if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
>>       {
>> -        unsigned long cr4;
>> -
>>           if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
>>               cr4 = X86_CR4_OSXMMEXCPT;
>>           generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);
> 
> This change looks okay to me, but I'd like to strongly encourage
> you to split both changes. They're of different nature, and for
> the latter it may even be worthwhile pointing out when exactly
> this duplication of variables was introduced (it clearly would
> better have been avoided).
> 

I did it this way because they are the only violations of R5.3 left in 
this file (among those not subject to deviation). By splitting you mean 
two patches in this series or a separate patch just for this change?

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

Re: [XEN PATCH 4/4] x86: avoid shadowing to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 27.07.2023 17:58, Nicola Vetrini wrote:
> 
> 
> On 27/07/23 17:41, Jan Beulich wrote:
>> On 27.07.2023 12:48, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -1483,7 +1483,7 @@ x86_emulate(
>>>       {
>>>           enum x86_segment seg;
>>>           struct segment_register cs, sreg;
>>> -        struct cpuid_leaf cpuid_leaf;
>>> +        struct cpuid_leaf res;
>>
>> This is too generic a name for a variable with a scope of several
>> thousand lines. Perhaps just "leaf"?
> 
> It can also be defined inside the switch clause, since it has no other 
> purpose than store a result.

That would be more code churn, though.

>>> @@ -8408,8 +8408,6 @@ x86_emulate(
>>>           generate_exception(X86_EXC_MF);
>>>       if ( stub_exn.info.fields.trapnr == X86_EXC_XM )
>>>       {
>>> -        unsigned long cr4;
>>> -
>>>           if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
>>>               cr4 = X86_CR4_OSXMMEXCPT;
>>>           generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);
>>
>> This change looks okay to me, but I'd like to strongly encourage
>> you to split both changes. They're of different nature, and for
>> the latter it may even be worthwhile pointing out when exactly
>> this duplication of variables was introduced (it clearly would
>> better have been avoided).
>>
> 
> I did it this way because they are the only violations of R5.3 left in 
> this file (among those not subject to deviation). By splitting you mean 
> two patches in this series or a separate patch just for this change?

Separate or within a series doesn't matter. Just preferably not in
the same patch. (And btw, if you split larger patches more, some of
your changes may also go in more quickly. Yet of course this shouldn't
get too fine grained.)

Jan

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

[Nicola Vetrini]

This series aims to address some 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"

To resolve such violations two strategies have been applied:

1. rename several function parameters or local variables;
2. remove a local variable declaration in an inner scope, if
   using the outer variable does not change the semantics of the function.

Nicola Vetrini (4):
  x86: mechanically rename to address MISRA C:2012 Rule 5.3
  x86/emulate: move a variable declaration to address MISRA C:2012 Rule
    5.3
  x86/mm: mechanically rename variable to avoid shadowing
  x86: address MISRA C:2012 Rule 5.3

 xen/arch/x86/apic.c                    |  8 ++++----
 xen/arch/x86/cpu/mcheck/mce.c          | 12 ++++++------
 xen/arch/x86/cpu/mwait-idle.c          | 24 ++++++++++++------------
 xen/arch/x86/domain.c                  |  2 +-
 xen/arch/x86/e820.c                    |  6 +++---
 xen/arch/x86/emul-i8254.c              |  2 +-
 xen/arch/x86/hvm/emulate.c             |  6 +++---
 xen/arch/x86/include/asm/e820.h        |  2 +-
 xen/arch/x86/include/asm/hvm/save.h    |  8 ++++----
 xen/arch/x86/include/asm/hvm/vpt.h     |  2 +-
 xen/arch/x86/mm/p2m.c                  |  6 +++---
 xen/arch/x86/psr.c                     |  4 ++--
 xen/arch/x86/traps.c                   |  4 ++--
 xen/arch/x86/x86_emulate/x86_emulate.c | 14 ++++++--------
 14 files changed, 49 insertions(+), 51 deletions(-)

--
2.34.1