[PATCH v2 0/7] hw/sysbus: Spring cleanups (part 1)

[PATCH v2 0/7] hw/sysbus: Spring cleanups (part 1)

[Philippe Mathieu-Daudé]

- Use proper SysBus accessors

Since v1:
- Reduce series

Philippe Mathieu-Daudé (7):
  hw/sysbus: Use memory_region_name()
  hw/i386/microvm: Use proper SysBus accessors
  hw/i386/ioapic: Use proper SysBus accessors
  hw/timer/hpet: Use proper SysBus accessors
  hw/acpi/cxl: Use proper SysBus accessors
  hw/ppc/e500: Use proper SysBus accessors
  hw/pci-bridge/pci_expander_bridge: Use proper SysBus accessors

 hw/acpi/cxl.c                       | 2 +-
 hw/core/sysbus.c                    | 2 +-
 hw/i386/kvm/ioapic.c                | 3 ++-
 hw/i386/microvm-dt.c                | 6 ++++--
 hw/pci-bridge/pci_expander_bridge.c | 6 +++---
 hw/ppc/e500.c                       | 2 +-
 hw/timer/hpet.c                     | 3 ++-
 7 files changed, 14 insertions(+), 10 deletions(-)

-- 
2.51.0

[PATCH v2 1/7] hw/sysbus: Use memory_region_name()

[Philippe Mathieu-Daudé]

