[XEN PATCH v2 0/5] x86: address MISRA C:2012 Rule 5.3

[XEN PATCH v2 0/5] x86: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

This series addresses shadowing issues to resolve violations of Rule 5.3, whose
headline states:
"An identifier declared in an inner scope shall not hide an identifier
declared in an outer scope". To do this, suitable renames are made.

The thread of the first submission is here [1]
---
Changes in v2:
- Addressed numerous review comments
- Dropped patch 2/6 [2], as that has already been committed

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg00538.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg00539.html
Nicola Vetrini (5):
  x86: address MISRA C:2012 Rule 5.3
  xen/delay: address MISRA C:2012 Rule 5.3.
  x86/include: address MISRA C:2012 Rule 5.3.
  x86/xstate: address MISRA C:2012 Rule 5.3
  x86: refactor macros in 'xen-mca.h'

 xen/arch/x86/e820.c                         | 36 +++++++++----------
 xen/arch/x86/guest/hyperv/hyperv.c          |  4 +--
 xen/arch/x86/guest/hypervisor.c             |  2 +-
 xen/arch/x86/guest/xen/xen.c                |  4 +--
 xen/arch/x86/include/asm/e820.h             |  3 +-
 xen/arch/x86/include/asm/guest/hypervisor.h |  2 +-
 xen/arch/x86/include/asm/mpspec.h           |  1 -
 xen/arch/x86/include/asm/pv/shim.h          |  2 +-
 xen/arch/x86/pv/shim.c                      | 10 +++---
 xen/arch/x86/setup.c                        |  8 ++---
 xen/arch/x86/x86_64/mmconf-fam10h.c         |  2 +-
 xen/arch/x86/xstate.c                       | 30 ++++++++--------
 xen/drivers/passthrough/amd/iommu_acpi.c    |  2 +-
 xen/include/public/arch-x86/xen-mca.h       | 38 ++++++++++-----------
 xen/include/xen/delay.h                     |  9 +++--
 15 files changed, 78 insertions(+), 75 deletions(-)

-- 
2.34.1

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

[Nicola Vetrini]

Address some occurrences of shadowing between the global
variable 'e820' in 'xen/arch/x86/e820.c' and function
parameter names (such as that of 'e820_add_range').

The parameter is removed in those functions whose call chain
ultimately supplies the global variable as a parameter, which
is already visible from their definitions in 'e820.c'.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Reworked the patch to remove the parameter e820 where possible
  and consequential changes to the involved functions.
---
 xen/arch/x86/e820.c                         | 36 ++++++++++-----------
 xen/arch/x86/guest/hyperv/hyperv.c          |  4 +--
 xen/arch/x86/guest/hypervisor.c             |  2 +-
 xen/arch/x86/guest/xen/xen.c                |  4 +--
 xen/arch/x86/include/asm/e820.h             |  3 +-
 xen/arch/x86/include/asm/guest/hypervisor.h |  2 +-
 xen/arch/x86/include/asm/pv/shim.h          |  2 +-
 xen/arch/x86/pv/shim.c                      | 10 +++---
 xen/arch/x86/setup.c                        |  8 ++---
 xen/arch/x86/x86_64/mmconf-fam10h.c         |  2 +-
 xen/drivers/passthrough/amd/iommu_acpi.c    |  2 +-
 11 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 4911e64b8c..875572b23e 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -543,27 +543,27 @@ static void __init machine_specific_memory_setup(struct e820map *raw)
         clip_to_limit(top_of_ram, "MTRRs do not cover all of memory.");
 }
 
-/* This function relies on the passed in e820->map[] being sorted. */
-int __init e820_add_range(
-    struct e820map *e820, uint64_t s, uint64_t e, uint32_t type)
+/* This function relies on the global e820->map[] being sorted. */
+int __init e820_add_range(uint64_t s, uint64_t e, uint32_t type)
 {
     unsigned int i;
+    struct e820entry *ei = e820.map;
 
-    for ( i = 0; i < e820->nr_map; ++i )
+    for ( i = 0; i < e820.nr_map; ++i )
     {
-        uint64_t rs = e820->map[i].addr;
-        uint64_t re = rs + e820->map[i].size;
+        uint64_t rs = ei[i].addr;
+        uint64_t re = rs + ei[i].size;
 
-        if ( rs == e && e820->map[i].type == type )
+        if ( rs == e && ei[i].type == type )
         {
-            e820->map[i].addr = s;
+            ei[i].addr = s;
             return 1;
         }
 
-        if ( re == s && e820->map[i].type == type &&
-             (i + 1 == e820->nr_map || e820->map[i + 1].addr >= e) )
+        if ( re == s && ei[i].type == type &&
+             (i + 1 == e820.nr_map || ei[i + 1].addr >= e) )
         {
-            e820->map[i].size += e - s;
+            ei[i].size += e - s;
             return 1;
         }
 
@@ -574,20 +574,20 @@ int __init e820_add_range(
             return 0;
     }
 
-    if ( e820->nr_map >= ARRAY_SIZE(e820->map) )
+    if ( e820.nr_map >= ARRAY_SIZE(e820.map) )
     {
         printk(XENLOG_WARNING "E820: overflow while adding region"
                " %"PRIx64"-%"PRIx64"\n", s, e);
         return 0;
     }
 
-    memmove(e820->map + i + 1, e820->map + i,
-            (e820->nr_map - i) * sizeof(*e820->map));
+    memmove(ei + i + 1, ei + i,
+            (e820.nr_map - i) * sizeof(*e820.map));
 
-    e820->nr_map++;
-    e820->map[i].addr = s;
-    e820->map[i].size = e - s;
-    e820->map[i].type = type;
+    e820.nr_map++;
+    ei[i].addr = s;
+    ei[i].size = e - s;
+    ei[i].type = type;
 
     return 1;
 }
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c
index aacc7a6167..912099564e 100644
--- a/xen/arch/x86/guest/hyperv/hyperv.c
+++ b/xen/arch/x86/guest/hyperv/hyperv.c
@@ -187,11 +187,11 @@ static int cf_check ap_setup(void)
     return setup_vp_assist();
 }
 
