[PATCH v2 0/6] hw: Clean up global variables shadowing

[PATCH v2 0/6] hw: Clean up global variables shadowing

[Philippe Mathieu-Daudé]

Missing review: #1 #2 #3

v2:
- Addressed Ani's review comments (patches #2, #3)
- Added R-b tags

Clean up global variables shadowing in hw/ in
order to be able to use -Wshadow with Clang.

Philippe Mathieu-Daudé (6):
  hw/core/cpu: Clean up global variable shadowing
  hw/loader: Clean up global variable shadowing in rom_add_file()
  hw/display/vga: Clean up global variable shadowing
  hw/acpi/pcihp: Clean up global variable shadowing in acpi_pcihp_init()
  hw/pci: Clean up global variable shadowing of address_space_io
    variable
  hw/s390x: Clean up global variable shadowing in
    quiesce_powerdown_req()

 hw/display/vga_int.h      |  4 ++--
 include/hw/acpi/pcihp.h   |  2 +-
 include/hw/core/cpu.h     |  8 ++++----
 include/hw/loader.h       | 11 ++++++-----
 include/hw/pci/pci.h      |  9 +++------
 cpu-common.c              |  6 +++---
 hw/acpi/pcihp.c           |  5 ++---
 hw/core/loader.c          | 10 +++++-----
 hw/display/vga.c          | 12 ++++++------
 hw/pci/pci.c              | 25 +++++++++----------------
 hw/s390x/sclpquiesce.c    |  8 ++++----
 target/s390x/cpu_models.c |  2 +-
 12 files changed, 46 insertions(+), 56 deletions(-)

-- 
2.41.0

[PATCH v2 1/6] hw/core/cpu: Clean up global variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/core/machine.c:1302:22: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
      const CPUArchId *cpus = possible_cpus->cpus;
                       ^
  hw/core/numa.c:69:17: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
      uint16List *cpus = NULL;
                  ^
  hw/acpi/aml-build.c:2005:20: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
      CPUArchIdList *cpus = ms->possible_cpus;
                     ^
  hw/core/machine-smp.c:77:14: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
      unsigned cpus    = config->has_cpus ? config->cpus : 0;
               ^
  include/hw/core/cpu.h:589:17: note: previous declaration is here
  extern CPUTailQ cpus;
                  ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/cpu.h     | 8 ++++----
 cpu-common.c              | 6 +++---
 target/s390x/cpu_models.c | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index e02bc5980f..d0dc0a1698 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -586,13 +586,13 @@ static inline CPUArchState *cpu_env(CPUState *cpu)
 }
 
 typedef QTAILQ_HEAD(CPUTailQ, CPUState) CPUTailQ;
-extern CPUTailQ cpus;
+extern CPUTailQ cpus_queue;
 
-#define first_cpu        QTAILQ_FIRST_RCU(&cpus)
+#define first_cpu        QTAILQ_FIRST_RCU(&cpus_queue)
 #define CPU_NEXT(cpu)    QTAILQ_NEXT_RCU(cpu, node)
-#define CPU_FOREACH(cpu) QTAILQ_FOREACH_RCU(cpu, &cpus, node)
+#define CPU_FOREACH(cpu) QTAILQ_FOREACH_RCU(cpu, &cpus_queue, node)
 #define CPU_FOREACH_SAFE(cpu, next_cpu) \
-    QTAILQ_FOREACH_SAFE_RCU(cpu, &cpus, node, next_cpu)
+    QTAILQ_FOREACH_SAFE_RCU(cpu, &cpus_queue, node, next_cpu)
 
 extern __thread CPUState *current_cpu;
 
diff --git a/cpu-common.c b/cpu-common.c
index 45c745ecf6..c81fd72d16 100644
--- a/cpu-common.c
+++ b/cpu-common.c
@@ -73,7 +73,7 @@ static int cpu_get_free_index(void)
     return max_cpu_index;
 }
 
-CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
+CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
 static unsigned int cpu_list_generation_id;
 
 unsigned int cpu_list_generation_id_get(void)
