* [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3
@ 2023-08-04 15:27 Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 1/6] x86: rename variable 'e820' to " Nicola Vetrini
` (5 more replies)
0 siblings, 6 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:27 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Nicola Vetrini, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, George Dunlap, Julien Grall
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.
In some cases global objects are modified, while other modifications concern
local variables, possibly lessening the probability of future clashes from
occurring, by suffixing variables in some macros.
No functional change.
Nicola Vetrini (6):
x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
libelf: 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' to address MISRA C:2012 Rule 5.3
xen/arch/x86/dom0_build.c | 10 ++--
xen/arch/x86/e820.c | 66 ++++++++++++------------
xen/arch/x86/guest/xen/xen.c | 4 +-
xen/arch/x86/hvm/dom0_build.c | 6 +--
xen/arch/x86/include/asm/e820.h | 2 +-
xen/arch/x86/include/asm/mpspec.h | 2 +-
xen/arch/x86/mm.c | 49 +++++++++---------
xen/arch/x86/numa.c | 8 +--
xen/arch/x86/setup.c | 22 ++++----
xen/arch/x86/srat.c | 6 +--
xen/arch/x86/x86_64/mmconf-fam10h.c | 2 +-
xen/arch/x86/xstate.c | 30 +++++------
xen/common/libelf/libelf-tools.c | 24 ++++-----
xen/drivers/passthrough/amd/iommu_acpi.c | 2 +-
xen/include/public/arch-x86/xen-mca.h | 38 +++++++-------
xen/include/xen/delay.h | 2 +-
16 files changed, 137 insertions(+), 136 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-04 15:27 [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3 Nicola Vetrini
@ 2023-08-04 15:27 ` Nicola Vetrini
2023-08-04 21:19 ` Stefano Stabellini
2023-08-07 8:09 ` Jan Beulich
2023-08-04 15:27 ` [XEN PATCH 2/6] libelf: " Nicola Vetrini
` (4 subsequent siblings)
5 siblings, 2 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:27 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Nicola Vetrini, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
The variable declared in the header file 'xen/arch/x86/include/asm/e820.h'
is shadowed by many function parameters, so it is renamed to avoid these
violations.
No functional changes.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
This patch is similar to other renames done on previous patches, and the
preferred strategy there was to rename the global variable. This one
has more occurrences that are spread in various files, but
the general pattern is the same.
---
xen/arch/x86/dom0_build.c | 10 ++--
xen/arch/x86/e820.c | 66 ++++++++++++------------
xen/arch/x86/guest/xen/xen.c | 4 +-
xen/arch/x86/hvm/dom0_build.c | 6 +--
xen/arch/x86/include/asm/e820.h | 2 +-
xen/arch/x86/mm.c | 49 +++++++++---------
xen/arch/x86/numa.c | 8 +--
xen/arch/x86/setup.c | 22 ++++----
xen/arch/x86/srat.c | 6 +--
xen/arch/x86/x86_64/mmconf-fam10h.c | 2 +-
xen/drivers/passthrough/amd/iommu_acpi.c | 2 +-
11 files changed, 89 insertions(+), 88 deletions(-)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 8b1fcc6471..bfb6400376 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -534,13 +534,13 @@ int __init dom0_setup_permissions(struct domain *d)
}
/* Remove access to E820_UNUSABLE I/O regions above 1MB. */
- for ( i = 0; i < e820.nr_map; i++ )
+ for ( i = 0; i < e820_map.nr_map; i++ )
{
unsigned long sfn, efn;
- sfn = max_t(unsigned long, paddr_to_pfn(e820.map[i].addr), 0x100ul);
- efn = paddr_to_pfn(e820.map[i].addr + e820.map[i].size - 1);
- if ( (e820.map[i].type == E820_UNUSABLE) &&
- (e820.map[i].size != 0) &&
+ sfn = max_t(unsigned long, paddr_to_pfn(e820_map.map[i].addr), 0x100ul);
+ efn = paddr_to_pfn(e820_map.map[i].addr + e820_map.map[i].size - 1);
+ if ( (e820_map.map[i].type == E820_UNUSABLE) &&
+ (e820_map.map[i].size != 0) &&
(sfn <= efn) )
rc |= iomem_deny_access(d, sfn, efn);
}
diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 0b89935510..4425011c01 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -34,7 +34,7 @@ boolean_param("e820-mtrr-clip", e820_mtrr_clip);
static bool __initdata e820_verbose;
boolean_param("e820-verbose", e820_verbose);
-struct e820map e820;
+struct e820map e820_map;
struct e820map __initdata e820_raw;
/*
@@ -47,8 +47,8 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
{
unsigned int i;
- for (i = 0; i < e820.nr_map; i++) {
- struct e820entry *ei = &e820.map[i];
+ for (i = 0; i < e820_map.nr_map; i++) {
+ struct e820entry *ei = &e820_map.map[i];
if (type && ei->type != type)
continue;
@@ -75,17 +75,17 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
static void __init add_memory_region(unsigned long long start,
unsigned long long size, int type)
{
- unsigned int x = e820.nr_map;
+ unsigned int x = e820_map.nr_map;
- if (x == ARRAY_SIZE(e820.map)) {
+ if (x == ARRAY_SIZE(e820_map.map)) {
printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
return;
}
- e820.map[x].addr = start;
- e820.map[x].size = size;
- e820.map[x].type = type;
- e820.nr_map++;
+ e820_map.map[x].addr = start;
+ e820_map.map[x].size = size;
+ e820_map.map[x].type = type;
+ e820_map.nr_map++;
}
void __init print_e820_memory_map(const struct e820entry *map,
@@ -347,13 +347,13 @@ static unsigned long __init find_max_pfn(void)
unsigned int i;
unsigned long max_pfn = 0;
- for (i = 0; i < e820.nr_map; i++) {
+ for (i = 0; i < e820_map.nr_map; i++) {
unsigned long start, end;
/* RAM? */
- if (e820.map[i].type != E820_RAM)
+ if (e820_map.map[i].type != E820_RAM)
continue;
- start = PFN_UP(e820.map[i].addr);
- end = PFN_DOWN(e820.map[i].addr + e820.map[i].size);
+ start = PFN_UP(e820_map.map[i].addr);
+ end = PFN_DOWN(e820_map.map[i].addr + e820_map.map[i].size);
if (start >= end)
continue;
if (end > max_pfn)
@@ -372,21 +372,21 @@ static void __init clip_to_limit(uint64_t limit, const char *warnmsg)
for ( ; ; )
{
/* Find a RAM region needing clipping. */
- for ( i = 0; i < e820.nr_map; i++ )
- if ( (e820.map[i].type == E820_RAM) &&
- ((e820.map[i].addr + e820.map[i].size) > limit) )
+ for ( i = 0; i < e820_map.nr_map; i++ )
+ if ( (e820_map.map[i].type == E820_RAM) &&
+ ((e820_map.map[i].addr + e820_map.map[i].size) > limit) )
break;
/* If none found, we are done. */
- if ( i == e820.nr_map )
- break;
+ if ( i == e820_map.nr_map )
+ break;
old_limit = max_t(
- uint64_t, old_limit, e820.map[i].addr + e820.map[i].size);
+ uint64_t, old_limit, e820_map.map[i].addr + e820_map.map[i].size);
/* We try to convert clipped RAM areas to E820_UNUSABLE. */
- if ( e820_change_range_type(&e820, max(e820.map[i].addr, limit),
- e820.map[i].addr + e820.map[i].size,
+ if ( e820_change_range_type(&e820_map, max(e820_map.map[i].addr, limit),
+ e820_map.map[i].addr + e820_map.map[i].size,
E820_RAM, E820_UNUSABLE) )
continue;
@@ -394,15 +394,15 @@ static void __init clip_to_limit(uint64_t limit, const char *warnmsg)
* If the type change fails (e.g., not space in table) then we clip or
* delete the region as appropriate.
*/
- if ( e820.map[i].addr < limit )
+ if ( e820_map.map[i].addr < limit )
{
- e820.map[i].size = limit - e820.map[i].addr;
+ e820_map.map[i].size = limit - e820_map.map[i].addr;
}
else
{
- memmove(&e820.map[i], &e820.map[i+1],
- (e820.nr_map - i - 1) * sizeof(struct e820entry));
- e820.nr_map--;
+ memmove(&e820_map.map[i], &e820_map.map[i+1],
+ (e820_map.nr_map - i - 1) * sizeof(struct e820entry));
+ e820_map.nr_map--;
}
}
@@ -497,7 +497,7 @@ static void __init reserve_dmi_region(void)
if ( !what )
break;
if ( ((base + len) > base) &&
- reserve_e820_ram(&e820, base, base + len) )
+ reserve_e820_ram(&e820_map, base, base + len) )
printk("WARNING: %s table located in E820 RAM %"PRIpaddr"-%"PRIpaddr". Fixed.\n",
what, base, base + len);
}
@@ -517,12 +517,12 @@ static void __init machine_specific_memory_setup(struct e820map *raw)
if ( opt_availmem )
{
- for ( i = size = 0; (i < e820.nr_map) && (size <= opt_availmem); i++ )
- if ( e820.map[i].type == E820_RAM )
- size += e820.map[i].size;
+ for ( i = size = 0; (i < e820_map.nr_map) && (size <= opt_availmem); i++ )
+ if ( e820_map.map[i].type == E820_RAM )
+ size += e820_map.map[i].size;
if ( size > opt_availmem )
clip_to_limit(
- e820.map[i-1].addr + e820.map[i-1].size - (size-opt_availmem),
+ e820_map.map[i-1].addr + e820_map.map[i-1].size - (size-opt_availmem),
NULL);
}
@@ -694,10 +694,10 @@ unsigned long __init init_e820(const char *str, struct e820map *raw)
machine_specific_memory_setup(raw);
if ( cpu_has_hypervisor )
- hypervisor_e820_fixup(&e820);
+ hypervisor_e820_fixup(&e820_map);
printk("%s RAM map:\n", str);
- print_e820_memory_map(e820.map, e820.nr_map);
+ print_e820_memory_map(e820_map.map, e820_map.nr_map);
return find_max_pfn();
}
diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
index f93dfc89f7..3ec828b98d 100644
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -147,9 +147,9 @@ static void __init init_memmap(void)
PFN_DOWN(GB(4) - 1))) )
panic("unable to add RAM to in-use PFN rangeset\n");
- for ( i = 0; i < e820.nr_map; i++ )
+ for ( i = 0; i < e820_map.nr_map; i++ )
{
- struct e820entry *e = &e820.map[i];
+ struct e820entry *e = &e820_map.map[i];
if ( rangeset_add_range(mem, PFN_DOWN(e->addr),
PFN_UP(e->addr + e->size - 1)) )
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index bc0e290db6..98203f7a52 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -333,13 +333,13 @@ static __init void pvh_setup_e820(struct domain *d, unsigned long nr_pages)
* Add an extra entry in case we have to split a RAM entry into a RAM and a
* UNUSABLE one in order to truncate it.
*/
- d->arch.e820 = xzalloc_array(struct e820entry, e820.nr_map + 1);
+ d->arch.e820 = xzalloc_array(struct e820entry, e820_map.nr_map + 1);
if ( !d->arch.e820 )
panic("Unable to allocate memory for Dom0 e820 map\n");
entry_guest = d->arch.e820;
/* Clamp e820 memory map to match the memory assigned to Dom0 */
- for ( i = 0, entry = e820.map; i < e820.nr_map; i++, entry++ )
+ for ( i = 0, entry = e820_map.map; i < e820_map.nr_map; i++, entry++ )
{
*entry_guest = *entry;
@@ -392,7 +392,7 @@ static __init void pvh_setup_e820(struct domain *d, unsigned long nr_pages)
next:
d->arch.nr_e820++;
entry_guest++;
- ASSERT(d->arch.nr_e820 <= e820.nr_map + 1);
+ ASSERT(d->arch.nr_e820 <= e820_map.nr_map + 1);
}
ASSERT(cur_pages == nr_pages);
}
diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
index 213d5b5dd2..0865825f7d 100644
--- a/xen/arch/x86/include/asm/e820.h
+++ b/xen/arch/x86/include/asm/e820.h
@@ -34,7 +34,7 @@ extern int e820_add_range(
extern unsigned long init_e820(const char *str, struct e820map *raw);
extern void print_e820_memory_map(const struct e820entry *map,
unsigned int entries);
-extern struct e820map e820;
+extern struct e820map e820_map;
extern struct e820map e820_raw;
/* These symbols live in the boot trampoline. */
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index be2b10a391..6920ac939f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -295,12 +295,12 @@ void __init arch_init_memory(void)
/* Any areas not specified as RAM by the e820 map are considered I/O. */
for ( i = 0, pfn = 0; pfn < max_page; i++ )
{
- while ( (i < e820.nr_map) &&
- (e820.map[i].type != E820_RAM) &&
- (e820.map[i].type != E820_UNUSABLE) )
+ while ( (i < e820_map.nr_map) &&
+ (e820_map.map[i].type != E820_RAM) &&
+ (e820_map.map[i].type != E820_UNUSABLE) )
i++;
- if ( i >= e820.nr_map )
+ if ( i >= e820_map.nr_map )
{
/* No more RAM regions: mark as I/O right to end of memory map. */
rstart_pfn = rend_pfn = max_page;
@@ -309,9 +309,10 @@ void __init arch_init_memory(void)
{
/* Mark as I/O just up as far as next RAM region. */
rstart_pfn = min_t(unsigned long, max_page,
- PFN_UP(e820.map[i].addr));
+ PFN_UP(e820_map.map[i].addr));
rend_pfn = max_t(unsigned long, rstart_pfn,
- PFN_DOWN(e820.map[i].addr + e820.map[i].size));
+ PFN_DOWN(e820_map.map[i].addr
+ + e820_map.map[i].size));
}
/*
@@ -387,9 +388,9 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
uint64_t maddr = pfn_to_paddr(mfn);
int i;
- for ( i = 0; i < e820.nr_map; i++ )
+ for ( i = 0; i < e820_map.nr_map; i++ )
{
- switch ( e820.map[i].type )
+ switch ( e820_map.map[i].type )
{
case E820_RAM:
if ( mem_type & RAM_TYPE_CONVENTIONAL )
@@ -414,8 +415,8 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
}
/* Test the range. */
- if ( (e820.map[i].addr <= maddr) &&
- ((e820.map[i].addr + e820.map[i].size) >= (maddr + PAGE_SIZE)) )
+ if ( (e820_map.map[i].addr <= maddr) &&
+ ((e820_map.map[i].addr + e820_map.map[i].size) >= (maddr + PAGE_SIZE)) )
return 1;
}
@@ -427,17 +428,17 @@ unsigned int page_get_ram_type(mfn_t mfn)
uint64_t last = 0, maddr = mfn_to_maddr(mfn);
unsigned int i, type = 0;
- for ( i = 0; i < e820.nr_map;
- last = e820.map[i].addr + e820.map[i].size, i++ )
+ for ( i = 0; i < e820_map.nr_map;
+ last = e820_map.map[i].addr + e820_map.map[i].size, i++ )
{
- if ( (maddr + PAGE_SIZE) > last && maddr < e820.map[i].addr )
+ if ( (maddr + PAGE_SIZE) > last && maddr < e820_map.map[i].addr )
type |= RAM_TYPE_UNKNOWN;
- if ( (maddr + PAGE_SIZE) <= e820.map[i].addr ||
- maddr >= (e820.map[i].addr + e820.map[i].size) )
+ if ( (maddr + PAGE_SIZE) <= e820_map.map[i].addr ||
+ maddr >= (e820_map.map[i].addr + e820_map.map[i].size) )
continue;
- switch ( e820.map[i].type )
+ switch ( e820_map.map[i].type )
{
case E820_RAM:
type |= RAM_TYPE_CONVENTIONAL;
@@ -778,9 +779,9 @@ bool is_memory_hole(mfn_t start, mfn_t end)
unsigned long e = mfn_x(end);
unsigned int i;
- for ( i = 0; i < e820.nr_map; i++ )
+ for ( i = 0; i < e820_map.nr_map; i++ )
{
- const struct e820entry *entry = &e820.map[i];
+ const struct e820entry *entry = &e820_map.map[i];
if ( !entry->size )
continue;
@@ -4763,16 +4764,16 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
store = !guest_handle_is_null(ctxt.map.buffer);
- if ( store && ctxt.map.nr_entries < e820.nr_map + 1 )
+ if ( store && ctxt.map.nr_entries < e820_map.nr_map + 1 )
return -EINVAL;
buffer = guest_handle_cast(ctxt.map.buffer, e820entry_t);
if ( store && !guest_handle_okay(buffer, ctxt.map.nr_entries) )
return -EFAULT;
- for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < e820.nr_map; ++i, ++ctxt.n )
+ for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < e820_map.nr_map; ++i, ++ctxt.n )
{
- unsigned long s = PFN_DOWN(e820.map[i].addr);
+ unsigned long s = PFN_DOWN(e820_map.map[i].addr);
if ( s > ctxt.s )
{
@@ -4786,12 +4787,12 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
}
if ( store )
{
- if ( ctxt.map.nr_entries <= ctxt.n + (e820.nr_map - i) )
+ if ( ctxt.map.nr_entries <= ctxt.n + (e820_map.nr_map - i) )
return -EINVAL;
- if ( __copy_to_guest_offset(buffer, ctxt.n, e820.map + i, 1) )
+ if ( __copy_to_guest_offset(buffer, ctxt.n, e820_map.map + i, 1) )
return -EFAULT;
}
- ctxt.s = PFN_UP(e820.map[i].addr + e820.map[i].size);
+ ctxt.s = PFN_UP(e820_map.map[i].addr + e820_map.map[i].size);
}
if ( ctxt.s )
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 4b0b297c7e..76827f5f32 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -102,14 +102,14 @@ unsigned int __init arch_get_dma_bitsize(void)
int __init arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)
{
- if ( idx >= e820.nr_map )
+ if ( idx >= e820_map.nr_map )
return -ENOENT;
- if ( e820.map[idx].type != E820_RAM )
+ if ( e820_map.map[idx].type != E820_RAM )
return -ENODATA;
- *start = e820.map[idx].addr;
- *end = *start + e820.map[idx].size;
+ *start = e820_map.map[idx].addr;
+ *end = *start + e820_map.map[idx].size;
return 0;
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 80ae973d64..9c6003e374 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1163,7 +1163,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
}
else if ( efi_enabled(EFI_BOOT) )
memmap_type = "EFI";
- else if ( (e820_raw.nr_map =
+ else if ( (e820_raw.nr_map =
copy_bios_e820(e820_raw.map,
ARRAY_SIZE(e820_raw.map))) != 0 )
{
@@ -1300,13 +1300,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
}
/* Create a temporary copy of the E820 map. */
- memcpy(&boot_e820, &e820, sizeof(e820));
+ memcpy(&boot_e820, &e820_map, sizeof(e820_map));
/* Early kexec reservation (explicit static start address). */
nr_pages = 0;
- for ( i = 0; i < e820.nr_map; i++ )
- if ( e820.map[i].type == E820_RAM )
- nr_pages += e820.map[i].size >> PAGE_SHIFT;
+ for ( i = 0; i < e820_map.nr_map; i++ )
+ if ( e820_map.map[i].type == E820_RAM )
+ nr_pages += e820_map.map[i].size >> PAGE_SHIFT;
set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
kexec_reserve_area(&boot_e820);
@@ -1631,7 +1631,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
- if ( e > s )
+ if ( e > s )
map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
_mfn(s), e - s, PAGE_HYPERVISOR);
}
@@ -1677,9 +1677,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
PAGE_HYPERVISOR_RO);
nr_pages = 0;
- for ( i = 0; i < e820.nr_map; i++ )
- if ( e820.map[i].type == E820_RAM )
- nr_pages += e820.map[i].size >> PAGE_SHIFT;
+ for ( i = 0; i < e820_map.nr_map; i++ )
+ if ( e820_map.map[i].type == E820_RAM )
+ nr_pages += e820_map.map[i].size >> PAGE_SHIFT;
printk("System RAM: %luMB (%lukB)\n",
nr_pages >> (20 - PAGE_SHIFT),
nr_pages << (PAGE_SHIFT - 10));
@@ -1771,7 +1771,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
open_softirq(NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ, new_tlbflush_clock_period);
- if ( opt_watchdog )
+ if ( opt_watchdog )
nmi_watchdog = NMI_LOCAL_APIC;
find_smp_config();
@@ -1983,7 +1983,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
do_initcalls();
- if ( opt_watchdog )
+ if ( opt_watchdog )
watchdog_setup();
if ( !tboot_protect_mem_regions() )
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 3f70338e6e..bbd04978ae 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -301,11 +301,11 @@ void __init srat_parse_regions(paddr_t addr)
acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
srat_parse_region, 0);
- for (mask = srat_region_mask, i = 0; mask && i < e820.nr_map; i++) {
- if (e820.map[i].type != E820_RAM)
+ for (mask = srat_region_mask, i = 0; mask && i < e820_map.nr_map; i++) {
+ if (e820_map.map[i].type != E820_RAM)
continue;
- if (~mask & pdx_region_mask(e820.map[i].addr, e820.map[i].size))
+ if (~mask & pdx_region_mask(e820_map.map[i].addr, e820_map.map[i].size))
mask = 0;
}
diff --git a/xen/arch/x86/x86_64/mmconf-fam10h.c b/xen/arch/x86/x86_64/mmconf-fam10h.c
index a834ab3149..bbebf9219f 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(&e820_map, 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..7ad9e12b8a 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(&e820_map, addr, addr + PAGE_SIZE,
E820_RESERVED) )
continue;
AMD_IOMMU_ERROR("IVMD: page at %lx couldn't be reserved\n",
--
2.34.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [XEN PATCH 2/6] libelf: address MISRA C:2012 Rule 5.3
2023-08-04 15:27 [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3 Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 1/6] x86: rename variable 'e820' to " Nicola Vetrini
@ 2023-08-04 15:27 ` Nicola Vetrini
2023-08-04 21:21 ` Stefano Stabellini
2023-08-07 8:11 ` Jan Beulich
2023-08-04 15:27 ` [XEN PATCH 3/6] xen/delay: " Nicola Vetrini
` (3 subsequent siblings)
5 siblings, 2 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:27 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Nicola Vetrini, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Wei Liu
The types u{8,16,32,64} defined in 'xen/arch/x86/include/asm/types.h'
shadow the variables in the modified function, hence violating Rule 5.3.
Therefore, the rename takes care of the shadowing.
No functional changes.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/common/libelf/libelf-tools.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
index a9edb6a8dc..f0d5da1abf 100644
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -91,10 +91,10 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
{
elf_ptrval ptrval = base + moreoffset;
bool need_swap = elf_swap(elf);
- const uint8_t *u8;
- const uint16_t *u16;
- const uint32_t *u32;
- const uint64_t *u64;
+ const uint8_t *uint8;
+ const uint16_t *uint16;
+ const uint32_t *uint32;
+ const uint64_t *uint64;
if ( !elf_access_ok(elf, ptrval, size) )
return 0;
@@ -102,17 +102,17 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
switch ( size )
{
case 1:
- u8 = (const void*)ptrval;
- return *u8;
+ uint8 = (const void*)ptrval;
+ return *uint8;
case 2:
- u16 = (const void*)ptrval;
- return need_swap ? bswap_16(*u16) : *u16;
+ uint16 = (const void*)ptrval;
+ return need_swap ? bswap_16(*uint16) : *uint16;
case 4:
- u32 = (const void*)ptrval;
- return need_swap ? bswap_32(*u32) : *u32;
+ uint32 = (const void*)ptrval;
+ return need_swap ? bswap_32(*uint32) : *uint32;
case 8:
- u64 = (const void*)ptrval;
- return need_swap ? bswap_64(*u64) : *u64;
+ uint64 = (const void*)ptrval;
+ return need_swap ? bswap_64(*uint64) : *uint64;
default:
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-04 15:27 [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3 Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 1/6] x86: rename variable 'e820' to " Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 2/6] libelf: " Nicola Vetrini
@ 2023-08-04 15:27 ` Nicola Vetrini
2023-08-04 21:23 ` Stefano Stabellini
2023-08-07 8:14 ` Jan Beulich
2023-08-04 15:27 ` [XEN PATCH 4/6] x86/include: " Nicola Vetrini
` (2 subsequent siblings)
5 siblings, 2 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:27 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Nicola Vetrini, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Wei Liu
The variable 'msec' shadows the local variable in
'ehci_dbgp_bios_handoff', but to prevent any future clashes, the one in
the macro gains a suffix.
No functional change.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Unless that file should remain as-is, because it's clearly taken from
linux, but this does not prevent future name clashes with msec.
---
xen/include/xen/delay.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/xen/delay.h b/xen/include/xen/delay.h
index 9d70ef035f..f2d9270e83 100644
--- a/xen/include/xen/delay.h
+++ b/xen/include/xen/delay.h
@@ -5,6 +5,6 @@
#include <asm/delay.h>
#define mdelay(n) (\
- {unsigned long msec=(n); while (msec--) udelay(1000);})
+ {unsigned long msec_=(n); while (msec_--) udelay(1000);})
#endif /* defined(_LINUX_DELAY_H) */
--
2.34.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [XEN PATCH 4/6] x86/include: address MISRA C:2012 Rule 5.3.
2023-08-04 15:27 [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3 Nicola Vetrini
` (2 preceding siblings ...)
2023-08-04 15:27 ` [XEN PATCH 3/6] xen/delay: " Nicola Vetrini
@ 2023-08-04 15:27 ` Nicola Vetrini
2023-08-04 21:24 ` Stefano Stabellini
2023-08-04 15:27 ` [XEN PATCH 5/6] x86/xstate: " Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to " Nicola Vetrini
5 siblings, 1 reply; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:27 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Nicola Vetrini, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
s/mpc_default_type/mpc_default in 'xen/arch/x86/include/asm/mpspec.h'
to avoid clashing with function parameter names in 'mpparse.c'.
No functional changes.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Even though the global variable does not seem to be used anywhere and is
perhaps better to remove it entirely.
---
xen/arch/x86/include/asm/mpspec.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/include/asm/mpspec.h b/xen/arch/x86/include/asm/mpspec.h
index 1246eece0b..cc96ee63bd 100644
--- a/xen/arch/x86/include/asm/mpspec.h
+++ b/xen/arch/x86/include/asm/mpspec.h
@@ -15,7 +15,7 @@ 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 int mpc_default;
extern unsigned long mp_lapic_addr;
extern bool pic_mode;
--
2.34.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [XEN PATCH 5/6] x86/xstate: address MISRA C:2012 Rule 5.3
2023-08-04 15:27 [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3 Nicola Vetrini
` (3 preceding siblings ...)
2023-08-04 15:27 ` [XEN PATCH 4/6] x86/include: " Nicola Vetrini
@ 2023-08-04 15:27 ` Nicola Vetrini
2023-08-04 21:26 ` Stefano Stabellini
2023-08-07 8:23 ` Jan Beulich
2023-08-04 15:27 ` [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to " Nicola Vetrini
5 siblings, 2 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:27 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Nicola Vetrini, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
Rename the local variables to avoid clashing with function 'xstate'
defined below, but declared in the corresponding header file.
No functional changes.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
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 3d566252ea..180455b26d 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 *xsave_area = 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 = xsave_area->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 ( !(xsave_area->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
{
- memcpy(dest, xsave, size);
+ memcpy(dest, xsave_area, size);
return;
}
- ASSERT(xsave_area_compressed(xsave));
- setup_xstate_comp(comp_offsets, xsave->xsave_hdr.xcomp_bv);
+ ASSERT(xsave_area_compressed(xsave_area));
+ setup_xstate_comp(comp_offsets, xsave_area->xsave_hdr.xcomp_bv);
/*
* Copy legacy XSAVE area and XSAVE hdr area.
*/
- memcpy(dest, xsave, XSTATE_AREA_MIN_SIZE);
+ memcpy(dest, xsave_area, 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 = xsave_area;
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 *xsave_area = 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(xsave_area, 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(xsave_area, 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;
+ xsave_area->xsave_hdr.xstate_bv = xstate_bv;
+ xsave_area->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, xsave_area->xsave_hdr.xcomp_bv);
/*
* Copy each region from the non-compacted offset to the
* possibly compacted offset.
*/
- dest = xsave;
+ dest = xsave_area;
valid = xstate_bv & ~XSTATE_FP_SSE;
while ( valid )
{
--
2.34.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to address MISRA C:2012 Rule 5.3
2023-08-04 15:27 [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3 Nicola Vetrini
` (4 preceding siblings ...)
2023-08-04 15:27 ` [XEN PATCH 5/6] x86/xstate: " Nicola Vetrini
@ 2023-08-04 15:27 ` Nicola Vetrini
2023-08-04 15:38 ` Nicola Vetrini
2023-08-07 8:31 ` Jan Beulich
5 siblings, 2 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:27 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Nicola Vetrini, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
The macros defined 'xen/include/public/arch-x86/xen-mca.h' have needless
underscore prefixes for parameter names and variable names that cause
shadowing with e.g. the variable 'i' in function 'mce_action'.
Therefore, the renaming aims to resolve present shadowing issues and
lessen the probability of future ones.
No functional change.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
The spirit of this patch is similar to this one [1] made by Jan that arose from
a violation of this rule.
[1] https://gitlab.com/xen-project/xen/-/commit/c0579c65f6bef794cd449fbc946feacccf485f2e
---
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..55b999ab21 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
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to " Nicola Vetrini
@ 2023-08-04 15:38 ` Nicola Vetrini
2023-08-04 21:30 ` Stefano Stabellini
2023-08-07 8:31 ` Jan Beulich
1 sibling, 1 reply; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-04 15:38 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu
> - _mic = x86_mcinfo_next(_mic); \
> + mic_ = x86_mcinfo_next(mic_); \
> } \
> - (_ret) = found ? _mic : NULL; \
> + (ret) = found_ ? mic_ : NULL; \
> } while (0)
>
>
> --
> 2.34.1
Stray blanks here, sorry.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 1/6] x86: rename variable 'e820' to " Nicola Vetrini
@ 2023-08-04 21:19 ` Stefano Stabellini
2023-08-07 7:14 ` Nicola Vetrini
2023-08-07 8:09 ` Jan Beulich
1 sibling, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-04 21:19 UTC (permalink / raw)
To: Nicola Vetrini
Cc: xen-devel, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> The variable declared in the header file 'xen/arch/x86/include/asm/e820.h'
> is shadowed by many function parameters, so it is renamed to avoid these
> violations.
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> This patch is similar to other renames done on previous patches, and the
> preferred strategy there was to rename the global variable. This one
> has more occurrences that are spread in various files, but
> the general pattern is the same.
> ---
> xen/arch/x86/dom0_build.c | 10 ++--
> xen/arch/x86/e820.c | 66 ++++++++++++------------
> xen/arch/x86/guest/xen/xen.c | 4 +-
> xen/arch/x86/hvm/dom0_build.c | 6 +--
> xen/arch/x86/include/asm/e820.h | 2 +-
> xen/arch/x86/mm.c | 49 +++++++++---------
> xen/arch/x86/numa.c | 8 +--
> xen/arch/x86/setup.c | 22 ++++----
> xen/arch/x86/srat.c | 6 +--
> xen/arch/x86/x86_64/mmconf-fam10h.c | 2 +-
> xen/drivers/passthrough/amd/iommu_acpi.c | 2 +-
There are missing changes to xen/arch/x86/tboot.c
There a few stray changes below.
Everything else is correct.
> 11 files changed, 89 insertions(+), 88 deletions(-)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 8b1fcc6471..bfb6400376 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -534,13 +534,13 @@ int __init dom0_setup_permissions(struct domain *d)
> }
>
> /* Remove access to E820_UNUSABLE I/O regions above 1MB. */
> - for ( i = 0; i < e820.nr_map; i++ )
> + for ( i = 0; i < e820_map.nr_map; i++ )
> {
> unsigned long sfn, efn;
> - sfn = max_t(unsigned long, paddr_to_pfn(e820.map[i].addr), 0x100ul);
> - efn = paddr_to_pfn(e820.map[i].addr + e820.map[i].size - 1);
> - if ( (e820.map[i].type == E820_UNUSABLE) &&
> - (e820.map[i].size != 0) &&
> + sfn = max_t(unsigned long, paddr_to_pfn(e820_map.map[i].addr), 0x100ul);
> + efn = paddr_to_pfn(e820_map.map[i].addr + e820_map.map[i].size - 1);
> + if ( (e820_map.map[i].type == E820_UNUSABLE) &&
> + (e820_map.map[i].size != 0) &&
> (sfn <= efn) )
> rc |= iomem_deny_access(d, sfn, efn);
> }
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 0b89935510..4425011c01 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -34,7 +34,7 @@ boolean_param("e820-mtrr-clip", e820_mtrr_clip);
> static bool __initdata e820_verbose;
> boolean_param("e820-verbose", e820_verbose);
>
> -struct e820map e820;
> +struct e820map e820_map;
> struct e820map __initdata e820_raw;
>
> /*
> @@ -47,8 +47,8 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
> {
> unsigned int i;
>
> - for (i = 0; i < e820.nr_map; i++) {
> - struct e820entry *ei = &e820.map[i];
> + for (i = 0; i < e820_map.nr_map; i++) {
> + struct e820entry *ei = &e820_map.map[i];
>
> if (type && ei->type != type)
> continue;
> @@ -75,17 +75,17 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
> static void __init add_memory_region(unsigned long long start,
> unsigned long long size, int type)
> {
> - unsigned int x = e820.nr_map;
> + unsigned int x = e820_map.nr_map;
>
> - if (x == ARRAY_SIZE(e820.map)) {
> + if (x == ARRAY_SIZE(e820_map.map)) {
> printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
> return;
> }
>
> - e820.map[x].addr = start;
> - e820.map[x].size = size;
> - e820.map[x].type = type;
> - e820.nr_map++;
> + e820_map.map[x].addr = start;
> + e820_map.map[x].size = size;
> + e820_map.map[x].type = type;
> + e820_map.nr_map++;
> }
>
> void __init print_e820_memory_map(const struct e820entry *map,
> @@ -347,13 +347,13 @@ static unsigned long __init find_max_pfn(void)
> unsigned int i;
> unsigned long max_pfn = 0;
>
> - for (i = 0; i < e820.nr_map; i++) {
> + for (i = 0; i < e820_map.nr_map; i++) {
> unsigned long start, end;
> /* RAM? */
> - if (e820.map[i].type != E820_RAM)
> + if (e820_map.map[i].type != E820_RAM)
> continue;
> - start = PFN_UP(e820.map[i].addr);
> - end = PFN_DOWN(e820.map[i].addr + e820.map[i].size);
> + start = PFN_UP(e820_map.map[i].addr);
> + end = PFN_DOWN(e820_map.map[i].addr + e820_map.map[i].size);
> if (start >= end)
> continue;
> if (end > max_pfn)
> @@ -372,21 +372,21 @@ static void __init clip_to_limit(uint64_t limit, const char *warnmsg)
> for ( ; ; )
> {
> /* Find a RAM region needing clipping. */
> - for ( i = 0; i < e820.nr_map; i++ )
> - if ( (e820.map[i].type == E820_RAM) &&
> - ((e820.map[i].addr + e820.map[i].size) > limit) )
> + for ( i = 0; i < e820_map.nr_map; i++ )
> + if ( (e820_map.map[i].type == E820_RAM) &&
> + ((e820_map.map[i].addr + e820_map.map[i].size) > limit) )
> break;
>
> /* If none found, we are done. */
> - if ( i == e820.nr_map )
> - break;
> + if ( i == e820_map.nr_map )
> + break;
>
> old_limit = max_t(
> - uint64_t, old_limit, e820.map[i].addr + e820.map[i].size);
> + uint64_t, old_limit, e820_map.map[i].addr + e820_map.map[i].size);
>
> /* We try to convert clipped RAM areas to E820_UNUSABLE. */
> - if ( e820_change_range_type(&e820, max(e820.map[i].addr, limit),
> - e820.map[i].addr + e820.map[i].size,
> + if ( e820_change_range_type(&e820_map, max(e820_map.map[i].addr, limit),
> + e820_map.map[i].addr + e820_map.map[i].size,
> E820_RAM, E820_UNUSABLE) )
> continue;
>
> @@ -394,15 +394,15 @@ static void __init clip_to_limit(uint64_t limit, const char *warnmsg)
> * If the type change fails (e.g., not space in table) then we clip or
> * delete the region as appropriate.
> */
> - if ( e820.map[i].addr < limit )
> + if ( e820_map.map[i].addr < limit )
> {
> - e820.map[i].size = limit - e820.map[i].addr;
> + e820_map.map[i].size = limit - e820_map.map[i].addr;
> }
> else
> {
> - memmove(&e820.map[i], &e820.map[i+1],
> - (e820.nr_map - i - 1) * sizeof(struct e820entry));
> - e820.nr_map--;
> + memmove(&e820_map.map[i], &e820_map.map[i+1],
> + (e820_map.nr_map - i - 1) * sizeof(struct e820entry));
> + e820_map.nr_map--;
> }
> }
>
> @@ -497,7 +497,7 @@ static void __init reserve_dmi_region(void)
> if ( !what )
> break;
> if ( ((base + len) > base) &&
> - reserve_e820_ram(&e820, base, base + len) )
> + reserve_e820_ram(&e820_map, base, base + len) )
> printk("WARNING: %s table located in E820 RAM %"PRIpaddr"-%"PRIpaddr". Fixed.\n",
> what, base, base + len);
> }
> @@ -517,12 +517,12 @@ static void __init machine_specific_memory_setup(struct e820map *raw)
>
> if ( opt_availmem )
> {
> - for ( i = size = 0; (i < e820.nr_map) && (size <= opt_availmem); i++ )
> - if ( e820.map[i].type == E820_RAM )
> - size += e820.map[i].size;
> + for ( i = size = 0; (i < e820_map.nr_map) && (size <= opt_availmem); i++ )
> + if ( e820_map.map[i].type == E820_RAM )
> + size += e820_map.map[i].size;
> if ( size > opt_availmem )
> clip_to_limit(
> - e820.map[i-1].addr + e820.map[i-1].size - (size-opt_availmem),
> + e820_map.map[i-1].addr + e820_map.map[i-1].size - (size-opt_availmem),
> NULL);
> }
>
> @@ -694,10 +694,10 @@ unsigned long __init init_e820(const char *str, struct e820map *raw)
> machine_specific_memory_setup(raw);
>
> if ( cpu_has_hypervisor )
> - hypervisor_e820_fixup(&e820);
> + hypervisor_e820_fixup(&e820_map);
>
> printk("%s RAM map:\n", str);
> - print_e820_memory_map(e820.map, e820.nr_map);
> + print_e820_memory_map(e820_map.map, e820_map.nr_map);
>
> return find_max_pfn();
> }
> diff --git a/xen/arch/x86/guest/xen/xen.c b/xen/arch/x86/guest/xen/xen.c
> index f93dfc89f7..3ec828b98d 100644
> --- a/xen/arch/x86/guest/xen/xen.c
> +++ b/xen/arch/x86/guest/xen/xen.c
> @@ -147,9 +147,9 @@ static void __init init_memmap(void)
> PFN_DOWN(GB(4) - 1))) )
> panic("unable to add RAM to in-use PFN rangeset\n");
>
> - for ( i = 0; i < e820.nr_map; i++ )
> + for ( i = 0; i < e820_map.nr_map; i++ )
> {
> - struct e820entry *e = &e820.map[i];
> + struct e820entry *e = &e820_map.map[i];
>
> if ( rangeset_add_range(mem, PFN_DOWN(e->addr),
> PFN_UP(e->addr + e->size - 1)) )
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index bc0e290db6..98203f7a52 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -333,13 +333,13 @@ static __init void pvh_setup_e820(struct domain *d, unsigned long nr_pages)
> * Add an extra entry in case we have to split a RAM entry into a RAM and a
> * UNUSABLE one in order to truncate it.
> */
> - d->arch.e820 = xzalloc_array(struct e820entry, e820.nr_map + 1);
> + d->arch.e820 = xzalloc_array(struct e820entry, e820_map.nr_map + 1);
> if ( !d->arch.e820 )
> panic("Unable to allocate memory for Dom0 e820 map\n");
> entry_guest = d->arch.e820;
>
> /* Clamp e820 memory map to match the memory assigned to Dom0 */
> - for ( i = 0, entry = e820.map; i < e820.nr_map; i++, entry++ )
> + for ( i = 0, entry = e820_map.map; i < e820_map.nr_map; i++, entry++ )
> {
> *entry_guest = *entry;
>
> @@ -392,7 +392,7 @@ static __init void pvh_setup_e820(struct domain *d, unsigned long nr_pages)
> next:
> d->arch.nr_e820++;
> entry_guest++;
> - ASSERT(d->arch.nr_e820 <= e820.nr_map + 1);
> + ASSERT(d->arch.nr_e820 <= e820_map.nr_map + 1);
> }
> ASSERT(cur_pages == nr_pages);
> }
> diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
> index 213d5b5dd2..0865825f7d 100644
> --- a/xen/arch/x86/include/asm/e820.h
> +++ b/xen/arch/x86/include/asm/e820.h
> @@ -34,7 +34,7 @@ extern int e820_add_range(
> extern unsigned long init_e820(const char *str, struct e820map *raw);
> extern void print_e820_memory_map(const struct e820entry *map,
> unsigned int entries);
> -extern struct e820map e820;
> +extern struct e820map e820_map;
> extern struct e820map e820_raw;
>
> /* These symbols live in the boot trampoline. */
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index be2b10a391..6920ac939f 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -295,12 +295,12 @@ void __init arch_init_memory(void)
> /* Any areas not specified as RAM by the e820 map are considered I/O. */
> for ( i = 0, pfn = 0; pfn < max_page; i++ )
> {
> - while ( (i < e820.nr_map) &&
> - (e820.map[i].type != E820_RAM) &&
> - (e820.map[i].type != E820_UNUSABLE) )
> + while ( (i < e820_map.nr_map) &&
> + (e820_map.map[i].type != E820_RAM) &&
> + (e820_map.map[i].type != E820_UNUSABLE) )
> i++;
>
> - if ( i >= e820.nr_map )
> + if ( i >= e820_map.nr_map )
> {
> /* No more RAM regions: mark as I/O right to end of memory map. */
> rstart_pfn = rend_pfn = max_page;
> @@ -309,9 +309,10 @@ void __init arch_init_memory(void)
> {
> /* Mark as I/O just up as far as next RAM region. */
> rstart_pfn = min_t(unsigned long, max_page,
> - PFN_UP(e820.map[i].addr));
> + PFN_UP(e820_map.map[i].addr));
> rend_pfn = max_t(unsigned long, rstart_pfn,
> - PFN_DOWN(e820.map[i].addr + e820.map[i].size));
> + PFN_DOWN(e820_map.map[i].addr
> + + e820_map.map[i].size));
> }
>
> /*
> @@ -387,9 +388,9 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
> uint64_t maddr = pfn_to_paddr(mfn);
> int i;
>
> - for ( i = 0; i < e820.nr_map; i++ )
> + for ( i = 0; i < e820_map.nr_map; i++ )
> {
> - switch ( e820.map[i].type )
> + switch ( e820_map.map[i].type )
> {
> case E820_RAM:
> if ( mem_type & RAM_TYPE_CONVENTIONAL )
> @@ -414,8 +415,8 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
> }
>
> /* Test the range. */
> - if ( (e820.map[i].addr <= maddr) &&
> - ((e820.map[i].addr + e820.map[i].size) >= (maddr + PAGE_SIZE)) )
> + if ( (e820_map.map[i].addr <= maddr) &&
> + ((e820_map.map[i].addr + e820_map.map[i].size) >= (maddr + PAGE_SIZE)) )
> return 1;
> }
>
> @@ -427,17 +428,17 @@ unsigned int page_get_ram_type(mfn_t mfn)
> uint64_t last = 0, maddr = mfn_to_maddr(mfn);
> unsigned int i, type = 0;
>
> - for ( i = 0; i < e820.nr_map;
> - last = e820.map[i].addr + e820.map[i].size, i++ )
> + for ( i = 0; i < e820_map.nr_map;
> + last = e820_map.map[i].addr + e820_map.map[i].size, i++ )
> {
> - if ( (maddr + PAGE_SIZE) > last && maddr < e820.map[i].addr )
> + if ( (maddr + PAGE_SIZE) > last && maddr < e820_map.map[i].addr )
> type |= RAM_TYPE_UNKNOWN;
>
> - if ( (maddr + PAGE_SIZE) <= e820.map[i].addr ||
> - maddr >= (e820.map[i].addr + e820.map[i].size) )
> + if ( (maddr + PAGE_SIZE) <= e820_map.map[i].addr ||
> + maddr >= (e820_map.map[i].addr + e820_map.map[i].size) )
> continue;
>
> - switch ( e820.map[i].type )
> + switch ( e820_map.map[i].type )
> {
> case E820_RAM:
> type |= RAM_TYPE_CONVENTIONAL;
> @@ -778,9 +779,9 @@ bool is_memory_hole(mfn_t start, mfn_t end)
> unsigned long e = mfn_x(end);
> unsigned int i;
>
> - for ( i = 0; i < e820.nr_map; i++ )
> + for ( i = 0; i < e820_map.nr_map; i++ )
> {
> - const struct e820entry *entry = &e820.map[i];
> + const struct e820entry *entry = &e820_map.map[i];
>
> if ( !entry->size )
> continue;
> @@ -4763,16 +4764,16 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>
> store = !guest_handle_is_null(ctxt.map.buffer);
>
> - if ( store && ctxt.map.nr_entries < e820.nr_map + 1 )
> + if ( store && ctxt.map.nr_entries < e820_map.nr_map + 1 )
> return -EINVAL;
>
> buffer = guest_handle_cast(ctxt.map.buffer, e820entry_t);
> if ( store && !guest_handle_okay(buffer, ctxt.map.nr_entries) )
> return -EFAULT;
>
> - for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < e820.nr_map; ++i, ++ctxt.n )
> + for ( i = 0, ctxt.n = 0, ctxt.s = 0; i < e820_map.nr_map; ++i, ++ctxt.n )
> {
> - unsigned long s = PFN_DOWN(e820.map[i].addr);
> + unsigned long s = PFN_DOWN(e820_map.map[i].addr);
>
> if ( s > ctxt.s )
> {
> @@ -4786,12 +4787,12 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> }
> if ( store )
> {
> - if ( ctxt.map.nr_entries <= ctxt.n + (e820.nr_map - i) )
> + if ( ctxt.map.nr_entries <= ctxt.n + (e820_map.nr_map - i) )
> return -EINVAL;
> - if ( __copy_to_guest_offset(buffer, ctxt.n, e820.map + i, 1) )
> + if ( __copy_to_guest_offset(buffer, ctxt.n, e820_map.map + i, 1) )
> return -EFAULT;
> }
> - ctxt.s = PFN_UP(e820.map[i].addr + e820.map[i].size);
> + ctxt.s = PFN_UP(e820_map.map[i].addr + e820_map.map[i].size);
> }
>
> if ( ctxt.s )
> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> index 4b0b297c7e..76827f5f32 100644
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -102,14 +102,14 @@ unsigned int __init arch_get_dma_bitsize(void)
>
> int __init arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)
> {
> - if ( idx >= e820.nr_map )
> + if ( idx >= e820_map.nr_map )
> return -ENOENT;
>
> - if ( e820.map[idx].type != E820_RAM )
> + if ( e820_map.map[idx].type != E820_RAM )
> return -ENODATA;
>
> - *start = e820.map[idx].addr;
> - *end = *start + e820.map[idx].size;
> + *start = e820_map.map[idx].addr;
> + *end = *start + e820_map.map[idx].size;
>
> return 0;
> }
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 80ae973d64..9c6003e374 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1163,7 +1163,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> }
> else if ( efi_enabled(EFI_BOOT) )
> memmap_type = "EFI";
> - else if ( (e820_raw.nr_map =
> + else if ( (e820_raw.nr_map =
stray change
> copy_bios_e820(e820_raw.map,
> ARRAY_SIZE(e820_raw.map))) != 0 )
> {
> @@ -1300,13 +1300,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> }
>
> /* Create a temporary copy of the E820 map. */
> - memcpy(&boot_e820, &e820, sizeof(e820));
> + memcpy(&boot_e820, &e820_map, sizeof(e820_map));
>
> /* Early kexec reservation (explicit static start address). */
> nr_pages = 0;
> - for ( i = 0; i < e820.nr_map; i++ )
> - if ( e820.map[i].type == E820_RAM )
> - nr_pages += e820.map[i].size >> PAGE_SHIFT;
> + for ( i = 0; i < e820_map.nr_map; i++ )
> + if ( e820_map.map[i].type == E820_RAM )
> + nr_pages += e820_map.map[i].size >> PAGE_SHIFT;
> set_kexec_crash_area_size((u64)nr_pages << PAGE_SHIFT);
> kexec_reserve_area(&boot_e820);
>
> @@ -1631,7 +1631,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
> PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
>
> - if ( e > s )
> + if ( e > s )
stray change
> map_pages_to_xen((unsigned long)__va(kexec_crash_area.start),
> _mfn(s), e - s, PAGE_HYPERVISOR);
> }
> @@ -1677,9 +1677,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> PAGE_HYPERVISOR_RO);
>
> nr_pages = 0;
> - for ( i = 0; i < e820.nr_map; i++ )
> - if ( e820.map[i].type == E820_RAM )
> - nr_pages += e820.map[i].size >> PAGE_SHIFT;
> + for ( i = 0; i < e820_map.nr_map; i++ )
> + if ( e820_map.map[i].type == E820_RAM )
> + nr_pages += e820_map.map[i].size >> PAGE_SHIFT;
> printk("System RAM: %luMB (%lukB)\n",
> nr_pages >> (20 - PAGE_SHIFT),
> nr_pages << (PAGE_SHIFT - 10));
> @@ -1771,7 +1771,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>
> open_softirq(NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ, new_tlbflush_clock_period);
>
> - if ( opt_watchdog )
> + if ( opt_watchdog )
> nmi_watchdog = NMI_LOCAL_APIC;
stray change
> find_smp_config();
> @@ -1983,7 +1983,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>
> do_initcalls();
>
> - if ( opt_watchdog )
> + if ( opt_watchdog )
> watchdog_setup();
stray change
> if ( !tboot_protect_mem_regions() )
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index 3f70338e6e..bbd04978ae 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -301,11 +301,11 @@ void __init srat_parse_regions(paddr_t addr)
> acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
> srat_parse_region, 0);
>
> - for (mask = srat_region_mask, i = 0; mask && i < e820.nr_map; i++) {
> - if (e820.map[i].type != E820_RAM)
> + for (mask = srat_region_mask, i = 0; mask && i < e820_map.nr_map; i++) {
> + if (e820_map.map[i].type != E820_RAM)
> continue;
>
> - if (~mask & pdx_region_mask(e820.map[i].addr, e820.map[i].size))
> + if (~mask & pdx_region_mask(e820_map.map[i].addr, e820_map.map[i].size))
> mask = 0;
> }
>
> diff --git a/xen/arch/x86/x86_64/mmconf-fam10h.c b/xen/arch/x86/x86_64/mmconf-fam10h.c
> index a834ab3149..bbebf9219f 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(&e820_map, 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..7ad9e12b8a 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(&e820_map, addr, addr + PAGE_SIZE,
> E820_RESERVED) )
> continue;
> AMD_IOMMU_ERROR("IVMD: page at %lx couldn't be reserved\n",
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 2/6] libelf: address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 2/6] libelf: " Nicola Vetrini
@ 2023-08-04 21:21 ` Stefano Stabellini
2023-08-07 8:11 ` Jan Beulich
1 sibling, 0 replies; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-04 21:21 UTC (permalink / raw)
To: Nicola Vetrini
Cc: xen-devel, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Wei Liu
On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> The types u{8,16,32,64} defined in 'xen/arch/x86/include/asm/types.h'
> shadow the variables in the modified function, hence violating Rule 5.3.
> Therefore, the rename takes care of the shadowing.
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> xen/common/libelf/libelf-tools.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
> index a9edb6a8dc..f0d5da1abf 100644
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -91,10 +91,10 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
> {
> elf_ptrval ptrval = base + moreoffset;
> bool need_swap = elf_swap(elf);
> - const uint8_t *u8;
> - const uint16_t *u16;
> - const uint32_t *u32;
> - const uint64_t *u64;
> + const uint8_t *uint8;
> + const uint16_t *uint16;
> + const uint32_t *uint32;
> + const uint64_t *uint64;
>
> if ( !elf_access_ok(elf, ptrval, size) )
> return 0;
> @@ -102,17 +102,17 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
> switch ( size )
> {
> case 1:
> - u8 = (const void*)ptrval;
> - return *u8;
> + uint8 = (const void*)ptrval;
> + return *uint8;
> case 2:
> - u16 = (const void*)ptrval;
> - return need_swap ? bswap_16(*u16) : *u16;
> + uint16 = (const void*)ptrval;
> + return need_swap ? bswap_16(*uint16) : *uint16;
> case 4:
> - u32 = (const void*)ptrval;
> - return need_swap ? bswap_32(*u32) : *u32;
> + uint32 = (const void*)ptrval;
> + return need_swap ? bswap_32(*uint32) : *uint32;
> case 8:
> - u64 = (const void*)ptrval;
> - return need_swap ? bswap_64(*u64) : *u64;
> + uint64 = (const void*)ptrval;
> + return need_swap ? bswap_64(*uint64) : *uint64;
> default:
> return 0;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-04 15:27 ` [XEN PATCH 3/6] xen/delay: " Nicola Vetrini
@ 2023-08-04 21:23 ` Stefano Stabellini
2023-08-07 8:14 ` Jan Beulich
1 sibling, 0 replies; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-04 21:23 UTC (permalink / raw)
To: Nicola Vetrini
Cc: xen-devel, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Wei Liu
On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> The variable 'msec' shadows the local variable in
> 'ehci_dbgp_bios_handoff', but to prevent any future clashes, the one in
> the macro gains a suffix.
>
> No functional change.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 4/6] x86/include: address MISRA C:2012 Rule 5.3.
2023-08-04 15:27 ` [XEN PATCH 4/6] x86/include: " Nicola Vetrini
@ 2023-08-04 21:24 ` Stefano Stabellini
2023-08-07 7:16 ` Nicola Vetrini
0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-04 21:24 UTC (permalink / raw)
To: Nicola Vetrini
Cc: xen-devel, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> s/mpc_default_type/mpc_default in 'xen/arch/x86/include/asm/mpspec.h'
> to avoid clashing with function parameter names in 'mpparse.c'.
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Even though the global variable does not seem to be used anywhere and is
> perhaps better to remove it entirely.
Please remove it
> ---
> xen/arch/x86/include/asm/mpspec.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/include/asm/mpspec.h b/xen/arch/x86/include/asm/mpspec.h
> index 1246eece0b..cc96ee63bd 100644
> --- a/xen/arch/x86/include/asm/mpspec.h
> +++ b/xen/arch/x86/include/asm/mpspec.h
> @@ -15,7 +15,7 @@ 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 int mpc_default;
> extern unsigned long mp_lapic_addr;
> extern bool pic_mode;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 5/6] x86/xstate: address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 5/6] x86/xstate: " Nicola Vetrini
@ 2023-08-04 21:26 ` Stefano Stabellini
2023-08-07 8:23 ` Jan Beulich
1 sibling, 0 replies; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-04 21:26 UTC (permalink / raw)
To: Nicola Vetrini
Cc: xen-devel, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> Rename the local variables to avoid clashing with function 'xstate'
> defined below, but declared in the corresponding header file.
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to address MISRA C:2012 Rule 5.3
2023-08-04 15:38 ` Nicola Vetrini
@ 2023-08-04 21:30 ` Stefano Stabellini
0 siblings, 0 replies; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-04 21:30 UTC (permalink / raw)
To: Nicola Vetrini
Cc: xen-devel, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
On Fri, 4 Aug 2023, Nicola Vetrini wrote:
> > - _mic = x86_mcinfo_next(_mic); \
> > + mic_ = x86_mcinfo_next(mic_); \
> > } \
> > - (_ret) = found ? _mic : NULL; \
> > + (ret) = found_ ? mic_ : NULL; \
> > } while (0)
> >
> >
> > --
> > 2.34.1
>
> Stray blanks here, sorry.
Aside from the stray blanks:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-04 21:19 ` Stefano Stabellini
@ 2023-08-07 7:14 ` Nicola Vetrini
0 siblings, 0 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 7:14 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu
On 04/08/2023 23:19, Stefano Stabellini wrote:
> On Fri, 4 Aug 2023, Nicola Vetrini wrote:
>> The variable declared in the header file
>> 'xen/arch/x86/include/asm/e820.h'
>> is shadowed by many function parameters, so it is renamed to avoid
>> these
>> violations.
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> This patch is similar to other renames done on previous patches, and
>> the
>> preferred strategy there was to rename the global variable. This one
>> has more occurrences that are spread in various files, but
>> the general pattern is the same.
>> ---
>> xen/arch/x86/dom0_build.c | 10 ++--
>> xen/arch/x86/e820.c | 66
>> ++++++++++++------------
>> xen/arch/x86/guest/xen/xen.c | 4 +-
>> xen/arch/x86/hvm/dom0_build.c | 6 +--
>> xen/arch/x86/include/asm/e820.h | 2 +-
>> xen/arch/x86/mm.c | 49 +++++++++---------
>> xen/arch/x86/numa.c | 8 +--
>> xen/arch/x86/setup.c | 22 ++++----
>> xen/arch/x86/srat.c | 6 +--
>> xen/arch/x86/x86_64/mmconf-fam10h.c | 2 +-
>> xen/drivers/passthrough/amd/iommu_acpi.c | 2 +-
>
> There are missing changes to xen/arch/x86/tboot.c
>
Thanks, I'll replace them.
> There a few stray changes below.
>
> Everything else is correct.
>
>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 80ae973d64..9c6003e374 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1163,7 +1163,7 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>> }
>> else if ( efi_enabled(EFI_BOOT) )
>> memmap_type = "EFI";
>> - else if ( (e820_raw.nr_map =
>> + else if ( (e820_raw.nr_map =
>
> stray change
>
>
>>
>> @@ -1631,7 +1631,7 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>> unsigned long e = min(s + PFN_UP(kexec_crash_area.size),
>> PFN_UP(__pa(HYPERVISOR_VIRT_END - 1)));
>>
>> - if ( e > s )
>> + if ( e > s )
>
> stray change
>
>
>> @@ -1771,7 +1771,7 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>>
>> open_softirq(NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ,
>> new_tlbflush_clock_period);
>>
>> - if ( opt_watchdog )
>> + if ( opt_watchdog )
>> nmi_watchdog = NMI_LOCAL_APIC;
>
> stray change
>
>
>> find_smp_config();
>> @@ -1983,7 +1983,7 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>>
>> do_initcalls();
>>
>> - if ( opt_watchdog )
>> + if ( opt_watchdog )
>> watchdog_setup();
>
> stray change
>
>
I looked at those, and there were trailing blanks on those lines, which
apparently
got trimmed when renaming the variable. I think these changes are ok
(though maybe they should be mentioned in the commit message).
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 4/6] x86/include: address MISRA C:2012 Rule 5.3.
2023-08-04 21:24 ` Stefano Stabellini
@ 2023-08-07 7:16 ` Nicola Vetrini
0 siblings, 0 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 7:16 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Jan Beulich, Andrew Cooper, Roger Pau Monné,
Wei Liu
On 04/08/2023 23:24, Stefano Stabellini wrote:
> On Fri, 4 Aug 2023, Nicola Vetrini wrote:
>> s/mpc_default_type/mpc_default in 'xen/arch/x86/include/asm/mpspec.h'
>> to avoid clashing with function parameter names in 'mpparse.c'.
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Even though the global variable does not seem to be used anywhere and
>> is
>> perhaps better to remove it entirely.
>
> Please remove it
>
Noted.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 1/6] x86: rename variable 'e820' to " Nicola Vetrini
2023-08-04 21:19 ` Stefano Stabellini
@ 2023-08-07 8:09 ` Jan Beulich
2023-08-07 8:59 ` Nicola Vetrini
` (2 more replies)
1 sibling, 3 replies; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 8:09 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 04.08.2023 17:27, Nicola Vetrini wrote:
> The variable declared in the header file 'xen/arch/x86/include/asm/e820.h'
> is shadowed by many function parameters, so it is renamed to avoid these
> violations.
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> This patch is similar to other renames done on previous patches, and the
> preferred strategy there was to rename the global variable. This one
> has more occurrences that are spread in various files, but
> the general pattern is the same.
Still I think it would be better done the other way around, and perhaps in
more than a single patch. It looks like "many == 3", i.e.
- e820_add_range(), which is only ever called with "e820" as its argument,
and hence the parameter could be dropped,
- e820_change_range_type(), which is in the same situation, and
- reserve_e820_ram(), which wants its parameter renamed.
Alternatively, if we really were to change the name of the global, we'd
want to take a more complete approach: Right now we have e820_raw[],
boot_e820[], and e820[]. We'd want them to follow a uniform naming scheme
then (e820_ first or _e820 last), with the other part of the name suitably
describing the purpose (which "map" doesn't do).
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 2/6] libelf: address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 2/6] libelf: " Nicola Vetrini
2023-08-04 21:21 ` Stefano Stabellini
@ 2023-08-07 8:11 ` Jan Beulich
2023-08-07 9:03 ` Nicola Vetrini
1 sibling, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 8:11 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, George Dunlap, Julien Grall, Wei Liu,
xen-devel
On 04.08.2023 17:27, Nicola Vetrini wrote:
> The types u{8,16,32,64} defined in 'xen/arch/x86/include/asm/types.h'
> shadow the variables in the modified function, hence violating Rule 5.3.
> Therefore, the rename takes care of the shadowing.
>
> No functional changes.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/common/libelf/libelf-tools.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/xen/common/libelf/libelf-tools.c b/xen/common/libelf/libelf-tools.c
> index a9edb6a8dc..f0d5da1abf 100644
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -91,10 +91,10 @@ uint64_t elf_access_unsigned(struct elf_binary * elf, elf_ptrval base,
> {
> elf_ptrval ptrval = base + moreoffset;
> bool need_swap = elf_swap(elf);
> - const uint8_t *u8;
> - const uint16_t *u16;
> - const uint32_t *u32;
> - const uint64_t *u64;
> + const uint8_t *uint8;
> + const uint16_t *uint16;
> + const uint32_t *uint32;
> + const uint64_t *uint64;
While the chosen names won't collide with stdint.h's, I still consider
them odd. These all being pointers, why not simply pu<N> as names?
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-04 15:27 ` [XEN PATCH 3/6] xen/delay: " Nicola Vetrini
2023-08-04 21:23 ` Stefano Stabellini
@ 2023-08-07 8:14 ` Jan Beulich
2023-08-07 9:01 ` Julien Grall
1 sibling, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 8:14 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, George Dunlap, Julien Grall, Wei Liu,
xen-devel
On 04.08.2023 17:27, Nicola Vetrini wrote:
> --- a/xen/include/xen/delay.h
> +++ b/xen/include/xen/delay.h
> @@ -5,6 +5,6 @@
>
> #include <asm/delay.h>
> #define mdelay(n) (\
> - {unsigned long msec=(n); while (msec--) udelay(1000);})
> + {unsigned long msec_=(n); while (msec_--) udelay(1000);})
As elsewhere, please also adjust style while touching the line, at
least as far as the obviously wrong case goes:
#define mdelay(n) (\
{unsigned long msec_ = (n); while (msec_--) udelay(1000);})
Even better would be
#define mdelay(n) ({ \
unsigned long msec_ = (n); while (msec_--) udelay(1000); \
})
or some such. I can take care of this while committing.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 5/6] x86/xstate: address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 5/6] x86/xstate: " Nicola Vetrini
2023-08-04 21:26 ` Stefano Stabellini
@ 2023-08-07 8:23 ` Jan Beulich
2023-08-07 9:20 ` Nicola Vetrini
1 sibling, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 8:23 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 04.08.2023 17:27, Nicola Vetrini wrote:
> Rename the local variables to avoid clashing with function 'xstate'
> defined below, but declared in the corresponding header file.
Hmm, there are two functions with such a local variable, but you don't
change those. You change "xsave" instead. The new name you use you took
from older functions afaict; newer ones use "xstate" (and use of this
name is extended in pending patches), so preferably we would follow
that naming model (and eventually rename all "xsave_area" as well).
Also - does "below" really matter and hence warrant the "but"?
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to address MISRA C:2012 Rule 5.3
2023-08-04 15:27 ` [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to " Nicola Vetrini
2023-08-04 15:38 ` Nicola Vetrini
@ 2023-08-07 8:31 ` Jan Beulich
2023-08-07 10:01 ` Nicola Vetrini
1 sibling, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 8:31 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 04.08.2023 17:27, Nicola Vetrini wrote:
> The macros defined 'xen/include/public/arch-x86/xen-mca.h' have needless
> underscore prefixes for parameter names and variable names that cause
> shadowing with e.g. the variable 'i' in function 'mce_action'.
> Therefore, the renaming aims to resolve present shadowing issues and
> lessen the probability of future ones.
>
> No functional change.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
I'm okay with the code adjustments here, but I'm afraid I don't follow
the description: How is shadowing of "i" connected to the use of
leading underscores in macro parameter names? I think you need to
separate the two aspects in the wording.
> --- 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
Isn't there another rule demanding parenthization of the whole
construct? If so, adding the then-missing parentheses right here would
be quite desirable. (Personally I'm happy about them not being there on
suffix expressions, as kind of an exception to the general rule.)
> /* 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; \
The lack of parentheses here definitely wants dealing with right away.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 8:09 ` Jan Beulich
@ 2023-08-07 8:59 ` Nicola Vetrini
2023-08-07 9:10 ` Jan Beulich
2023-08-07 15:03 ` Nicola Vetrini
2023-08-07 18:39 ` Stefano Stabellini
2 siblings, 1 reply; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 8:59 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07/08/2023 10:09, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
>> The variable declared in the header file
>> 'xen/arch/x86/include/asm/e820.h'
>> is shadowed by many function parameters, so it is renamed to avoid
>> these
>> violations.
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> This patch is similar to other renames done on previous patches, and
>> the
>> preferred strategy there was to rename the global variable. This one
>> has more occurrences that are spread in various files, but
>> the general pattern is the same.
>
> Still I think it would be better done the other way around, and perhaps
> in
> more than a single patch. It looks like "many == 3", i.e.
> - e820_add_range(), which is only ever called with "e820" as its
> argument,
> and hence the parameter could be dropped,
> - e820_change_range_type(), which is in the same situation, and
> - reserve_e820_ram(), which wants its parameter renamed.
> Alternatively, if we really were to change the name of the global, we'd
> want to take a more complete approach: Right now we have e820_raw[],
> boot_e820[], and e820[]. We'd want them to follow a uniform naming
> scheme
> then (e820_ first or _e820 last), with the other part of the name
> suitably
> describing the purpose (which "map" doesn't do).
>
> Jan
Besides the one you listed, there are these other occurrences:
- xen/arch/x86/mm.c:4678 in 'arch_memory_op' as local variable 'struct
e820entry'
- xen/arch/x86/include/asm/guest/hypervisor.h:55 in
'hypervisor_e820_fixup'
- xen/arch/x86/include/asm/pv/shim.h:88 in 'pv_shim_fixup'
- xen/arch/x86/setup.c:689 in 'kexec_reserve_area'
We can take the first approach you suggested (which was my original
attempt, but then upon feedback on other
patches I reworked this patch before submitting). My doubt about it was
that it would introduce a naming
inconsistency with other e820-related objects/types. Anyway, if e820_map
is not a good name, could e820_arr be it?
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-07 8:14 ` Jan Beulich
@ 2023-08-07 9:01 ` Julien Grall
2023-08-07 9:15 ` Jan Beulich
0 siblings, 1 reply; 40+ messages in thread
From: Julien Grall @ 2023-08-07 9:01 UTC (permalink / raw)
To: Jan Beulich, Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, George Dunlap, Wei Liu, xen-devel
Hi Jan,
On 07/08/2023 09:14, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
>> --- a/xen/include/xen/delay.h
>> +++ b/xen/include/xen/delay.h
>> @@ -5,6 +5,6 @@
>>
>> #include <asm/delay.h>
>> #define mdelay(n) (\
>> - {unsigned long msec=(n); while (msec--) udelay(1000);})
>> + {unsigned long msec_=(n); while (msec_--) udelay(1000);})
>
> As elsewhere, please also adjust style while touching the line, at
> least as far as the obviously wrong case goes:
>
> #define mdelay(n) (\
> {unsigned long msec_ = (n); while (msec_--) udelay(1000);})
>
> Even better would be
>
> #define mdelay(n) ({ \
> unsigned long msec_ = (n); while (msec_--) udelay(1000); \
> })
If you are touching the style, about converting to a staging inline and
also splitting the line in multiple one?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 2/6] libelf: address MISRA C:2012 Rule 5.3
2023-08-07 8:11 ` Jan Beulich
@ 2023-08-07 9:03 ` Nicola Vetrini
0 siblings, 0 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 9:03 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, George Dunlap, Julien Grall, Wei Liu,
xen-devel
On 07/08/2023 10:11, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
>> The types u{8,16,32,64} defined in 'xen/arch/x86/include/asm/types.h'
>> shadow the variables in the modified function, hence violating Rule
>> 5.3.
>> Therefore, the rename takes care of the shadowing.
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> xen/common/libelf/libelf-tools.c | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/common/libelf/libelf-tools.c
>> b/xen/common/libelf/libelf-tools.c
>> index a9edb6a8dc..f0d5da1abf 100644
>> --- a/xen/common/libelf/libelf-tools.c
>> +++ b/xen/common/libelf/libelf-tools.c
>> @@ -91,10 +91,10 @@ uint64_t elf_access_unsigned(struct elf_binary *
>> elf, elf_ptrval base,
>> {
>> elf_ptrval ptrval = base + moreoffset;
>> bool need_swap = elf_swap(elf);
>> - const uint8_t *u8;
>> - const uint16_t *u16;
>> - const uint32_t *u32;
>> - const uint64_t *u64;
>> + const uint8_t *uint8;
>> + const uint16_t *uint16;
>> + const uint32_t *uint32;
>> + const uint64_t *uint64;
>
> While the chosen names won't collide with stdint.h's, I still consider
> them odd. These all being pointers, why not simply pu<N> as names?
>
> Jan
lgtm.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 8:59 ` Nicola Vetrini
@ 2023-08-07 9:10 ` Jan Beulich
2023-08-07 11:12 ` Nicola Vetrini
2023-08-08 7:08 ` Nicola Vetrini
0 siblings, 2 replies; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 9:10 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07.08.2023 10:59, Nicola Vetrini wrote:
> On 07/08/2023 10:09, Jan Beulich wrote:
>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>> The variable declared in the header file
>>> 'xen/arch/x86/include/asm/e820.h'
>>> is shadowed by many function parameters, so it is renamed to avoid
>>> these
>>> violations.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>> This patch is similar to other renames done on previous patches, and
>>> the
>>> preferred strategy there was to rename the global variable. This one
>>> has more occurrences that are spread in various files, but
>>> the general pattern is the same.
>>
>> Still I think it would be better done the other way around, and perhaps
>> in
>> more than a single patch. It looks like "many == 3", i.e.
>> - e820_add_range(), which is only ever called with "e820" as its
>> argument,
>> and hence the parameter could be dropped,
>> - e820_change_range_type(), which is in the same situation, and
>> - reserve_e820_ram(), which wants its parameter renamed.
>> Alternatively, if we really were to change the name of the global, we'd
>> want to take a more complete approach: Right now we have e820_raw[],
>> boot_e820[], and e820[]. We'd want them to follow a uniform naming
>> scheme
>> then (e820_ first or _e820 last), with the other part of the name
>> suitably
>> describing the purpose (which "map" doesn't do).
>
> Besides the one you listed, there are these other occurrences:
> - xen/arch/x86/mm.c:4678 in 'arch_memory_op' as local variable 'struct
> e820entry'
This probably wants renaming; my suggestion would be just "e" here.
> - xen/arch/x86/include/asm/guest/hypervisor.h:55 in
> 'hypervisor_e820_fixup'
> - xen/arch/x86/include/asm/pv/shim.h:88 in 'pv_shim_fixup'
These can likely again have their parameters dropped, for it only
ever being the "e820" global which is passed. (Really I think in such
cases the names being the same should be permitted.)
> - xen/arch/x86/setup.c:689 in 'kexec_reserve_area'
This surely can quite sensibly have boot_e820 use moved into the
function itself.
> We can take the first approach you suggested (which was my original
> attempt, but then upon feedback on other
> patches I reworked this patch before submitting). My doubt about it was
> that it would introduce a naming
> inconsistency with other e820-related objects/types. Anyway, if e820_map
> is not a good name, could e820_arr be it?
But how does "arr" describe the purpose? I would have suggested a name,
but none I can think of (e820_real, e820_final) I'd be really happy with.
Just e820 is pretty likely the best name we can have here.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-07 9:01 ` Julien Grall
@ 2023-08-07 9:15 ` Jan Beulich
2023-08-07 9:23 ` Nicola Vetrini
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 9:15 UTC (permalink / raw)
To: Julien Grall, Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, George Dunlap, Wei Liu, xen-devel
On 07.08.2023 11:01, Julien Grall wrote:
> On 07/08/2023 09:14, Jan Beulich wrote:
>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/delay.h
>>> +++ b/xen/include/xen/delay.h
>>> @@ -5,6 +5,6 @@
>>>
>>> #include <asm/delay.h>
>>> #define mdelay(n) (\
>>> - {unsigned long msec=(n); while (msec--) udelay(1000);})
>>> + {unsigned long msec_=(n); while (msec_--) udelay(1000);})
>>
>> As elsewhere, please also adjust style while touching the line, at
>> least as far as the obviously wrong case goes:
>>
>> #define mdelay(n) (\
>> {unsigned long msec_ = (n); while (msec_--) udelay(1000);})
>>
>> Even better would be
>>
>> #define mdelay(n) ({ \
>> unsigned long msec_ = (n); while (msec_--) udelay(1000); \
>> })
>
> If you are touching the style, about converting to a staging inline and
> also splitting the line in multiple one?
I'd be happy about this being done, but I wouldn't want to go as far with
on-commit adjustments. Nicola, are you up to doing so in v2?
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 5/6] x86/xstate: address MISRA C:2012 Rule 5.3
2023-08-07 8:23 ` Jan Beulich
@ 2023-08-07 9:20 ` Nicola Vetrini
0 siblings, 0 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 9:20 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07/08/2023 10:23, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
>> Rename the local variables to avoid clashing with function 'xstate'
>> defined below, but declared in the corresponding header file.
>
> Hmm, there are two functions with such a local variable, but you don't
> change those. You change "xsave" instead. The new name you use you took
> from older functions afaict; newer ones use "xstate" (and use of this
> name is extended in pending patches), so preferably we would follow
> that naming model (and eventually rename all "xsave_area" as well).
>
> Also - does "below" really matter and hence warrant the "but"?
>
> Jan
I made a typo in the commit message. Indeed 'xsave' here is the culprit.
I think 'xstate'
is ok for a rename, as it does not shadow anything afaict.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-07 9:15 ` Jan Beulich
@ 2023-08-07 9:23 ` Nicola Vetrini
2023-08-07 9:32 ` Jan Beulich
0 siblings, 1 reply; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 9:23 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Andrew Cooper, George Dunlap,
Wei Liu, xen-devel
On 07/08/2023 11:15, Jan Beulich wrote:
> On 07.08.2023 11:01, Julien Grall wrote:
>> On 07/08/2023 09:14, Jan Beulich wrote:
>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>>> --- a/xen/include/xen/delay.h
>>>> +++ b/xen/include/xen/delay.h
>>>> @@ -5,6 +5,6 @@
>>>>
>>>> #include <asm/delay.h>
>>>> #define mdelay(n) (\
>>>> - {unsigned long msec=(n); while (msec--) udelay(1000);})
>>>> + {unsigned long msec_=(n); while (msec_--) udelay(1000);})
>>>
>>> As elsewhere, please also adjust style while touching the line, at
>>> least as far as the obviously wrong case goes:
>>>
>>> #define mdelay(n) (\
>>> {unsigned long msec_ = (n); while (msec_--) udelay(1000);})
>>>
>>> Even better would be
>>>
>>> #define mdelay(n) ({ \
>>> unsigned long msec_ = (n); while (msec_--) udelay(1000); \
>>> })
>>
>> If you are touching the style, about converting to a staging inline
>> and
>> also splitting the line in multiple one?
>
> I'd be happy about this being done, but I wouldn't want to go as far
> with
> on-commit adjustments. Nicola, are you up to doing so in v2?
>
> Jan
I'm afraid I don't understand what "staging inline" refers to. Other
than that, sure thing.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-07 9:23 ` Nicola Vetrini
@ 2023-08-07 9:32 ` Jan Beulich
2023-08-07 9:33 ` Julien Grall
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 9:32 UTC (permalink / raw)
To: Nicola Vetrini
Cc: Julien Grall, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Andrew Cooper, George Dunlap,
Wei Liu, xen-devel
On 07.08.2023 11:23, Nicola Vetrini wrote:
> On 07/08/2023 11:15, Jan Beulich wrote:
>> On 07.08.2023 11:01, Julien Grall wrote:
>>> On 07/08/2023 09:14, Jan Beulich wrote:
>>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>>>> --- a/xen/include/xen/delay.h
>>>>> +++ b/xen/include/xen/delay.h
>>>>> @@ -5,6 +5,6 @@
>>>>>
>>>>> #include <asm/delay.h>
>>>>> #define mdelay(n) (\
>>>>> - {unsigned long msec=(n); while (msec--) udelay(1000);})
>>>>> + {unsigned long msec_=(n); while (msec_--) udelay(1000);})
>>>>
>>>> As elsewhere, please also adjust style while touching the line, at
>>>> least as far as the obviously wrong case goes:
>>>>
>>>> #define mdelay(n) (\
>>>> {unsigned long msec_ = (n); while (msec_--) udelay(1000);})
>>>>
>>>> Even better would be
>>>>
>>>> #define mdelay(n) ({ \
>>>> unsigned long msec_ = (n); while (msec_--) udelay(1000); \
>>>> })
>>>
>>> If you are touching the style, about converting to a staging inline
>>> and
>>> also splitting the line in multiple one?
>>
>> I'd be happy about this being done, but I wouldn't want to go as far
>> with
>> on-commit adjustments. Nicola, are you up to doing so in v2?
>
> I'm afraid I don't understand what "staging inline" refers to. Other
> than that, sure thing.
Surely Julien meant static inline.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 3/6] xen/delay: address MISRA C:2012 Rule 5.3.
2023-08-07 9:32 ` Jan Beulich
@ 2023-08-07 9:33 ` Julien Grall
0 siblings, 0 replies; 40+ messages in thread
From: Julien Grall @ 2023-08-07 9:33 UTC (permalink / raw)
To: Jan Beulich, Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, George Dunlap, Wei Liu, xen-devel
On 07/08/2023 10:32, Jan Beulich wrote:
> On 07.08.2023 11:23, Nicola Vetrini wrote:
>> On 07/08/2023 11:15, Jan Beulich wrote:
>>> On 07.08.2023 11:01, Julien Grall wrote:
>>>> On 07/08/2023 09:14, Jan Beulich wrote:
>>>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>>>>> --- a/xen/include/xen/delay.h
>>>>>> +++ b/xen/include/xen/delay.h
>>>>>> @@ -5,6 +5,6 @@
>>>>>>
>>>>>> #include <asm/delay.h>
>>>>>> #define mdelay(n) (\
>>>>>> - {unsigned long msec=(n); while (msec--) udelay(1000);})
>>>>>> + {unsigned long msec_=(n); while (msec_--) udelay(1000);})
>>>>>
>>>>> As elsewhere, please also adjust style while touching the line, at
>>>>> least as far as the obviously wrong case goes:
>>>>>
>>>>> #define mdelay(n) (\
>>>>> {unsigned long msec_ = (n); while (msec_--) udelay(1000);})
>>>>>
>>>>> Even better would be
>>>>>
>>>>> #define mdelay(n) ({ \
>>>>> unsigned long msec_ = (n); while (msec_--) udelay(1000); \
>>>>> })
>>>>
>>>> If you are touching the style, about converting to a staging inline
>>>> and
>>>> also splitting the line in multiple one?
>>>
>>> I'd be happy about this being done, but I wouldn't want to go as far
>>> with
>>> on-commit adjustments. Nicola, are you up to doing so in v2?
>>
>> I'm afraid I don't understand what "staging inline" refers to. Other
>> than that, sure thing.
>
> Surely Julien meant static inline.
Yes. That was a typo. Sorry for that.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to address MISRA C:2012 Rule 5.3
2023-08-07 8:31 ` Jan Beulich
@ 2023-08-07 10:01 ` Nicola Vetrini
0 siblings, 0 replies; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 10:01 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07/08/2023 10:31, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
>> The macros defined 'xen/include/public/arch-x86/xen-mca.h' have
>> needless
>> underscore prefixes for parameter names and variable names that cause
>> shadowing with e.g. the variable 'i' in function 'mce_action'.
>> Therefore, the renaming aims to resolve present shadowing issues and
>> lessen the probability of future ones.
>>
>> No functional change.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>
> I'm okay with the code adjustments here, but I'm afraid I don't follow
> the description: How is shadowing of "i" connected to the use of
> leading underscores in macro parameter names? I think you need to
> separate the two aspects in the wording.
>
The shadowing remark is tied to the second part of the sentence, but
I'll try
to make that clearer in v2.
>> --- 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
>
> Isn't there another rule demanding parenthization of the whole
> construct? If so, adding the then-missing parentheses right here would
> be quite desirable. (Personally I'm happy about them not being there on
> suffix expressions, as kind of an exception to the general rule.)
>
If you're referring to Rule 20.7 then it does not require the whole
expression to be enclosed in
parentheses, as it's concerned with macro parameters (their full
expansion must parenthesize arguments at
some point, to prevent wrong evaluations due to operator precedence).
>> - 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; \
>
> The lack of parentheses here definitely wants dealing with right away.
>
> Jan
Good catch
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 9:10 ` Jan Beulich
@ 2023-08-07 11:12 ` Nicola Vetrini
2023-08-07 12:02 ` Jan Beulich
2023-08-08 7:08 ` Nicola Vetrini
1 sibling, 1 reply; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 11:12 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
>> Besides the one you listed, there are these other occurrences:
>> - xen/arch/x86/mm.c:4678 in 'arch_memory_op' as local variable 'struct
>> e820entry'
>
> This probably wants renaming; my suggestion would be just "e" here.
Ok
>
>> - xen/arch/x86/include/asm/guest/hypervisor.h:55 in
>> 'hypervisor_e820_fixup'
>> - xen/arch/x86/include/asm/pv/shim.h:88 in 'pv_shim_fixup'
>
> These can likely again have their parameters dropped, for it only
> ever being the "e820" global which is passed. (Really I think in such
> cases the names being the same should be permitted.)
>
>> - xen/arch/x86/setup.c:689 in 'kexec_reserve_area'
>
> This surely can quite sensibly have boot_e820 use moved into the
> function itself.
>
Ok, although your suggestion of breaking these renames/deletions in more
than one patch may not be applicable,
as 'kexec_reserve_area' calls 'reserve_e820_ram', which in turn calls
'e820_change_range_type'.
Similarly, the call stack containing 'e820_add_range' includes other
calls to the modified functions, so
effectively it's best to drop the parameter everywhere all at once to
prevent accidental mistakes.
>> We can take the first approach you suggested (which was my original
>> attempt, but then upon feedback on other
>> patches I reworked this patch before submitting). My doubt about it
>> was
>> that it would introduce a naming
>> inconsistency with other e820-related objects/types. Anyway, if
>> e820_map
>> is not a good name, could e820_arr be it?
>
> But how does "arr" describe the purpose? I would have suggested a name,
> but none I can think of (e820_real, e820_final) I'd be really happy
> with.
> Just e820 is pretty likely the best name we can have here.
>
Ok, so perhaps the best way is using the strategy above, although I'm
curious why in other places this
was not the preferred alternative (as the global may be dropped or the
callers may use a e820map other
than the global one, but here I recognize my lack of knowledge on the
internals of Xen).
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 11:12 ` Nicola Vetrini
@ 2023-08-07 12:02 ` Jan Beulich
0 siblings, 0 replies; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 12:02 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07.08.2023 13:12, Nicola Vetrini wrote:
>
>>> Besides the one you listed, there are these other occurrences:
>>> - xen/arch/x86/mm.c:4678 in 'arch_memory_op' as local variable 'struct
>>> e820entry'
>>
>> This probably wants renaming; my suggestion would be just "e" here.
>
> Ok
>
>>
>>> - xen/arch/x86/include/asm/guest/hypervisor.h:55 in
>>> 'hypervisor_e820_fixup'
>>> - xen/arch/x86/include/asm/pv/shim.h:88 in 'pv_shim_fixup'
>>
>> These can likely again have their parameters dropped, for it only
>> ever being the "e820" global which is passed. (Really I think in such
>> cases the names being the same should be permitted.)
>>
>>> - xen/arch/x86/setup.c:689 in 'kexec_reserve_area'
>>
>> This surely can quite sensibly have boot_e820 use moved into the
>> function itself.
>>
>
> Ok, although your suggestion of breaking these renames/deletions in more
> than one patch may not be applicable,
> as 'kexec_reserve_area' calls 'reserve_e820_ram', which in turn calls
> 'e820_change_range_type'.
> Similarly, the call stack containing 'e820_add_range' includes other
> calls to the modified functions, so
> effectively it's best to drop the parameter everywhere all at once to
> prevent accidental mistakes.
Well, this still allows splitting parameter removal changes from
parameter renaming ones.
>>> We can take the first approach you suggested (which was my original
>>> attempt, but then upon feedback on other
>>> patches I reworked this patch before submitting). My doubt about it
>>> was
>>> that it would introduce a naming
>>> inconsistency with other e820-related objects/types. Anyway, if
>>> e820_map
>>> is not a good name, could e820_arr be it?
>>
>> But how does "arr" describe the purpose? I would have suggested a name,
>> but none I can think of (e820_real, e820_final) I'd be really happy
>> with.
>> Just e820 is pretty likely the best name we can have here.
>
> Ok, so perhaps the best way is using the strategy above, although I'm
> curious why in other places this
> was not the preferred alternative (as the global may be dropped or the
> callers may use a e820map other
> than the global one, but here I recognize my lack of knowledge on the
> internals of Xen).
Other x86 maintainers may voice a different opinion. My take is that
since we've now lived with the set of functions we have for quite a
long time, problematic changes like ones you outline are not very
likely to appear.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 8:09 ` Jan Beulich
2023-08-07 8:59 ` Nicola Vetrini
@ 2023-08-07 15:03 ` Nicola Vetrini
2023-08-07 15:07 ` Jan Beulich
2023-08-07 18:39 ` Stefano Stabellini
2 siblings, 1 reply; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-07 15:03 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07/08/2023 10:09, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
>> The variable declared in the header file
>> 'xen/arch/x86/include/asm/e820.h'
>> is shadowed by many function parameters, so it is renamed to avoid
>> these
>> violations.
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> This patch is similar to other renames done on previous patches, and
>> the
>> preferred strategy there was to rename the global variable. This one
>> has more occurrences that are spread in various files, but
>> the general pattern is the same.
>
> Still I think it would be better done the other way around, and perhaps
> in
> more than a single patch. It looks like "many == 3", i.e.
> - e820_add_range(), which is only ever called with "e820" as its
> argument,
> and hence the parameter could be dropped,
> - e820_change_range_type(), which is in the same situation, and
> - reserve_e820_ram(), which wants its parameter renamed.
This one is defined as
return e820_change_range_type(e820, s, e, E820_RAM, E820_RESERVED);
so I'm not certain that the parameter can be dropped from that function,
because the cascade effect
would eliminate the need to have a 'boot_e820' in 'xen/arch/x86/setup.c'
afaict and since the comment says
/* A temporary copy of the e820 map that we can mess with during
bootstrap. */
static struct e820map __initdata boot_e820;
I'm not sure it's a good idea to alter this call chain.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 15:03 ` Nicola Vetrini
@ 2023-08-07 15:07 ` Jan Beulich
0 siblings, 0 replies; 40+ messages in thread
From: Jan Beulich @ 2023-08-07 15:07 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07.08.2023 17:03, Nicola Vetrini wrote:
> On 07/08/2023 10:09, Jan Beulich wrote:
>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>> The variable declared in the header file
>>> 'xen/arch/x86/include/asm/e820.h'
>>> is shadowed by many function parameters, so it is renamed to avoid
>>> these
>>> violations.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>> This patch is similar to other renames done on previous patches, and
>>> the
>>> preferred strategy there was to rename the global variable. This one
>>> has more occurrences that are spread in various files, but
>>> the general pattern is the same.
>>
>> Still I think it would be better done the other way around, and perhaps
>> in
>> more than a single patch. It looks like "many == 3", i.e.
>> - e820_add_range(), which is only ever called with "e820" as its
>> argument,
>> and hence the parameter could be dropped,
>> - e820_change_range_type(), which is in the same situation, and
>> - reserve_e820_ram(), which wants its parameter renamed.
>
> This one is defined as
>
> return e820_change_range_type(e820, s, e, E820_RAM, E820_RESERVED);
>
> so I'm not certain that the parameter can be dropped from that function,
You're right, it can't. I didn't pay enough attention on the interaction
of both. So renaming it is here then as well.
Jan
> because the cascade effect
> would eliminate the need to have a 'boot_e820' in 'xen/arch/x86/setup.c'
> afaict and since the comment says
>
> /* A temporary copy of the e820 map that we can mess with during
> bootstrap. */
> static struct e820map __initdata boot_e820;
>
> I'm not sure it's a good idea to alter this call chain.
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 8:09 ` Jan Beulich
2023-08-07 8:59 ` Nicola Vetrini
2023-08-07 15:03 ` Nicola Vetrini
@ 2023-08-07 18:39 ` Stefano Stabellini
2 siblings, 0 replies; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-07 18:39 UTC (permalink / raw)
To: Jan Beulich
Cc: Nicola Vetrini, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Andrew Cooper,
Roger Pau Monné,
Wei Liu, xen-devel
On Mon, 7 Aug 2023, Jan Beulich wrote:
> On 04.08.2023 17:27, Nicola Vetrini wrote:
> > The variable declared in the header file 'xen/arch/x86/include/asm/e820.h'
> > is shadowed by many function parameters, so it is renamed to avoid these
> > violations.
> >
> > No functional changes.
> >
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > ---
> > This patch is similar to other renames done on previous patches, and the
> > preferred strategy there was to rename the global variable. This one
> > has more occurrences that are spread in various files, but
> > the general pattern is the same.
>
> Still I think it would be better done the other way around, and perhaps in
> more than a single patch. It looks like "many == 3", i.e.
> - e820_add_range(), which is only ever called with "e820" as its argument,
> and hence the parameter could be dropped,
> - e820_change_range_type(), which is in the same situation, and
> - reserve_e820_ram(), which wants its parameter renamed.
Let me premise that this is x86 code and I am happy with whatever you
prefer.
I would like to point out that renaming the global var is a lot safer as
a change than renaming the local var. That is because renaming the
global, if you forget one invocation it won't compile (now of course it
can still happen for an optional feature like tboot, but in general
you'll catch everything with a compilation). If you rename the local and
you missed a rename, it will change the behavior of the code as it will
fall back to the global and compile without issues.
So I think it would be best to rename the global when possible.
> Alternatively, if we really were to change the name of the global, we'd
> want to take a more complete approach: Right now we have e820_raw[],
> boot_e820[], and e820[]. We'd want them to follow a uniform naming scheme
> then (e820_ first or _e820 last), with the other part of the name suitably
> describing the purpose (which "map" doesn't do).
I would go with this option
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-07 9:10 ` Jan Beulich
2023-08-07 11:12 ` Nicola Vetrini
@ 2023-08-08 7:08 ` Nicola Vetrini
2023-08-08 7:21 ` Jan Beulich
1 sibling, 1 reply; 40+ messages in thread
From: Nicola Vetrini @ 2023-08-08 7:08 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 07/08/2023 11:10, Jan Beulich wrote:
> On 07.08.2023 10:59, Nicola Vetrini wrote:
>> On 07/08/2023 10:09, Jan Beulich wrote:
>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>>> The variable declared in the header file
>>>> 'xen/arch/x86/include/asm/e820.h'
>>>> is shadowed by many function parameters, so it is renamed to avoid
>>>> these
>>>> violations.
>>>>
>>>> No functional changes.
>>>>
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> ---
>>>> This patch is similar to other renames done on previous patches, and
>>>> the
>>>> preferred strategy there was to rename the global variable. This one
>>>> has more occurrences that are spread in various files, but
>>>> the general pattern is the same.
>>>
>>> Still I think it would be better done the other way around, and
>>> perhaps
>>> in
>>> more than a single patch. It looks like "many == 3", i.e.
>>> - e820_add_range(), which is only ever called with "e820" as its
>>> argument,
>>> and hence the parameter could be dropped,
I see another downside with this approach (I should have spotted this
sooner):
Since e820_add_range and the other functions expected e820 as a pointer,
they are
written like this:
for ( i = 0; i < e820->nr_map; ++i )
{
uint64_t rs = e820->map[i].addr;
uint64_t re = rs + e820->map[i].size;
if ( rs == e && e820->map[i].type == type )
{
e820->map[i].addr = s;
return 1;
}
...
Dropping the parameter would either mean
1. Use a local parameter that stores the address of e820, which kind of
nullifies the purpose of dropping the parameter imho;
2. Rewrite it so that it operates on a "struct e820map", which would
mean
substantial churn;
3. Make the global a pointer, which is reminiscent of (1)
All in all, I do like the global renaming approach more, because it
lessens
the amount of code that needs to change to accomodate a case of
shadowing.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-08 7:08 ` Nicola Vetrini
@ 2023-08-08 7:21 ` Jan Beulich
2023-08-08 21:21 ` Stefano Stabellini
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2023-08-08 7:21 UTC (permalink / raw)
To: Nicola Vetrini
Cc: sstabellini, michal.orzel, xenia.ragiadakou, ayan.kumar.halder,
consulting, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 08.08.2023 09:08, Nicola Vetrini wrote:
> On 07/08/2023 11:10, Jan Beulich wrote:
>> On 07.08.2023 10:59, Nicola Vetrini wrote:
>>> On 07/08/2023 10:09, Jan Beulich wrote:
>>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
>>>>> The variable declared in the header file
>>>>> 'xen/arch/x86/include/asm/e820.h'
>>>>> is shadowed by many function parameters, so it is renamed to avoid
>>>>> these
>>>>> violations.
>>>>>
>>>>> No functional changes.
>>>>>
>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>> ---
>>>>> This patch is similar to other renames done on previous patches, and
>>>>> the
>>>>> preferred strategy there was to rename the global variable. This one
>>>>> has more occurrences that are spread in various files, but
>>>>> the general pattern is the same.
>>>>
>>>> Still I think it would be better done the other way around, and
>>>> perhaps
>>>> in
>>>> more than a single patch. It looks like "many == 3", i.e.
>>>> - e820_add_range(), which is only ever called with "e820" as its
>>>> argument,
>>>> and hence the parameter could be dropped,
>
> I see another downside with this approach (I should have spotted this
> sooner):
> Since e820_add_range and the other functions expected e820 as a pointer,
> they are
> written like this:
>
> for ( i = 0; i < e820->nr_map; ++i )
> {
> uint64_t rs = e820->map[i].addr;
> uint64_t re = rs + e820->map[i].size;
>
> if ( rs == e && e820->map[i].type == type )
> {
> e820->map[i].addr = s;
> return 1;
> }
> ...
>
> Dropping the parameter would either mean
> 1. Use a local parameter that stores the address of e820, which kind of
> nullifies the purpose of dropping the parameter imho;
This isn't an unusual thing to do; it is only the name being short which
may make it look "unnecessary" here. But especially if the local variable
was made of type struct e820entry * (and updated in the for()) I think
this could be useful overall.
> 2. Rewrite it so that it operates on a "struct e820map", which would
> mean
> substantial churn;
> 3. Make the global a pointer, which is reminiscent of (1)
>
> All in all, I do like the global renaming approach more, because it
> lessens
> the amount of code that needs to change to accomodate a case of
> shadowing.
Well, to go that route you need to come up with a suitable new name (no
prior proposal was really meeting that requirement) and you'd need to
convince at least one of the x86 maintainers.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-08 7:21 ` Jan Beulich
@ 2023-08-08 21:21 ` Stefano Stabellini
2023-08-08 21:24 ` Stefano Stabellini
0 siblings, 1 reply; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-08 21:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Nicola Vetrini, sstabellini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Andrew Cooper,
Roger Pau Monné,
Wei Liu, xen-devel
On Tue, 8 Aug 2023, Jan Beulich wrote:
> On 08.08.2023 09:08, Nicola Vetrini wrote:
> > On 07/08/2023 11:10, Jan Beulich wrote:
> >> On 07.08.2023 10:59, Nicola Vetrini wrote:
> >>> On 07/08/2023 10:09, Jan Beulich wrote:
> >>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
> >>>>> The variable declared in the header file
> >>>>> 'xen/arch/x86/include/asm/e820.h'
> >>>>> is shadowed by many function parameters, so it is renamed to avoid
> >>>>> these
> >>>>> violations.
> >>>>>
> >>>>> No functional changes.
> >>>>>
> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> >>>>> ---
> >>>>> This patch is similar to other renames done on previous patches, and
> >>>>> the
> >>>>> preferred strategy there was to rename the global variable. This one
> >>>>> has more occurrences that are spread in various files, but
> >>>>> the general pattern is the same.
> >>>>
> >>>> Still I think it would be better done the other way around, and
> >>>> perhaps
> >>>> in
> >>>> more than a single patch. It looks like "many == 3", i.e.
> >>>> - e820_add_range(), which is only ever called with "e820" as its
> >>>> argument,
> >>>> and hence the parameter could be dropped,
> >
> > I see another downside with this approach (I should have spotted this
> > sooner):
> > Since e820_add_range and the other functions expected e820 as a pointer,
> > they are
> > written like this:
> >
> > for ( i = 0; i < e820->nr_map; ++i )
> > {
> > uint64_t rs = e820->map[i].addr;
> > uint64_t re = rs + e820->map[i].size;
> >
> > if ( rs == e && e820->map[i].type == type )
> > {
> > e820->map[i].addr = s;
> > return 1;
> > }
> > ...
> >
> > Dropping the parameter would either mean
> > 1. Use a local parameter that stores the address of e820, which kind of
> > nullifies the purpose of dropping the parameter imho;
>
> This isn't an unusual thing to do; it is only the name being short which
> may make it look "unnecessary" here. But especially if the local variable
> was made of type struct e820entry * (and updated in the for()) I think
> this could be useful overall.
>
> > 2. Rewrite it so that it operates on a "struct e820map", which would
> > mean
> > substantial churn;
> > 3. Make the global a pointer, which is reminiscent of (1)
> >
> > All in all, I do like the global renaming approach more, because it
> > lessens
> > the amount of code that needs to change to accomodate a case of
> > shadowing.
>
> Well, to go that route you need to come up with a suitable new name (no
> prior proposal was really meeting that requirement) and you'd need to
> convince at least one of the x86 maintainers.
Would you be OK with your previous suggestion:
> Alternatively, if we really were to change the name of the global, we'd
> want to take a more complete approach: Right now we have e820_raw[],
> boot_e820[], and e820[]. We'd want them to follow a uniform naming scheme
> then (e820_ first or _e820 last), with the other part of the name suitably
> describing the purpose (which "map" doesn't do).
So:
e820_raw -> unchanged
boot_e820 -> e820_boot
e820 -> e820_table (or e820_list or e820_ranges)?
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [XEN PATCH 1/6] x86: rename variable 'e820' to address MISRA C:2012 Rule 5.3
2023-08-08 21:21 ` Stefano Stabellini
@ 2023-08-08 21:24 ` Stefano Stabellini
0 siblings, 0 replies; 40+ messages in thread
From: Stefano Stabellini @ 2023-08-08 21:24 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Jan Beulich, Nicola Vetrini, michal.orzel, xenia.ragiadakou,
ayan.kumar.halder, consulting, Andrew Cooper,
Roger Pau Monné,
Wei Liu, xen-devel
On Tue, 8 Aug 2023, Stefano Stabellini wrote:
> On Tue, 8 Aug 2023, Jan Beulich wrote:
> > On 08.08.2023 09:08, Nicola Vetrini wrote:
> > > On 07/08/2023 11:10, Jan Beulich wrote:
> > >> On 07.08.2023 10:59, Nicola Vetrini wrote:
> > >>> On 07/08/2023 10:09, Jan Beulich wrote:
> > >>>> On 04.08.2023 17:27, Nicola Vetrini wrote:
> > >>>>> The variable declared in the header file
> > >>>>> 'xen/arch/x86/include/asm/e820.h'
> > >>>>> is shadowed by many function parameters, so it is renamed to avoid
> > >>>>> these
> > >>>>> violations.
> > >>>>>
> > >>>>> No functional changes.
> > >>>>>
> > >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> > >>>>> ---
> > >>>>> This patch is similar to other renames done on previous patches, and
> > >>>>> the
> > >>>>> preferred strategy there was to rename the global variable. This one
> > >>>>> has more occurrences that are spread in various files, but
> > >>>>> the general pattern is the same.
> > >>>>
> > >>>> Still I think it would be better done the other way around, and
> > >>>> perhaps
> > >>>> in
> > >>>> more than a single patch. It looks like "many == 3", i.e.
> > >>>> - e820_add_range(), which is only ever called with "e820" as its
> > >>>> argument,
> > >>>> and hence the parameter could be dropped,
> > >
> > > I see another downside with this approach (I should have spotted this
> > > sooner):
> > > Since e820_add_range and the other functions expected e820 as a pointer,
> > > they are
> > > written like this:
> > >
> > > for ( i = 0; i < e820->nr_map; ++i )
> > > {
> > > uint64_t rs = e820->map[i].addr;
> > > uint64_t re = rs + e820->map[i].size;
> > >
> > > if ( rs == e && e820->map[i].type == type )
> > > {
> > > e820->map[i].addr = s;
> > > return 1;
> > > }
> > > ...
> > >
> > > Dropping the parameter would either mean
> > > 1. Use a local parameter that stores the address of e820, which kind of
> > > nullifies the purpose of dropping the parameter imho;
> >
> > This isn't an unusual thing to do; it is only the name being short which
> > may make it look "unnecessary" here. But especially if the local variable
> > was made of type struct e820entry * (and updated in the for()) I think
> > this could be useful overall.
> >
> > > 2. Rewrite it so that it operates on a "struct e820map", which would
> > > mean
> > > substantial churn;
> > > 3. Make the global a pointer, which is reminiscent of (1)
> > >
> > > All in all, I do like the global renaming approach more, because it
> > > lessens
> > > the amount of code that needs to change to accomodate a case of
> > > shadowing.
> >
> > Well, to go that route you need to come up with a suitable new name (no
> > prior proposal was really meeting that requirement) and you'd need to
> > convince at least one of the x86 maintainers.
>
> Would you be OK with your previous suggestion:
>
> > Alternatively, if we really were to change the name of the global, we'd
> > want to take a more complete approach: Right now we have e820_raw[],
> > boot_e820[], and e820[]. We'd want them to follow a uniform naming scheme
> > then (e820_ first or _e820 last), with the other part of the name suitably
> > describing the purpose (which "map" doesn't do).
>
> So:
>
> e820_raw -> unchanged
> boot_e820 -> e820_boot
> e820 -> e820_table (or e820_list or e820_ranges)?
Nevermind, I saw now the updated patch
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2023-08-08 21:25 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-04 15:27 [XEN PATCH 0/6] xen: address MISRA C:2012 Rule 5.3 Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 1/6] x86: rename variable 'e820' to " Nicola Vetrini
2023-08-04 21:19 ` Stefano Stabellini
2023-08-07 7:14 ` Nicola Vetrini
2023-08-07 8:09 ` Jan Beulich
2023-08-07 8:59 ` Nicola Vetrini
2023-08-07 9:10 ` Jan Beulich
2023-08-07 11:12 ` Nicola Vetrini
2023-08-07 12:02 ` Jan Beulich
2023-08-08 7:08 ` Nicola Vetrini
2023-08-08 7:21 ` Jan Beulich
2023-08-08 21:21 ` Stefano Stabellini
2023-08-08 21:24 ` Stefano Stabellini
2023-08-07 15:03 ` Nicola Vetrini
2023-08-07 15:07 ` Jan Beulich
2023-08-07 18:39 ` Stefano Stabellini
2023-08-04 15:27 ` [XEN PATCH 2/6] libelf: " Nicola Vetrini
2023-08-04 21:21 ` Stefano Stabellini
2023-08-07 8:11 ` Jan Beulich
2023-08-07 9:03 ` Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 3/6] xen/delay: " Nicola Vetrini
2023-08-04 21:23 ` Stefano Stabellini
2023-08-07 8:14 ` Jan Beulich
2023-08-07 9:01 ` Julien Grall
2023-08-07 9:15 ` Jan Beulich
2023-08-07 9:23 ` Nicola Vetrini
2023-08-07 9:32 ` Jan Beulich
2023-08-07 9:33 ` Julien Grall
2023-08-04 15:27 ` [XEN PATCH 4/6] x86/include: " Nicola Vetrini
2023-08-04 21:24 ` Stefano Stabellini
2023-08-07 7:16 ` Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 5/6] x86/xstate: " Nicola Vetrini
2023-08-04 21:26 ` Stefano Stabellini
2023-08-07 8:23 ` Jan Beulich
2023-08-07 9:20 ` Nicola Vetrini
2023-08-04 15:27 ` [XEN PATCH 6/6] x86: refactor macros in 'xen-mca.h' to " Nicola Vetrini
2023-08-04 15:38 ` Nicola Vetrini
2023-08-04 21:30 ` Stefano Stabellini
2023-08-07 8:31 ` Jan Beulich
2023-08-07 10:01 ` Nicola Vetrini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.