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 <balaton@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