-static void __init cf_check e820_fixup(struct e820map *e820)
+static void __init cf_check e820_fixup(void)
 {
     uint64_t s = HV_HCALL_MFN << PAGE_SHIFT;
 
-    if ( !e820_add_range(e820, s, s + PAGE_SIZE, E820_RESERVED) )
+    if ( !e820_add_range(s, s + PAGE_SIZE, E820_RESERVED) )
         panic("Unable to reserve Hyper-V hypercall range\n");
 }
 
diff --git a/xen/arch/x86/guest/hypervisor.c b/xen/arch/x86/guest/hypervisor.c
index b8549a131a..bf5be97bc3 100644
--- a/xen/arch/x86/guest/hypervisor.c
+++ b/xen/arch/x86/guest/hypervisor.c
@@ -63,7 +63,7 @@ void hypervisor_resume(void)
 void __init hypervisor_e820_fixup(struct e820map *e820)
 {
     if ( ops.e820_fixup )
-        ops.e820_fixup(e820);
+        ops.e820_fixup();
 }
 
 int hypervisor_flush_tlb(const cpumask_t *mask, const void *va,
diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
index f93dfc89f7..139489c666 100644
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -306,10 +306,10 @@ static void cf_check resume(void)
         pv_console_init();
 }
 
-static void __init cf_check e820_fixup(struct e820map *e820)
+static void __init cf_check e820_fixup()
 {
     if ( pv_shim )
-        pv_shim_fixup_e820(e820);
+        pv_shim_fixup_e820();
 }
 
 static int cf_check flush_tlb(
diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
index 213d5b5dd2..af90085d65 100644
--- a/xen/arch/x86/include/asm/e820.h
+++ b/xen/arch/x86/include/asm/e820.h
@@ -29,8 +29,7 @@ extern int reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e);
 extern int e820_change_range_type(
     struct e820map *e820, uint64_t s, uint64_t e,
     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 int e820_add_range(uint64_t s, uint64_t e, uint32_t type);
 extern unsigned long init_e820(const char *str, struct e820map *raw);
 extern void print_e820_memory_map(const struct e820entry *map,
     unsigned int entries);
diff --git a/xen/arch/x86/include/asm/guest/hypervisor.h b/xen/arch/x86/include/asm/guest/hypervisor.h
index 4cffea3866..9c8a893da3 100644
--- a/xen/arch/x86/include/asm/guest/hypervisor.h
+++ b/xen/arch/x86/include/asm/guest/hypervisor.h
@@ -22,7 +22,7 @@ struct hypervisor_ops {
     /* Resume from suspension */
     void (*resume)(void);
     /* Fix up e820 map */
-    void (*e820_fixup)(struct e820map *e820);
+    void (*e820_fixup)(void);
     /* L0 assisted TLB flush */
     int (*flush_tlb)(const cpumask_t *mask, const void *va, unsigned int flags);
 };
diff --git a/xen/arch/x86/include/asm/pv/shim.h b/xen/arch/x86/include/asm/pv/shim.h
index 5625b90b72..7bae9ae372 100644
--- a/xen/arch/x86/include/asm/pv/shim.h
+++ b/xen/arch/x86/include/asm/pv/shim.h
@@ -85,7 +85,7 @@ static inline uint64_t pv_shim_mem(uint64_t avail)
     ASSERT_UNREACHABLE();
     return 0;
 }
-static inline void pv_shim_fixup_e820(struct e820map *e820)
+static inline void pv_shim_fixup_e820(void)
 {
     ASSERT_UNREACHABLE();
 }
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 4044087119..a8883a1ebd 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -98,17 +98,17 @@ uint64_t pv_shim_mem(uint64_t avail)
     return shim_nrpages;
 }
 