MemoryRegion::name is kind of internal. Use the proper
memory_region_name() accessor, which might return a different
name.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/sysbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index ae447c1196a..d33be6b2b52 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -154,7 +154,7 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr)
 int sysbus_mmio_map_name(SysBusDevice *dev, const char *name, hwaddr addr)
 {
     for (int i = 0; i < dev->num_mmio; i++) {
-        if (!strcmp(dev->mmio[i].memory->name, name)) {
+        if (!strcmp(memory_region_name(dev->mmio[i].memory), name)) {
             sysbus_mmio_map(dev, i, addr);
             return i;
         }
-- 
2.51.0

[PATCH v2 2/7] hw/i386/microvm: Use proper SysBus accessors

[Philippe Mathieu-Daudé]

SysBusDevice::mmio[] is kind of internal. Use the proper
sysbus_mmio_get_region() accessor.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/microvm-dt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/microvm-dt.c b/hw/i386/microvm-dt.c
index cb27dfd732e..d7f49bc1b5f 100644
--- a/hw/i386/microvm-dt.c
+++ b/hw/i386/microvm-dt.c
@@ -71,7 +71,8 @@ static void dt_add_virtio(MicrovmMachineState *mms, VirtIOMMIOProxy *mmio)
         return;
     }
 
-    hwaddr base = dev->mmio[0].addr;
+    MemoryRegion *mr = sysbus_mmio_get_region(dev, 0);
+    hwaddr base = mr->addr;
     hwaddr size = 512;
     unsigned index = (base - VIRTIO_MMIO_BASE) / size;
     uint32_t irq = mms->virtio_irq_base + index;
@@ -150,7 +151,8 @@ static void dt_add_pcie(MicrovmMachineState *mms)
 
 static void dt_add_ioapic(MicrovmMachineState *mms, SysBusDevice *dev)
 {
-    hwaddr base = dev->mmio[0].addr;
+    MemoryRegion *mr = sysbus_mmio_get_region(dev, 0);
+    hwaddr base = mr->addr;
     char *nodename;
     uint32_t ph;
     int index;
-- 
2.51.0

[PATCH v2 3/7] hw/i386/ioapic: Use proper SysBus accessors

[Philippe Mathieu-Daudé]

SysBusDevice::mmio[] is kind of internal. Use the proper
sysbus_mmio_get_region() accessor.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/kvm/ioapic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 693ee978a12..0519432edbe 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -80,6 +80,7 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
 {
     struct kvm_irqchip chip;
     struct kvm_ioapic_state *kioapic;
+    MemoryRegion *mr = sysbus_mmio_get_region(s->busdev, 0);
     int ret, i;
 
     chip.chip_id = KVM_IRQCHIP_IOAPIC;
@@ -87,7 +88,7 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
 
     kioapic->id = s->id;
     kioapic->ioregsel = s->ioregsel;
-    kioapic->base_address = s->busdev.mmio[0].addr;
+    kioapic->base_address = mr->addr;
     kioapic->irr = s->irr;
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         kioapic->redirtbl[i].bits = s->ioredtbl[i];
-- 
2.51.0

[PATCH v2 4/7] hw/timer/hpet: Use proper SysBus accessors

[Philippe Mathieu-Daudé]

SysBusDevice::mmio[] is kind of internal. Use the proper
sysbus_mmio_get_region() accessor.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/timer/hpet.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 1acba4fa9db..4ed79d72620 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -655,6 +655,7 @@ static void hpet_reset(DeviceState *d)
 {
     HPETState *s = HPET(d);
     SysBusDevice *sbd = SYS_BUS_DEVICE(d);
+    MemoryRegion *mr = sysbus_mmio_get_region(sbd, 0);
     int i;
 
     for (i = 0; i < s->num_timers; i++) {
@@ -677,7 +678,7 @@ static void hpet_reset(DeviceState *d)
     s->hpet_offset = 0ULL;
     s->config = 0ULL;
     hpet_fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability;
-    hpet_fw_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr;
+    hpet_fw_cfg.hpet[s->hpet_id].address = mr->addr;
 
     /* to document that the RTC lowers its output on reset as well */
     s->rtc_irq_level = 0;
-- 
2.51.0

[PATCH v2 5/7] hw/acpi/cxl: Use proper SysBus accessors

[Philippe Mathieu-Daudé]

SysBusDevice::mmio[] is kind of internal. Use the proper
sysbus_mmio_get_region() accessor.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/acpi/cxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
index 75d5b30bb8b..77c99dfb184 100644
--- a/hw/acpi/cxl.c
+++ b/hw/acpi/cxl.c
@@ -104,7 +104,7 @@ static void cedt_build_chbs(GArray *table_data, PXBCXLDev *cxl)
 {
     PXBDev *pxb = PXB_DEV(cxl);
     SysBusDevice *sbd = SYS_BUS_DEVICE(cxl->cxl_host_bridge);
-    struct MemoryRegion *mr = sbd->mmio[0].memory;
+    MemoryRegion *mr = sysbus_mmio_get_region(sbd, 0);
 
     /* Type */
     build_append_int_noprefix(table_data, 0, 1);
-- 
2.51.0

[PATCH v2 6/7] hw/ppc/e500: Use proper SysBus accessors

[Philippe Mathieu-Daudé]

SysBusDevice::mmio[] is kind of internal. Use the proper
sysbus_mmio_get_region() accessor.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ppc/e500.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 8842f7f6b88..fe1aeffe676 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -887,7 +887,7 @@ static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
 
     s = SYS_BUS_DEVICE(dev);
     memory_region_add_subregion(ccsr, MPC8544_MPIC_REGS_OFFSET,
-                                s->mmio[0].memory);
+                                sysbus_mmio_get_region(s, 0));
 
     return dev;
 }
-- 
2.51.0

[PATCH v2 7/7] hw/pci-bridge/pci_expander_bridge: Use proper SysBus accessors

[Philippe Mathieu-Daudé]

SysBusDevice::mmio[] is kind of internal. Use the proper
sysbus_mmio_get_region() accessor.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-bridge/pci_expander_bridge.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 1bcceddbc4d..b4e2c95ae4c 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -157,9 +157,9 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
     main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
     main_host_sbd = SYS_BUS_DEVICE(main_host);
 
-    if (main_host_sbd->num_mmio > 0) {
-        return g_strdup_printf(HWADDR_FMT_plx ",%x",
-                               main_host_sbd->mmio[0].addr, position + 1);
+    if (sysbus_has_mmio(main_host_sbd, 0)) {
+        MemoryRegion *mr = sysbus_mmio_get_region(main_host_sbd, 0);
+        return g_strdup_printf(HWADDR_FMT_plx ",%x", mr->addr, position + 1);
     }
     if (main_host_sbd->num_pio > 0) {
         return g_strdup_printf("i%04x,%x",
-- 
2.51.0

Re: [PATCH v2 1/7] hw/sysbus: Use memory_region_name()

[Marc-André Lureau]

On Tue, Oct 28, 2025 at 12:06 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> MemoryRegion::name is kind of internal. Use the proper
> memory_region_name() accessor, which might return a different
> name.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  hw/core/sysbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index ae447c1196a..d33be6b2b52 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -154,7 +154,7 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr)
>  int sysbus_mmio_map_name(SysBusDevice *dev, const char *name, hwaddr addr)
>  {
>      for (int i = 0; i < dev->num_mmio; i++) {
> -        if (!strcmp(dev->mmio[i].memory->name, name)) {
> +        if (!strcmp(memory_region_name(dev->mmio[i].memory), name)) {
>              sysbus_mmio_map(dev, i, addr);
>              return i;
>          }
> --
> 2.51.0
>
>


--
Marc-André Lureau

Re: [PATCH v2 0/7] hw/sysbus: Spring cleanups (part 1)

[Marc-André Lureau]

Hi

On Tue, Oct 28, 2025 at 12:04 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> - Use proper SysBus accessors
>
> Since v1:
> - Reduce series
>
> Philippe Mathieu-Daudé (7):
>   hw/sysbus: Use memory_region_name()
>   hw/i386/microvm: Use proper SysBus accessors
>   hw/i386/ioapic: Use proper SysBus accessors
>   hw/timer/hpet: Use proper SysBus accessors
>   hw/acpi/cxl: Use proper SysBus accessors
>   hw/ppc/e500: Use proper SysBus accessors
>   hw/pci-bridge/pci_expander_bridge: Use proper SysBus accessors
>

There are a bunch of compilation issues that I will let you address for v3.

It looks like it should be possible to remove the "addr" field from
SysBusDevice.mmio. On the surface it looks redundante with
MemoryRegion.addr. I might be missing something.



>  hw/acpi/cxl.c                       | 2 +-
>  hw/core/sysbus.c                    | 2 +-
>  hw/i386/kvm/ioapic.c                | 3 ++-
>  hw/i386/microvm-dt.c                | 6 ++++--
>  hw/pci-bridge/pci_expander_bridge.c | 6 +++---
>  hw/ppc/e500.c                       | 2 +-
>  hw/timer/hpet.c                     | 3 ++-
>  7 files changed, 14 insertions(+), 10 deletions(-)
>
> --
> 2.51.0
>
>


-- 
Marc-André Lureau

Re: [PATCH v2 0/7] hw/sysbus: Spring cleanups (part 1)

[Peter Maydell]

On Tue, 28 Oct 2025 at 08:50, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Tue, Oct 28, 2025 at 12:04 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > - Use proper SysBus accessors
> >
> > Since v1:
> > - Reduce series
> >
> > Philippe Mathieu-Daudé (7):
> >   hw/sysbus: Use memory_region_name()
> >   hw/i386/microvm: Use proper SysBus accessors
> >   hw/i386/ioapic: Use proper SysBus accessors
> >   hw/timer/hpet: Use proper SysBus accessors
> >   hw/acpi/cxl: Use proper SysBus accessors
> >   hw/ppc/e500: Use proper SysBus accessors
> >   hw/pci-bridge/pci_expander_bridge: Use proper SysBus accessors
> >
>
> There are a bunch of compilation issues that I will let you address for v3.
>
> It looks like it should be possible to remove the "addr" field from
> SysBusDevice.mmio. On the surface it looks redundante with
> MemoryRegion.addr. I might be missing something.

Yes, I think they'll always be the same value. But
MemoryRegion::addr is private data of MemoryRegion
and there's no 'get' function provided; SysBus shouldn't
be reaching inside an MR struct to look at its internals.

(There is a memory_region_set_address(), so we could
I guess provide a memory_region_get_address() ?)

-- PMM

Re: [PATCH v2 0/7] hw/sysbus: Spring cleanups (part 1)

[Philippe Mathieu-Daudé]

On 28/10/25 09:49, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 28, 2025 at 12:04 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> - Use proper SysBus accessors
>>
>> Since v1:
>> - Reduce series
>>
>> Philippe Mathieu-Daudé (7):
>>    hw/sysbus: Use memory_region_name()
>>    hw/i386/microvm: Use proper SysBus accessors
>>    hw/i386/ioapic: Use proper SysBus accessors
>>    hw/timer/hpet: Use proper SysBus accessors
>>    hw/acpi/cxl: Use proper SysBus accessors
>>    hw/ppc/e500: Use proper SysBus accessors
>>    hw/pci-bridge/pci_expander_bridge: Use proper SysBus accessors
>>
> 
> There are a bunch of compilation issues that I will let you address for v3.

Sorry, this is based on my latest PR:
https://github.com/philmd/qemu/releases/tag/hw-misc-20251028
Based-on: <20251028074901.22062-1-philmd@linaro.org>

Re: [PATCH v2 0/7] hw/sysbus: Spring cleanups (part 1)

[Philippe Mathieu-Daudé]

On 28/10/25 10:16, Peter Maydell wrote:
> On Tue, 28 Oct 2025 at 08:50, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>>
>> Hi
>>
>> On Tue, Oct 28, 2025 at 12:04 PM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>>
>>> - Use proper SysBus accessors
>>>
>>> Since v1:
>>> - Reduce series
>>>
>>> Philippe Mathieu-Daudé (7):
>>>    hw/sysbus: Use memory_region_name()
>>>    hw/i386/microvm: Use proper SysBus accessors
>>>    hw/i386/ioapic: Use proper SysBus accessors
>>>    hw/timer/hpet: Use proper SysBus accessors
>>>    hw/acpi/cxl: Use proper SysBus accessors
>>>    hw/ppc/e500: Use proper SysBus accessors
>>>    hw/pci-bridge/pci_expander_bridge: Use proper SysBus accessors
>>>
>>
>> There are a bunch of compilation issues that I will let you address for v3.
>>
>> It looks like it should be possible to remove the "addr" field from
>> SysBusDevice.mmio. On the surface it looks redundante with
>> MemoryRegion.addr. I might be missing something.
> 
> Yes, I think they'll always be the same value. But
> MemoryRegion::addr is private data of MemoryRegion
> and there's no 'get' function provided; SysBus shouldn't
> be reaching inside an MR struct to look at its internals.
> 
> (There is a memory_region_set_address(), so we could
> I guess provide a memory_region_get_address() ?)

OK, thanks!

Re: [PATCH v2 6/7] hw/ppc/e500: Use proper SysBus accessors

[BALATON Zoltan]

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

On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
> SysBusDevice::mmio[] is kind of internal. Use the proper
> sysbus_mmio_get_region() accessor.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/ppc/e500.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 8842f7f6b88..fe1aeffe676 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -887,7 +887,7 @@ static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
>
>     s = SYS_BUS_DEVICE(dev);
>     memory_region_add_subregion(ccsr, MPC8544_MPIC_REGS_OFFSET,
> -                                s->mmio[0].memory);
> +                                sysbus_mmio_get_region(s, 0));
>
>     return dev;
> }
>