@@ -90,7 +90,7 @@ void cpu_list_add(CPUState *cpu)
     } else {
         assert(!cpu_index_auto_assigned);
     }
-    QTAILQ_INSERT_TAIL_RCU(&cpus, cpu, node);
+    QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
     cpu_list_generation_id++;
 }
 
@@ -102,7 +102,7 @@ void cpu_list_remove(CPUState *cpu)
         return;
     }
 
-    QTAILQ_REMOVE_RCU(&cpus, cpu, node);
+    QTAILQ_REMOVE_RCU(&cpus_queue, cpu, node);
     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
     cpu_list_generation_id++;
 }
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 98f14c09c2..b1e77b3a2b 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -756,7 +756,7 @@ void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga,
     const S390CPUDef *def = s390_find_cpu_def(type, gen, ec_ga, NULL);
 
     g_assert(def);
-    g_assert(QTAILQ_EMPTY_RCU(&cpus));
+    g_assert(QTAILQ_EMPTY_RCU(&cpus_queue));
 
     /* build the CPU model */
     s390_qemu_cpu_model.def = def;
-- 
2.41.0

Re: [PATCH v2 1/6] hw/core/cpu: Clean up global variable shadowing

[Ani Sinha]

> On 10-Oct-2023, at 5:20 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Fix:
> 
>  hw/core/machine.c:1302:22: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
>      const CPUArchId *cpus = possible_cpus->cpus;
>                       ^
>  hw/core/numa.c:69:17: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
>      uint16List *cpus = NULL;
>                  ^
>  hw/acpi/aml-build.c:2005:20: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
>      CPUArchIdList *cpus = ms->possible_cpus;
>                     ^
>  hw/core/machine-smp.c:77:14: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
>      unsigned cpus    = config->has_cpus ? config->cpus : 0;
>               ^
>  include/hw/core/cpu.h:589:17: note: previous declaration is here
>  extern CPUTailQ cpus;
>                  ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Ani Sinha <anisinha@redhat.com>

