[PATCH 0/9] hw/riscv/virt.c: memmap usage cleanup

[PATCH 0/9] hw/riscv/virt.c: memmap usage cleanup

[Daniel Henrique Barboza]

I had this on my backlog and decided to get it out of the way.

We have a lot of 'memmap' uses in virt that aren't made via s->memmap.
In fact most of the accesses are done via the static array virt_memmap
directly. Some fdt functions are using it via an extra argument, which
is unneeded since we can access it directly or via s->memmap.

This current state of affairs will hurt us in the long run when we
decide to version the 'virt' board. We might have multiple memmaps for
each version and then we'll have to deal with all these static
references to a specific memmap.  Using s->memmap right now will make
our lives easier in the future.

Aside from a couple of patches the changes are rather trivial. Patch 9
is a change I decided to make after noticing the amount of 'long' casts
we have when formating a fdt string.


Patches based on alistair/riscv-to-apply.next.

Daniel Henrique Barboza (9):
  hw/riscv/virt.c: enforce s->memmap use in machine_init()
  hw/riscv/virt.c: remove trivial virt_memmap references
  hw/riscv/virt.c: use s->memmap in virt_machine_done()
  hw/riscv/virt.c: add 'base' arg in create_fw_cfg()
  hw/riscv/virt.c: use s->memmap in create_fdt() path
  hw/riscv/virt.c: use s->memmap in create_fdt_sockets() path
  hw/riscv/virt.c: use s->memmap in create_fdt_virtio()
  hw/riscv/virt.c: use s->memmap in finalize_fdt() functions
  hw/riscv/virt.c: remove 'long' casts in fmt strings

 hw/riscv/virt.c | 269 +++++++++++++++++++++++++-----------------------
 1 file changed, 138 insertions(+), 131 deletions(-)

-- 
2.49.0

[PATCH 1/9] hw/riscv/virt.c: enforce s->memmap use in machine_init()

[Daniel Henrique Barboza]

Throughout the code we're accessing the board memmap, most of the time,
by accessing it statically via 'virt_memmap'. This static map is also
assigned in the machine state in s->memmap.

We're also passing it as a variable to some fdt functions, which is
unorthodox since we can spare a function argument by accessing it
statically or via the machine state.

All the current forms are valid but not all of the are scalable. In the
future we will version this board, and then all this code will need
rework because it should point to the updated memmap. In this case,
we'll want to assign the adequate versioned memmap once during init,
in s->memmap like it is being done today, and the rest of the code
will access the updated map via s->memmap.

We're also enforcing the pattern of using s->memmap instead of assigning
it to a temp variable 'memmap'. Code is copy/pasted around all the time
and being consistent is important.

