[XEN PATCH 0/4] xen: address MISRA C:2012 Rule 5.3

[XEN PATCH 0/4] xen: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

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

The following two strategies are adopted to deal with som violations
of this rule:
- renaming of local variables, functions or parameters;
- removal of unnecessary parameters from function definitions.

No functional changes.

Nicola Vetrini (4):
  xen/pci: rename local variable to address MISRA C:2012 Rule 5.3
  amd/iommu: rename functions to address MISRA C:2012 Rule 5.3
  xen: rename variables and parameters to address MISRA C:2012 Rule 5.3
  arm/efi: address MISRA C:2012 Rule 5.3

 xen/arch/arm/efi/efi-boot.h               | 29 +++++++++---------
 xen/common/compat/memory.c                |  6 ++--
 xen/common/numa.c                         | 36 +++++++++++------------
 xen/drivers/char/ehci-dbgp.c              |  4 +--
 xen/drivers/char/ns16550.c                |  4 +--
 xen/drivers/passthrough/amd/iommu_guest.c | 14 ++++-----
 xen/drivers/passthrough/pci.c             | 12 ++++----
 7 files changed, 53 insertions(+), 52 deletions(-)

--
2.34.1

[XEN PATCH 1/4] xen/pci: rename local variable to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The rename s/pdev_type/pci_dev_type/ is done to avoid shadowing
the homonymous function declaration.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/drivers/passthrough/pci.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 95846e84f2..1dc519d1b6 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -650,12 +650,12 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
     struct pci_seg *pseg;
     struct pci_dev *pdev;
     unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
-    const char *pdev_type;
+    const char *pci_dev_type;
     int ret;
     bool pf_is_extfn = false;
 
     if ( !info )
-        pdev_type = "device";
+        pci_dev_type = "device";
     else if ( info->is_virtfn )
     {
         pcidevs_lock();
@@ -668,12 +668,12 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
         if ( !pdev )
             pci_add_device(seg, info->physfn.bus, info->physfn.devfn,
                            NULL, node);
-        pdev_type = "virtual function";
+        pci_dev_type = "virtual function";
     }
     else if ( info->is_extfn )
-        pdev_type = "extended function";
+        pci_dev_type = "extended function";
     else
-        pdev_type = "device";
+        pci_dev_type = "device";
 
     ret = xsm_resource_plug_pci(XSM_PRIV, (seg << 16) | (bus << 8) | devfn);
     if ( ret )