> ---
> include/hw/core/cpu.h     | 8 ++++----
> cpu-common.c              | 6 +++---
> target/s390x/cpu_models.c | 2 +-
> 3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index e02bc5980f..d0dc0a1698 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -586,13 +586,13 @@ static inline CPUArchState *cpu_env(CPUState *cpu)
> }
> 
> typedef QTAILQ_HEAD(CPUTailQ, CPUState) CPUTailQ;
> -extern CPUTailQ cpus;
> +extern CPUTailQ cpus_queue;
> 
> -#define first_cpu        QTAILQ_FIRST_RCU(&cpus)
> +#define first_cpu        QTAILQ_FIRST_RCU(&cpus_queue)
> #define CPU_NEXT(cpu)    QTAILQ_NEXT_RCU(cpu, node)
> -#define CPU_FOREACH(cpu) QTAILQ_FOREACH_RCU(cpu, &cpus, node)
> +#define CPU_FOREACH(cpu) QTAILQ_FOREACH_RCU(cpu, &cpus_queue, node)
> #define CPU_FOREACH_SAFE(cpu, next_cpu) \
> -    QTAILQ_FOREACH_SAFE_RCU(cpu, &cpus, node, next_cpu)
> +    QTAILQ_FOREACH_SAFE_RCU(cpu, &cpus_queue, node, next_cpu)
> 
> extern __thread CPUState *current_cpu;
> 
> diff --git a/cpu-common.c b/cpu-common.c
> index 45c745ecf6..c81fd72d16 100644
> --- a/cpu-common.c
> +++ b/cpu-common.c
> @@ -73,7 +73,7 @@ static int cpu_get_free_index(void)
>     return max_cpu_index;
> }
> 
> -CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
> +CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> static unsigned int cpu_list_generation_id;
> 
> unsigned int cpu_list_generation_id_get(void)
> @@ -90,7 +90,7 @@ void cpu_list_add(CPUState *cpu)
>     } else {
>         assert(!cpu_index_auto_assigned);
>     }
> -    QTAILQ_INSERT_TAIL_RCU(&cpus, cpu, node);
> +    QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
>     cpu_list_generation_id++;
> }
> 
> @@ -102,7 +102,7 @@ void cpu_list_remove(CPUState *cpu)
>         return;
>     }
> 
> -    QTAILQ_REMOVE_RCU(&cpus, cpu, node);
> +    QTAILQ_REMOVE_RCU(&cpus_queue, cpu, node);
>     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>     cpu_list_generation_id++;
> }
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 98f14c09c2..b1e77b3a2b 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -756,7 +756,7 @@ void s390_set_qemu_cpu_model(uint16_t type, uint8_t gen, uint8_t ec_ga,
>     const S390CPUDef *def = s390_find_cpu_def(type, gen, ec_ga, NULL);
> 
>     g_assert(def);
> -    g_assert(QTAILQ_EMPTY_RCU(&cpus));
> +    g_assert(QTAILQ_EMPTY_RCU(&cpus_queue));
> 
>     /* build the CPU model */
>     s390_qemu_cpu_model.def = def;
> -- 
> 2.41.0
> 

[PATCH v2 2/6] hw/loader: Clean up global variable shadowing in rom_add_file()

[Philippe Mathieu-Daudé]

Invert 'option_rom' logic and rename it as 'option_rom_has_mr' to fix:

  hw/core/loader.c:1073:27: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
                       bool option_rom, MemoryRegion *mr,
                            ^
  include/sysemu/sysemu.h:57:22: note: previous declaration is here
  extern QEMUOptionRom option_rom[MAX_OPTION_ROMS];
                       ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/loader.h | 11 ++++++-----
 hw/core/loader.c    | 10 +++++-----
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/hw/loader.h b/include/hw/loader.h
index c4c14170ea..9dbb9f7fdb 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -272,7 +272,8 @@ void pstrcpy_targphys(const char *name,
 
 ssize_t rom_add_file(const char *file, const char *fw_dir,
                      hwaddr addr, int32_t bootindex,
-                     bool option_rom, MemoryRegion *mr, AddressSpace *as);
+                     bool option_rom_has_mr,
+                     MemoryRegion *mr, AddressSpace *as);
 MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            size_t max_len, hwaddr addr,
                            const char *fw_file_name,
@@ -341,15 +342,15 @@ void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size);
 void hmp_info_roms(Monitor *mon, const QDict *qdict);
 
 #define rom_add_file_fixed(_f, _a, _i)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
+    rom_add_file(_f, NULL, _a, _i, true, NULL, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
 #define rom_add_file_mr(_f, _mr, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
+    rom_add_file(_f, NULL, 0, _i, true, _mr, NULL)
 #define rom_add_file_as(_f, _as, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
+    rom_add_file(_f, NULL, 0, _i, true, NULL, _as)
 #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
-    rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
+    rom_add_file(_f, NULL, _a, _i, true, NULL, _as)
 #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 4dd5a71fb7..f0d20fd776 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1070,8 +1070,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
 
 ssize_t rom_add_file(const char *file, const char *fw_dir,
                      hwaddr addr, int32_t bootindex,
-                     bool option_rom, MemoryRegion *mr,
-                     AddressSpace *as)
+                     bool option_rom_has_mr,
+                     MemoryRegion *mr, AddressSpace *as)
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;
@@ -1139,7 +1139,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
                  basename);
         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
 
