Move the lines related to creating the bus master address space
together and reduce the number of memory regions stored in the device
state. These are used once to create the address space and can be
tracked with their owner object so no need to keep track of them in
the device state. Keep only the address space that is used later in a
callback.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
hw/pci-host/raven.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index bf4f4b7f71..ebf0c511dc 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -46,9 +46,6 @@ struct PREPPCIState {
MemoryRegion pci_discontiguous_io;
MemoryRegion pci_memory;
MemoryRegion pci_intack;
- MemoryRegion bm;
- MemoryRegion bm_ram_alias;
- MemoryRegion bm_pci_memory_alias;
AddressSpace bm_as;
};
@@ -178,7 +175,8 @@ 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 *mr, *address_space_mem = get_system_memory();
+ Object *o = OBJECT(d);
+ MemoryRegion *mr, *bm, *address_space_mem = get_system_memory();
qdev_init_gpio_in(d, raven_change_gpio, 1);
@@ -187,26 +185,37 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
&s->irq, &s->pci_memory, &s->pci_io, 0, 1,
TYPE_PCI_BUS);
- memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
+ memory_region_init_io(&h->conf_mem, o, &pci_host_conf_le_ops, s,
"pci-conf-idx", 4);
memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
- memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
+ memory_region_init_io(&h->data_mem, o, &pci_host_data_le_ops, s,
"pci-conf-data", 4);
memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
mr = g_new0(MemoryRegion, 1);
- memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
+ memory_region_init_io(mr, o, &raven_mmcfg_ops, h->bus,
"pci-mmcfg", 8 * MiB);
memory_region_add_subregion(&s->pci_io, 0x800000, mr);
- memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
+ memory_region_init_io(&s->pci_intack, o, &raven_intack_ops, s,
"pci-intack", 1);
memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
pci_create_simple(h->bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
- address_space_init(&s->bm_as, &s->bm, "raven-bm");
+ /* Bus master address space */
+ bm = g_new0(MemoryRegion, 1);
+ memory_region_init(bm, o, "raven-bm", 4 * GiB);
+ mr = g_new0(MemoryRegion, 1);
+ memory_region_init_alias(mr, o, "bm-pci-memory", &s->pci_memory, 0,
+ memory_region_size(&s->pci_memory));
+ memory_region_add_subregion(bm, 0, mr);
+ mr = g_new0(MemoryRegion, 1);
+ memory_region_init_alias(mr, o, "bm-system", get_system_memory(),
+ 0, 0x80000000);
+ memory_region_add_subregion(bm, 0x80000000, mr);
+ address_space_init(&s->bm_as, bm, "raven-bm-as");
pci_setup_iommu(h->bus, &raven_iommu_ops, s);
}
@@ -228,16 +237,6 @@ static void raven_pcihost_initfn(Object *obj)
&s->pci_discontiguous_io, 1);
memory_region_set_enabled(&s->pci_discontiguous_io, false);
memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-
- /* Bus master address space */
- memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
- memory_region_init_alias(&s->bm_pci_memory_alias, obj, "bm-pci-memory",
- &s->pci_memory, 0,
- memory_region_size(&s->pci_memory));
- memory_region_init_alias(&s->bm_ram_alias, obj, "bm-system",
- get_system_memory(), 0, 0x80000000);
- memory_region_add_subregion(&s->bm, 0 , &s->bm_pci_memory_alias);
- memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
}
static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
--
2.41.3On 18/09/2025 19:50, BALATON Zoltan wrote:
> Move the lines related to creating the bus master address space
> together and reduce the number of memory regions stored in the device
> state. These are used once to create the address space and can be
> tracked with their owner object so no need to keep track of them in
> the device state. Keep only the address space that is used later in a
> callback.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/pci-host/raven.c | 37 ++++++++++++++++++-------------------
> 1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index bf4f4b7f71..ebf0c511dc 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -46,9 +46,6 @@ struct PREPPCIState {
> MemoryRegion pci_discontiguous_io;
> MemoryRegion pci_memory;
> MemoryRegion pci_intack;
> - MemoryRegion bm;
> - MemoryRegion bm_ram_alias;
> - MemoryRegion bm_pci_memory_alias;
> AddressSpace bm_as;
> };
>
> @@ -178,7 +175,8 @@ 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 *mr, *address_space_mem = get_system_memory();
> + Object *o = OBJECT(d);
> + MemoryRegion *mr, *bm, *address_space_mem = get_system_memory();
>
> qdev_init_gpio_in(d, raven_change_gpio, 1);
>
> @@ -187,26 +185,37 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
> &s->irq, &s->pci_memory, &s->pci_io, 0, 1,
> TYPE_PCI_BUS);
>
> - memory_region_init_io(&h->conf_mem, OBJECT(h), &pci_host_conf_le_ops, s,
> + memory_region_init_io(&h->conf_mem, o, &pci_host_conf_le_ops, s,
> "pci-conf-idx", 4);
> memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
>
> - memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops, s,
> + memory_region_init_io(&h->data_mem, o, &pci_host_data_le_ops, s,
> "pci-conf-data", 4);
> memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>
> mr = g_new0(MemoryRegion, 1);
> - memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
> + memory_region_init_io(mr, o, &raven_mmcfg_ops, h->bus,
> "pci-mmcfg", 8 * MiB);
> memory_region_add_subregion(&s->pci_io, 0x800000, mr);
>
> - memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops, s,
> + memory_region_init_io(&s->pci_intack, o, &raven_intack_ops, s,
> "pci-intack", 1);
> memory_region_add_subregion(address_space_mem, 0xbffffff0, &s->pci_intack);
>
> pci_create_simple(h->bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
>
> - address_space_init(&s->bm_as, &s->bm, "raven-bm");
> + /* Bus master address space */
> + bm = g_new0(MemoryRegion, 1);
> + memory_region_init(bm, o, "raven-bm", 4 * GiB);
> + mr = g_new0(MemoryRegion, 1);
> + memory_region_init_alias(mr, o, "bm-pci-memory", &s->pci_memory, 0,
> + memory_region_size(&s->pci_memory));
> + memory_region_add_subregion(bm, 0, mr);
> + mr = g_new0(MemoryRegion, 1);
> + memory_region_init_alias(mr, o, "bm-system", get_system_memory(),
> + 0, 0x80000000);
> + memory_region_add_subregion(bm, 0x80000000, mr);
> + address_space_init(&s->bm_as, bm, "raven-bm-as");
> pci_setup_iommu(h->bus, &raven_iommu_ops, s);
> }
>
> @@ -228,16 +237,6 @@ static void raven_pcihost_initfn(Object *obj)
> &s->pci_discontiguous_io, 1);
> memory_region_set_enabled(&s->pci_discontiguous_io, false);
> memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
> -
> - /* Bus master address space */
> - memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
> - memory_region_init_alias(&s->bm_pci_memory_alias, obj, "bm-pci-memory",
> - &s->pci_memory, 0,
> - memory_region_size(&s->pci_memory));
> - memory_region_init_alias(&s->bm_ram_alias, obj, "bm-system",
> - get_system_memory(), 0, 0x80000000);
> - memory_region_add_subregion(&s->bm, 0 , &s->bm_pci_memory_alias);
> - memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
> }
>
> static void raven_pcihost_class_init(ObjectClass *klass, const void *data)
Same comment here for patch 9: what is the advantage of changing this if the
lifetimes are not different?
ATB,
Mark.
On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, BALATON Zoltan wrote:
>> Move the lines related to creating the bus master address space
>> together and reduce the number of memory regions stored in the device
>> state. These are used once to create the address space and can be
>> tracked with their owner object so no need to keep track of them in
>> the device state. Keep only the address space that is used later in a
>> callback.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/pci-host/raven.c | 37 ++++++++++++++++++-------------------
>> 1 file changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
>> index bf4f4b7f71..ebf0c511dc 100644
>> --- a/hw/pci-host/raven.c
>> +++ b/hw/pci-host/raven.c
>> @@ -46,9 +46,6 @@ struct PREPPCIState {
>> MemoryRegion pci_discontiguous_io;
>> MemoryRegion pci_memory;
>> MemoryRegion pci_intack;
>> - MemoryRegion bm;
>> - MemoryRegion bm_ram_alias;
>> - MemoryRegion bm_pci_memory_alias;
>> AddressSpace bm_as;
>> };
>> @@ -178,7 +175,8 @@ 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 *mr, *address_space_mem = get_system_memory();
>> + Object *o = OBJECT(d);
>> + MemoryRegion *mr, *bm, *address_space_mem = get_system_memory();
>> qdev_init_gpio_in(d, raven_change_gpio, 1);
>> @@ -187,26 +185,37 @@ static void raven_pcihost_realizefn(DeviceState *d,
>> Error **errp)
>> &s->irq, &s->pci_memory, &s->pci_io,
>> 0, 1,
>> TYPE_PCI_BUS);
>> - memory_region_init_io(&h->conf_mem, OBJECT(h),
>> &pci_host_conf_le_ops, s,
>> + memory_region_init_io(&h->conf_mem, o, &pci_host_conf_le_ops, s,
>> "pci-conf-idx", 4);
>> memory_region_add_subregion(&s->pci_io, 0xcf8, &h->conf_mem);
>> - memory_region_init_io(&h->data_mem, OBJECT(h),
>> &pci_host_data_le_ops, s,
>> + memory_region_init_io(&h->data_mem, o, &pci_host_data_le_ops, s,
>> "pci-conf-data", 4);
>> memory_region_add_subregion(&s->pci_io, 0xcfc, &h->data_mem);
>> mr = g_new0(MemoryRegion, 1);
>> - memory_region_init_io(mr, OBJECT(h), &raven_mmcfg_ops, h->bus,
>> + memory_region_init_io(mr, o, &raven_mmcfg_ops, h->bus,
>> "pci-mmcfg", 8 * MiB);
>> memory_region_add_subregion(&s->pci_io, 0x800000, mr);
>> - memory_region_init_io(&s->pci_intack, OBJECT(s), &raven_intack_ops,
>> s,
>> + memory_region_init_io(&s->pci_intack, o, &raven_intack_ops, s,
>> "pci-intack", 1);
>> memory_region_add_subregion(address_space_mem, 0xbffffff0,
>> &s->pci_intack);
>> pci_create_simple(h->bus, PCI_DEVFN(0, 0), TYPE_RAVEN_PCI_DEVICE);
>> - address_space_init(&s->bm_as, &s->bm, "raven-bm");
>> + /* Bus master address space */
>> + bm = g_new0(MemoryRegion, 1);
>> + memory_region_init(bm, o, "raven-bm", 4 * GiB);
>> + mr = g_new0(MemoryRegion, 1);
>> + memory_region_init_alias(mr, o, "bm-pci-memory", &s->pci_memory, 0,
>> + memory_region_size(&s->pci_memory));
>> + memory_region_add_subregion(bm, 0, mr);
>> + mr = g_new0(MemoryRegion, 1);
>> + memory_region_init_alias(mr, o, "bm-system", get_system_memory(),
>> + 0, 0x80000000);
>> + memory_region_add_subregion(bm, 0x80000000, mr);
>> + address_space_init(&s->bm_as, bm, "raven-bm-as");
>> pci_setup_iommu(h->bus, &raven_iommu_ops, s);
>> }
>> @@ -228,16 +237,6 @@ static void raven_pcihost_initfn(Object *obj)
>> &s->pci_discontiguous_io, 1);
>> memory_region_set_enabled(&s->pci_discontiguous_io, false);
>> memory_region_add_subregion(address_space_mem, 0xc0000000,
>> &s->pci_memory);
>> -
>> - /* Bus master address space */
>> - memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
>> - memory_region_init_alias(&s->bm_pci_memory_alias, obj,
>> "bm-pci-memory",
>> - &s->pci_memory, 0,
>> - memory_region_size(&s->pci_memory));
>> - memory_region_init_alias(&s->bm_ram_alias, obj, "bm-system",
>> - get_system_memory(), 0, 0x80000000);
>> - memory_region_add_subregion(&s->bm, 0 ,
>> &s->bm_pci_memory_alias);
>> - memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
>> }
>> static void raven_pcihost_class_init(ObjectClass *klass, const void
>> *data)
>
> Same comment here for patch 9: what is the advantage of changing this if the
> lifetimes are not different?
Same answer. We can get rid of fields from the state struct. We don't need
to store things there that we never use just to manage the lifetime of the
memory region.
Regards,
BALATON Zoltan
© 2016 - 2025 Red Hat, Inc.