On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, 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;
>
> Is there any reason that the lifetime of the MemoryRegion needs to be
> separate from that of the device itself? If not, I'm struggling to understand
> why this needs to be changed.
I've tried to explain that in the comment message. The lifetime is still
the same as the device as the memory region is owned by the device but
this way we can remove the mmcfg field from the parent object that does
not belong there and only used by this device (maybe for historical
reasons as this is an old device model).
Regards,
BALATON Zoltan