-        if ((!option_rom || mc->option_rom_has_mr) && mc->rom_file_has_mr) {
+        if ((option_rom_has_mr || mc->option_rom_has_mr) && mc->rom_file_has_mr) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, true);
         } else {
             data = rom->data;
@@ -1239,12 +1239,12 @@ int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
 
 ssize_t rom_add_vga(const char *file)
 {
-    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
+    return rom_add_file(file, "vgaroms", 0, -1, false, NULL, NULL);
 }
 
 ssize_t rom_add_option(const char *file, int32_t bootindex)
 {
-    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
+    return rom_add_file(file, "genroms", 0, bootindex, false, NULL, NULL);
 }
 
 static void rom_reset(void *unused)
-- 
2.41.0

Re: [PATCH v2 2/6] hw/loader: Clean up global variable shadowing in rom_add_file()

[Ani Sinha]

> On 10-Oct-2023, at 5:20 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Invert 'option_rom' logic and rename it as 'option_rom_has_mr' to fix:
> 
>  hw/core/loader.c:1073:27: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
>                       bool option_rom, MemoryRegion *mr,
>                            ^
>  include/sysemu/sysemu.h:57:22: note: previous declaration is here
>  extern QEMUOptionRom option_rom[MAX_OPTION_ROMS];
>                       ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Ani Sinha <anisinha@redhat.com>

> ---
> include/hw/loader.h | 11 ++++++-----
> hw/core/loader.c    | 10 +++++-----
> 2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index c4c14170ea..9dbb9f7fdb 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -272,7 +272,8 @@ void pstrcpy_targphys(const char *name,
> 
> ssize_t rom_add_file(const char *file, const char *fw_dir,
>                      hwaddr addr, int32_t bootindex,
> -                     bool option_rom, MemoryRegion *mr, AddressSpace *as);
> +                     bool option_rom_has_mr,
> +                     MemoryRegion *mr, AddressSpace *as);
> MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>                            size_t max_len, hwaddr addr,
>                            const char *fw_file_name,
> @@ -341,15 +342,15 @@ void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size);
> void hmp_info_roms(Monitor *mon, const QDict *qdict);
> 
> #define rom_add_file_fixed(_f, _a, _i)          \
> -    rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
> +    rom_add_file(_f, NULL, _a, _i, true, NULL, NULL)
> #define rom_add_blob_fixed(_f, _b, _l, _a)      \
>     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
> #define rom_add_file_mr(_f, _mr, _i)            \
> -    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
> +    rom_add_file(_f, NULL, 0, _i, true, _mr, NULL)
> #define rom_add_file_as(_f, _as, _i)            \
> -    rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
> +    rom_add_file(_f, NULL, 0, _i, true, NULL, _as)
> #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
> -    rom_add_file(_f, NULL, _a, _i, false, NULL, _as)
> +    rom_add_file(_f, NULL, _a, _i, true, NULL, _as)
> #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as)      \
>     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as, true)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 4dd5a71fb7..f0d20fd776 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1070,8 +1070,8 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name, bool ro)
> 
> ssize_t rom_add_file(const char *file, const char *fw_dir,
>                      hwaddr addr, int32_t bootindex,
> -                     bool option_rom, MemoryRegion *mr,
> -                     AddressSpace *as)
> +                     bool option_rom_has_mr,
> +                     MemoryRegion *mr, AddressSpace *as)
> {
>     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>     Rom *rom;
> @@ -1139,7 +1139,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
>                  basename);
>         snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
> 
> -        if ((!option_rom || mc->option_rom_has_mr) && mc->rom_file_has_mr) {
> +        if ((option_rom_has_mr || mc->option_rom_has_mr) && mc->rom_file_has_mr) {
>             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, true);
>         } else {
>             data = rom->data;
> @@ -1239,12 +1239,12 @@ int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
> 
> ssize_t rom_add_vga(const char *file)
> {
> -    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
> +    return rom_add_file(file, "vgaroms", 0, -1, false, NULL, NULL);
> }
> 
> ssize_t rom_add_option(const char *file, int32_t bootindex)
> {
> -    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
> +    return rom_add_file(file, "genroms", 0, bootindex, false, NULL, NULL);
> }
> 
> static void rom_reset(void *unused)
> -- 
> 2.41.0
> 

[PATCH v2 3/6] hw/display/vga: Clean up global variable shadowing

[Philippe Mathieu-Daudé]

Rename 'address_space' -> 'legacy_io' and
'address_space_io' -> 'io' to fix:

  hw/display/vga.c:2307:29: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
                MemoryRegion *address_space_io, bool init_vga_ports)
                            ^
  include/exec/address-spaces.h:35:21: note: previous declaration is here
  extern AddressSpace address_space_io;
                      ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/vga_int.h |  4 ++--
 hw/display/vga.c     | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 7cf0d11201..0f5db522c2 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -157,8 +157,8 @@ static inline int c6_to_8(int v)
 }
 
 bool vga_common_init(VGACommonState *s, Object *obj, Error **errp);
