Re: [PATCH v5 07/13] hw/pci-host/raven: Do not use parent object for mmcfg region

[BALATON Zoltan]

On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
On 23/10/2025 16:26, BALATON Zoltan wrote:
The mmcfg field in PCIHostState is only used by raven for the PCI
config direct access but is not actually needed as the memory region
lifetime can be managed by the object given during init so use that
and remove the unused field from PCIHostState.
Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
---
  hw/pci-host/raven.c       | 7 ++++---
  include/hw/pci/pci_host.h | 1 -
  2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 2057a1869f..23020fd09f 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -216,7 +216,7 @@ static void raven_pcihost_realizefn(DeviceState *d, 
Error **errp)
      SysBusDevice *dev = SYS_BUS_DEVICE(d);
      PCIHostState *h = PCI_HOST_BRIDGE(dev);
      PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
-    MemoryRegion *address_space_mem = get_system_memory();
+    MemoryRegion *mr, *address_space_mem = get_system_memory();
        qdev_init_gpio_in(d, raven_change_gpio, 1);
  @@ -233,9 +233,10 @@ static void raven_pcihost_realizefn(DeviceState *d, 
Error **errp)
                            "pci-conf-data", 4);
      memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
  -    memory_region_init_io(&h->mmcfg, OBJECT(h), &raven_mmcfg_ops, 
h->bus,
+    mr = g_new0(MemoryRegion, 1);
+    memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
                            "pci-mmcfg", 0x00400000);
-    memory_region_add_subregion(address_space_mem, 0x80800000, &h->mmcfg);
+    memory_region_add_subregion(address_space_mem, 0x80800000, mr);
        memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, 
s,
                            "pci-intack", 1);
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 954dd446fa..a13f879872 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -41,7 +41,6 @@ struct PCIHostState {
        MemoryRegion conf_mem;
      MemoryRegion data_mem;
-    MemoryRegion mmcfg;
      uint32_t config_reg;
      bool mig_enabled;
      PCIBus *bus;

Looking back at this patch in respect of patch 2, should RavenPCIState 
actually be kept and mmcfg moved there instead? It feels like the wrong 
approach simply because it isn't possible to access the MR directly from the 
device state when debugging.

No, that's not even the same object. What I removed in patch 2 is an empty 
subclass for the PCI facing part of the host bridge which is a subclass of 
PCIDevice and this is part of the sysbus facing part (PCIHostState) that 
already has a state struct but we don't need to store this memory region 
there. Not even for debugging, because all it does is just forwarding to 
PCI (look at the read/write ops) so you can fully debug it by -trace 
enable="pci*" or add debug logs to the read/write ops if you want. We also 
don't need to put it in a state struct to free it as 
memory_region_init_io(mr, OBJECT...) takes care of that and frees it with 
the OBJECT so I don't think what you say is a real concern or should block 
this patch.

Regards,
BALATON Zoltan