-static void __init mark_pfn_as_ram(struct e820map *e820, uint64_t pfn)
+static void __init mark_pfn_as_ram(uint64_t pfn)
 {
-    if ( !e820_add_range(e820, pfn << PAGE_SHIFT,
+    if ( !e820_add_range(pfn << PAGE_SHIFT,
                          (pfn << PAGE_SHIFT) + PAGE_SIZE, E820_RAM) &&
-         !e820_change_range_type(e820, pfn << PAGE_SHIFT,
+         !e820_change_range_type(&e820, pfn << PAGE_SHIFT,
                                  (pfn << PAGE_SHIFT) + PAGE_SIZE,
                                  E820_RESERVED, E820_RAM) )
         panic("Unable to add/change memory type of pfn %#lx to RAM\n", pfn);
 }
 
-void __init pv_shim_fixup_e820(struct e820map *e820)
+void __init pv_shim_fixup_e820(void)
 {
     uint64_t pfn = 0;
     unsigned int i = 0;
@@ -120,7 +120,7 @@ void __init pv_shim_fixup_e820(struct e820map *e820)
     rc = xen_hypercall_hvm_get_param(p, &pfn);  \
     if ( rc )                                   \
         panic("Unable to get " #p "\n");        \
-    mark_pfn_as_ram(e820, pfn);                 \
+    mark_pfn_as_ram(pfn);                 \
     ASSERT(i < ARRAY_SIZE(reserved_pages));     \
     reserved_pages[i++].mfn = pfn;              \
 })
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 80ae973d64..03f9a03180 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -686,7 +686,7 @@ static void __init parse_video_info(void)
 #endif
 }
 
-static void __init kexec_reserve_area(struct e820map *e820)
+static void __init kexec_reserve_area(void)
 {
 #ifdef CONFIG_KEXEC
     unsigned long kdump_start = kexec_crash_area.start;
@@ -700,7 +700,7 @@ static void __init kexec_reserve_area(struct e820map *e820)
 
     is_reserved = true;
 
-    if ( !reserve_e820_ram(e820, kdump_start, kdump_start + kdump_size) )
+    if ( !reserve_e820_ram(&boot_e820, kdump_start, kdump_start + kdump_size) )
     {
         printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at %#lx)"
                "\n", kdump_size >> 20, kdump_size >> 10, kdump_start);
@@ -1308,7 +1308,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         if ( e820.map[i].type == E820_RAM )
             nr_pages += e820.map[i].size >> PAGE_SHIFT;
     set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
-    kexec_reserve_area(&boot_e820);
+    kexec_reserve_area();
 
     initial_images = mod;
     nr_initial_images = mbi->mods_count;
@@ -1495,7 +1495,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
 
     /* Late kexec reservation (dynamic start address). */
-    kexec_reserve_area(&boot_e820);
+    kexec_reserve_area();
 
     setup_max_pdx(raw_max_page);
     if ( highmem_start )
diff --git a/xen/arch/x86/x86_64/mmconf-fam10h.c b/xen/arch/x86/x86_64/mmconf-fam10h.c
index a834ab3149..36b32eb769 100644
--- a/xen/arch/x86/x86_64/mmconf-fam10h.c
+++ b/xen/arch/x86/x86_64/mmconf-fam10h.c
@@ -135,7 +135,7 @@ static void __init get_fam10h_pci_mmconf_base(void)
 	return;
 
 out:
-	if (e820_add_range(&e820, start, start + SIZE, E820_RESERVED))
+	if (e820_add_range(start, start + SIZE, E820_RESERVED))
 		fam10h_pci_mmconf_base = start;
 }
 
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 3b577c9b39..db993d6df2 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -418,7 +418,7 @@ static int __init parse_ivmd_block(const struct acpi_ivrs_memory *ivmd_block)
 
             if ( type == RAM_TYPE_UNKNOWN )
             {
-                if ( e820_add_range(&e820, addr, addr + PAGE_SIZE,
+                if ( e820_add_range(addr, addr + PAGE_SIZE,
                                     E820_RESERVED) )
                     continue;
                 AMD_IOMMU_ERROR("IVMD: page at %lx couldn't be reserved\n",
-- 
2.34.1

[XEN PATCH v2 2/5] xen/delay: address MISRA C:2012 Rule 5.3.

[Nicola Vetrini]

The variable 'msec' declared in the macro shadows the local
variable in 'ehci_dbgp_bios_handoff', but to prevent any
future clashes with other functions the macro is converted to
a static inline function.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Switched to a static inline function, as suggested by Julien
---
 xen/include/xen/delay.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/delay.h b/xen/include/xen/delay.h
index 9d70ef035f..6ecee851f6 100644
--- a/xen/include/xen/delay.h
+++ b/xen/include/xen/delay.h
@@ -4,7 +4,12 @@
 /* Copyright (C) 1993 Linus Torvalds */
 
 #include <asm/delay.h>
-#define mdelay(n) (\
-	{unsigned long msec=(n); while (msec--) udelay(1000);})
+
+static inline void mdelay(unsigned long n)
+{
+    unsigned long msec=n;
+    while ( msec-- )
+        udelay(1000);
+}
 
 #endif /* defined(_LINUX_DELAY_H) */
-- 
2.34.1

[XEN PATCH v2 3/5] x86/include: address MISRA C:2012 Rule 5.3.

[Nicola Vetrini]

Variable 'mpc_default_type' in 'xen/arch/x86/include/asm/mpspec.h'
has no uses and causes shadowing with function parameter names
in 'mpparse.c'. Therefore, it is removed.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Removed the variable.
---
 xen/arch/x86/include/asm/mpspec.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/mpspec.h b/xen/arch/x86/include/asm/mpspec.h
index 1246eece0b..45e474dfd1 100644
--- a/xen/arch/x86/include/asm/mpspec.h
+++ b/xen/arch/x86/include/asm/mpspec.h
@@ -15,7 +15,6 @@ extern void get_smp_config (void);
 extern unsigned char apic_version [MAX_APICS];
 extern int mp_irq_entries;
 extern struct mpc_config_intsrc mp_irqs [MAX_IRQ_SOURCES];
-extern int mpc_default_type;
 extern unsigned long mp_lapic_addr;
 extern bool pic_mode;
 
-- 
2.34.1

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

[Nicola Vetrini]

Rename the local variables s/xsave/xstate/ to avoid clashing
with function 'xsave' defined in 'xen/arch/x86/include/asm/xstate.h'.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- s/xsave_area/xstate/, since that is the newest common name, also
used in other parts of the modified file.
---
 xen/arch/x86/xstate.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 770747d88f..f442610fc5 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -174,10 +174,10 @@ static void setup_xstate_comp(uint16_t *comp_offsets,
  */
 void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
 {
-    const struct xsave_struct *xsave = v->arch.xsave_area;
+    const struct xsave_struct *xstate = v->arch.xsave_area;
     const void *src;
     uint16_t comp_offsets[sizeof(xfeature_mask)*8];
-    u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
+    u64 xstate_bv = xstate->xsave_hdr.xstate_bv;
     u64 valid;
 
     /* Check there is state to serialise (i.e. at least an XSAVE_HDR) */
@@ -185,19 +185,19 @@ void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
     /* Check there is the correct room to decompress into. */
     BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
 
-    if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
+    if ( !(xstate->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
     {
-        memcpy(dest, xsave, size);
+        memcpy(dest, xstate, size);
         return;
     }
 
-    ASSERT(xsave_area_compressed(xsave));
-    setup_xstate_comp(comp_offsets, xsave->xsave_hdr.xcomp_bv);
+    ASSERT(xsave_area_compressed(xstate));
+    setup_xstate_comp(comp_offsets, xstate->xsave_hdr.xcomp_bv);
 
     /*
      * Copy legacy XSAVE area and XSAVE hdr area.
      */
-    memcpy(dest, xsave, XSTATE_AREA_MIN_SIZE);
+    memcpy(dest, xstate, XSTATE_AREA_MIN_SIZE);
     memset(dest + XSTATE_AREA_MIN_SIZE, 0, size - XSTATE_AREA_MIN_SIZE);
 
     ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
@@ -206,7 +206,7 @@ void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
      * Copy each region from the possibly compacted offset to the
      * non-compacted offset.
      */
-    src = xsave;
+    src = xstate;
     valid = xstate_bv & ~XSTATE_FP_SSE;
     while ( valid )
     {
@@ -239,7 +239,7 @@ void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
  */
 void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
 {
-    struct xsave_struct *xsave = v->arch.xsave_area;
+    struct xsave_struct *xstate = v->arch.xsave_area;
     void *dest;
     uint16_t comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv, valid;
@@ -252,7 +252,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
 
     if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
     {
-        memcpy(xsave, src, size);
+        memcpy(xstate, src, size);
         return;
     }
 
@@ -260,19 +260,19 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
      * Copy legacy XSAVE area, to avoid complications with CPUID
      * leaves 0 and 1 in the loop below.
      */
-    memcpy(xsave, src, FXSAVE_SIZE);
+    memcpy(xstate, src, FXSAVE_SIZE);
 
     /* Set XSTATE_BV and XCOMP_BV.  */
-    xsave->xsave_hdr.xstate_bv = xstate_bv;
-    xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
+    xstate->xsave_hdr.xstate_bv = xstate_bv;
+    xstate->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
 
-    setup_xstate_comp(comp_offsets, xsave->xsave_hdr.xcomp_bv);
+    setup_xstate_comp(comp_offsets, xstate->xsave_hdr.xcomp_bv);
 
     /*
      * Copy each region from the non-compacted offset to the
      * possibly compacted offset.
      */
-    dest = xsave;
+    dest = xstate;
     valid = xstate_bv & ~XSTATE_FP_SSE;
     while ( valid )
     {
-- 
2.34.1

[XEN PATCH v2 5/5] x86: refactor macros in 'xen-mca.h'

[Nicola Vetrini]

The macros defined 'xen/include/public/arch-x86/xen-mca.h' are revised
to address the following concerns:
- needless underscore prefixes for parameter names;
- the variable 'i' in function 'mce_action' that is shadowed
  by the local variable in the macro.

Therefore, the refactoring aims to resolve present shadowing
issues, which violate MISRA C:2012 Rule 5.3, and lessen the
probability of future ones with some renames.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- Added missing parentheses.
- Clarified commit message.
---
 xen/include/public/arch-x86/xen-mca.h | 38 +++++++++++++--------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h
index b897536ec5..bb1b12f14f 100644
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -280,39 +280,39 @@ DEFINE_XEN_GUEST_HANDLE(xen_mc_logical_cpu_t);
 /* Prototype:
  *    uint32_t x86_mcinfo_nentries(struct mc_info *mi);
  */
-#define x86_mcinfo_nentries(_mi)    \
-    (_mi)->mi_nentries
+#define x86_mcinfo_nentries(mi)    \
+    (mi)->mi_nentries
 /* Prototype:
  *    struct mcinfo_common *x86_mcinfo_first(struct mc_info *mi);
  */
-#define x86_mcinfo_first(_mi)       \
-    ((struct mcinfo_common *)(_mi)->mi_data)
+#define x86_mcinfo_first(mi)       \
+    ((struct mcinfo_common *)(mi)->mi_data)
 /* Prototype:
  *    struct mcinfo_common *x86_mcinfo_next(struct mcinfo_common *mic);
  */
-#define x86_mcinfo_next(_mic)       \
-    ((struct mcinfo_common *)((uint8_t *)(_mic) + (_mic)->size))
+#define x86_mcinfo_next(mic)       \
+    ((struct mcinfo_common *)((uint8_t *)(mic) + (mic)->size))
 
 /* Prototype:
- *    void x86_mcinfo_lookup(void *ret, struct mc_info *mi, uint16_t type);
+ *    void x86_mcinfo_lookup(void *ret, struct mc_info *mi, uint16_t mc_type);
  */
-#define x86_mcinfo_lookup(_ret, _mi, _type)    \
+#define x86_mcinfo_lookup(ret, mi, mc_type)                     \
     do {                                                        \
-        uint32_t found, i;                                      \
-        struct mcinfo_common *_mic;                             \
+        uint32_t found_, i_;                                    \
+        struct mcinfo_common *mic_;                             \
                                                                 \
-        found = 0;                                              \
-        (_ret) = NULL;                                          \
-        if (_mi == NULL) break;                                 \
-        _mic = x86_mcinfo_first(_mi);                           \
-        for (i = 0; i < x86_mcinfo_nentries(_mi); i++) {        \
-            if (_mic->type == (_type)) {                        \
-                found = 1;                                      \
+        found_ = 0;                                             \
+        (ret) = NULL;                                           \
+        if ((mi) == NULL) break;                                \
+        mic_ = x86_mcinfo_first(mi);                            \
+        for (i_ = 0; i_ < x86_mcinfo_nentries(mi); i_++) {      \
+            if (mic_->type == (mc_type)) {                      \
+                found_ = 1;                                     \
                 break;                                          \
             }                                                   \
-            _mic = x86_mcinfo_next(_mic);                       \
+            mic_ = x86_mcinfo_next(mic_);                       \
         }                                                       \
-        (_ret) = found ? _mic : NULL;                           \
+        (ret) = found_ ? mic_ : NULL;                           \
     } while (0)
 
 
-- 
2.34.1

Re: [XEN PATCH v2 5/5] x86: refactor macros in 'xen-mca.h'

[Nicola Vetrini]

On 08/08/2023 13:08, Nicola Vetrini wrote:
> The macros defined 'xen/include/public/arch-x86/xen-mca.h' are revised
> to address the following concerns:
> - needless underscore prefixes for parameter names;
> - the variable 'i' in function 'mce_action' that is shadowed
>   by the local variable in the macro.
> 
> Therefore, the refactoring aims to resolve present shadowing
> issues, which violate MISRA C:2012 Rule 5.3, and lessen the
> probability of future ones with some renames.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Changes in v2:
> - Added missing parentheses.
> - Clarified commit message.
> ---

Missing a
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
from the other version, since nothing changed code-wise.
-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH v2 5/5] x86: refactor macros in 'xen-mca.h'

[Jan Beulich]

On 08.08.2023 14:02, Nicola Vetrini wrote:
> On 08/08/2023 13:08, Nicola Vetrini wrote:
>> The macros defined 'xen/include/public/arch-x86/xen-mca.h' are revised
>> to address the following concerns:
>> - needless underscore prefixes for parameter names;
>> - the variable 'i' in function 'mce_action' that is shadowed
>>   by the local variable in the macro.
>>
>> Therefore, the refactoring aims to resolve present shadowing
>> issues, which violate MISRA C:2012 Rule 5.3, and lessen the
>> probability of future ones with some renames.
>>
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Changes in v2:
>> - Added missing parentheses.

This ...

>> - Clarified commit message.
>> ---
> 
> Missing a
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> from the other version, since nothing changed code-wise.

... isn't exactly "nothing", but well.

Jan

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

[Jan Beulich]

On 08.08.2023 13:08, Nicola Vetrini wrote:
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -543,27 +543,27 @@ static void __init machine_specific_memory_setup(struct e820map *raw)
>          clip_to_limit(top_of_ram, "MTRRs do not cover all of memory.");
>  }
>  
> -/* This function relies on the passed in e820->map[] being sorted. */
> -int __init e820_add_range(
> -    struct e820map *e820, uint64_t s, uint64_t e, uint32_t type)
> +/* This function relies on the global e820->map[] being sorted. */
> +int __init e820_add_range(uint64_t s, uint64_t e, uint32_t type)
>  {
>      unsigned int i;
> +    struct e820entry *ei = e820.map;
>  
> -    for ( i = 0; i < e820->nr_map; ++i )
> +    for ( i = 0; i < e820.nr_map; ++i )
>      {
> -        uint64_t rs = e820->map[i].addr;
> -        uint64_t re = rs + e820->map[i].size;
> +        uint64_t rs = ei[i].addr;
> +        uint64_t re = rs + ei[i].size;
>  
> -        if ( rs == e && e820->map[i].type == type )
> +        if ( rs == e && ei[i].type == type )
>          {
> -            e820->map[i].addr = s;
> +            ei[i].addr = s;
>              return 1;
>          }
>  
> -        if ( re == s && e820->map[i].type == type &&
> -             (i + 1 == e820->nr_map || e820->map[i + 1].addr >= e) )
> +        if ( re == s && ei[i].type == type &&
> +             (i + 1 == e820.nr_map || ei[i + 1].addr >= e) )
>          {
> -            e820->map[i].size += e - s;
> +            ei[i].size += e - s;
>              return 1;
>          }
>  
> @@ -574,20 +574,20 @@ int __init e820_add_range(
>              return 0;
>      }
>  
> -    if ( e820->nr_map >= ARRAY_SIZE(e820->map) )
> +    if ( e820.nr_map >= ARRAY_SIZE(e820.map) )
>      {
>          printk(XENLOG_WARNING "E820: overflow while adding region"
>                 " %"PRIx64"-%"PRIx64"\n", s, e);
>          return 0;
>      }
>  
> -    memmove(e820->map + i + 1, e820->map + i,
> -            (e820->nr_map - i) * sizeof(*e820->map));
> +    memmove(ei + i + 1, ei + i,
> +            (e820.nr_map - i) * sizeof(*e820.map));
>  
> -    e820->nr_map++;
> -    e820->map[i].addr = s;
> -    e820->map[i].size = e - s;
> -    e820->map[i].type = type;
> +    e820.nr_map++;
> +    ei[i].addr = s;
> +    ei[i].size = e - s;
> +    ei[i].type = type;
>  
>      return 1;
>  }

To be honest this isn't quite what I was hoping for; the many ei[i]. are
(imo) quite a bit harder to read than ei-> would have been (hence my
earlier suggestion to also update that pointer in the for() loop header).
Then again I see there is one use of ei[i + 1], which would likely look
less neat as ei[1].addr when everywhere else we have ei->. So I guess up
to you whether you adjust further; I'll ack either form.

> --- a/xen/arch/x86/guest/hypervisor.c
> +++ b/xen/arch/x86/guest/hypervisor.c
> @@ -63,7 +63,7 @@ void hypervisor_resume(void)
>  void __init hypervisor_e820_fixup(struct e820map *e820)

What about this one? The function parameter ...

>  {
>      if ( ops.e820_fixup )
> -        ops.e820_fixup(e820);
> +        ops.e820_fixup();
>  }

... isn't used anymore, and the sole call site passes &e820.

> --- a/xen/arch/x86/include/asm/e820.h
> +++ b/xen/arch/x86/include/asm/e820.h
> @@ -29,8 +29,7 @@ extern int reserve_e820_ram(struct e820map *e820, uint64_t s, uint64_t e);
>  extern int e820_change_range_type(
>      struct e820map *e820, uint64_t s, uint64_t e,
>      uint32_t orig_type, uint32_t new_type);

And what about this one? None of the other subjects in the series suggest
this is then taken care of in a separate patch (as per the earlier
discussion it indeed doesn't want dealing with right here).

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -686,7 +686,7 @@ static void __init parse_video_info(void)
>  #endif
>  }
>  
> -static void __init kexec_reserve_area(struct e820map *e820)
> +static void __init kexec_reserve_area(void)
>  {
>  #ifdef CONFIG_KEXEC
>      unsigned long kdump_start = kexec_crash_area.start;
> @@ -700,7 +700,7 @@ static void __init kexec_reserve_area(struct e820map *e820)
>  
>      is_reserved = true;
>  
> -    if ( !reserve_e820_ram(e820, kdump_start, kdump_start + kdump_size) )
> +    if ( !reserve_e820_ram(&boot_e820, kdump_start, kdump_start + kdump_size) )
>      {
>          printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at %#lx)"
>                 "\n", kdump_size >> 20, kdump_size >> 10, kdump_start);
> @@ -1308,7 +1308,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          if ( e820.map[i].type == E820_RAM )
>              nr_pages += e820.map[i].size >> PAGE_SHIFT;
>      set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
> -    kexec_reserve_area(&boot_e820);
> +    kexec_reserve_area();
>  
>      initial_images = mod;
>      nr_initial_images = mbi->mods_count;
> @@ -1495,7 +1495,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
>  
>      /* Late kexec reservation (dynamic start address). */
> -    kexec_reserve_area(&boot_e820);
> +    kexec_reserve_area();
>  
>      setup_max_pdx(raw_max_page);
>      if ( highmem_start )

Seeing all the knock-on effects for the add_range() change, I think this
separate adjustment would better have been an independent patch.

Jan

Re: [XEN PATCH v2 2/5] xen/delay: address MISRA C:2012 Rule 5.3.

[Jan Beulich]

On 08.08.2023 13:08, Nicola Vetrini wrote:
> --- a/xen/include/xen/delay.h
> +++ b/xen/include/xen/delay.h
> @@ -4,7 +4,12 @@
>  /* Copyright (C) 1993 Linus Torvalds */
>  
>  #include <asm/delay.h>
> -#define mdelay(n) (\
> -	{unsigned long msec=(n); while (msec--) udelay(1000);})
> +
> +static inline void mdelay(unsigned long n)
> +{
> +    unsigned long msec=n;
> +    while ( msec-- )
> +        udelay(1000);
> +}

Nit: Style (blanks around = and blank line between declaration and
statement). If need be I guess this again can be taken care of while
committing. With the adjustments
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [XEN PATCH v2 2/5] xen/delay: address MISRA C:2012 Rule 5.3.

[Jan Beulich]

On 08.08.2023 15:56, Jan Beulich wrote:
> On 08.08.2023 13:08, Nicola Vetrini wrote:
>> --- a/xen/include/xen/delay.h
>> +++ b/xen/include/xen/delay.h
>> @@ -4,7 +4,12 @@
>>  /* Copyright (C) 1993 Linus Torvalds */
>>  
>>  #include <asm/delay.h>
>> -#define mdelay(n) (\
>> -	{unsigned long msec=(n); while (msec--) udelay(1000);})
>> +
>> +static inline void mdelay(unsigned long n)
>> +{
>> +    unsigned long msec=n;
>> +    while ( msec-- )
>> +        udelay(1000);
>> +}
> 
> Nit: Style (blanks around = and blank line between declaration and
> statement). If need be I guess this again can be taken care of while
> committing. With the adjustments
> Acked-by: Jan Beulich <jbeulich@suse.com>

Actually - why have a local variable here at all? Just name the
function parameter "msec".

Jan

Re: [XEN PATCH v2 3/5] x86/include: address MISRA C:2012 Rule 5.3.

[Jan Beulich]

On 08.08.2023 13:08, Nicola Vetrini wrote:
> Variable 'mpc_default_type' in 'xen/arch/x86/include/asm/mpspec.h'
> has no uses and causes shadowing with function parameter names
> in 'mpparse.c'. Therefore, it is removed.

Personally I'd again wish this could be expressed more precisely than
just "variable". What you're removing is a declaration which has no
definition. So aiui shadowing is only one of the concerns Misra would
have here; the lack of a definition would be another.

> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

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

[Jan Beulich]

On 08.08.2023 13:08, Nicola Vetrini wrote:
> Rename the local variables s/xsave/xstate/ to avoid clashing
> with function 'xsave' defined in 'xen/arch/x86/include/asm/xstate.h'.

s/defined/declared/ (in some areas of Misra the distinction looks to
be quite relevant, so being precise everywhere is helpful even if only
to not leave uncertainty)

> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Re: [XEN PATCH v2 5/5] x86: refactor macros in 'xen-mca.h'

[Jan Beulich]

On 08.08.2023 13:08, Nicola Vetrini wrote:
> The macros defined 'xen/include/public/arch-x86/xen-mca.h' are revised
> to address the following concerns:
> - needless underscore prefixes for parameter names;
> - the variable 'i' in function 'mce_action' that is shadowed
>   by the local variable in the macro.
> 
> Therefore, the refactoring aims to resolve present shadowing
> issues, which violate MISRA C:2012 Rule 5.3, and lessen the
> probability of future ones with some renames.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

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

[Nicola Vetrini]

On 08/08/2023 15:46, Jan Beulich wrote:
> On 08.08.2023 13:08, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/e820.c
>> +++ b/xen/arch/x86/e820.c
>> @@ -543,27 +543,27 @@ static void __init 
>> machine_specific_memory_setup(struct e820map *raw)
>>          clip_to_limit(top_of_ram, "MTRRs do not cover all of 
>> memory.");
>>  }
>> 
>> -/* This function relies on the passed in e820->map[] being sorted. */
>> -int __init e820_add_range(
>> -    struct e820map *e820, uint64_t s, uint64_t e, uint32_t type)
>> +/* This function relies on the global e820->map[] being sorted. */
>> +int __init e820_add_range(uint64_t s, uint64_t e, uint32_t type)
>>  {
>>      unsigned int i;
>> +    struct e820entry *ei = e820.map;
>> 
>> -    for ( i = 0; i < e820->nr_map; ++i )
>> +    for ( i = 0; i < e820.nr_map; ++i )
>>      {
>> -        uint64_t rs = e820->map[i].addr;
>> -        uint64_t re = rs + e820->map[i].size;
>> +        uint64_t rs = ei[i].addr;
>> +        uint64_t re = rs + ei[i].size;
>> 
>> -        if ( rs == e && e820->map[i].type == type )
>> +        if ( rs == e && ei[i].type == type )
>>          {
>> -            e820->map[i].addr = s;
>> +            ei[i].addr = s;
>>              return 1;
>>          }
>> 
>> -        if ( re == s && e820->map[i].type == type &&
>> -             (i + 1 == e820->nr_map || e820->map[i + 1].addr >= e) )
>> +        if ( re == s && ei[i].type == type &&
>> +             (i + 1 == e820.nr_map || ei[i + 1].addr >= e) )
>>          {
>> -            e820->map[i].size += e - s;
>> +            ei[i].size += e - s;
>>              return 1;
>>          }
>> 
>> @@ -574,20 +574,20 @@ int __init e820_add_range(
>>              return 0;
>>      }
>> 
>> -    if ( e820->nr_map >= ARRAY_SIZE(e820->map) )
>> +    if ( e820.nr_map >= ARRAY_SIZE(e820.map) )
>>      {
>>          printk(XENLOG_WARNING "E820: overflow while adding region"
>>                 " %"PRIx64"-%"PRIx64"\n", s, e);
>>          return 0;
>>      }
>> 
>> -    memmove(e820->map + i + 1, e820->map + i,
>> -            (e820->nr_map - i) * sizeof(*e820->map));
>> +    memmove(ei + i + 1, ei + i,
>> +            (e820.nr_map - i) * sizeof(*e820.map));
>> 
>> -    e820->nr_map++;
>> -    e820->map[i].addr = s;
>> -    e820->map[i].size = e - s;
>> -    e820->map[i].type = type;
>> +    e820.nr_map++;
>> +    ei[i].addr = s;
>> +    ei[i].size = e - s;
>> +    ei[i].type = type;
>> 
>>      return 1;
>>  }
> 
> To be honest this isn't quite what I was hoping for; the many ei[i]. 
> are
> (imo) quite a bit harder to read than ei-> would have been (hence my
> earlier suggestion to also update that pointer in the for() loop 
> header).
> Then again I see there is one use of ei[i + 1], which would likely look
> less neat as ei[1].addr when everywhere else we have ei->. So I guess 
> up
> to you whether you adjust further; I'll ack either form.
> 

I'll leave it as is.

>> --- a/xen/arch/x86/guest/hypervisor.c
>> +++ b/xen/arch/x86/guest/hypervisor.c
>> @@ -63,7 +63,7 @@ void hypervisor_resume(void)
>>  void __init hypervisor_e820_fixup(struct e820map *e820)
> 
> What about this one? The function parameter ...
> 
>>  {
>>      if ( ops.e820_fixup )
>> -        ops.e820_fixup(e820);
>> +        ops.e820_fixup();
>>  }
> 
> ... isn't used anymore, and the sole call site passes &e820.
> 

It remained there by accident.

>> --- a/xen/arch/x86/include/asm/e820.h
>> +++ b/xen/arch/x86/include/asm/e820.h
>> @@ -29,8 +29,7 @@ extern int reserve_e820_ram(struct e820map *e820, 
>> uint64_t s, uint64_t e);
>>  extern int e820_change_range_type(
>>      struct e820map *e820, uint64_t s, uint64_t e,
>>      uint32_t orig_type, uint32_t new_type);
> 
> And what about this one? None of the other subjects in the series 
> suggest
> this is then taken care of in a separate patch (as per the earlier
> discussion it indeed doesn't want dealing with right here).
> 

I'll mention this detail. While I work on other rules I'll think of a 
good way to rename.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -686,7 +686,7 @@ static void __init parse_video_info(void)
>>  #endif
>>  }
>> 
>> -static void __init kexec_reserve_area(struct e820map *e820)
>> +static void __init kexec_reserve_area(void)
>>  {
>>  #ifdef CONFIG_KEXEC
>>      unsigned long kdump_start = kexec_crash_area.start;
>> @@ -700,7 +700,7 @@ static void __init kexec_reserve_area(struct 
>> e820map *e820)
>> 
>>      is_reserved = true;
>> 
>> -    if ( !reserve_e820_ram(e820, kdump_start, kdump_start + 
>> kdump_size) )
>> +    if ( !reserve_e820_ram(&boot_e820, kdump_start, kdump_start + 
>> kdump_size) )
>>      {
>>          printk("Kdump: DISABLED (failed to reserve %luMB (%lukB) at 
>> %#lx)"
>>                 "\n", kdump_size >> 20, kdump_size >> 10, 
>> kdump_start);
>> @@ -1308,7 +1308,7 @@ void __init noreturn __start_xen(unsigned long 
>> mbi_p)
>>          if ( e820.map[i].type == E820_RAM )
>>              nr_pages += e820.map[i].size >> PAGE_SHIFT;
>>      set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
>> -    kexec_reserve_area(&boot_e820);
>> +    kexec_reserve_area();
>> 
>>      initial_images = mod;
>>      nr_initial_images = mbi->mods_count;
>> @@ -1495,7 +1495,7 @@ void __init noreturn __start_xen(unsigned long 
>> mbi_p)
>>          reserve_e820_ram(&boot_e820, __pa(_stext), 
>> __pa(__2M_rwdata_end));
>> 
>>      /* Late kexec reservation (dynamic start address). */
>> -    kexec_reserve_area(&boot_e820);
>> +    kexec_reserve_area();
>> 
>>      setup_max_pdx(raw_max_page);
>>      if ( highmem_start )
> 
> Seeing all the knock-on effects for the add_range() change, I think 
> this
> separate adjustment would better have been an independent patch.
> 
> Jan

I can submit it standalone and put together x86/vmsi and delay

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