-void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
-              MemoryRegion *address_space_io, bool init_vga_ports);
+void vga_init(VGACommonState *s, Object *obj,
+              MemoryRegion *legacy_io, MemoryRegion *io, bool init_vga_ports);
 MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
                           const MemoryRegionPortio **vga_ports,
                           const MemoryRegionPortio **vbe_ports);
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 37557c3442..4dc6535ae5 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2303,8 +2303,8 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
     return vga_mem;
 }
 
-void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
-              MemoryRegion *address_space_io, bool init_vga_ports)
+void vga_init(VGACommonState *s, Object *obj,
+              MemoryRegion *legacy_io, MemoryRegion *io, bool init_vga_ports)
 {
     MemoryRegion *vga_io_memory;
     const MemoryRegionPortio *vga_ports, *vbe_ports;
@@ -2313,10 +2313,10 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
 
     s->bank_offset = 0;
 
-    s->legacy_address_space = address_space;
+    s->legacy_address_space = legacy_io;
 
     vga_io_memory = vga_init_io(s, obj, &vga_ports, &vbe_ports);
-    memory_region_add_subregion_overlap(address_space,
+    memory_region_add_subregion_overlap(legacy_io,
                                         0x000a0000,
                                         vga_io_memory,
                                         1);
@@ -2324,10 +2324,10 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
     if (init_vga_ports) {
         portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
         portio_list_set_flush_coalesced(&s->vga_port_list);
-        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
+        portio_list_add(&s->vga_port_list, io, 0x3b0);
     }
     if (vbe_ports) {
         portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
-        portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce);
+        portio_list_add(&s->vbe_port_list, io, 0x1ce);
     }
 }
-- 
2.41.0

Re: [PATCH v2 3/6] hw/display/vga: Clean up global variable shadowing

[Ani Sinha]

> On 10-Oct-2023, at 5:20 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> Rename 'address_space' -> 'legacy_io' and
> 'address_space_io' -> 'io' to fix:

This works but why not replace address_space with legacy_address_space instead?