We'll start these rather mechanical changes with virt_machine_init().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 54 ++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index c9d255d8a8..6e3e34879f 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1527,7 +1527,6 @@ static void virt_machine_done(Notifier *notifier, void *data)
 
 static void virt_machine_init(MachineState *machine)
 {
-    const MemMapEntry *memmap = virt_memmap;
     RISCVVirtState *s = RISCV_VIRT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
@@ -1535,6 +1534,8 @@ static void virt_machine_init(MachineState *machine)
     int i, base_hartid, hart_count;
     int socket_count = riscv_socket_count(machine);
 
+    s->memmap = virt_memmap;
+
     /* Check socket count limit */
     if (VIRT_SOCKETS_MAX < socket_count) {
         error_report("number of sockets/nodes should be less than %d",
@@ -1582,7 +1583,7 @@ static void virt_machine_init(MachineState *machine)
         if (virt_aclint_allowed() && s->have_aclint) {
             if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
                 /* Per-socket ACLINT MTIMER */
-                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
+                riscv_aclint_mtimer_create(s->memmap[VIRT_CLINT].base +
                             i * RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
                         RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
                         base_hartid, hart_count,
@@ -1591,28 +1592,28 @@ static void virt_machine_init(MachineState *machine)
                         RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
             } else {
                 /* Per-socket ACLINT MSWI, MTIMER, and SSWI */
-                riscv_aclint_swi_create(memmap[VIRT_CLINT].base +
-                            i * memmap[VIRT_CLINT].size,
+                riscv_aclint_swi_create(s->memmap[VIRT_CLINT].base +
+                            i * s->memmap[VIRT_CLINT].size,
                         base_hartid, hart_count, false);
-                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
-                            i * memmap[VIRT_CLINT].size +
+                riscv_aclint_mtimer_create(s->memmap[VIRT_CLINT].base +
+                            i * s->memmap[VIRT_CLINT].size +
                             RISCV_ACLINT_SWI_SIZE,
                         RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
                         base_hartid, hart_count,
                         RISCV_ACLINT_DEFAULT_MTIMECMP,
                         RISCV_ACLINT_DEFAULT_MTIME,
                         RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
-                riscv_aclint_swi_create(memmap[VIRT_ACLINT_SSWI].base +
-                            i * memmap[VIRT_ACLINT_SSWI].size,
+                riscv_aclint_swi_create(s->memmap[VIRT_ACLINT_SSWI].base +
+                            i * s->memmap[VIRT_ACLINT_SSWI].size,
                         base_hartid, hart_count, true);
             }
         } else if (tcg_enabled()) {
             /* Per-socket SiFive CLINT */
             riscv_aclint_swi_create(
-                    memmap[VIRT_CLINT].base + i * memmap[VIRT_CLINT].size,
+                    s->memmap[VIRT_CLINT].base + i * s->memmap[VIRT_CLINT].size,
                     base_hartid, hart_count, false);
-            riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
-                        i * memmap[VIRT_CLINT].size + RISCV_ACLINT_SWI_SIZE,
+            riscv_aclint_mtimer_create(s->memmap[VIRT_CLINT].base +
+                    i * s->memmap[VIRT_CLINT].size + RISCV_ACLINT_SWI_SIZE,
                     RISCV_ACLINT_DEFAULT_MTIMER_SIZE, base_hartid, hart_count,
                     RISCV_ACLINT_DEFAULT_MTIMECMP, RISCV_ACLINT_DEFAULT_MTIME,
                     RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
@@ -1620,11 +1621,11 @@ static void virt_machine_init(MachineState *machine)
 
         /* Per-socket interrupt controller */
         if (s->aia_type == VIRT_AIA_TYPE_NONE) {
-            s->irqchip[i] = virt_create_plic(memmap, i,
+            s->irqchip[i] = virt_create_plic(s->memmap, i,
                                              base_hartid, hart_count);
         } else {
             s->irqchip[i] = virt_create_aia(s->aia_type, s->aia_guests,
-                                            memmap, i, base_hartid,
+                                            s->memmap, i, base_hartid,
                                             hart_count);
         }
 
@@ -1646,8 +1647,8 @@ static void virt_machine_init(MachineState *machine)
     if (kvm_enabled() && virt_use_kvm_aia_aplic_imsic(s->aia_type)) {
         kvm_riscv_aia_create(machine, IMSIC_MMIO_GROUP_MIN_SHIFT,
                              VIRT_IRQCHIP_NUM_SOURCES, VIRT_IRQCHIP_NUM_MSIS,
-                             memmap[VIRT_APLIC_S].base,
-                             memmap[VIRT_IMSIC_S].base,
+                             s->memmap[VIRT_APLIC_S].base,
+                             s->memmap[VIRT_IMSIC_S].base,
                              s->aia_guests);
     }
 
@@ -1663,21 +1664,20 @@ static void virt_machine_init(MachineState *machine)
         virt_high_pcie_memmap.size = VIRT32_HIGH_PCIE_MMIO_SIZE;
     } else {
         virt_high_pcie_memmap.size = VIRT64_HIGH_PCIE_MMIO_SIZE;
-        virt_high_pcie_memmap.base = memmap[VIRT_DRAM].base + machine->ram_size;
+        virt_high_pcie_memmap.base = s->memmap[VIRT_DRAM].base +
+                                     machine->ram_size;
         virt_high_pcie_memmap.base =
             ROUND_UP(virt_high_pcie_memmap.base, virt_high_pcie_memmap.size);
     }
 
-    s->memmap = virt_memmap;
-
     /* register system main memory (actual RAM) */
-    memory_region_add_subregion(system_memory, memmap[VIRT_DRAM].base,
-        machine->ram);
+    memory_region_add_subregion(system_memory, s->memmap[VIRT_DRAM].base,
+                                machine->ram);
 
     /* boot rom */
     memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
-                           memmap[VIRT_MROM].size, &error_fatal);
-    memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
+                           s->memmap[VIRT_MROM].size, &error_fatal);
+    memory_region_add_subregion(system_memory, s->memmap[VIRT_MROM].base,
                                 mask_rom);
 
     /*
@@ -1688,12 +1688,12 @@ static void virt_machine_init(MachineState *machine)
     rom_set_fw(s->fw_cfg);
 
     /* SiFive Test MMIO device */
-    sifive_test_create(memmap[VIRT_TEST].base);
+    sifive_test_create(s->memmap[VIRT_TEST].base);
 
     /* VirtIO MMIO devices */
     for (i = 0; i < VIRTIO_COUNT; i++) {
         sysbus_create_simple("virtio-mmio",
-            memmap[VIRT_VIRTIO].base + i * memmap[VIRT_VIRTIO].size,
+            s->memmap[VIRT_VIRTIO].base + i * s->memmap[VIRT_VIRTIO].size,
             qdev_get_gpio_in(virtio_irqchip, VIRTIO_IRQ + i));
     }
 
@@ -1701,11 +1701,11 @@ static void virt_machine_init(MachineState *machine)
 
     create_platform_bus(s, mmio_irqchip);
 
-    serial_mm_init(system_memory, memmap[VIRT_UART0].base,
+    serial_mm_init(system_memory, s->memmap[VIRT_UART0].base,
         0, qdev_get_gpio_in(mmio_irqchip, UART0_IRQ), 399193,
         serial_hd(0), DEVICE_LITTLE_ENDIAN);
 
-    sysbus_create_simple("goldfish_rtc", memmap[VIRT_RTC].base,
+    sysbus_create_simple("goldfish_rtc", s->memmap[VIRT_RTC].base,
         qdev_get_gpio_in(mmio_irqchip, RTC_IRQ));
 
     for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
@@ -1723,7 +1723,7 @@ static void virt_machine_init(MachineState *machine)
             exit(1);
         }
     } else {
-        create_fdt(s, memmap);
+        create_fdt(s, s->memmap);
     }
 
     if (virt_is_iommu_sys_enabled(s)) {
-- 
2.49.0

[PATCH 2/9] hw/riscv/virt.c: remove trivial virt_memmap references

[Daniel Henrique Barboza]

We should use s->memmap instead of virt_memmap to be able to use an
updated memmap when we start versioning the board.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 6e3e34879f..fdf0318e15 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -166,8 +166,8 @@ static void virt_flash_map1(PFlashCFI01 *flash,
 static void virt_flash_map(RISCVVirtState *s,
                            MemoryRegion *sysmem)
 {
-    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
-    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
+    hwaddr flashsize = s->memmap[VIRT_FLASH].size / 2;
+    hwaddr flashbase = s->memmap[VIRT_FLASH].base;
 
     virt_flash_map1(s->flash[0], flashbase, flashsize,
                     sysmem);
@@ -999,8 +999,8 @@ static void create_fdt_rtc(RISCVVirtState *s, const MemMapEntry *memmap,
 static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
 {
     MachineState *ms = MACHINE(s);
-    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
-    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
+    hwaddr flashsize = s->memmap[VIRT_FLASH].size / 2;
+    hwaddr flashbase = s->memmap[VIRT_FLASH].base;
     g_autofree char *name = g_strdup_printf("/flash@%" PRIx64, flashbase);
 
     qemu_fdt_add_subnode(ms->fdt, name);
@@ -1035,7 +1035,7 @@ static void create_fdt_virtio_iommu(RISCVVirtState *s, uint16_t bdf)
     g_autofree char *pci_node = NULL;
 
     pci_node = g_strdup_printf("/soc/pci@%lx",
-                               (long) virt_memmap[VIRT_PCIE_ECAM].base);
+                               (long) s->memmap[VIRT_PCIE_ECAM].base);
     iommu_node = g_strdup_printf("%s/virtio_iommu@%x,%x", pci_node,
                                  PCI_SLOT(bdf), PCI_FUNC(bdf));
     iommu_phandle = qemu_fdt_alloc_phandle(fdt);
@@ -1104,7 +1104,7 @@ static void create_fdt_iommu(RISCVVirtState *s, uint16_t bdf)
     g_autofree char *pci_node = NULL;
 
     pci_node = g_strdup_printf("/soc/pci@%lx",
-                               (long) virt_memmap[VIRT_PCIE_ECAM].base);
+                               (long) s->memmap[VIRT_PCIE_ECAM].base);
     iommu_node = g_strdup_printf("%s/iommu@%x", pci_node, bdf);
     iommu_phandle = qemu_fdt_alloc_phandle(fdt);
     qemu_fdt_add_subnode(fdt, iommu_node);
@@ -1126,24 +1126,24 @@ static void finalize_fdt(RISCVVirtState *s)
     uint32_t irq_pcie_phandle = 1, irq_virtio_phandle = 1;
     uint32_t iommu_sys_phandle = 1;
 
-    create_fdt_sockets(s, virt_memmap, &phandle, &irq_mmio_phandle,
+    create_fdt_sockets(s, s->memmap, &phandle, &irq_mmio_phandle,
                        &irq_pcie_phandle, &irq_virtio_phandle,
                        &msi_pcie_phandle);
 
-    create_fdt_virtio(s, virt_memmap, irq_virtio_phandle);
+    create_fdt_virtio(s, s->memmap, irq_virtio_phandle);
 
     if (virt_is_iommu_sys_enabled(s)) {
         create_fdt_iommu_sys(s, irq_mmio_phandle, msi_pcie_phandle,
                              &iommu_sys_phandle);
     }
-    create_fdt_pcie(s, virt_memmap, irq_pcie_phandle, msi_pcie_phandle,
+    create_fdt_pcie(s, s->memmap, irq_pcie_phandle, msi_pcie_phandle,
                     iommu_sys_phandle);
 
-    create_fdt_reset(s, virt_memmap, &phandle);
+    create_fdt_reset(s, s->memmap, &phandle);
 
-    create_fdt_uart(s, virt_memmap, irq_mmio_phandle);
+    create_fdt_uart(s, s->memmap, irq_mmio_phandle);
 
-    create_fdt_rtc(s, virt_memmap, irq_mmio_phandle);
+    create_fdt_rtc(s, s->memmap, irq_mmio_phandle);
 }
 
 static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
@@ -1366,14 +1366,13 @@ static void create_platform_bus(RISCVVirtState *s, DeviceState *irqchip)
 {
     DeviceState *dev;
     SysBusDevice *sysbus;
-    const MemMapEntry *memmap = virt_memmap;
     int i;
     MemoryRegion *sysmem = get_system_memory();
 
     dev = qdev_new(TYPE_PLATFORM_BUS_DEVICE);
     dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE);
     qdev_prop_set_uint32(dev, "num_irqs", VIRT_PLATFORM_BUS_NUM_IRQS);
-    qdev_prop_set_uint32(dev, "mmio_size", memmap[VIRT_PLATFORM_BUS].size);
+    qdev_prop_set_uint32(dev, "mmio_size", s->memmap[VIRT_PLATFORM_BUS].size);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     s->platform_bus_dev = dev;
 
@@ -1384,7 +1383,7 @@ static void create_platform_bus(RISCVVirtState *s, DeviceState *irqchip)
     }
 
     memory_region_add_subregion(sysmem,
-                                memmap[VIRT_PLATFORM_BUS].base,
+                                s->memmap[VIRT_PLATFORM_BUS].base,
                                 sysbus_mmio_get_region(sysbus, 0));
 }
 
-- 
2.49.0

[PATCH 3/9] hw/riscv/virt.c: use s->memmap in virt_machine_done()

[Daniel Henrique Barboza]

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index fdf0318e15..473416bcf3 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1430,9 +1430,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
 {
     RISCVVirtState *s = container_of(notifier, RISCVVirtState,
                                      machine_done);
-    const MemMapEntry *memmap = virt_memmap;
     MachineState *machine = MACHINE(s);
-    hwaddr start_addr = memmap[VIRT_DRAM].base;
+    hwaddr start_addr = s->memmap[VIRT_DRAM].base;
     target_ulong firmware_end_addr, kernel_start_addr;
     const char *firmware_name = riscv_default_firmware_name(&s->soc[0]);
     uint64_t fdt_load_addr;
@@ -1476,14 +1475,14 @@ static void virt_machine_done(Notifier *notifier, void *data)
              * let's overwrite the address we jump to after reset to
              * the base of the flash.
              */
-            start_addr = virt_memmap[VIRT_FLASH].base;
+            start_addr = s->memmap[VIRT_FLASH].base;
         } else {
             /*
              * Pflash was supplied but either KVM guest or bios is not none.
              * In this case, base of the flash would contain S-mode payload.
              */
             riscv_setup_firmware_boot(machine);
-            kernel_entry = virt_memmap[VIRT_FLASH].base;
+            kernel_entry = s->memmap[VIRT_FLASH].base;
         }
     }
 
@@ -1497,15 +1496,15 @@ static void virt_machine_done(Notifier *notifier, void *data)
         kernel_entry = boot_info.image_low_addr;
     }
 
-    fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
-                                           memmap[VIRT_DRAM].size,
+    fdt_load_addr = riscv_compute_fdt_addr(s->memmap[VIRT_DRAM].base,
+                                           s->memmap[VIRT_DRAM].size,
                                            machine, &boot_info);
     riscv_load_fdt(fdt_load_addr, machine->fdt);
 
     /* load the reset vector */
     riscv_setup_rom_reset_vec(machine, &s->soc[0], start_addr,
-                              virt_memmap[VIRT_MROM].base,
-                              virt_memmap[VIRT_MROM].size, kernel_entry,
+                              s->memmap[VIRT_MROM].base,
+                              s->memmap[VIRT_MROM].size, kernel_entry,
                               fdt_load_addr);
 
     /*
-- 
2.49.0

[PATCH 4/9] hw/riscv/virt.c: add 'base' arg in create_fw_cfg()

[Daniel Henrique Barboza]

The function can receive the value via s->memmap[VIRT_FW_CFG].base from
the caller, avoiding the use of virt_memmap.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 473416bcf3..77ebb517f2 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1262,9 +1262,8 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion *sys_mem,
     return dev;
 }
 
-static FWCfgState *create_fw_cfg(const MachineState *ms)
+static FWCfgState *create_fw_cfg(const MachineState *ms, hwaddr base)
 {
-    hwaddr base = virt_memmap[VIRT_FW_CFG].base;
     FWCfgState *fw_cfg;
 
     fw_cfg = fw_cfg_init_mem_wide(base + 8, base, 8, base + 16,
@@ -1682,7 +1681,7 @@ static void virt_machine_init(MachineState *machine)
      * Init fw_cfg. Must be done before riscv_load_fdt, otherwise the
      * device tree cannot be altered and we get FDT_ERR_NOSPACE.
      */
-    s->fw_cfg = create_fw_cfg(machine);
+    s->fw_cfg = create_fw_cfg(machine, s->memmap[VIRT_FW_CFG].base);
     rom_set_fw(s->fw_cfg);
 
     /* SiFive Test MMIO device */
-- 
2.49.0

[PATCH 5/9] hw/riscv/virt.c: use s->memmap in create_fdt() path

[Daniel Henrique Barboza]

create_fdt(), create_fdt_flash() and create_fdt_fw_cfg() can access the
memmap via their RISCVVirtState pointers.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 77ebb517f2..02f3659d38 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -996,7 +996,7 @@ static void create_fdt_rtc(RISCVVirtState *s, const MemMapEntry *memmap,
     }
 }
 
-static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
+static void create_fdt_flash(RISCVVirtState *s)
 {
     MachineState *ms = MACHINE(s);
     hwaddr flashsize = s->memmap[VIRT_FLASH].size / 2;
@@ -1011,11 +1011,11 @@ static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
     qemu_fdt_setprop_cell(ms->fdt, name, "bank-width", 4);
 }
 
-static void create_fdt_fw_cfg(RISCVVirtState *s, const MemMapEntry *memmap)
+static void create_fdt_fw_cfg(RISCVVirtState *s)
 {
     MachineState *ms = MACHINE(s);
-    hwaddr base = memmap[VIRT_FW_CFG].base;
-    hwaddr size = memmap[VIRT_FW_CFG].size;
+    hwaddr base = s->memmap[VIRT_FW_CFG].base;
+    hwaddr size = s->memmap[VIRT_FW_CFG].size;
     g_autofree char *nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
 
     qemu_fdt_add_subnode(ms->fdt, nodename);
@@ -1146,7 +1146,7 @@ static void finalize_fdt(RISCVVirtState *s)
     create_fdt_rtc(s, s->memmap, irq_mmio_phandle);
 }
 
-static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
+static void create_fdt(RISCVVirtState *s)
 {
     MachineState *ms = MACHINE(s);
     uint8_t rng_seed[32];
@@ -1173,7 +1173,8 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
      * The "/soc/pci@..." node is needed for PCIE hotplugs
      * that might happen before finalize_fdt().
      */
-    name = g_strdup_printf("/soc/pci@%lx", (long) memmap[VIRT_PCIE_ECAM].base);
+    name = g_strdup_printf("/soc/pci@%lx",
+                           (long) s->memmap[VIRT_PCIE_ECAM].base);
     qemu_fdt_add_subnode(ms->fdt, name);
 
     qemu_fdt_add_subnode(ms->fdt, "/chosen");
@@ -1185,8 +1186,8 @@ static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
 
     qemu_fdt_add_subnode(ms->fdt, "/aliases");
 
-    create_fdt_flash(s, memmap);
-    create_fdt_fw_cfg(s, memmap);
+    create_fdt_flash(s);
+    create_fdt_fw_cfg(s);
     create_fdt_pmu(s);
 }
 
@@ -1720,7 +1721,7 @@ static void virt_machine_init(MachineState *machine)
             exit(1);
         }
     } else {
-        create_fdt(s, s->memmap);
+        create_fdt(s);
     }
 
     if (virt_is_iommu_sys_enabled(s)) {
-- 
2.49.0

[PATCH 6/9] hw/riscv/virt.c: use s->memmap in create_fdt_sockets() path

[Daniel Henrique Barboza]

create_fdt_sockets() and all its fdt helpers (create_fdt_socket_aplic(),
create_fdt_imsic(), create_fdt_socket_plic(), create_fdt_socket_aclint()
and create_fdt_socket_memory()) can use s->memmap from their
RISCVVirtState pointer instead of having an extra memmap argument.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 89 ++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 02f3659d38..dcaf28d1a4 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -301,14 +301,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
     }
 }
 
-static void create_fdt_socket_memory(RISCVVirtState *s,
-                                     const MemMapEntry *memmap, int socket)
+static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
 {
     g_autofree char *mem_name = NULL;
     uint64_t addr, size;
     MachineState *ms = MACHINE(s);
 
-    addr = memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
+    addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
     size = riscv_socket_mem_size(ms, socket);
     mem_name = g_strdup_printf("/memory@%lx", (long)addr);
     qemu_fdt_add_subnode(ms->fdt, mem_name);
@@ -319,7 +318,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s,
 }
 
 static void create_fdt_socket_clint(RISCVVirtState *s,
-                                    const MemMapEntry *memmap, int socket,
+                                    int socket,
                                     uint32_t *intc_phandles)
 {
     int cpu;
@@ -340,21 +339,22 @@ static void create_fdt_socket_clint(RISCVVirtState *s,
         clint_cells[cpu * 4 + 3] = cpu_to_be32(IRQ_M_TIMER);
     }
 
-    clint_addr = memmap[VIRT_CLINT].base + (memmap[VIRT_CLINT].size * socket);
+    clint_addr = s->memmap[VIRT_CLINT].base +
+                 (s->memmap[VIRT_CLINT].size * socket);
     clint_name = g_strdup_printf("/soc/clint@%lx", clint_addr);
     qemu_fdt_add_subnode(ms->fdt, clint_name);
     qemu_fdt_setprop_string_array(ms->fdt, clint_name, "compatible",
                                   (char **)&clint_compat,
                                   ARRAY_SIZE(clint_compat));
     qemu_fdt_setprop_cells(ms->fdt, clint_name, "reg",
-        0x0, clint_addr, 0x0, memmap[VIRT_CLINT].size);
+        0x0, clint_addr, 0x0, s->memmap[VIRT_CLINT].size);
     qemu_fdt_setprop(ms->fdt, clint_name, "interrupts-extended",
         clint_cells, s->soc[socket].num_harts * sizeof(uint32_t) * 4);
     riscv_socket_fdt_write_id(ms, clint_name, socket);
 }
 
 static void create_fdt_socket_aclint(RISCVVirtState *s,
-                                     const MemMapEntry *memmap, int socket,
+                                     int socket,
                                      uint32_t *intc_phandles)
 {
     int cpu;
@@ -381,8 +381,10 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
     aclint_cells_size = s->soc[socket].num_harts * sizeof(uint32_t) * 2;
 
     if (s->aia_type != VIRT_AIA_TYPE_APLIC_IMSIC) {
-        addr = memmap[VIRT_CLINT].base + (memmap[VIRT_CLINT].size * socket);
+        addr = s->memmap[VIRT_CLINT].base +
+               (s->memmap[VIRT_CLINT].size * socket);
         name = g_strdup_printf("/soc/mswi@%lx", addr);
+
         qemu_fdt_add_subnode(ms->fdt, name);
         qemu_fdt_setprop_string(ms->fdt, name, "compatible",
             "riscv,aclint-mswi");
@@ -397,13 +399,13 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
     }
 
     if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
-        addr = memmap[VIRT_CLINT].base +
+        addr = s->memmap[VIRT_CLINT].base +
                (RISCV_ACLINT_DEFAULT_MTIMER_SIZE * socket);
         size = RISCV_ACLINT_DEFAULT_MTIMER_SIZE;
     } else {
-        addr = memmap[VIRT_CLINT].base + RISCV_ACLINT_SWI_SIZE +
-            (memmap[VIRT_CLINT].size * socket);
-        size = memmap[VIRT_CLINT].size - RISCV_ACLINT_SWI_SIZE;
+        addr = s->memmap[VIRT_CLINT].base + RISCV_ACLINT_SWI_SIZE +
+               (s->memmap[VIRT_CLINT].size * socket);
+        size = s->memmap[VIRT_CLINT].size - RISCV_ACLINT_SWI_SIZE;
     }
     name = g_strdup_printf("/soc/mtimer@%lx", addr);
     qemu_fdt_add_subnode(ms->fdt, name);
@@ -420,14 +422,15 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
     g_free(name);
 
     if (s->aia_type != VIRT_AIA_TYPE_APLIC_IMSIC) {
-        addr = memmap[VIRT_ACLINT_SSWI].base +
-            (memmap[VIRT_ACLINT_SSWI].size * socket);
+        addr = s->memmap[VIRT_ACLINT_SSWI].base +
+               (s->memmap[VIRT_ACLINT_SSWI].size * socket);
+
         name = g_strdup_printf("/soc/sswi@%lx", addr);
         qemu_fdt_add_subnode(ms->fdt, name);
         qemu_fdt_setprop_string(ms->fdt, name, "compatible",
             "riscv,aclint-sswi");
         qemu_fdt_setprop_cells(ms->fdt, name, "reg",
-            0x0, addr, 0x0, memmap[VIRT_ACLINT_SSWI].size);
+            0x0, addr, 0x0, s->memmap[VIRT_ACLINT_SSWI].size);
         qemu_fdt_setprop(ms->fdt, name, "interrupts-extended",
             aclint_sswi_cells, aclint_cells_size);
         qemu_fdt_setprop(ms->fdt, name, "interrupt-controller", NULL, 0);
@@ -438,7 +441,7 @@ static void create_fdt_socket_aclint(RISCVVirtState *s,
 }
 
 static void create_fdt_socket_plic(RISCVVirtState *s,
-                                   const MemMapEntry *memmap, int socket,
+                                   int socket,
                                    uint32_t *phandle, uint32_t *intc_phandles,
                                    uint32_t *plic_phandles)
 {
@@ -452,7 +455,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
     };
 
     plic_phandles[socket] = (*phandle)++;
-    plic_addr = memmap[VIRT_PLIC].base + (memmap[VIRT_PLIC].size * socket);
+    plic_addr = s->memmap[VIRT_PLIC].base +
+                (s->memmap[VIRT_PLIC].size * socket);
     plic_name = g_strdup_printf("/soc/plic@%lx", plic_addr);
     qemu_fdt_add_subnode(ms->fdt, plic_name);
     qemu_fdt_setprop_cell(ms->fdt, plic_name,
@@ -491,7 +495,7 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
     }
 
     qemu_fdt_setprop_cells(ms->fdt, plic_name, "reg",
-        0x0, plic_addr, 0x0, memmap[VIRT_PLIC].size);
+        0x0, plic_addr, 0x0, s->memmap[VIRT_PLIC].size);
     qemu_fdt_setprop_cell(ms->fdt, plic_name, "riscv,ndev",
                           VIRT_IRQCHIP_NUM_SOURCES - 1);
     riscv_socket_fdt_write_id(ms, plic_name, socket);
