On 25/10/25 16:49, BALATON Zoltan wrote:
Hello,
Added a few more people to cc hoping to get some opinion to clear this
up. This is brought up by my patch trying to simplify hw/pci-host/
raven.c part of this series:
https://patchew.org/QEMU/[email protected]/
(First submitted in May here:
https://patchew.org/QEMU/[email protected]/
but that went relatively unnoticed and missed the previous release.)
Find discussion below the patch.
On Sat, 25 Oct 2025, BALATON Zoltan wrote:
On Fri, 24 Oct 2025, Mark Cave-Ayland wrote:
On 23/10/2025 16:26, BALATON Zoltan wrote:
Export memory regions as sysbus mmio regions and let the board code
map them similar to how it is done in grackle. While at it rename
raven_pcihost_realizefn to raven_pcihost_realize.
Signed-off-by: BALATON Zoltan <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
---
hw/pci-host/raven.c | 38 +++++++++++++-------------------------
hw/ppc/prep.c | 10 ++++++++--
2 files changed, 21 insertions(+), 27 deletions(-)
@@ -180,7 +178,18 @@ static void raven_pcihost_realizefn(DeviceState
*d, Error **errp)
qdev_init_gpio_in(d, raven_change_gpio, 1);
+ memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
+ memory_region_init_io(&s->pci_discontiguous_io, o,
+ &raven_io_ops, &s->pci_io,
+ "pci-discontiguous-io", 8 * MiB);
+ memory_region_set_enabled(&s->pci_discontiguous_io, false);
+ memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
+
+ sysbus_init_mmio(dev, &s->pci_io);
+ sysbus_init_mmio(dev, &s->pci_discontiguous_io);
+ sysbus_init_mmio(dev, &s->pci_memory);
sysbus_init_irq(dev, &s->irq);
+
h->bus = pci_register_root_bus(d, NULL, raven_set_irq,
raven_map_irq,
&s->irq, &s->pci_memory, &s-
>pci_io, 0, 1,
TYPE_PCI_BUS);
@@ -219,32 +228,12 @@ static void
raven_pcihost_realizefn(DeviceState *d, Error **errp)
pci_setup_iommu(h->bus, &raven_iommu_ops, s);
}
-static void raven_pcihost_initfn(Object *obj)
-{
- PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
- MemoryRegion *address_space_mem = get_system_memory();
-
- memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
- memory_region_init_io(&s->pci_discontiguous_io, obj,
- &raven_io_ops, &s->pci_io,
- "pci-discontiguous-io", 8 * MiB);
- memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
-
- /* CPU address space */
- memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
- &s->pci_io);
- memory_region_add_subregion_overlap(address_space_mem,
PCI_IO_BASE_ADDR,
- &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);
-}
-
@@ -293,6 +296,9 @@ static void ibm_40p_init(MachineState *machine)
pcihost = SYS_BUS_DEVICE(dev);
object_property_add_child(qdev_get_machine(), "raven",
OBJECT(dev));
sysbus_realize_and_unref(pcihost, &error_fatal);
+ sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
+ sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
+ sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
if (!pci_bus) {
error_report("could not create PCI host controller");
In general the expectation is that memory regions should be
initialised in the _init() function, unless they depend upon a
property in which case they should be initialised in the _realize()
function. Why do you feel this needs to be different?
Is any of it needed before realize? If not why have an init method at
all? As shown here this works perfectly without one and is more
comprehensible that way for people reading it without deep knowledge
about Qdev. In general I think simple devices only need a realize
method and the init method is rarely needed, e.g. if there are some
child objects that need to be init for passing properties that can be
set before realize or similar unusual cases but for most classes init
is not needed at all. I only want to keep what's necessary and remove
everything that's not needed. I think that makes the device model
easier to understand.
I've checked documentation here:
https://www.qemu.org/docs/master/devel/qdev-api.html
but it's not really clear on what's the preferred way of using init and
realize. It's not even very clear on when to use which to me. So becuase
that did not help I did a quick survey on what other pci-host models do.
Of the 32 .c files in hw/pci-host 16 have an init method:
aspeed_pcie.c, astro.c, designware.c, gpex.c, grackle.c, i440fx.c,
pnv_phb3.c, pnv_phb3_msi.c, pnv_phb3_pbcq.c, pnv_phb4.c, q35.c, raven.c,
sabre.c, uninorth.c, versatile.c, xilinx-pcie.c
Of these astro.c has an empty init function that should be removed;
grackle.c, sabre.c and uninorth.c are maintained by you so I'll ignore
them here; we're discussing raven.c now and i440fx.c has two
memory_region_init_io calls in init that could be in realize where all
others are and otherwise all other models do this in realize and only
init child objects and add properties in init methods when that's needed
because they need to be available before realize. The other 16 device
models don't have an init method at all and do all in the realize like I
proposed in this patch for raven. Since only device models that you
maintain do it differently I think what you say is not following the
preferred way so you should not block this patch.
I'd be interested if there is a consensus on this or can we cone to one
that we can document to avoid this repeating every time.
I've been told to stop arguing about QDev on the mailing list, and
instead spend my time and energy in documenting QDev, so we'll discuss
the documentation patches :)
Also we'll try to provide a QDev meaningful state machine, which will
help to enforce doing in the correct places.
Meanwhile...
.instance_init is actually QOM layer, it is called once, and can
not fail. What is allocated here has to be de-allocated in
.instance_finalize.
.realize is QDev where we check the device properties, reporting error.
What is allocated/configured there has to be de-allocated in .unrealize.
The big difference is for hot-pluggable devices, where unplug calls
unrealize(), keeping the device initialized. Re-plug calls .realize()
again, and we should be able to do that multiple times.
With that in mind, IMO it is better to allocate all we can once in
.instance_init().