> 
>  hw/display/vga.c:2307:29: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
>                MemoryRegion *address_space_io, bool init_vga_ports)
>                            ^
>  include/exec/address-spaces.h:35:21: note: previous declaration is here
>  extern AddressSpace address_space_io;
>                      ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/display/vga_int.h |  4 ++--
> hw/display/vga.c     | 12 ++++++------
> 2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
> index 7cf0d11201..0f5db522c2 100644
> --- a/hw/display/vga_int.h
> +++ b/hw/display/vga_int.h
> @@ -157,8 +157,8 @@ static inline int c6_to_8(int v)
> }
> 
> bool vga_common_init(VGACommonState *s, Object *obj, Error **errp);
> -void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
> -              MemoryRegion *address_space_io, bool init_vga_ports);
> +void vga_init(VGACommonState *s, Object *obj,
> +              MemoryRegion *legacy_io, MemoryRegion *io, bool init_vga_ports);
> MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
>                           const MemoryRegionPortio **vga_ports,
>                           const MemoryRegionPortio **vbe_ports);
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 37557c3442..4dc6535ae5 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2303,8 +2303,8 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
>     return vga_mem;
> }
> 
> -void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
> -              MemoryRegion *address_space_io, bool init_vga_ports)
> +void vga_init(VGACommonState *s, Object *obj,
> +              MemoryRegion *legacy_io, MemoryRegion *io, bool init_vga_ports)
> {
>     MemoryRegion *vga_io_memory;
>     const MemoryRegionPortio *vga_ports, *vbe_ports;
> @@ -2313,10 +2313,10 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
> 
>     s->bank_offset = 0;
> 
> -    s->legacy_address_space = address_space;
> +    s->legacy_address_space = legacy_io;
> 
>     vga_io_memory = vga_init_io(s, obj, &vga_ports, &vbe_ports);
> -    memory_region_add_subregion_overlap(address_space,
> +    memory_region_add_subregion_overlap(legacy_io,
>                                         0x000a0000,
>                                         vga_io_memory,
>                                         1);
> @@ -2324,10 +2324,10 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>     if (init_vga_ports) {
>         portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
>         portio_list_set_flush_coalesced(&s->vga_port_list);
> -        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
> +        portio_list_add(&s->vga_port_list, io, 0x3b0);
>     }
>     if (vbe_ports) {
>         portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
> -        portio_list_add(&s->vbe_port_list, address_space_io, 0x1ce);
> +        portio_list_add(&s->vbe_port_list, io, 0x1ce);
>     }
> }
> -- 
> 2.41.0
> 

Re: [PATCH v2 3/6] hw/display/vga: Clean up global variable shadowing

[Philippe Mathieu-Daudé]

On 10/10/23 14:46, Ani Sinha wrote:
> 
> 
>> On 10-Oct-2023, at 5:20 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Rename 'address_space' -> 'legacy_io' and
>> 'address_space_io' -> 'io' to fix:
> 
> This works but why not replace address_space with legacy_address_space instead?

I'd rather avoid using 'address_space' for MemoryRegion, since we
also have an AddressSpace type.

Besides, both regions are mapped in I/O space.

> 
>>
>>   hw/display/vga.c:2307:29: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
>>                 MemoryRegion *address_space_io, bool init_vga_ports)
>>                             ^
>>   include/exec/address-spaces.h:35:21: note: previous declaration is here
>>   extern AddressSpace address_space_io;
>>                       ^
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/display/vga_int.h |  4 ++--
>> hw/display/vga.c     | 12 ++++++------
>> 2 files changed, 8 insertions(+), 8 deletions(-)

Re: [PATCH v2 3/6] hw/display/vga: Clean up global variable shadowing

[Philippe Mathieu-Daudé]

On 10/10/23 14:50, Philippe Mathieu-Daudé wrote:
> On 10/10/23 14:46, Ani Sinha wrote:
>>
>>
>>> On 10-Oct-2023, at 5:20 PM, Philippe Mathieu-Daudé 
>>> <philmd@linaro.org> wrote:
>>>
>>> Rename 'address_space' -> 'legacy_io' and
>>> 'address_space_io' -> 'io' to fix:
>>
>> This works but why not replace address_space with legacy_address_space 
>> instead?
> 
> I'd rather avoid using 'address_space' for MemoryRegion, since we
> also have an AddressSpace type.
> 
> Besides, both regions are mapped in I/O space.

That said, I should clarify that in the patch description.

> 
>>
>>>
>>>   hw/display/vga.c:2307:29: error: declaration shadows a variable in 
>>> the global scope [-Werror,-Wshadow]
>>>                 MemoryRegion *address_space_io, bool init_vga_ports)
>>>                             ^
>>>   include/exec/address-spaces.h:35:21: note: previous declaration is 
>>> here
>>>   extern AddressSpace address_space_io;
>>>                       ^
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/display/vga_int.h |  4 ++--
>>> hw/display/vga.c     | 12 ++++++------
>>> 2 files changed, 8 insertions(+), 8 deletions(-)
> 

[PATCH v2 4/6] hw/acpi/pcihp: Clean up global variable shadowing in acpi_pcihp_init()

[Philippe Mathieu-Daudé]

Fix:

  hw/acpi/pcihp.c:499:36: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
                       MemoryRegion *address_space_io,
                                     ^
  include/exec/address-spaces.h:35:21: note: previous declaration is here
  extern AddressSpace address_space_io;
                      ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Ani Sinha <anisinha@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/pcihp.h | 2 +-
 hw/acpi/pcihp.c         | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index ef59810c17..ac21a95913 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -56,7 +56,7 @@ typedef struct AcpiPciHpState {
 } AcpiPciHpState;
 
 void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