@@ -780,7 +780,7 @@ out:
     pcidevs_unlock();
     if ( !ret )
     {
-        printk(XENLOG_DEBUG "PCI add %s %pp\n", pdev_type,  &pdev->sbdf);
+        printk(XENLOG_DEBUG "PCI add %s %pp\n", pci_dev_type,  &pdev->sbdf);
         while ( pdev->phantom_stride )
         {
             func += pdev->phantom_stride;
-- 
2.34.1

[XEN PATCH 2/4] amd/iommu: rename functions to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

The functions 'machine_bfd' and 'guest_bfd' have gained the
prefix 'get_' to avoid the mutual shadowing with the homonymous
parameters in these functions.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/drivers/passthrough/amd/iommu_guest.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 80a331f546..47a912126a 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -38,12 +38,12 @@
         (reg)->hi = (val) >> 32; \
     } while (0)
 
-static unsigned int machine_bdf(struct domain *d, uint16_t guest_bdf)
+static unsigned int get_machine_bdf(struct domain *d, uint16_t guest_bdf)
 {
     return guest_bdf;
 }
 
-static uint16_t guest_bdf(struct domain *d, uint16_t machine_bdf)
+static uint16_t get_guest_bdf(struct domain *d, uint16_t machine_bdf)
 {
     return machine_bdf;
 }
@@ -195,7 +195,7 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
     log = map_domain_page(_mfn(mfn)) + (tail & ~PAGE_MASK);
 
     /* Convert physical device id back into virtual device id */
-    gdev_id = guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
+    gdev_id = get_guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
     iommu_set_devid_to_cmd(&entry[0], gdev_id);
 
     memcpy(log, entry, sizeof(ppr_entry_t));
@@ -245,7 +245,7 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
     log = map_domain_page(_mfn(mfn)) + (tail & ~PAGE_MASK);
 
     /* re-write physical device id into virtual device id */
-    dev_id = guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
+    dev_id = get_guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
     iommu_set_devid_to_cmd(&entry[0], dev_id);
     memcpy(log, entry, sizeof(event_entry_t));
 
@@ -268,7 +268,7 @@ static int do_complete_ppr_request(struct domain *d, cmd_entry_t *cmd)
     uint16_t dev_id;
     struct amd_iommu *iommu;
 
-    dev_id = machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
+    dev_id = get_machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
     iommu = find_iommu_for_device(0, dev_id);
 
     if ( !iommu )
@@ -320,7 +320,7 @@ static int do_invalidate_iotlb_pages(struct domain *d, cmd_entry_t *cmd)
     struct amd_iommu *iommu;
     uint16_t dev_id;
 
-    dev_id = machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
+    dev_id = get_machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
 
     iommu = find_iommu_for_device(0, dev_id);
     if ( !iommu )
@@ -396,7 +396,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
 
     g_iommu = domain_iommu(d);
     gbdf = iommu_get_devid_from_cmd(cmd->data[0]);
-    mbdf = machine_bdf(d, gbdf);
+    mbdf = get_machine_bdf(d, gbdf);
 
     /* Guest can only update DTEs for its passthru devices */
     if ( mbdf == 0 || gbdf == 0 )
-- 
2.34.1

[XEN PATCH 3/4] xen: rename variables and parameters to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

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

Local variables have been suitably renamed to address some violations
of this rule:
- s/cmp/c/ because it shadows the union declared at line 87.
- s/nodes/numa_nodes/ shadows the static variable declared at line 18.
- s/ctrl/controller/ because the homonymous function parameter is later
  read.
- s/baud/baud_rate/ to avoid shadowing the enum constant defined
  at line 1391.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/compat/memory.c   |  6 +++---
 xen/common/numa.c            | 36 ++++++++++++++++++------------------
 xen/drivers/char/ehci-dbgp.c |  4 ++--
 xen/drivers/char/ns16550.c   |  4 ++--
 4 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 8ca63ceda6..3cf0a37d00 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
 
         case XENMEM_remove_from_physmap:
         {
-            struct compat_remove_from_physmap cmp;
+            struct compat_remove_from_physmap c;
 
-            if ( copy_from_guest(&cmp, compat, 1) )
+            if ( copy_from_guest(&c, compat, 1) )
                 return -EFAULT;
 
-            XLAT_remove_from_physmap(nat.xrfp, &cmp);
+            XLAT_remove_from_physmap(nat.xrfp, &c);
 
             break;
         }
diff --git a/xen/common/numa.c b/xen/common/numa.c
index fc1f7f665b..301f2cf374 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -382,7 +382,7 @@ static bool __init numa_process_nodes(paddr_t start, paddr_t end)
  * 0 if memnodmap[] too small (or shift too small)
  * -1 if node overlap or lost ram (shift too big)
  */
-static int __init populate_memnodemap(const struct node *nodes,
+static int __init populate_memnodemap(const struct node *numa_nodes,
                                       unsigned int numnodes, unsigned int shift,
                                       const nodeid_t *nodeids)
 {
@@ -393,8 +393,8 @@ static int __init populate_memnodemap(const struct node *nodes,
 
     for ( i = 0; i < numnodes; i++ )
     {
-        unsigned long spdx = paddr_to_pdx(nodes[i].start);
-        unsigned long epdx = paddr_to_pdx(nodes[i].end - 1);
+        unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
+        unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1);
 
         if ( spdx > epdx )
             continue;
@@ -440,7 +440,7 @@ static int __init allocate_cachealigned_memnodemap(void)
  * The LSB of all start addresses in the node map is the value of the
  * maximum possible shift.
  */
-static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
+static unsigned int __init extract_lsb_from_nodes(const struct node *numa_nodes,
                                                   nodeid_t numnodes,
                                                   const nodeid_t *nodeids)
 {
@@ -449,8 +449,8 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
 
     for ( i = 0; i < numnodes; i++ )
     {
-        unsigned long spdx = paddr_to_pdx(nodes[i].start);
-        unsigned long epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
+        unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
+        unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1) + 1;
 
         if ( spdx >= epdx )
             continue;
@@ -475,10 +475,10 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
     return i;
 }
 
-int __init compute_hash_shift(const struct node *nodes,
+int __init compute_hash_shift(const struct node *numa_nodes,
                               unsigned int numnodes, const nodeid_t *nodeids)
 {
-    unsigned int shift = extract_lsb_from_nodes(nodes, numnodes, nodeids);
+    unsigned int shift = extract_lsb_from_nodes(numa_nodes, numnodes, nodeids);
 
     if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
         memnodemap = _memnodemap;
@@ -487,7 +487,7 @@ int __init compute_hash_shift(const struct node *nodes,
 
     printk(KERN_DEBUG "NUMA: Using %u for the hash shift\n", shift);
 
-    if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
+    if ( populate_memnodemap(numa_nodes, numnodes, shift, nodeids) != 1 )
     {
         printk(KERN_INFO "Your memory is not aligned you need to "
                "rebuild your hypervisor with a bigger NODEMAPSIZE "
@@ -541,7 +541,7 @@ static int __init numa_emulation(unsigned long start_pfn,
 {
     int ret;
     unsigned int i;
-    struct node nodes[MAX_NUMNODES];
+    struct node numa_nodes[MAX_NUMNODES];
     uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
 
     /* Kludge needed for the hash function */
@@ -556,22 +556,22 @@ static int __init numa_emulation(unsigned long start_pfn,
         sz = x;
     }
 
-    memset(&nodes, 0, sizeof(nodes));
+    memset(&numa_nodes, 0, sizeof(numa_nodes));
     for ( i = 0; i < numa_fake; i++ )
     {
-        nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
+        numa_nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
 
         if ( i == numa_fake - 1 )
-            sz = pfn_to_paddr(end_pfn) - nodes[i].start;
+            sz = pfn_to_paddr(end_pfn) - numa_nodes[i].start;
 
-        nodes[i].end = nodes[i].start + sz;
+        numa_nodes[i].end = numa_nodes[i].start + sz;
         printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64" (%"PRIu64"MB)\n",
-               i, nodes[i].start, nodes[i].end,
-               (nodes[i].end - nodes[i].start) >> 20);
+               i, numa_nodes[i].start, numa_nodes[i].end,
+               (numa_nodes[i].end - numa_nodes[i].start) >> 20);
         node_set_online(i);
     }
 
-    ret = compute_hash_shift(nodes, numa_fake, NULL);
+    ret = compute_hash_shift(numa_nodes, numa_fake, NULL);
     if ( ret < 0 )
     {
         printk(KERN_ERR "No NUMA hash function found. Emulation disabled.\n");
@@ -580,7 +580,7 @@ static int __init numa_emulation(unsigned long start_pfn,
     memnode_shift = ret;
 
     for_each_online_node ( i )
-        setup_node_bootmem(i, nodes[i].start, nodes[i].end);
+        setup_node_bootmem(i, numa_nodes[i].start, numa_nodes[i].end);
 
     numa_init_array();
 
diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
index 4d8d765122..22650663dc 100644
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -424,9 +424,9 @@ static void dbgp_issue_command(struct ehci_dbgp *dbgp, u32 ctrl,
          * checks to see if ACPI or some other initialization also
          * reset the EHCI debug port.
          */
-        u32 ctrl = readl(&dbgp->ehci_debug->control);
+        u32 controller = readl(&dbgp->ehci_debug->control);
 
-        if ( ctrl & DBGP_ENABLED )
+        if ( controller & DBGP_ENABLED )
         {
             cmd |= CMD_RUN;
             writel(cmd, &dbgp->ehci_regs->command);
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 212a9c49ae..61c7a19c4d 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1473,7 +1473,7 @@ static enum __init serial_param_type get_token(char *token, char **value)
 
 static bool __init parse_positional(struct ns16550 *uart, char **str)
 {
-    int baud;
+    int baud_rate;
     const char *conf;
     char *name_val_pos;
 
@@ -1504,7 +1504,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
         uart->baud = BAUD_AUTO;
         conf += 4;
     }
-    else if ( (baud = simple_strtoul(conf, &conf, 10)) != 0 )
+    else if ( (baud_rate = simple_strtoul(conf, &conf, 10)) != 0 )
         uart->baud = baud;
 
     if ( *conf == '/' )
-- 
2.34.1

[XEN PATCH 4/4] arm/efi: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

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

The parameter 'fdt' in static function within this file is removed,
as they served no purpose and shadowed the homonymous variable.

For the same reason the local variable in 'lookup_fdt_config_table'
is dropped and the function is now consequently static void.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/arm/efi/efi-boot.h | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6126a71400..ec328885a3 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -52,7 +52,7 @@ static struct file __initdata dtbfile;
 static void __initdata *fdt;
 static void __initdata *memmap;
 
-static int __init setup_chosen_node(void *fdt, int *addr_cells, int *size_cells)
+static int __init setup_chosen_node(int *addr_cells, int *size_cells)
 {
     int node;
     const struct fdt_property *prop;
@@ -114,7 +114,7 @@ static int __init setup_chosen_node(void *fdt, int *addr_cells, int *size_cells)
  * Set a single 'reg' property taking into account the
  * configured addr and size cell sizes.
  */
-static int __init fdt_set_reg(void *fdt, int node, int addr_cells,
+static int __init fdt_set_reg(int node, int addr_cells,
                               int size_cells, uint64_t addr, uint64_t len)
 {
     __be32 val[4]; /* At most 2 64 bit values to be stored */
@@ -138,13 +138,16 @@ static int __init fdt_set_reg(void *fdt, int node, int addr_cells,
     return(fdt_setprop(fdt, node, "reg", val, sizeof(*cellp) * (cellp - val)));
 }
 
-static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
+/*
+ * Set the variable 'fdt' if a matching guid is found.
+ */
+static void __init lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
 {
     static const EFI_GUID __initconst fdt_guid = DEVICE_TREE_GUID;
     EFI_CONFIGURATION_TABLE *tables;
-    void *fdt = NULL;
     int i;
 
+    fdt = NULL;
     tables = sys_table->ConfigurationTable;
     for ( i = 0; i < sys_table->NumberOfTableEntries; i++ )
     {
@@ -154,7 +157,6 @@ static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
             break;
         }
     }
-    return fdt;
 }
 
 static bool __init meminfo_add_bank(struct meminfo *mem,
@@ -228,7 +230,6 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
  * and memory map information.
  */
 EFI_STATUS __init fdt_add_uefi_nodes(EFI_SYSTEM_TABLE *sys_table,
-                                            void *fdt,
                                             EFI_MEMORY_DESCRIPTOR *memory_map,
                                             UINTN map_size,
                                             UINTN desc_size,
@@ -383,7 +384,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
     if ( EFI_ERROR(status) )
         blexit(L"EFI memory map processing failed");
 
-    status = fdt_add_uefi_nodes(SystemTable, fdt, map, map_size, desc_size,
+    status = fdt_add_uefi_nodes(SystemTable, map, map_size, desc_size,
                                 desc_ver);
     if ( EFI_ERROR(status) )
         PrintErrMesg(L"Updating FDT failed", status);
@@ -542,7 +543,7 @@ static void __init efi_arch_handle_module(const struct file *file,
 
     if ( file == &dtbfile )
         return;
-    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
+    chosen = setup_chosen_node(&addr_len, &size_len);
     if ( chosen < 0 )
         blexit(L"Unable to setup chosen node");
 
@@ -557,7 +558,7 @@ static void __init efi_arch_handle_module(const struct file *file,
         if ( fdt_setprop(fdt, node, "compatible", ramdisk_compat,
                          sizeof(ramdisk_compat)) < 0 )
             blexit(L"Unable to set compatible property.");
-        if ( fdt_set_reg(fdt, node, addr_len, size_len, ramdisk.addr,
+        if ( fdt_set_reg(node, addr_len, size_len, ramdisk.addr,
                     ramdisk.size) < 0 )
             blexit(L"Unable to set reg property.");
     }
@@ -572,7 +573,7 @@ static void __init efi_arch_handle_module(const struct file *file,
         if ( fdt_setprop(fdt, node, "compatible", xsm_compat,
                          sizeof(xsm_compat)) < 0 )
             blexit(L"Unable to set compatible property.");
-        if ( fdt_set_reg(fdt, node, addr_len, size_len, xsm.addr,
+        if ( fdt_set_reg(node, addr_len, size_len, xsm.addr,
                     xsm.size) < 0 )
             blexit(L"Unable to set reg property.");
     }
@@ -589,7 +590,7 @@ static void __init efi_arch_handle_module(const struct file *file,
             blexit(L"Unable to set compatible property.");
         if ( options && fdt_setprop_string(fdt, node, "bootargs", options) < 0 )
             blexit(L"Unable to set bootargs property.");
-        if ( fdt_set_reg(fdt, node, addr_len, size_len, kernel.addr,
+        if ( fdt_set_reg(node, addr_len, size_len, kernel.addr,
                          kernel.size) < 0 )
             blexit(L"Unable to set reg property.");
     }
@@ -757,7 +758,7 @@ static int __init handle_module_node(const EFI_LOADED_IMAGE *loaded_image,
         return ERROR_RENAME_MODULE_NAME;
     }
 
-    if ( fdt_set_reg(fdt, module_node_offset, reg_addr_cells, reg_size_cells,
+    if ( fdt_set_reg(module_node_offset, reg_addr_cells, reg_size_cells,
                      file->addr, file->size) < 0 )
     {
         PrintMessage(L"Unable to set module reg property.");
@@ -862,7 +863,7 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
     EFI_FILE_HANDLE dir_handle = NULL;
 
     /* Check for the chosen node in the current DTB */
-    chosen = setup_chosen_node(fdt, &addr_len, &size_len);
+    chosen = setup_chosen_node(&addr_len, &size_len);
     if ( chosen < 0 )
     {
         PrintMessage(L"Unable to setup chosen node");
@@ -951,7 +952,7 @@ static bool __init efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
      * node to decide whether to skip the UEFI Xen configuration file or not.
      */
 
-    fdt = lookup_fdt_config_table(SystemTable);
+    lookup_fdt_config_table(SystemTable);
     dtbfile.ptr = fdt;
     dtbfile.need_to_free = false; /* Config table memory can't be freed. */
 
-- 
2.34.1

Re: [XEN PATCH 4/4] arm/efi: address MISRA C:2012 Rule 5.3

[Julien Grall]

Hi,

On 31/07/2023 14:35, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> The parameter 'fdt' in static function within this file is removed,
> as they served no purpose and shadowed the homonymous variable.

I am not convinced this is a good idea to keep 'fdt' as static variable 
around because the name is too generic. Also in the future we may be 
able to remove the global static variable.

This means that most of this patch will be reverted.

So as I wrote on Matrix, I would rather prefer if the global variable is 
renamed to 'fdt_efi'.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 1/4] xen/pci: rename local variable to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 31.07.2023 15:34, Nicola Vetrini wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -650,12 +650,12 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      struct pci_seg *pseg;
>      struct pci_dev *pdev;
>      unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
> -    const char *pdev_type;
> +    const char *pci_dev_type;

I've always been wondering what purpose the pdev_ prefix served here.
There's no other "type" variable in the function, so why make the name
longer? (I'm okay to adjust on commit, provided you agree.)

Jan

Re: [XEN PATCH 2/4] amd/iommu: rename functions to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 31.07.2023 15:35, Nicola Vetrini wrote:
> The functions 'machine_bfd' and 'guest_bfd' have gained the
> prefix 'get_' to avoid the mutual shadowing with the homonymous
> parameters in these functions.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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

Of course there are several other oddities, but in the end the entire file
in a single big one, I'm afraid.

Jan

Re: [XEN PATCH 3/4] xen: rename variables and parameters to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 31.07.2023 15:35, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> Local variables have been suitably renamed to address some violations
> of this rule:
> - s/cmp/c/ because it shadows the union declared at line 87.
> - s/nodes/numa_nodes/ shadows the static variable declared at line 18.
> - s/ctrl/controller/ because the homonymous function parameter is later
>   read.
> - s/baud/baud_rate/ to avoid shadowing the enum constant defined
>   at line 1391.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/common/compat/memory.c   |  6 +++---
>  xen/common/numa.c            | 36 ++++++++++++++++++------------------
>  xen/drivers/char/ehci-dbgp.c |  4 ++--
>  xen/drivers/char/ns16550.c   |  4 ++--
>  4 files changed, 25 insertions(+), 25 deletions(-)

This is an odd mix of files touched in a single patch. How about splitting
into two, one for common/ and one for drivers/?

> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
>  
>          case XENMEM_remove_from_physmap:
>          {
> -            struct compat_remove_from_physmap cmp;
> +            struct compat_remove_from_physmap c;

The intention of the outer scope cmp is to avoid such inner scope
ones then consuming extra stack space. This wants making part of the
union there.

> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -382,7 +382,7 @@ static bool __init numa_process_nodes(paddr_t start, paddr_t end)
>   * 0 if memnodmap[] too small (or shift too small)
>   * -1 if node overlap or lost ram (shift too big)
>   */
> -static int __init populate_memnodemap(const struct node *nodes,
> +static int __init populate_memnodemap(const struct node *numa_nodes,
>                                        unsigned int numnodes, unsigned int shift,
>                                        const nodeid_t *nodeids)
>  {
> @@ -393,8 +393,8 @@ static int __init populate_memnodemap(const struct node *nodes,
>  
>      for ( i = 0; i < numnodes; i++ )
>      {
> -        unsigned long spdx = paddr_to_pdx(nodes[i].start);
> -        unsigned long epdx = paddr_to_pdx(nodes[i].end - 1);
> +        unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
> +        unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1);
>  
>          if ( spdx > epdx )
>              continue;
> @@ -440,7 +440,7 @@ static int __init allocate_cachealigned_memnodemap(void)
>   * The LSB of all start addresses in the node map is the value of the
>   * maximum possible shift.
>   */
> -static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
> +static unsigned int __init extract_lsb_from_nodes(const struct node *numa_nodes,
>                                                    nodeid_t numnodes,
>                                                    const nodeid_t *nodeids)
>  {
> @@ -449,8 +449,8 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
>  
>      for ( i = 0; i < numnodes; i++ )
>      {
> -        unsigned long spdx = paddr_to_pdx(nodes[i].start);
> -        unsigned long epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
> +        unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
> +        unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1) + 1;
>  
>          if ( spdx >= epdx )
>              continue;
> @@ -475,10 +475,10 @@ static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
>      return i;
>  }
>  
> -int __init compute_hash_shift(const struct node *nodes,
> +int __init compute_hash_shift(const struct node *numa_nodes,
>                                unsigned int numnodes, const nodeid_t *nodeids)
>  {
> -    unsigned int shift = extract_lsb_from_nodes(nodes, numnodes, nodeids);
> +    unsigned int shift = extract_lsb_from_nodes(numa_nodes, numnodes, nodeids);
>  
>      if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
>          memnodemap = _memnodemap;
> @@ -487,7 +487,7 @@ int __init compute_hash_shift(const struct node *nodes,
>  
>      printk(KERN_DEBUG "NUMA: Using %u for the hash shift\n", shift);
>  
> -    if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
> +    if ( populate_memnodemap(numa_nodes, numnodes, shift, nodeids) != 1 )
>      {
>          printk(KERN_INFO "Your memory is not aligned you need to "
>                 "rebuild your hypervisor with a bigger NODEMAPSIZE "
> @@ -541,7 +541,7 @@ static int __init numa_emulation(unsigned long start_pfn,
>  {
>      int ret;
>      unsigned int i;
> -    struct node nodes[MAX_NUMNODES];
> +    struct node numa_nodes[MAX_NUMNODES];
>      uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
>  
>      /* Kludge needed for the hash function */
> @@ -556,22 +556,22 @@ static int __init numa_emulation(unsigned long start_pfn,
>          sz = x;
>      }
>  
> -    memset(&nodes, 0, sizeof(nodes));
> +    memset(&numa_nodes, 0, sizeof(numa_nodes));
>      for ( i = 0; i < numa_fake; i++ )
>      {
> -        nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
> +        numa_nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
>  
>          if ( i == numa_fake - 1 )
> -            sz = pfn_to_paddr(end_pfn) - nodes[i].start;
> +            sz = pfn_to_paddr(end_pfn) - numa_nodes[i].start;
>  
> -        nodes[i].end = nodes[i].start + sz;
> +        numa_nodes[i].end = numa_nodes[i].start + sz;
>          printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64" (%"PRIu64"MB)\n",
> -               i, nodes[i].start, nodes[i].end,
> -               (nodes[i].end - nodes[i].start) >> 20);
> +               i, numa_nodes[i].start, numa_nodes[i].end,
> +               (numa_nodes[i].end - numa_nodes[i].start) >> 20);
>          node_set_online(i);
>      }
>  
> -    ret = compute_hash_shift(nodes, numa_fake, NULL);
> +    ret = compute_hash_shift(numa_nodes, numa_fake, NULL);
>      if ( ret < 0 )
>      {
>          printk(KERN_ERR "No NUMA hash function found. Emulation disabled.\n");
> @@ -580,7 +580,7 @@ static int __init numa_emulation(unsigned long start_pfn,
>      memnode_shift = ret;
>  
>      for_each_online_node ( i )
> -        setup_node_bootmem(i, nodes[i].start, nodes[i].end);
> +        setup_node_bootmem(i, numa_nodes[i].start, numa_nodes[i].end);
>  
>      numa_init_array();
>  

I think renaming the file-scope variable this way would be more logical
and less risky (the way you do it it's easy to miss one place without
the build breaking).

> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -424,9 +424,9 @@ static void dbgp_issue_command(struct ehci_dbgp *dbgp, u32 ctrl,
>           * checks to see if ACPI or some other initialization also
>           * reset the EHCI debug port.
>           */
> -        u32 ctrl = readl(&dbgp->ehci_debug->control);
> +        u32 controller = readl(&dbgp->ehci_debug->control);

Why "controller" when the field read is named "control"? Perhaps
easiest would be to drop the variablöe altogether: It's used exactly
once, ...

> -        if ( ctrl & DBGP_ENABLED )
> +        if ( controller & DBGP_ENABLED )

... here.

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1473,7 +1473,7 @@ static enum __init serial_param_type get_token(char *token, char **value)
>  
>  static bool __init parse_positional(struct ns16550 *uart, char **str)
>  {
> -    int baud;
> +    int baud_rate;
>      const char *conf;
>      char *name_val_pos;
>  
> @@ -1504,7 +1504,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>          uart->baud = BAUD_AUTO;
>          conf += 4;
>      }
> -    else if ( (baud = simple_strtoul(conf, &conf, 10)) != 0 )
> +    else if ( (baud_rate = simple_strtoul(conf, &conf, 10)) != 0 )
>          uart->baud = baud;

So along the lines of the earlier remark on common/numa.c: Here you're
actively introducing a bug, by not also renaming the further use of the
variable. Please reconsider the name change.

Jan

Re: [XEN PATCH 1/4] xen/pci: rename local variable to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 31/07/2023 16:16, Jan Beulich wrote:
> On 31.07.2023 15:34, Nicola Vetrini wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -650,12 +650,12 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>      struct pci_seg *pseg;
>>      struct pci_dev *pdev;
>>      unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
>> -    const char *pdev_type;
>> +    const char *pci_dev_type;
> 
> I've always been wondering what purpose the pdev_ prefix served here.
> There's no other "type" variable in the function, so why make the name
> longer? (I'm okay to adjust on commit, provided you agree.)
> 
> Jan

No objections.

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

Re: [XEN PATCH 1/4] xen/pci: rename local variable to address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Mon, 31 Jul 2023, Nicola Vetrini wrote:
> On 31/07/2023 16:16, Jan Beulich wrote:
> > On 31.07.2023 15:34, Nicola Vetrini wrote:
> > > --- a/xen/drivers/passthrough/pci.c
> > > +++ b/xen/drivers/passthrough/pci.c
> > > @@ -650,12 +650,12 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> > >      struct pci_seg *pseg;
> > >      struct pci_dev *pdev;
> > >      unsigned int slot = PCI_SLOT(devfn), func = PCI_FUNC(devfn);
> > > -    const char *pdev_type;
> > > +    const char *pci_dev_type;
> > 
> > I've always been wondering what purpose the pdev_ prefix served here.
> > There's no other "type" variable in the function, so why make the name
> > longer? (I'm okay to adjust on commit, provided you agree.)
> > 
> > Jan
> 
> No objections.

I reviewed the patch and it is correct:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


Jan, feel free to pick any name you prefer on commit, e.g. "type".

Re: [XEN PATCH 2/4] amd/iommu: rename functions to address MISRA C:2012 Rule 5.3

[Stefano Stabellini]

On Mon, 31 Jul 2023, Nicola Vetrini wrote:
> The functions 'machine_bfd' and 'guest_bfd' have gained the
> prefix 'get_' to avoid the mutual shadowing with the homonymous
> parameters in these functions.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/drivers/passthrough/amd/iommu_guest.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
> index 80a331f546..47a912126a 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -38,12 +38,12 @@
>          (reg)->hi = (val) >> 32; \
>      } while (0)
>  
> -static unsigned int machine_bdf(struct domain *d, uint16_t guest_bdf)
> +static unsigned int get_machine_bdf(struct domain *d, uint16_t guest_bdf)
>  {
>      return guest_bdf;
>  }
>  
> -static uint16_t guest_bdf(struct domain *d, uint16_t machine_bdf)
> +static uint16_t get_guest_bdf(struct domain *d, uint16_t machine_bdf)
>  {
>      return machine_bdf;
>  }
> @@ -195,7 +195,7 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
>      log = map_domain_page(_mfn(mfn)) + (tail & ~PAGE_MASK);
>  
>      /* Convert physical device id back into virtual device id */
> -    gdev_id = guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
> +    gdev_id = get_guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
>      iommu_set_devid_to_cmd(&entry[0], gdev_id);
>  
>      memcpy(log, entry, sizeof(ppr_entry_t));
> @@ -245,7 +245,7 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
>      log = map_domain_page(_mfn(mfn)) + (tail & ~PAGE_MASK);
>  
>      /* re-write physical device id into virtual device id */
> -    dev_id = guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
> +    dev_id = get_guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
>      iommu_set_devid_to_cmd(&entry[0], dev_id);
>      memcpy(log, entry, sizeof(event_entry_t));
>  
> @@ -268,7 +268,7 @@ static int do_complete_ppr_request(struct domain *d, cmd_entry_t *cmd)
>      uint16_t dev_id;
>      struct amd_iommu *iommu;
>  
> -    dev_id = machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
> +    dev_id = get_machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
>      iommu = find_iommu_for_device(0, dev_id);
>  
>      if ( !iommu )
> @@ -320,7 +320,7 @@ static int do_invalidate_iotlb_pages(struct domain *d, cmd_entry_t *cmd)
>      struct amd_iommu *iommu;
>      uint16_t dev_id;
>  
> -    dev_id = machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
> +    dev_id = get_machine_bdf(d, iommu_get_devid_from_cmd(cmd->data[0]));
>  
>      iommu = find_iommu_for_device(0, dev_id);
>      if ( !iommu )
> @@ -396,7 +396,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>  
>      g_iommu = domain_iommu(d);
>      gbdf = iommu_get_devid_from_cmd(cmd->data[0]);
> -    mbdf = machine_bdf(d, gbdf);
> +    mbdf = get_machine_bdf(d, gbdf);
>  
>      /* Guest can only update DTEs for its passthru devices */
>      if ( mbdf == 0 || gbdf == 0 )
> -- 
> 2.34.1
> 

Re: [XEN PATCH 3/4] xen: rename variables and parameters to address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 31/07/2023 16:34, Jan Beulich wrote:
> On 31.07.2023 15:35, Nicola Vetrini wrote:
>> Rule 5.3 has the following headline:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>> 
>> Local variables have been suitably renamed to address some violations
>> of this rule:
>> - s/cmp/c/ because it shadows the union declared at line 87.
>> - s/nodes/numa_nodes/ shadows the static variable declared at line 18.
>> - s/ctrl/controller/ because the homonymous function parameter is 
>> later
>>   read.
>> - s/baud/baud_rate/ to avoid shadowing the enum constant defined
>>   at line 1391.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/common/compat/memory.c   |  6 +++---
>>  xen/common/numa.c            | 36 
>> ++++++++++++++++++------------------
>>  xen/drivers/char/ehci-dbgp.c |  4 ++--
>>  xen/drivers/char/ns16550.c   |  4 ++--
>>  4 files changed, 25 insertions(+), 25 deletions(-)
> 
> This is an odd mix of files touched in a single patch. How about 
> splitting
> into two, one for common/ and one for drivers/?
> 

Ok, I'll do it.

>> --- a/xen/common/compat/memory.c
>> +++ b/xen/common/compat/memory.c
>> @@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>> 
>>          case XENMEM_remove_from_physmap:
>>          {
>> -            struct compat_remove_from_physmap cmp;
>> +            struct compat_remove_from_physmap c;
> 
> The intention of the outer scope cmp is to avoid such inner scope
> ones then consuming extra stack space. This wants making part of the
> union there.
> 

Makes sense, though I've not been able to find a definition for the type
'struct compat_remove_from_physmap'.

>> --- a/xen/common/numa.c
>> +++ b/xen/common/numa.c
>> @@ -382,7 +382,7 @@ static bool __init numa_process_nodes(paddr_t 
>> start, paddr_t end)
>>   * 0 if memnodmap[] too small (or shift too small)
>>   * -1 if node overlap or lost ram (shift too big)
>>   */
>> -static int __init populate_memnodemap(const struct node *nodes,
>> +static int __init populate_memnodemap(const struct node *numa_nodes,
>>                                        unsigned int numnodes, unsigned 
>> int shift,
>>                                        const nodeid_t *nodeids)
>>  {
>> @@ -393,8 +393,8 @@ static int __init populate_memnodemap(const struct 
>> node *nodes,
>> 
>>      for ( i = 0; i < numnodes; i++ )
>>      {
>> -        unsigned long spdx = paddr_to_pdx(nodes[i].start);
>> -        unsigned long epdx = paddr_to_pdx(nodes[i].end - 1);
>> +        unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
>> +        unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1);
>> 
>>          if ( spdx > epdx )
>>              continue;
>> @@ -440,7 +440,7 @@ static int __init 
>> allocate_cachealigned_memnodemap(void)
>>   * The LSB of all start addresses in the node map is the value of the
>>   * maximum possible shift.
>>   */
>> -static unsigned int __init extract_lsb_from_nodes(const struct node 
>> *nodes,
>> +static unsigned int __init extract_lsb_from_nodes(const struct node 
>> *numa_nodes,
>>                                                    nodeid_t numnodes,
>>                                                    const nodeid_t 
>> *nodeids)
>>  {
>> @@ -449,8 +449,8 @@ static unsigned int __init 
>> extract_lsb_from_nodes(const struct node *nodes,
>> 
>>      for ( i = 0; i < numnodes; i++ )
>>      {
>> -        unsigned long spdx = paddr_to_pdx(nodes[i].start);
>> -        unsigned long epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
>> +        unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
>> +        unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1) + 1;
>> 
>>          if ( spdx >= epdx )
>>              continue;
>> @@ -475,10 +475,10 @@ static unsigned int __init 
>> extract_lsb_from_nodes(const struct node *nodes,
>>      return i;
>>  }
>> 
>> -int __init compute_hash_shift(const struct node *nodes,
>> +int __init compute_hash_shift(const struct node *numa_nodes,
>>                                unsigned int numnodes, const nodeid_t 
>> *nodeids)
>>  {
>> -    unsigned int shift = extract_lsb_from_nodes(nodes, numnodes, 
>> nodeids);
>> +    unsigned int shift = extract_lsb_from_nodes(numa_nodes, numnodes, 
>> nodeids);
>> 
>>      if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
>>          memnodemap = _memnodemap;
>> @@ -487,7 +487,7 @@ int __init compute_hash_shift(const struct node 
>> *nodes,
>> 
>>      printk(KERN_DEBUG "NUMA: Using %u for the hash shift\n", shift);
>> 
>> -    if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
>> +    if ( populate_memnodemap(numa_nodes, numnodes, shift, nodeids) != 
>> 1 )
>>      {
>>          printk(KERN_INFO "Your memory is not aligned you need to "
>>                 "rebuild your hypervisor with a bigger NODEMAPSIZE "
>> @@ -541,7 +541,7 @@ static int __init numa_emulation(unsigned long 
>> start_pfn,
>>  {
>>      int ret;
>>      unsigned int i;
>> -    struct node nodes[MAX_NUMNODES];
>> +    struct node numa_nodes[MAX_NUMNODES];
>>      uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
>> 
>>      /* Kludge needed for the hash function */
>> @@ -556,22 +556,22 @@ static int __init numa_emulation(unsigned long 
>> start_pfn,
>>          sz = x;
>>      }
>> 
>> -    memset(&nodes, 0, sizeof(nodes));
>> +    memset(&numa_nodes, 0, sizeof(numa_nodes));
>>      for ( i = 0; i < numa_fake; i++ )
>>      {
>> -        nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
>> +        numa_nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
>> 
>>          if ( i == numa_fake - 1 )
>> -            sz = pfn_to_paddr(end_pfn) - nodes[i].start;
>> +            sz = pfn_to_paddr(end_pfn) - numa_nodes[i].start;
>> 
>> -        nodes[i].end = nodes[i].start + sz;
>> +        numa_nodes[i].end = numa_nodes[i].start + sz;
>>          printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64" 
>> (%"PRIu64"MB)\n",
>> -               i, nodes[i].start, nodes[i].end,
>> -               (nodes[i].end - nodes[i].start) >> 20);
>> +               i, numa_nodes[i].start, numa_nodes[i].end,
>> +               (numa_nodes[i].end - numa_nodes[i].start) >> 20);
>>          node_set_online(i);
>>      }
>> 
>> -    ret = compute_hash_shift(nodes, numa_fake, NULL);
>> +    ret = compute_hash_shift(numa_nodes, numa_fake, NULL);
>>      if ( ret < 0 )
>>      {
>>          printk(KERN_ERR "No NUMA hash function found. Emulation 
>> disabled.\n");
>> @@ -580,7 +580,7 @@ static int __init numa_emulation(unsigned long 
>> start_pfn,
>>      memnode_shift = ret;
>> 
>>      for_each_online_node ( i )
>> -        setup_node_bootmem(i, nodes[i].start, nodes[i].end);
>> +        setup_node_bootmem(i, numa_nodes[i].start, 
>> numa_nodes[i].end);
>> 
>>      numa_init_array();
>> 
> 
> I think renaming the file-scope variable this way would be more logical
> and less risky (the way you do it it's easy to miss one place without
> the build breaking).
> 

Ok

>> --- a/xen/drivers/char/ehci-dbgp.c
>> +++ b/xen/drivers/char/ehci-dbgp.c
>> @@ -424,9 +424,9 @@ static void dbgp_issue_command(struct ehci_dbgp 
>> *dbgp, u32 ctrl,
>>           * checks to see if ACPI or some other initialization also
>>           * reset the EHCI debug port.
>>           */
>> -        u32 ctrl = readl(&dbgp->ehci_debug->control);
>> +        u32 controller = readl(&dbgp->ehci_debug->control);
> 
> Why "controller" when the field read is named "control"? Perhaps
> easiest would be to drop the variablöe altogether: It's used exactly
> once, ...
> 
>> -        if ( ctrl & DBGP_ENABLED )
>> +        if ( controller & DBGP_ENABLED )
> 
> ... here.
> 

I was fooled by the comment above its declaration. At first sight I 
don't
see anything wrong with dropping the variable, but I'll check it 
properly before submitting
a v2.

>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1473,7 +1473,7 @@ static enum __init serial_param_type 
>> get_token(char *token, char **value)
>> 
>>  static bool __init parse_positional(struct ns16550 *uart, char **str)
>>  {
>> -    int baud;
>> +    int baud_rate;
>>      const char *conf;
>>      char *name_val_pos;
>> 
>> @@ -1504,7 +1504,7 @@ static bool __init parse_positional(struct 
>> ns16550 *uart, char **str)
>>          uart->baud = BAUD_AUTO;
>>          conf += 4;
>>      }
>> -    else if ( (baud = simple_strtoul(conf, &conf, 10)) != 0 )
>> +    else if ( (baud_rate = simple_strtoul(conf, &conf, 10)) != 0 )
>>          uart->baud = baud;
> 
> So along the lines of the earlier remark on common/numa.c: Here you're
> actively introducing a bug, by not also renaming the further use of the
> variable. Please reconsider the name change.
> 

Good catch, I'll do as suggested in v2.

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

Re: [XEN PATCH 4/4] arm/efi: address MISRA C:2012 Rule 5.3

[Nicola Vetrini]

On 31/07/2023 16:01, Julien Grall wrote:
> Hi,
> 
> On 31/07/2023 14:35, Nicola Vetrini wrote:
>> Rule 5.3 has the following headline:
>> "An identifier declared in an inner scope shall not hide an
>> identifier declared in an outer scope"
>> 
>> The parameter 'fdt' in static function within this file is removed,
>> as they served no purpose and shadowed the homonymous variable.
> 
> I am not convinced this is a good idea to keep 'fdt' as static
> variable around because the name is too generic. Also in the future we
> may be able to remove the global static variable.
> 
> This means that most of this patch will be reverted.
> 
> So as I wrote on Matrix, I would rather prefer if the global variable
> is renamed to 'fdt_efi'.
> 
> Cheers,

Will do. Changing the parameters seemed less messy, but I'm fine either 
way, so
I'll submit a v2 with that rename.

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

Re: [XEN PATCH 3/4] xen: rename variables and parameters to address MISRA C:2012 Rule 5.3

[Jan Beulich]

On 01.08.2023 09:20, Nicola Vetrini wrote:
> On 31/07/2023 16:34, Jan Beulich wrote:
>> On 31.07.2023 15:35, Nicola Vetrini wrote:
>>> --- a/xen/common/compat/memory.c
>>> +++ b/xen/common/compat/memory.c
>>> @@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd, 
>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>
>>>          case XENMEM_remove_from_physmap:
>>>          {
>>> -            struct compat_remove_from_physmap cmp;
>>> +            struct compat_remove_from_physmap c;
>>
>> The intention of the outer scope cmp is to avoid such inner scope
>> ones then consuming extra stack space. This wants making part of the
>> union there.
>>
> 
> Makes sense, though I've not been able to find a definition for the type
> 'struct compat_remove_from_physmap'.

This is a generated type, so you'll need to look in the build tree
under xen/include/compat/. (In this context I'm curious how Misra and
Eclair deal with generated code.)

Jan