@@ -500,8 +504,8 @@ static void create_fdt_socket_plic(RISCVVirtState *s,
 
     if (!socket) {
         platform_bus_add_all_fdt_nodes(ms->fdt, plic_name,
-                                       memmap[VIRT_PLATFORM_BUS].base,
-                                       memmap[VIRT_PLATFORM_BUS].size,
+                                       s->memmap[VIRT_PLATFORM_BUS].base,
+                                       s->memmap[VIRT_PLATFORM_BUS].size,
                                        VIRT_PLATFORM_BUS_IRQ);
     }
 }
@@ -588,7 +592,7 @@ static void create_fdt_one_imsic(RISCVVirtState *s, hwaddr base_addr,
     qemu_fdt_setprop_cell(ms->fdt, imsic_name, "phandle", msi_phandle);
 }
 
-static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
+static void create_fdt_imsic(RISCVVirtState *s,
                              uint32_t *phandle, uint32_t *intc_phandles,
                              uint32_t *msi_m_phandle, uint32_t *msi_s_phandle)
 {
@@ -597,12 +601,12 @@ static void create_fdt_imsic(RISCVVirtState *s, const MemMapEntry *memmap,
 
     if (!kvm_enabled()) {
         /* M-level IMSIC node */
-        create_fdt_one_imsic(s, memmap[VIRT_IMSIC_M].base, intc_phandles,
+        create_fdt_one_imsic(s, s->memmap[VIRT_IMSIC_M].base, intc_phandles,
                              *msi_m_phandle, true, 0);
     }
 
     /* S-level IMSIC node */
-    create_fdt_one_imsic(s, memmap[VIRT_IMSIC_S].base, intc_phandles,
+    create_fdt_one_imsic(s, s->memmap[VIRT_IMSIC_S].base, intc_phandles,
                          *msi_s_phandle, false,
                          imsic_num_bits(s->aia_guests + 1));
 
@@ -679,7 +683,7 @@ static void create_fdt_one_aplic(RISCVVirtState *s, int socket,
 }
 
 static void create_fdt_socket_aplic(RISCVVirtState *s,
-                                    const MemMapEntry *memmap, int socket,
+                                    int socket,
                                     uint32_t msi_m_phandle,
                                     uint32_t msi_s_phandle,
                                     uint32_t *phandle,
@@ -696,18 +700,19 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
 
     if (!kvm_enabled()) {
         /* M-level APLIC node */
-        aplic_addr = memmap[VIRT_APLIC_M].base +
-                     (memmap[VIRT_APLIC_M].size * socket);
-        create_fdt_one_aplic(s, socket, aplic_addr, memmap[VIRT_APLIC_M].size,
+        aplic_addr = s->memmap[VIRT_APLIC_M].base +
+                     (s->memmap[VIRT_APLIC_M].size * socket);
+        create_fdt_one_aplic(s, socket, aplic_addr,
+                             s->memmap[VIRT_APLIC_M].size,
                              msi_m_phandle, intc_phandles,
                              aplic_m_phandle, aplic_s_phandle,
                              true, num_harts);
     }
 
     /* S-level APLIC node */
-    aplic_addr = memmap[VIRT_APLIC_S].base +
-                 (memmap[VIRT_APLIC_S].size * socket);
-    create_fdt_one_aplic(s, socket, aplic_addr, memmap[VIRT_APLIC_S].size,
+    aplic_addr = s->memmap[VIRT_APLIC_S].base +
+                 (s->memmap[VIRT_APLIC_S].size * socket);
+    create_fdt_one_aplic(s, socket, aplic_addr, s->memmap[VIRT_APLIC_S].size,
                          msi_s_phandle, intc_phandles,
                          aplic_s_phandle, 0,
                          false, num_harts);
@@ -715,8 +720,8 @@ static void create_fdt_socket_aplic(RISCVVirtState *s,
     if (!socket) {
         g_autofree char *aplic_name = fdt_get_aplic_nodename(aplic_addr);
         platform_bus_add_all_fdt_nodes(ms->fdt, aplic_name,
-                                       memmap[VIRT_PLATFORM_BUS].base,
-                                       memmap[VIRT_PLATFORM_BUS].size,
+                                       s->memmap[VIRT_PLATFORM_BUS].base,
+                                       s->memmap[VIRT_PLATFORM_BUS].size,
                                        VIRT_PLATFORM_BUS_IRQ);
     }
 
@@ -734,7 +739,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
     riscv_pmu_generate_fdt_node(ms->fdt, hart.pmu_avail_ctrs, pmu_name);
 }
 
-static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
+static void create_fdt_sockets(RISCVVirtState *s,
                                uint32_t *phandle,
                                uint32_t *irq_mmio_phandle,
                                uint32_t *irq_pcie_phandle,
@@ -770,20 +775,20 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
         create_fdt_socket_cpus(s, socket, clust_name, phandle,
                                &intc_phandles[phandle_pos]);
 
-        create_fdt_socket_memory(s, memmap, socket);
+        create_fdt_socket_memory(s, socket);
 
         if (virt_aclint_allowed() && s->have_aclint) {
-            create_fdt_socket_aclint(s, memmap, socket,
+            create_fdt_socket_aclint(s, socket,
                                      &intc_phandles[phandle_pos]);
         } else if (tcg_enabled()) {
-            create_fdt_socket_clint(s, memmap, socket,
+            create_fdt_socket_clint(s, socket,
                                     &intc_phandles[phandle_pos]);
         }
     }
 
     if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
-        create_fdt_imsic(s, memmap, phandle, intc_phandles,
-            &msi_m_phandle, &msi_s_phandle);
+        create_fdt_imsic(s, phandle, intc_phandles,
+                         &msi_m_phandle, &msi_s_phandle);
         *msi_pcie_phandle = msi_s_phandle;
     }
 
@@ -792,7 +797,7 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
      * mode, we'll use only one APLIC instance.
      */
     if (!virt_use_emulated_aplic(s->aia_type)) {
-        create_fdt_socket_aplic(s, memmap, 0,
+        create_fdt_socket_aplic(s, 0,
                                 msi_m_phandle, msi_s_phandle, phandle,
                                 &intc_phandles[0], xplic_phandles,
                                 ms->smp.cpus);
@@ -806,11 +811,11 @@ static void create_fdt_sockets(RISCVVirtState *s, const MemMapEntry *memmap,
             phandle_pos -= s->soc[socket].num_harts;
 
             if (s->aia_type == VIRT_AIA_TYPE_NONE) {
-                create_fdt_socket_plic(s, memmap, socket, phandle,
+                create_fdt_socket_plic(s, socket, phandle,
                                        &intc_phandles[phandle_pos],
                                        xplic_phandles);
             } else {
-                create_fdt_socket_aplic(s, memmap, socket,
+                create_fdt_socket_aplic(s, socket,
                                         msi_m_phandle, msi_s_phandle, phandle,
                                         &intc_phandles[phandle_pos],
                                         xplic_phandles,
@@ -1126,7 +1131,7 @@ static void finalize_fdt(RISCVVirtState *s)
     uint32_t irq_pcie_phandle = 1, irq_virtio_phandle = 1;
     uint32_t iommu_sys_phandle = 1;
 
-    create_fdt_sockets(s, s->memmap, &phandle, &irq_mmio_phandle,
+    create_fdt_sockets(s, &phandle, &irq_mmio_phandle,
                        &irq_pcie_phandle, &irq_virtio_phandle,
                        &msi_pcie_phandle);
 
-- 
2.49.0

[PATCH 7/9] hw/riscv/virt.c: use s->memmap in create_fdt_virtio()

[Daniel Henrique Barboza]

create_fdt_virtio() can use s->memmap instead of having an extra
argument for it.

While we're at it rewrite it a little bit to avoid the clunky line in
'name' and code repetition:

- declare 'virtio_base' out of the loop since it never changes;
- declare a 'size' variable. Use it to calculate the address of the
  virtio device in an 'addr' variable;
- use 'addr' in the 'name' g_strdup_printf();
- use 'addr' and 'size' when creating the 'reg' property.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index dcaf28d1a4..07ee8e144f 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -842,21 +842,24 @@ static void create_fdt_sockets(RISCVVirtState *s,
     riscv_socket_fdt_write_distance_matrix(ms);
 }
 
-static void create_fdt_virtio(RISCVVirtState *s, const MemMapEntry *memmap,
-                              uint32_t irq_virtio_phandle)
+static void create_fdt_virtio(RISCVVirtState *s, uint32_t irq_virtio_phandle)
 {
     int i;
     MachineState *ms = MACHINE(s);
+    hwaddr virtio_base = s->memmap[VIRT_VIRTIO].base;
 
     for (i = 0; i < VIRTIO_COUNT; i++) {
-        g_autofree char *name =  g_strdup_printf("/soc/virtio_mmio@%lx",
-            (long)(memmap[VIRT_VIRTIO].base + i * memmap[VIRT_VIRTIO].size));
+        g_autofree char *name = NULL;
+        uint64_t size = s->memmap[VIRT_VIRTIO].size;
+        hwaddr addr = virtio_base + i * size;
+
+        name = g_strdup_printf("/soc/virtio_mmio@%"HWADDR_PRIx, addr);
 
         qemu_fdt_add_subnode(ms->fdt, name);
         qemu_fdt_setprop_string(ms->fdt, name, "compatible", "virtio,mmio");
         qemu_fdt_setprop_cells(ms->fdt, name, "reg",
-            0x0, memmap[VIRT_VIRTIO].base + i * memmap[VIRT_VIRTIO].size,
-            0x0, memmap[VIRT_VIRTIO].size);
+                               0x0, addr,
+                               0x0, size);
         qemu_fdt_setprop_cell(ms->fdt, name, "interrupt-parent",
             irq_virtio_phandle);
         if (s->aia_type == VIRT_AIA_TYPE_NONE) {
@@ -1135,7 +1138,7 @@ static void finalize_fdt(RISCVVirtState *s)
                        &irq_pcie_phandle, &irq_virtio_phandle,
                        &msi_pcie_phandle);
 
-    create_fdt_virtio(s, s->memmap, irq_virtio_phandle);
+    create_fdt_virtio(s, irq_virtio_phandle);
 
     if (virt_is_iommu_sys_enabled(s)) {
         create_fdt_iommu_sys(s, irq_mmio_phandle, msi_pcie_phandle,
-- 
2.49.0

[PATCH 8/9] hw/riscv/virt.c: use s->memmap in finalize_fdt() functions

[Daniel Henrique Barboza]

Change create_fdt_pcie(), create_fdt_reset(), create_fdt_uart() and
create_fdt_rtc() to use s->memmap in their logic.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 07ee8e144f..036a0a9bfb 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -872,7 +872,7 @@ static void create_fdt_virtio(RISCVVirtState *s, uint32_t irq_virtio_phandle)
     }
 }
 
-static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
+static void create_fdt_pcie(RISCVVirtState *s,
                             uint32_t irq_pcie_phandle,
                             uint32_t msi_pcie_phandle,
                             uint32_t iommu_sys_phandle)
@@ -881,7 +881,7 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
     MachineState *ms = MACHINE(s);
 
     name = g_strdup_printf("/soc/pci@%lx",
-        (long) memmap[VIRT_PCIE_ECAM].base);
+        (long) s->memmap[VIRT_PCIE_ECAM].base);
     qemu_fdt_setprop_cell(ms->fdt, name, "#address-cells",
         FDT_PCI_ADDR_CELLS);
     qemu_fdt_setprop_cell(ms->fdt, name, "#interrupt-cells",
@@ -892,19 +892,19 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
     qemu_fdt_setprop_string(ms->fdt, name, "device_type", "pci");
     qemu_fdt_setprop_cell(ms->fdt, name, "linux,pci-domain", 0);
     qemu_fdt_setprop_cells(ms->fdt, name, "bus-range", 0,
-        memmap[VIRT_PCIE_ECAM].size / PCIE_MMCFG_SIZE_MIN - 1);
+        s->memmap[VIRT_PCIE_ECAM].size / PCIE_MMCFG_SIZE_MIN - 1);
     qemu_fdt_setprop(ms->fdt, name, "dma-coherent", NULL, 0);
     if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
         qemu_fdt_setprop_cell(ms->fdt, name, "msi-parent", msi_pcie_phandle);
     }
     qemu_fdt_setprop_cells(ms->fdt, name, "reg", 0,
-        memmap[VIRT_PCIE_ECAM].base, 0, memmap[VIRT_PCIE_ECAM].size);
+        s->memmap[VIRT_PCIE_ECAM].base, 0, s->memmap[VIRT_PCIE_ECAM].size);
     qemu_fdt_setprop_sized_cells(ms->fdt, name, "ranges",
         1, FDT_PCI_RANGE_IOPORT, 2, 0,
-        2, memmap[VIRT_PCIE_PIO].base, 2, memmap[VIRT_PCIE_PIO].size,
+        2, s->memmap[VIRT_PCIE_PIO].base, 2, s->memmap[VIRT_PCIE_PIO].size,
         1, FDT_PCI_RANGE_MMIO,
-        2, memmap[VIRT_PCIE_MMIO].base,
-        2, memmap[VIRT_PCIE_MMIO].base, 2, memmap[VIRT_PCIE_MMIO].size,
+        2, s->memmap[VIRT_PCIE_MMIO].base,
+        2, s->memmap[VIRT_PCIE_MMIO].base, 2, s->memmap[VIRT_PCIE_MMIO].size,
         1, FDT_PCI_RANGE_MMIO_64BIT,
         2, virt_high_pcie_memmap.base,
         2, virt_high_pcie_memmap.base, 2, virt_high_pcie_memmap.size);
@@ -918,8 +918,7 @@ static void create_fdt_pcie(RISCVVirtState *s, const MemMapEntry *memmap,
     create_pcie_irq_map(s, ms->fdt, name, irq_pcie_phandle);
 }
 
-static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
-                             uint32_t *phandle)
+static void create_fdt_reset(RISCVVirtState *s, uint32_t *phandle)
 {
     char *name;
     uint32_t test_phandle;
@@ -927,7 +926,7 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
 
     test_phandle = (*phandle)++;
     name = g_strdup_printf("/soc/test@%lx",
-        (long)memmap[VIRT_TEST].base);
+        (long)s->memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(ms->fdt, name);
     {
         static const char * const compat[3] = {
@@ -937,7 +936,7 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
                                       (char **)&compat, ARRAY_SIZE(compat));
     }
     qemu_fdt_setprop_cells(ms->fdt, name, "reg",
-        0x0, memmap[VIRT_TEST].base, 0x0, memmap[VIRT_TEST].size);
+        0x0, s->memmap[VIRT_TEST].base, 0x0, s->memmap[VIRT_TEST].size);
     qemu_fdt_setprop_cell(ms->fdt, name, "phandle", test_phandle);
     test_phandle = qemu_fdt_get_phandle(ms->fdt, name);
     g_free(name);
@@ -959,18 +958,19 @@ static void create_fdt_reset(RISCVVirtState *s, const MemMapEntry *memmap,
     g_free(name);
 }
 
-static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
+static void create_fdt_uart(RISCVVirtState *s,
                             uint32_t irq_mmio_phandle)
 {
     g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
-    name = g_strdup_printf("/soc/serial@%lx", (long)memmap[VIRT_UART0].base);
+    name = g_strdup_printf("/soc/serial@%lx",
+                           (long)s->memmap[VIRT_UART0].base);
     qemu_fdt_add_subnode(ms->fdt, name);
     qemu_fdt_setprop_string(ms->fdt, name, "compatible", "ns16550a");
     qemu_fdt_setprop_cells(ms->fdt, name, "reg",
-        0x0, memmap[VIRT_UART0].base,
-        0x0, memmap[VIRT_UART0].size);
+        0x0, s->memmap[VIRT_UART0].base,
+        0x0, s->memmap[VIRT_UART0].size);
     qemu_fdt_setprop_cell(ms->fdt, name, "clock-frequency", 3686400);
     qemu_fdt_setprop_cell(ms->fdt, name, "interrupt-parent", irq_mmio_phandle);
     if (s->aia_type == VIRT_AIA_TYPE_NONE) {
@@ -983,18 +983,18 @@ static void create_fdt_uart(RISCVVirtState *s, const MemMapEntry *memmap,
     qemu_fdt_setprop_string(ms->fdt, "/aliases", "serial0", name);
 }
 
-static void create_fdt_rtc(RISCVVirtState *s, const MemMapEntry *memmap,
+static void create_fdt_rtc(RISCVVirtState *s,
                            uint32_t irq_mmio_phandle)
 {
     g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
-    name = g_strdup_printf("/soc/rtc@%lx", (long)memmap[VIRT_RTC].base);
+    name = g_strdup_printf("/soc/rtc@%lx", (long)s->memmap[VIRT_RTC].base);
     qemu_fdt_add_subnode(ms->fdt, name);
     qemu_fdt_setprop_string(ms->fdt, name, "compatible",
         "google,goldfish-rtc");
     qemu_fdt_setprop_cells(ms->fdt, name, "reg",
-        0x0, memmap[VIRT_RTC].base, 0x0, memmap[VIRT_RTC].size);
+        0x0, s->memmap[VIRT_RTC].base, 0x0, s->memmap[VIRT_RTC].size);
     qemu_fdt_setprop_cell(ms->fdt, name, "interrupt-parent",
         irq_mmio_phandle);
     if (s->aia_type == VIRT_AIA_TYPE_NONE) {
@@ -1144,14 +1144,14 @@ static void finalize_fdt(RISCVVirtState *s)
         create_fdt_iommu_sys(s, irq_mmio_phandle, msi_pcie_phandle,
                              &iommu_sys_phandle);
     }
-    create_fdt_pcie(s, s->memmap, irq_pcie_phandle, msi_pcie_phandle,
+    create_fdt_pcie(s, irq_pcie_phandle, msi_pcie_phandle,
                     iommu_sys_phandle);
 
-    create_fdt_reset(s, s->memmap, &phandle);
+    create_fdt_reset(s, &phandle);
 
-    create_fdt_uart(s, s->memmap, irq_mmio_phandle);
+    create_fdt_uart(s, irq_mmio_phandle);
 
-    create_fdt_rtc(s, s->memmap, irq_mmio_phandle);
+    create_fdt_rtc(s, irq_mmio_phandle);
 }
 
 static void create_fdt(RISCVVirtState *s)
-- 
2.49.0

[PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Daniel Henrique Barboza]

We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
strings for uint64_t and hwaddr types.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/virt.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 036a0a9bfb..075c035f25 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
 
     addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
     size = riscv_socket_mem_size(ms, socket);
-    mem_name = g_strdup_printf("/memory@%lx", (long)addr);
+    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
     qemu_fdt_add_subnode(ms->fdt, mem_name);
     qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
         addr >> 32, addr, size >> 32, size);
@@ -880,8 +880,8 @@ static void create_fdt_pcie(RISCVVirtState *s,
     g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
-    name = g_strdup_printf("/soc/pci@%lx",
-        (long) s->memmap[VIRT_PCIE_ECAM].base);
+    name = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+                           s->memmap[VIRT_PCIE_ECAM].base);
     qemu_fdt_setprop_cell(ms->fdt, name, "#address-cells",
         FDT_PCI_ADDR_CELLS);
     qemu_fdt_setprop_cell(ms->fdt, name, "#interrupt-cells",
@@ -925,8 +925,8 @@ static void create_fdt_reset(RISCVVirtState *s, uint32_t *phandle)
     MachineState *ms = MACHINE(s);
 
     test_phandle = (*phandle)++;
-    name = g_strdup_printf("/soc/test@%lx",
-        (long)s->memmap[VIRT_TEST].base);
+    name = g_strdup_printf("/soc/test@%"HWADDR_PRIx,
+                           s->memmap[VIRT_TEST].base);
     qemu_fdt_add_subnode(ms->fdt, name);
     {
         static const char * const compat[3] = {
@@ -964,8 +964,8 @@ static void create_fdt_uart(RISCVVirtState *s,
     g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
-    name = g_strdup_printf("/soc/serial@%lx",
-                           (long)s->memmap[VIRT_UART0].base);
+    name = g_strdup_printf("/soc/serial@%"HWADDR_PRIx,
+                           s->memmap[VIRT_UART0].base);
     qemu_fdt_add_subnode(ms->fdt, name);
     qemu_fdt_setprop_string(ms->fdt, name, "compatible", "ns16550a");
     qemu_fdt_setprop_cells(ms->fdt, name, "reg",
@@ -989,7 +989,8 @@ static void create_fdt_rtc(RISCVVirtState *s,
     g_autofree char *name = NULL;
     MachineState *ms = MACHINE(s);
 
-    name = g_strdup_printf("/soc/rtc@%lx", (long)s->memmap[VIRT_RTC].base);
+    name = g_strdup_printf("/soc/rtc@%"HWADDR_PRIx,
+                           s->memmap[VIRT_RTC].base);
     qemu_fdt_add_subnode(ms->fdt, name);
     qemu_fdt_setprop_string(ms->fdt, name, "compatible",
         "google,goldfish-rtc");
@@ -1042,8 +1043,8 @@ static void create_fdt_virtio_iommu(RISCVVirtState *s, uint16_t bdf)
     g_autofree char *iommu_node = NULL;
     g_autofree char *pci_node = NULL;
 
-    pci_node = g_strdup_printf("/soc/pci@%lx",
-                               (long) s->memmap[VIRT_PCIE_ECAM].base);
+    pci_node = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+                               s->memmap[VIRT_PCIE_ECAM].base);
     iommu_node = g_strdup_printf("%s/virtio_iommu@%x,%x", pci_node,
                                  PCI_SLOT(bdf), PCI_FUNC(bdf));
     iommu_phandle = qemu_fdt_alloc_phandle(fdt);
@@ -1111,8 +1112,8 @@ static void create_fdt_iommu(RISCVVirtState *s, uint16_t bdf)
     g_autofree char *iommu_node = NULL;
     g_autofree char *pci_node = NULL;
 
-    pci_node = g_strdup_printf("/soc/pci@%lx",
-                               (long) s->memmap[VIRT_PCIE_ECAM].base);
+    pci_node = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+                               s->memmap[VIRT_PCIE_ECAM].base);
     iommu_node = g_strdup_printf("%s/iommu@%x", pci_node, bdf);
     iommu_phandle = qemu_fdt_alloc_phandle(fdt);
     qemu_fdt_add_subnode(fdt, iommu_node);
@@ -1181,8 +1182,8 @@ static void create_fdt(RISCVVirtState *s)
      * The "/soc/pci@..." node is needed for PCIE hotplugs
      * that might happen before finalize_fdt().
      */
-    name = g_strdup_printf("/soc/pci@%lx",
-                           (long) s->memmap[VIRT_PCIE_ECAM].base);
+    name = g_strdup_printf("/soc/pci@%"HWADDR_PRIx,
+                           s->memmap[VIRT_PCIE_ECAM].base);
     qemu_fdt_add_subnode(ms->fdt, name);
 
     qemu_fdt_add_subnode(ms->fdt, "/chosen");
-- 
2.49.0

Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Philippe Mathieu-Daudé]

On 23/4/25 13:06, Daniel Henrique Barboza wrote:
> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
> strings for uint64_t and hwaddr types.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/virt.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Joel Stanley]

On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
> strings for uint64_t and hwaddr types.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/virt.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 036a0a9bfb..075c035f25 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>
>      addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>      size = riscv_socket_mem_size(ms, socket);
> -    mem_name = g_strdup_printf("/memory@%lx", (long)addr);
> +    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);

I wondered why this wasn't a HWADDR_PRIx.

addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
everything more consistent.

Cheers,

Joel

Re: [PATCH 1/9] hw/riscv/virt.c: enforce s->memmap use in machine_init()

[Joel Stanley]

On Wed, 23 Apr 2025 at 20:37, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Throughout the code we're accessing the board memmap, most of the time,
> by accessing it statically via 'virt_memmap'. This static map is also
> assigned in the machine state in s->memmap.
>
> We're also passing it as a variable to some fdt functions, which is
> unorthodox since we can spare a function argument by accessing it
> statically or via the machine state.
>
> All the current forms are valid but not all of the are scalable. In the
> future we will version this board, and then all this code will need
> rework because it should point to the updated memmap. In this case,
> we'll want to assign the adequate versioned memmap once during init,
> in s->memmap like it is being done today, and the rest of the code
> will access the updated map via s->memmap.

I was writing a patch for a machine and came across the same
inconsistencies. Nice clean up.

Some of the device initlisation code could be refactored out to be
shared by other machines within the riscv directory. Related, parts of
the device tree creation could belong to the model, instead of to the
machine, as the properties are a property (!) of the device.

With that in mind we should consider passing the eg. fdt pointer and
the MemMap pointer instead of machine state, where practical.

> We're also enforcing the pattern of using s->memmap instead of assigning
> it to a temp variable 'memmap'. Code is copy/pasted around all the time
> and being consistent is important.

Reviewed-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel

Re: [PATCH 1/9] hw/riscv/virt.c: enforce s->memmap use in machine_init()

[Alistair Francis]

On Wed, Apr 23, 2025 at 9:11 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Throughout the code we're accessing the board memmap, most of the time,
> by accessing it statically via 'virt_memmap'. This static map is also
> assigned in the machine state in s->memmap.
>
> We're also passing it as a variable to some fdt functions, which is
> unorthodox since we can spare a function argument by accessing it
> statically or via the machine state.
>
> All the current forms are valid but not all of the are scalable. In the
> future we will version this board, and then all this code will need
> rework because it should point to the updated memmap. In this case,
> we'll want to assign the adequate versioned memmap once during init,
> in s->memmap like it is being done today, and the rest of the code
> will access the updated map via s->memmap.
>
> We're also enforcing the pattern of using s->memmap instead of assigning
> it to a temp variable 'memmap'. Code is copy/pasted around all the time
> and being consistent is important.
>
> We'll start these rather mechanical changes with virt_machine_init().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 54 ++++++++++++++++++++++++-------------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index c9d255d8a8..6e3e34879f 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1527,7 +1527,6 @@ static void virt_machine_done(Notifier *notifier, void *data)
>
>  static void virt_machine_init(MachineState *machine)
>  {
> -    const MemMapEntry *memmap = virt_memmap;
>      RISCVVirtState *s = RISCV_VIRT_MACHINE(machine);
>      MemoryRegion *system_memory = get_system_memory();
>      MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> @@ -1535,6 +1534,8 @@ static void virt_machine_init(MachineState *machine)
>      int i, base_hartid, hart_count;
>      int socket_count = riscv_socket_count(machine);
>
> +    s->memmap = virt_memmap;
> +
>      /* Check socket count limit */
>      if (VIRT_SOCKETS_MAX < socket_count) {
>          error_report("number of sockets/nodes should be less than %d",
> @@ -1582,7 +1583,7 @@ static void virt_machine_init(MachineState *machine)
>          if (virt_aclint_allowed() && s->have_aclint) {
>              if (s->aia_type == VIRT_AIA_TYPE_APLIC_IMSIC) {
>                  /* Per-socket ACLINT MTIMER */
> -                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
> +                riscv_aclint_mtimer_create(s->memmap[VIRT_CLINT].base +
>                              i * RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
>                          RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
>                          base_hartid, hart_count,
> @@ -1591,28 +1592,28 @@ static void virt_machine_init(MachineState *machine)
>                          RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
>              } else {
>                  /* Per-socket ACLINT MSWI, MTIMER, and SSWI */
> -                riscv_aclint_swi_create(memmap[VIRT_CLINT].base +
> -                            i * memmap[VIRT_CLINT].size,
> +                riscv_aclint_swi_create(s->memmap[VIRT_CLINT].base +
> +                            i * s->memmap[VIRT_CLINT].size,
>                          base_hartid, hart_count, false);
> -                riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
> -                            i * memmap[VIRT_CLINT].size +
> +                riscv_aclint_mtimer_create(s->memmap[VIRT_CLINT].base +
> +                            i * s->memmap[VIRT_CLINT].size +
>                              RISCV_ACLINT_SWI_SIZE,
>                          RISCV_ACLINT_DEFAULT_MTIMER_SIZE,
>                          base_hartid, hart_count,
>                          RISCV_ACLINT_DEFAULT_MTIMECMP,
>                          RISCV_ACLINT_DEFAULT_MTIME,
>                          RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
> -                riscv_aclint_swi_create(memmap[VIRT_ACLINT_SSWI].base +
> -                            i * memmap[VIRT_ACLINT_SSWI].size,
> +                riscv_aclint_swi_create(s->memmap[VIRT_ACLINT_SSWI].base +
> +                            i * s->memmap[VIRT_ACLINT_SSWI].size,
>                          base_hartid, hart_count, true);
>              }
>          } else if (tcg_enabled()) {
>              /* Per-socket SiFive CLINT */
>              riscv_aclint_swi_create(
> -                    memmap[VIRT_CLINT].base + i * memmap[VIRT_CLINT].size,
> +                    s->memmap[VIRT_CLINT].base + i * s->memmap[VIRT_CLINT].size,
>                      base_hartid, hart_count, false);
> -            riscv_aclint_mtimer_create(memmap[VIRT_CLINT].base +
> -                        i * memmap[VIRT_CLINT].size + RISCV_ACLINT_SWI_SIZE,
> +            riscv_aclint_mtimer_create(s->memmap[VIRT_CLINT].base +
> +                    i * s->memmap[VIRT_CLINT].size + RISCV_ACLINT_SWI_SIZE,
>                      RISCV_ACLINT_DEFAULT_MTIMER_SIZE, base_hartid, hart_count,
>                      RISCV_ACLINT_DEFAULT_MTIMECMP, RISCV_ACLINT_DEFAULT_MTIME,
>                      RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, true);
> @@ -1620,11 +1621,11 @@ static void virt_machine_init(MachineState *machine)
>
>          /* Per-socket interrupt controller */
>          if (s->aia_type == VIRT_AIA_TYPE_NONE) {
> -            s->irqchip[i] = virt_create_plic(memmap, i,
> +            s->irqchip[i] = virt_create_plic(s->memmap, i,
>                                               base_hartid, hart_count);
>          } else {
>              s->irqchip[i] = virt_create_aia(s->aia_type, s->aia_guests,
> -                                            memmap, i, base_hartid,
> +                                            s->memmap, i, base_hartid,
>                                              hart_count);
>          }
>
> @@ -1646,8 +1647,8 @@ static void virt_machine_init(MachineState *machine)
>      if (kvm_enabled() && virt_use_kvm_aia_aplic_imsic(s->aia_type)) {
>          kvm_riscv_aia_create(machine, IMSIC_MMIO_GROUP_MIN_SHIFT,
>                               VIRT_IRQCHIP_NUM_SOURCES, VIRT_IRQCHIP_NUM_MSIS,
> -                             memmap[VIRT_APLIC_S].base,
> -                             memmap[VIRT_IMSIC_S].base,
> +                             s->memmap[VIRT_APLIC_S].base,
> +                             s->memmap[VIRT_IMSIC_S].base,
>                               s->aia_guests);
>      }
>
> @@ -1663,21 +1664,20 @@ static void virt_machine_init(MachineState *machine)
>          virt_high_pcie_memmap.size = VIRT32_HIGH_PCIE_MMIO_SIZE;
>      } else {
>          virt_high_pcie_memmap.size = VIRT64_HIGH_PCIE_MMIO_SIZE;
> -        virt_high_pcie_memmap.base = memmap[VIRT_DRAM].base + machine->ram_size;
> +        virt_high_pcie_memmap.base = s->memmap[VIRT_DRAM].base +
> +                                     machine->ram_size;
>          virt_high_pcie_memmap.base =
>              ROUND_UP(virt_high_pcie_memmap.base, virt_high_pcie_memmap.size);
>      }
>
> -    s->memmap = virt_memmap;
> -
>      /* register system main memory (actual RAM) */
> -    memory_region_add_subregion(system_memory, memmap[VIRT_DRAM].base,
> -        machine->ram);
> +    memory_region_add_subregion(system_memory, s->memmap[VIRT_DRAM].base,
> +                                machine->ram);
>
>      /* boot rom */
>      memory_region_init_rom(mask_rom, NULL, "riscv_virt_board.mrom",
> -                           memmap[VIRT_MROM].size, &error_fatal);
> -    memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
> +                           s->memmap[VIRT_MROM].size, &error_fatal);
> +    memory_region_add_subregion(system_memory, s->memmap[VIRT_MROM].base,
>                                  mask_rom);
>
>      /*
> @@ -1688,12 +1688,12 @@ static void virt_machine_init(MachineState *machine)
>      rom_set_fw(s->fw_cfg);
>
>      /* SiFive Test MMIO device */
> -    sifive_test_create(memmap[VIRT_TEST].base);
> +    sifive_test_create(s->memmap[VIRT_TEST].base);
>
>      /* VirtIO MMIO devices */
>      for (i = 0; i < VIRTIO_COUNT; i++) {
>          sysbus_create_simple("virtio-mmio",
> -            memmap[VIRT_VIRTIO].base + i * memmap[VIRT_VIRTIO].size,
> +            s->memmap[VIRT_VIRTIO].base + i * s->memmap[VIRT_VIRTIO].size,
>              qdev_get_gpio_in(virtio_irqchip, VIRTIO_IRQ + i));
>      }
>
> @@ -1701,11 +1701,11 @@ static void virt_machine_init(MachineState *machine)
>
>      create_platform_bus(s, mmio_irqchip);
>
> -    serial_mm_init(system_memory, memmap[VIRT_UART0].base,
> +    serial_mm_init(system_memory, s->memmap[VIRT_UART0].base,
>          0, qdev_get_gpio_in(mmio_irqchip, UART0_IRQ), 399193,
>          serial_hd(0), DEVICE_LITTLE_ENDIAN);
>
> -    sysbus_create_simple("goldfish_rtc", memmap[VIRT_RTC].base,
> +    sysbus_create_simple("goldfish_rtc", s->memmap[VIRT_RTC].base,
>          qdev_get_gpio_in(mmio_irqchip, RTC_IRQ));
>
>      for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
> @@ -1723,7 +1723,7 @@ static void virt_machine_init(MachineState *machine)
>              exit(1);
>          }
>      } else {
> -        create_fdt(s, memmap);
> +        create_fdt(s, s->memmap);
>      }
>
>      if (virt_is_iommu_sys_enabled(s)) {
> --
> 2.49.0
>
>

Re: [PATCH 1/9] hw/riscv/virt.c: enforce s->memmap use in machine_init()

[Daniel Henrique Barboza]

On 4/24/25 6:51 AM, Joel Stanley wrote:
> On Wed, 23 Apr 2025 at 20:37, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> Throughout the code we're accessing the board memmap, most of the time,
>> by accessing it statically via 'virt_memmap'. This static map is also
>> assigned in the machine state in s->memmap.
>>
>> We're also passing it as a variable to some fdt functions, which is
>> unorthodox since we can spare a function argument by accessing it
>> statically or via the machine state.
>>
>> All the current forms are valid but not all of the are scalable. In the
>> future we will version this board, and then all this code will need
>> rework because it should point to the updated memmap. In this case,
>> we'll want to assign the adequate versioned memmap once during init,
>> in s->memmap like it is being done today, and the rest of the code
>> will access the updated map via s->memmap.
> 
> I was writing a patch for a machine and came across the same
> inconsistencies. Nice clean up.
> 
> Some of the device initlisation code could be refactored out to be
> shared by other machines within the riscv directory. Related, parts of
> the device tree creation could belong to the model, instead of to the
> machine, as the properties are a property (!) of the device.


Yes, delegating the FDT creation to the device, instead of having each machine
to create the (mostly) same FDT code over and over again, is something that
I've considering for awhile.

I keep postponing it mainly because I would like to verify with the DT folks if
there's a guarantee that a given device/CPU DT is always the same, i.e. a device
DT is always the same regardless of the machine. I have a guess that that this is
indeed the case but a confirmation would be nice .... Conor, care to comment?


In this refactor we could then create FDTs by passing along a memmap pointer and
a fdt pointer, as you've suggested.


All this said, there's no need to do such FDT refactory all at once. I think I'll
start with the most common devices between RISC-V boards and go from there.

Thanks,

Daniel


> 
> With that in mind we should consider passing the eg. fdt pointer and
> the MemMap pointer instead of machine state, where practical.
> 
>> We're also enforcing the pattern of using s->memmap instead of assigning
>> it to a temp variable 'memmap'. Code is copy/pasted around all the time
>> and being consistent is important.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> Cheers,
> 
> Joel

Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Daniel Henrique Barboza]

On 4/24/25 6:41 AM, Joel Stanley wrote:
> On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
>> strings for uint64_t and hwaddr types.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/virt.c | 29 +++++++++++++++--------------
>>   1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 036a0a9bfb..075c035f25 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>>
>>       addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>>       size = riscv_socket_mem_size(ms, socket);
>> -    mem_name = g_strdup_printf("/memory@%lx", (long)addr);
>> +    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> 
> I wondered why this wasn't a HWADDR_PRIx.
> 
> addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
> everything more consistent.

I didn't put too much thought about it in this patch. I saw that 'addr' was an
uint64_t and just matched the format string.

As for whether this 'addr' var and NodeInfo::node_mem could be a hwaddr, at first
glance I don't see why not. There are lots of 'hwaddr' references in the memory API
(memory_region_* functions, address_space* functions, etc) so using hwaddr in the
memory context is valid.

If we want to go further, the patch that introduced hwaddr in QEMU (commit a8170e5e)
states:

---------
Rename target_phys_addr_t to hwaddr

target_phys_addr_t is unwieldly, violates the C standard (_t suffixes are
reserved) and its purpose doesn't match the name (most target_phys_addr_t
addresses are not target specific).  Replace it with a finger-friendly,
standards conformant hwaddr.
---------

So it seems to me that the idea is to have an abstraction of target independent
physical addresses, and memory is a good example of that. I believe we're not
making full use of hwaddr and overusing uint64_t. Thanks,


Daniel

> 
> Cheers,
> 
> Joel

Re: [PATCH 1/9] hw/riscv/virt.c: enforce s->memmap use in machine_init()

[Joel Stanley]

On Fri, 25 Apr 2025 at 21:23, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 4/24/25 6:51 AM, Joel Stanley wrote:
> > On Wed, 23 Apr 2025 at 20:37, Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> Throughout the code we're accessing the board memmap, most of the time,
> >> by accessing it statically via 'virt_memmap'. This static map is also
> >> assigned in the machine state in s->memmap.
> >>
> >> We're also passing it as a variable to some fdt functions, which is
> >> unorthodox since we can spare a function argument by accessing it
> >> statically or via the machine state.
> >>
> >> All the current forms are valid but not all of the are scalable. In the
> >> future we will version this board, and then all this code will need
> >> rework because it should point to the updated memmap. In this case,
> >> we'll want to assign the adequate versioned memmap once during init,
> >> in s->memmap like it is being done today, and the rest of the code
> >> will access the updated map via s->memmap.
> >
> > I was writing a patch for a machine and came across the same
> > inconsistencies. Nice clean up.
> >
> > Some of the device initlisation code could be refactored out to be
> > shared by other machines within the riscv directory. Related, parts of
> > the device tree creation could belong to the model, instead of to the
> > machine, as the properties are a property (!) of the device.
>
>
> Yes, delegating the FDT creation to the device, instead of having each machine
> to create the (mostly) same FDT code over and over again, is something that
> I've considering for awhile.
>
> I keep postponing it mainly because I would like to verify with the DT folks if
> there's a guarantee that a given device/CPU DT is always the same, i.e. a device
> DT is always the same regardless of the machine. I have a guess that that this is
> indeed the case but a confirmation would be nice .... Conor, care to comment?

I'd be interested in Coner's thoughts on this.

My understanding is bindings strive to specify the hardware
independent of the machine it's part of. We have bindings in the
kernel tree, and associated drivers that use those bindings, that work
fine on different machines. The litex peripherals are an extreme case
of this; peripherals defined in python that are attached to soft cores
often running on a FPGA.

At a practical level generating the device tree for a given device
does need to take into account specifics of the machine.

Things like interrupt properties depend on the interrupt device you're
delivering to (some have two cells to provide a 'flags' parameter
alongside the irq number). In general anything that contains phandles
could end up being machine specific. Another case is the number of
cells in reg properties, which depend on the bus the device is
described to be on.

These are things that some common code would need to handle, not show
stoppers for the idea.

> In this refactor we could then create FDTs by passing along a memmap pointer and
> a fdt pointer, as you've suggested.
>
> All this said, there's no need to do such FDT refactory all at once. I think I'll
> start with the most common devices between RISC-V boards and go from there.

Agreed! It was just something to keep in mind when deciding what
pointers to pass around.

Cheers,

Joel

Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Joel Stanley]

On Fri, 25 Apr 2025 at 22:03, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 4/24/25 6:41 AM, Joel Stanley wrote:
> > On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
> >> strings for uint64_t and hwaddr types.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >>   hw/riscv/virt.c | 29 +++++++++++++++--------------
> >>   1 file changed, 15 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> index 036a0a9bfb..075c035f25 100644
> >> --- a/hw/riscv/virt.c
> >> +++ b/hw/riscv/virt.c
> >> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
> >>
> >>       addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
> >>       size = riscv_socket_mem_size(ms, socket);
> >> -    mem_name = g_strdup_printf("/memory@%lx", (long)addr);
> >> +    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> >
> > I wondered why this wasn't a HWADDR_PRIx.
> >
> > addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
> > everything more consistent.
>
> I didn't put too much thought about it in this patch. I saw that 'addr' was an
> uint64_t and just matched the format string.
>
> As for whether this 'addr' var and NodeInfo::node_mem could be a hwaddr, at first
> glance I don't see why not. There are lots of 'hwaddr' references in the memory API
> (memory_region_* functions, address_space* functions, etc) so using hwaddr in the
> memory context is valid.
>
> If we want to go further, the patch that introduced hwaddr in QEMU (commit a8170e5e)
> states:
>
> ---------
> Rename target_phys_addr_t to hwaddr
>
> target_phys_addr_t is unwieldly, violates the C standard (_t suffixes are
> reserved) and its purpose doesn't match the name (most target_phys_addr_t
> addresses are not target specific).  Replace it with a finger-friendly,
> standards conformant hwaddr.
> ---------
>
> So it seems to me that the idea is to have an abstraction of target independent
> physical addresses, and memory is a good example of that. I believe we're not
> making full use of hwaddr and overusing uint64_t. Thanks,

Cool. I've got a little series that I'll post. It cleans this one up,
and cleans up some other device tree things.

Cheers,

Joel

Re: [PATCH 1/9] hw/riscv/virt.c: enforce s->memmap use in machine_init()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5389 bytes --]

Yo,

On Tue, Apr 29, 2025 at 02:55:44PM +0930, Joel Stanley wrote:
> On Fri, 25 Apr 2025 at 21:23, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> > On 4/24/25 6:51 AM, Joel Stanley wrote:
> > > On Wed, 23 Apr 2025 at 20:37, Daniel Henrique Barboza
> > > <dbarboza@ventanamicro.com> wrote:
> > >>
> > >> Throughout the code we're accessing the board memmap, most of the time,
> > >> by accessing it statically via 'virt_memmap'. This static map is also
> > >> assigned in the machine state in s->memmap.
> > >>
> > >> We're also passing it as a variable to some fdt functions, which is
> > >> unorthodox since we can spare a function argument by accessing it
> > >> statically or via the machine state.
> > >>
> > >> All the current forms are valid but not all of the are scalable. In the
> > >> future we will version this board, and then all this code will need
> > >> rework because it should point to the updated memmap. In this case,
> > >> we'll want to assign the adequate versioned memmap once during init,
> > >> in s->memmap like it is being done today, and the rest of the code
> > >> will access the updated map via s->memmap.
> > >
> > > I was writing a patch for a machine and came across the same
> > > inconsistencies. Nice clean up.
> > >
> > > Some of the device initlisation code could be refactored out to be
> > > shared by other machines within the riscv directory. Related, parts of
> > > the device tree creation could belong to the model, instead of to the
> > > machine, as the properties are a property (!) of the device.
> >
> > Yes, delegating the FDT creation to the device, instead of having each machine
> > to create the (mostly) same FDT code over and over again, is something that
> > I've considering for awhile.
> >
> > I keep postponing it mainly because I would like to verify with the DT folks if
> > there's a guarantee that a given device/CPU DT is always the same, i.e. a device
> > DT is always the same regardless of the machine. I have a guess that that this is
> > indeed the case but a confirmation would be nice .... Conor, care to comment?

Uhm, if the device node was always the same, regardless of "machine"
then there'd be no need for any properties other than
compatible/reg/links to provider nodes! But unfortunately that's not what
properties are limited to, there's properties for a whole host of other
elements of device configuration.

Generally speaking, properties will be fixed for a given "machine"/board,
but some could change depending on things like AMP affecting which
mailbox channel you're permitted to use or using a different firmware
(or version of said firmware) can change a node. For example, the
extensions supported by a CPU can change based on what version of OpenSBI
you're running. The latter might not affect QEMU, because you're providing
a DT for the firmware to edit, but from the OS point of view there's
different, but valid and complete, descriptions for the same physical
hardware.

Also, on the same machine you might have variation between two instances
of the same IP, other than just the reg/provider links changing, for
example if there's two different dwmac ethernet controllers configured
to provide different maximum speeds.

So yeah, I can't give you any assurance that even one specific IP/CPU on
a specific SoC on a specific machine/board will have the same device
node unfortunately.

> My understanding is bindings strive to specify the hardware
> independent of the machine it's part of. We have bindings in the
> kernel tree, and associated drivers that use those bindings, that work
> fine on different machines. The litex peripherals are an extreme case
> of this; peripherals defined in python that are attached to soft cores
> often running on a FPGA.

Yeah, FPGA stuff is an extreme case, but a great thing to use as a test
for the idea, since you could have properties that change from
compilation to compilation. Take a look at something like snps,dwmac.yaml
where there's about seven thousand different properties, many of which
could be varied if it were in an FPGA design rather than an ASIC..

The other kinda extreme case in variability is the stuff that's on a
board rather than an SoC, for example the same mmc/sd card controller
on an SoC can have different device nodes depending on how a board has
been designed. What Linux calls "IIO" devices are also impacted
massively by how a board is configured or use-case, since they're often
measurement devices with different analogue circuitry connected to them.

I'm not 100% sure how all that translates to a machine in QEMU, dunno if
running a different AMP context would require a different machine etc,
but I hope that gives a general impression of what could potentially
vary? LMK if not.
Conor.

> At a practical level generating the device tree for a given device
> does need to take into account specifics of the machine.
> 
> Things like interrupt properties depend on the interrupt device you're
> delivering to (some have two cells to provide a 'flags' parameter
> alongside the irq number). In general anything that contains phandles
> could end up being machine specific. Another case is the number of
> cells in reg properties, which depend on the bus the device is
> described to be on.

> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Daniel Henrique Barboza]

Joel,

I'll make these changes in this patch to be consistent with what we've
been discussing:

- change addr to hwaddr
- use HWADDR_PRIx instead of PRIx64

i.e. this diff:

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 1eae84db15..0020d8f404 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -303,12 +303,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
  static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
  {
      g_autofree char *mem_name = NULL;
-    uint64_t addr, size;
+    hwaddr addr;
+    uint64_t size;
      MachineState *ms = MACHINE(s);
  
      addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
      size = riscv_socket_mem_size(ms, socket);
-    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
+    mem_name = g_strdup_printf("/memory@%"HWADDR_PRIx, addr);
      qemu_fdt_add_subnode(ms->fdt, mem_name);
      qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
          addr >> 32, addr, size >> 32, size);


I did this change and applied your "hw/riscv/virt: device tree reg cleanups" series on top
of it, and there are no conflicts. No change needed in your side.


Thanks,

Daniel



On 4/24/25 6:41 AM, Joel Stanley wrote:
> On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
>> strings for uint64_t and hwaddr types.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/virt.c | 29 +++++++++++++++--------------
>>   1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 036a0a9bfb..075c035f25 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>>
>>       addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>>       size = riscv_socket_mem_size(ms, socket);
>> -    mem_name = g_strdup_printf("/memory@%lx", (long)addr);
>> +    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> 
> I wondered why this wasn't a HWADDR_PRIx.
> 
> addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
> everything more consistent.
> 
> Cheers,
> 
> Joel

Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Daniel Henrique Barboza]

On 4/29/25 9:40 AM, Daniel Henrique Barboza wrote:
> Joel,
> 
> I'll make these changes in this patch to be consistent with what we've
> been discussing:
> 
> - change addr to hwaddr
> - use HWADDR_PRIx instead of PRIx64
> 
> i.e. this diff:
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 1eae84db15..0020d8f404 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -303,12 +303,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>   static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>   {
>       g_autofree char *mem_name = NULL;
> -    uint64_t addr, size;
> +    hwaddr addr;
> +    uint64_t size;
>       MachineState *ms = MACHINE(s);
> 
>       addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>       size = riscv_socket_mem_size(ms, socket);
> -    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> +    mem_name = g_strdup_printf("/memory@%"HWADDR_PRIx, addr);
>       qemu_fdt_add_subnode(ms->fdt, mem_name);
>       qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
>           addr >> 32, addr, size >> 32, size);
> 
> 
> I did this change and applied your "hw/riscv/virt: device tree reg cleanups" series on top
> of it, and there are no conflicts. No change needed in your side.

It seems I was wrong. The v2 will conflict with your patch 03. I think a rebase from
your series can't be avoided ...

Daniel

> 
> 
> Thanks,
> 
> Daniel
> 
> 
> 
> On 4/24/25 6:41 AM, Joel Stanley wrote:
>> On Wed, 23 Apr 2025 at 20:39, Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> We can avoid the 'long' casts by using PRIx64 and HWADDR_PRIx on the fmt
>>> strings for uint64_t and hwaddr types.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   hw/riscv/virt.c | 29 +++++++++++++++--------------
>>>   1 file changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> index 036a0a9bfb..075c035f25 100644
>>> --- a/hw/riscv/virt.c
>>> +++ b/hw/riscv/virt.c
>>> @@ -309,7 +309,7 @@ static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
>>>
>>>       addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
>>>       size = riscv_socket_mem_size(ms, socket);
>>> -    mem_name = g_strdup_printf("/memory@%lx", (long)addr);
>>> +    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
>>
>> I wondered why this wasn't a HWADDR_PRIx.
>>
>> addr (and NodeInfo::node_mem?) could be a hwaddr? That would make
>> everything more consistent.
>>
>> Cheers,
>>
>> Joel
> 

Re: [PATCH 9/9] hw/riscv/virt.c: remove 'long' casts in fmt strings

[Joel Stanley]

On Wed, 30 Apr 2025 at 02:41, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 4/29/25 9:40 AM, Daniel Henrique Barboza wrote:
> > Joel,
> >
> > I'll make these changes in this patch to be consistent with what we've
> > been discussing:
> >
> > - change addr to hwaddr
> > - use HWADDR_PRIx instead of PRIx64
> >
> > i.e. this diff:
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 1eae84db15..0020d8f404 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -303,12 +303,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> >   static void create_fdt_socket_memory(RISCVVirtState *s, int socket)
> >   {
> >       g_autofree char *mem_name = NULL;
> > -    uint64_t addr, size;
> > +    hwaddr addr;
> > +    uint64_t size;

Size should be a hwaddr too. This would be consistent with how
MemMapEntry describes the base/size pairs.

> >       MachineState *ms = MACHINE(s);
> >
> >       addr = s->memmap[VIRT_DRAM].base + riscv_socket_mem_offset(ms, socket);
> >       size = riscv_socket_mem_size(ms, socket);
> > -    mem_name = g_strdup_printf("/memory@%"PRIx64, addr);
> > +    mem_name = g_strdup_printf("/memory@%"HWADDR_PRIx, addr);
> >       qemu_fdt_add_subnode(ms->fdt, mem_name);
> >       qemu_fdt_setprop_cells(ms->fdt, mem_name, "reg",
> >           addr >> 32, addr, size >> 32, size);
> >
> >
> > I did this change and applied your "hw/riscv/virt: device tree reg cleanups" series on top
> > of it, and there are no conflicts. No change needed in your side.
>
> It seems I was wrong. The v2 will conflict with your patch 03. I think a rebase from
> your series can't be avoided ...

If you want to pick them up as part of your series, and send them as a
big patch set then that's fine with me.

Otherwise I'll wait until we've got yours staged and send a new version out.

Thanks!

Joel