Use fw_cfg_init_io_nodma() instead of open-coding it.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/sparc64/sun4u.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 82c3e7c855b..6dc9f64b74d 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
graphic_width, graphic_height, graphic_depth,
(uint8_t *)&macaddr);
- dev = qdev_new(TYPE_FW_CFG_IO);
- qdev_prop_set_bit(dev, "dma_enabled", false);
- object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev));
- sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
- memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
- &FW_CFG_IO(dev)->comb_iomem);
-
- fw_cfg = FW_CFG(dev);
+ fw_cfg = fw_cfg_init_io_nodma(pci_address_space_io(ebus), BIOS_CFG_IOPORT);
fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)machine->smp.cpus);
fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)machine->smp.max_cpus);
fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)machine->ram_size);
--
2.51.0
Hi Philippe,
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 82c3e7c855b..6dc9f64b74d 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> graphic_width, graphic_height, graphic_depth,
> (uint8_t *)&macaddr);
>
> - dev = qdev_new(TYPE_FW_CFG_IO);
> - qdev_prop_set_bit(dev, "dma_enabled", false);
> - object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev));
There's another difference: fw_cfg_init_io_nodma() uses `machine` as the
parent and here sun4uv uses `ebus`.
I think maybe one reason to use `ebus` is because...
> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> - memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
^^^^^^^^^^^^^^^^^^^^^^^^^^
... because the parent region is managed by ebus.
Perhaps we should add another argument: Object *parent?
> - &FW_CFG_IO(dev)->comb_iomem);
> -
Thanks,
Zhao
On 3/12/25 16:51, Zhao Liu wrote: > Hi Philippe, > >> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c >> index 82c3e7c855b..6dc9f64b74d 100644 >> --- a/hw/sparc64/sun4u.c >> +++ b/hw/sparc64/sun4u.c >> @@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem, >> graphic_width, graphic_height, graphic_depth, >> (uint8_t *)&macaddr); >> >> - dev = qdev_new(TYPE_FW_CFG_IO); >> - qdev_prop_set_bit(dev, "dma_enabled", false); >> - object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev)); > > There's another difference: fw_cfg_init_io_nodma() uses `machine` as the > parent and here sun4uv uses `ebus`. Ah yeah I wanted to comment it but forgot :facepalm: > > I think maybe one reason to use `ebus` is because... > >> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >> - memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > ... because the parent region is managed by ebus. > > Perhaps we should add another argument: Object *parent? I thought about it but don't think so, all instances but this one use the machine container. I'll improve the description. > >> - &FW_CFG_IO(dev)->comb_iomem); >> - > > Thanks, > Zhao >
On 03/12/2025 17:14, Philippe Mathieu-Daudé wrote: > On 3/12/25 16:51, Zhao Liu wrote: >> Hi Philippe, >> >>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c >>> index 82c3e7c855b..6dc9f64b74d 100644 >>> --- a/hw/sparc64/sun4u.c >>> +++ b/hw/sparc64/sun4u.c >>> @@ -683,14 +683,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem, >>> graphic_width, graphic_height, graphic_depth, >>> (uint8_t *)&macaddr); >>> - dev = qdev_new(TYPE_FW_CFG_IO); >>> - qdev_prop_set_bit(dev, "dma_enabled", false); >>> - object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev)); >> >> There's another difference: fw_cfg_init_io_nodma() uses `machine` as the >> parent and here sun4uv uses `ebus`. > > Ah yeah I wanted to comment it but forgot :facepalm: > >> >> I think maybe one reason to use `ebus` is because... >> >>> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >>> - memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, >> ^^^^^^^^^^^^^^^^^^^^^^^^^^ >> ... because the parent region is managed by ebus. >> >> Perhaps we should add another argument: Object *parent? > > I thought about it but don't think so, all instances but this one use > the machine container. > > I'll improve the description. The reason that the fw_cfg device lives under ebus on sun4u is because the ebus device is effectively a PCI-ISA bridge, and the fw_cfg port is mapped into I/O address space along with other ISA devices. I'm not sure that setting the parent to the machine is the right thing to do here. ATB, Mark.
> > > I think maybe one reason to use `ebus` is because... > > > > > > > - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > > > > - memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > ... because the parent region is managed by ebus. > > > > > > Perhaps we should add another argument: Object *parent? > > > > I thought about it but don't think so, all instances but this one use > > the machine container. > > > > I'll improve the description. > > The reason that the fw_cfg device lives under ebus on sun4u is because the > ebus device is effectively a PCI-ISA bridge, and the fw_cfg port is mapped > into I/O address space along with other ISA devices. I'm not sure that > setting the parent to the machine is the right thing to do here. Thanks Philippe and Mark, IIUC, ebus doesn't have the hotplug use case, so it lives long enough like the sun4u/v machine, therefore replacing the parent object "ebus" with machine is safe. And it might be better to explicitly set ebus as not supporting hotplug (via dc->hotpluggable = false). Adding a "parent" argument to the generic interface fw_cfg_init_io_nodma() doesn't seem necessary at the moment, since using the default machine as parent seems enough to meet all current needs in QEMU. What do you think? Regards, Zhao
On 18/12/25 09:52, Zhao Liu wrote: >>>> I think maybe one reason to use `ebus` is because... >>>> >>>>> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >>>>> - memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> ... because the parent region is managed by ebus. >>>> >>>> Perhaps we should add another argument: Object *parent? >>> >>> I thought about it but don't think so, all instances but this one use >>> the machine container. >>> >>> I'll improve the description. >> >> The reason that the fw_cfg device lives under ebus on sun4u is because the >> ebus device is effectively a PCI-ISA bridge, and the fw_cfg port is mapped >> into I/O address space along with other ISA devices. I'm not sure that >> setting the parent to the machine is the right thing to do here. > > Thanks Philippe and Mark, IIUC, ebus doesn't have the hotplug use case, > so it lives long enough like the sun4u/v machine, therefore replacing > the parent object "ebus" with machine is safe. > > And it might be better to explicitly set ebus as not supporting hotplug > (via dc->hotpluggable = false). > > Adding a "parent" argument to the generic interface fw_cfg_init_io_nodma() > doesn't seem necessary at the moment, since using the default machine as > parent seems enough to meet all current needs in QEMU. > > What do you think? fw_cfg is per guest, and there can only be once instance of it; so IMHO it makes sense to use the machine as parent. I should have posted a better commit description upfront, sorry.
On 18/12/2025 11:22, Philippe Mathieu-Daudé wrote: > On 18/12/25 09:52, Zhao Liu wrote: >>>>> I think maybe one reason to use `ebus` is because... >>>>> >>>>>> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >>>>>> - memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, >>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> ... because the parent region is managed by ebus. >>>>> >>>>> Perhaps we should add another argument: Object *parent? >>>> >>>> I thought about it but don't think so, all instances but this one use >>>> the machine container. >>>> >>>> I'll improve the description. >>> >>> The reason that the fw_cfg device lives under ebus on sun4u is because the >>> ebus device is effectively a PCI-ISA bridge, and the fw_cfg port is mapped >>> into I/O address space along with other ISA devices. I'm not sure that >>> setting the parent to the machine is the right thing to do here. >> >> Thanks Philippe and Mark, IIUC, ebus doesn't have the hotplug use case, >> so it lives long enough like the sun4u/v machine, therefore replacing >> the parent object "ebus" with machine is safe. >> >> And it might be better to explicitly set ebus as not supporting hotplug >> (via dc->hotpluggable = false). >> >> Adding a "parent" argument to the generic interface fw_cfg_init_io_nodma() >> doesn't seem necessary at the moment, since using the default machine as >> parent seems enough to meet all current needs in QEMU. >> >> What do you think? > > fw_cfg is per guest, and there can only be once instance of it; so IMHO it > makes sense to use the machine as parent. I should have posted a better > commit description upfront, sorry. That's true, but the I/O is mapped to the ISA bus and so if the ISA bus does not exist, it is impossible to access the device even if it exists on the machine. Following on from this: at some point should we not be aiming for a qdev bus to have its devices as QOM children? Assigning fw_cfg directly to the machine would also be wrong in this scenario. ATB, Mark.
> fw_cfg is per guest, and there can only be once instance of it; so IMHO it > makes sense to use the machine as parent. I should have posted a better > commit description upfront, sorry. You're welcome. It looks like most issues with the another series for removing v2.6 & 2.7 pc have been resolved, so I'm trying to help clarify this part to move things forward a bit. Regards, Zhao
On 18/12/2025 08:52, Zhao Liu wrote: >>>> I think maybe one reason to use `ebus` is because... >>>> >>>>> - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >>>>> - memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> ... because the parent region is managed by ebus. >>>> >>>> Perhaps we should add another argument: Object *parent? >>> >>> I thought about it but don't think so, all instances but this one use >>> the machine container. >>> >>> I'll improve the description. >> >> The reason that the fw_cfg device lives under ebus on sun4u is because the >> ebus device is effectively a PCI-ISA bridge, and the fw_cfg port is mapped >> into I/O address space along with other ISA devices. I'm not sure that >> setting the parent to the machine is the right thing to do here. > > Thanks Philippe and Mark, IIUC, ebus doesn't have the hotplug use case, > so it lives long enough like the sun4u/v machine, therefore replacing > the parent object "ebus" with machine is safe. It's safe, but it still doesn't make sense for sun4u/v because there is no machine-level I/O address space as per x86. It really does exist as a separate legacy bus under a PCI bridge. > And it might be better to explicitly set ebus as not supporting hotplug > (via dc->hotpluggable = false). That's correct, ebus does not support hotplug. > Adding a "parent" argument to the generic interface fw_cfg_init_io_nodma() > doesn't seem necessary at the moment, since using the default machine as > parent seems enough to meet all current needs in QEMU. > > What do you think? My preference would be add to add the parent argument as it's easy to do, and doesn't attempt to enforce x86-type constraints on other architectures. ATB, Mark.
> > > The reason that the fw_cfg device lives under ebus on sun4u is because the > > > ebus device is effectively a PCI-ISA bridge, and the fw_cfg port is mapped > > > into I/O address space along with other ISA devices. I'm not sure that > > > setting the parent to the machine is the right thing to do here. > > > > Thanks Philippe and Mark, IIUC, ebus doesn't have the hotplug use case, > > so it lives long enough like the sun4u/v machine, therefore replacing > > the parent object "ebus" with machine is safe. > > It's safe, but it still doesn't make sense for sun4u/v because there is no > machine-level I/O address space as per x86. It really does exist as a > separate legacy bus under a PCI bridge. I tend to agree with Philippe's perspective - since it's per-Guest, setting the parent as machine seems appropriate. > That's correct, ebus does not support hotplug. > > > Adding a "parent" argument to the generic interface fw_cfg_init_io_nodma() > > doesn't seem necessary at the moment, since using the default machine as > > parent seems enough to meet all current needs in QEMU. > > > > What do you think? > > My preference would be add to add the parent argument as it's easy to do, > and doesn't attempt to enforce x86-type constraints on other architectures. I think it's not a x86-specific issue. The parent parameter's role is lifecycle management - if we allowed custom parents, we'd have to account for various complications that could result from early parent destruction, so it's not much easy IMO. Thanks, Zhao
© 2016 - 2026 Red Hat, Inc.