-                     MemoryRegion *address_space_io, uint16_t io_base);
+                     MemoryRegion *io, uint16_t io_base);
 
 bool acpi_pcihp_is_hotpluggbale_bus(AcpiPciHpState *s, BusState *bus);
 void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index cdd6f775a1..4f75c873e2 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -496,8 +496,7 @@ static const MemoryRegionOps acpi_pcihp_io_ops = {
 };
 
 void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
-                     MemoryRegion *address_space_io,
-                     uint16_t io_base)
+                     MemoryRegion *io, uint16_t io_base)
 {
     s->io_len = ACPI_PCIHP_SIZE;
     s->io_base = io_base;
@@ -506,7 +505,7 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *s, PCIBus *root_bus,
 
     memory_region_init_io(&s->io, owner, &acpi_pcihp_io_ops, s,
                           "acpi-pci-hotplug", s->io_len);
-    memory_region_add_subregion(address_space_io, s->io_base, &s->io);
+    memory_region_add_subregion(io, s->io_base, &s->io);
 
     object_property_add_uint16_ptr(owner, ACPI_PCIHP_IO_BASE_PROP, &s->io_base,
                                    OBJ_PROP_FLAG_READ);
-- 
2.41.0

[PATCH v2 5/6] hw/pci: Clean up global variable shadowing of address_space_io variable

[Philippe Mathieu-Daudé]

Fix:

  hw/pci/pci.c:504:54: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
                                         MemoryRegion *address_space_io,
                                                       ^
  hw/pci/pci.c:533:38: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
                         MemoryRegion *address_space_io,
                                       ^
  hw/pci/pci.c:543:40: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
                           MemoryRegion *address_space_io,
                                         ^
  hw/pci/pci.c:590:45: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
                                MemoryRegion *address_space_io,
                                              ^
  include/exec/address-spaces.h:35:21: note: previous declaration is here
  extern AddressSpace address_space_io;
                      ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pci.h |  9 +++------
 hw/pci/pci.c         | 25 +++++++++----------------
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b70a0b95ff..ea5aff118b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -279,12 +279,10 @@ bool pci_bus_is_express(const PCIBus *bus);
 
 void pci_root_bus_init(PCIBus *bus, size_t bus_size, DeviceState *parent,
                        const char *name,
-                       MemoryRegion *address_space_mem,
-                       MemoryRegion *address_space_io,
+                       MemoryRegion *mem, MemoryRegion *io,
                        uint8_t devfn_min, const char *typename);
 PCIBus *pci_root_bus_new(DeviceState *parent, const char *name,
-                         MemoryRegion *address_space_mem,
-                         MemoryRegion *address_space_io,
+                         MemoryRegion *mem, MemoryRegion *io,
                          uint8_t devfn_min, const char *typename);
 void pci_root_bus_cleanup(PCIBus *bus);
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq,
@@ -304,8 +302,7 @@ int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
 PCIBus *pci_register_root_bus(DeviceState *parent, const char *name,
                               pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                               void *irq_opaque,
-                              MemoryRegion *address_space_mem,
-                              MemoryRegion *address_space_io,
+                              MemoryRegion *mem, MemoryRegion *io,
                               uint8_t devfn_min, int nirq,
                               const char *typename);
 void pci_unregister_root_bus(PCIBus *bus);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0d21bf43a..7d09e1a39d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -500,15 +500,14 @@ bool pci_bus_bypass_iommu(PCIBus *bus)
 }
 
 static void pci_root_bus_internal_init(PCIBus *bus, DeviceState *parent,
-                                       MemoryRegion *address_space_mem,
-                                       MemoryRegion *address_space_io,
+                                       MemoryRegion *mem, MemoryRegion *io,
                                        uint8_t devfn_min)
 {
     assert(PCI_FUNC(devfn_min) == 0);
     bus->devfn_min = devfn_min;
     bus->slot_reserved_mask = 0x0;
-    bus->address_space_mem = address_space_mem;
-    bus->address_space_io = address_space_io;
+    bus->address_space_mem = mem;
+    bus->address_space_io = io;
     bus->flags |= PCI_BUS_IS_ROOT;
 
     /* host bridge */
@@ -529,25 +528,21 @@ bool pci_bus_is_express(const PCIBus *bus)
 
 void pci_root_bus_init(PCIBus *bus, size_t bus_size, DeviceState *parent,
                        const char *name,
-                       MemoryRegion *address_space_mem,
-                       MemoryRegion *address_space_io,
+                       MemoryRegion *mem, MemoryRegion *io,
                        uint8_t devfn_min, const char *typename)
 {
     qbus_init(bus, bus_size, typename, parent, name);
-    pci_root_bus_internal_init(bus, parent, address_space_mem,
-                               address_space_io, devfn_min);
+    pci_root_bus_internal_init(bus, parent, mem, io, devfn_min);
 }
 
 PCIBus *pci_root_bus_new(DeviceState *parent, const char *name,
-                         MemoryRegion *address_space_mem,
-                         MemoryRegion *address_space_io,
+                         MemoryRegion *mem, MemoryRegion *io,
                          uint8_t devfn_min, const char *typename)
 {
     PCIBus *bus;
 
     bus = PCI_BUS(qbus_new(typename, parent, name));
-    pci_root_bus_internal_init(bus, parent, address_space_mem,
-                               address_space_io, devfn_min);
+    pci_root_bus_internal_init(bus, parent, mem, io, devfn_min);
     return bus;
 }
 
@@ -586,15 +581,13 @@ void pci_bus_irqs_cleanup(PCIBus *bus)
 PCIBus *pci_register_root_bus(DeviceState *parent, const char *name,
                               pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                               void *irq_opaque,
-                              MemoryRegion *address_space_mem,
-                              MemoryRegion *address_space_io,
+                              MemoryRegion *mem, MemoryRegion *io,
                               uint8_t devfn_min, int nirq,
                               const char *typename)
 {
     PCIBus *bus;
 
-    bus = pci_root_bus_new(parent, name, address_space_mem,
-                           address_space_io, devfn_min, typename);
+    bus = pci_root_bus_new(parent, name, mem, io, devfn_min, typename);
     pci_bus_irqs(bus, set_irq, irq_opaque, nirq);
     pci_bus_map_irqs(bus, map_irq);
     return bus;
-- 
2.41.0

[PATCH v2 6/6] hw/s390x: Clean up global variable shadowing in quiesce_powerdown_req()

[Philippe Mathieu-Daudé]

Fix:

  hw/s390x/sclpquiesce.c:90:22: error: declaration shadows a variable in the global scope [-Werror,-Wshadow]
      QuiesceNotifier *qn = container_of(n, QuiesceNotifier, notifier);
                       ^
  hw/s390x/sclpquiesce.c:86:3: note: previous declaration is here
  } qn;
    ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/sclpquiesce.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/sclpquiesce.c b/hw/s390x/sclpquiesce.c
index ce07b16884..a641089929 100644
--- a/hw/s390x/sclpquiesce.c
+++ b/hw/s390x/sclpquiesce.c
@@ -78,12 +78,10 @@ static const VMStateDescription vmstate_sclpquiesce = {
      }
 };
 
-typedef struct QuiesceNotifier QuiesceNotifier;
-
-static struct QuiesceNotifier {
+typedef struct QuiesceNotifier {
     Notifier notifier;
     SCLPEvent *event;
-} qn;
+} QuiesceNotifier;
 
 static void quiesce_powerdown_req(Notifier *n, void *opaque)
 {
@@ -97,6 +95,8 @@ static void quiesce_powerdown_req(Notifier *n, void *opaque)
 
 static int quiesce_init(SCLPEvent *event)
 {
+    static QuiesceNotifier qn;
+
     qn.notifier.notify = quiesce_powerdown_req;
     qn.event = event;
 
-- 
2.41.0