1. Patchew
  2. QEMU
  3. [PATCH 00/16] hw/pci-host/raven clean ups
  4. View patch

[PATCH 11/16] hw/pci-host/raven: Do not use parent object for mmcfg region

BALATON Zoltan posted 16 patches 7 months, 2 weeks ago
Maintainers: "Hervé Poussineau" <hpoussin@reactos.org>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
[PATCH 11/16] hw/pci-host/raven: Do not use parent object for mmcfg region
Posted by BALATON Zoltan 7 months, 2 weeks ago 
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 c39e95b45f..7550c291c6 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -212,7 +212,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);
 
@@ -229,9 +229,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 e52d8ec2cd..7c0285e2ff 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;
-- 
2.41.3
Re: [PATCH 11/16] hw/pci-host/raven: Do not use parent object for mmcfg region
Posted by Philippe Mathieu-Daudé 6 months, 2 weeks ago 
On 4/5/25 18:01, 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.
> 

Well, this is the recommended way to avoid leaking MemoryRegions.

If QOM object allocates something, it should keep a reference to it,
allowing simpler eventual implementation of DeviceUnrealize handler.

> 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 c39e95b45f..7550c291c6 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -212,7 +212,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);
>   
> @@ -229,9 +229,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 e52d8ec2cd..7c0285e2ff 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;
Re: [PATCH 11/16] hw/pci-host/raven: Do not use parent object for mmcfg region
Posted by BALATON Zoltan 6 months, 2 weeks ago 
On Tue, 3 Jun 2025, Philippe Mathieu-Daudé wrote:
> On 4/5/25 18:01, 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.
>> 
>
> Well, this is the recommended way to avoid leaking MemoryRegions.
>
> If QOM object allocates something, it should keep a reference to it,
> allowing simpler eventual implementation of DeviceUnrealize handler.

MemoryRegions are already tracked by owner object so no need to free them 
in unrealize or embed them in stat struct to avoid leaking them. I'd only 
store things in state that are really needed.

Regards,
BALATON Zoltan

>> 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 c39e95b45f..7550c291c6 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -212,7 +212,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);
>>   @@ -229,9 +229,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 e52d8ec2cd..7c0285e2ff 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;
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.