Hi Markus,
> -----Original Message-----
> From: Markus Armbruster <arm...@redhat.com>
> Sent: Tuesday, May 6, 2025 12:47 PM
> To: Shameer Kolothum via <qemu-devel@nongnu.org>
> Cc: qemu-...@nongnu.org; Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>; eric.au...@redhat.com;
> peter.mayd...@linaro.org; j...@nvidia.com; nicol...@nvidia.com;
> ddut...@redhat.com; berra...@redhat.com; nath...@nvidia.com;
> mo...@nvidia.com; smost...@google.com; Linuxarm
> <linux...@huawei.com>; Wangzhou (B) <wangzh...@hisilicon.com>;
> jiangkunkun <jiangkun...@huawei.com>; Jonathan Cameron
> <jonathan.came...@huawei.com>; zhangfei....@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
>
> Shameer Kolothum via <qemu-devel@nongnu.org> writes:
>
> > Although this change does not affect functionality at present, it lays
> > the groundwork for enabling user-created SMMUv3 devices in
> > future patches
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.th...@huawei.com>
> > ---
> > hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
> > hw/arm/virt.c | 3 ++-
> > 2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index ab67972353..605de9b721 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -24,6 +24,7 @@
> > #include "hw/qdev-properties.h"
> > #include "hw/qdev-core.h"
> > #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bridge.h"
> > #include "cpu.h"
> > #include "exec/target_page.h"
> > #include "trace.h"
> > @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
> ResetType type)
> > smmuv3_init_regs(s);
> > }
> >
> > +static int smmuv3_pcie_bus(Object *obj, void *opaque)
> > +{
> > + DeviceState *d = opaque;
> > + PCIBus *bus;
> > +
> > + if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> > + return 0;
> > + }
> > +
> > + bus = PCI_HOST_BRIDGE(obj)->bus;
> > + if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
> >name)) {
> > + object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> > + &error_abort);
> > + /* Return non-zero as we got the bus and don't need further
> iteration.*/
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > static void smmu_realize(DeviceState *d, Error **errp)
> > {
> > SMMUState *sys = ARM_SMMU(d);
> > @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d, Error
> **errp)
> > SysBusDevice *dev = SYS_BUS_DEVICE(d);
> > Error *local_err = NULL;
> >
> > + if (!object_property_get_link(OBJECT(d), "primary-bus", &error_abort))
> {
> > + object_child_foreach_recursive(object_get_root(),
> smmuv3_pcie_bus, d);
> > + }
> > +
> > c->parent_realize(d, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass
> *klass, const void *data)
> > device_class_set_parent_realize(dc, smmu_realize,
> > &c->parent_realize);
> > device_class_set_props(dc, smmuv3_properties);
> > + dc->hotpluggable = false;
> > + dc->bus_type = TYPE_PCIE_BUS;
>
> This is very, very wrong.
>
> The function serves as .class_init() for QOM type "arm-smmuv3", defined
> as:
>
> static const TypeInfo smmuv3_type_info = {
> .name = TYPE_ARM_SMMUV3,
> .parent = TYPE_ARM_SMMU,
>
> Subtype of "arm-smmuv3".
>
> .instance_size = sizeof(SMMUv3State),
> .instance_init = smmuv3_instance_init,
> .class_size = sizeof(SMMUv3Class),
> .class_init = smmuv3_class_init,
> };
>
>
> static const TypeInfo smmu_base_info = {
> .name = TYPE_ARM_SMMU,
> .parent = TYPE_SYS_BUS_DEVICE,
>
> Subtype of "sys-bus-device".
>
> .instance_size = sizeof(SMMUState),
> .class_data = NULL,
> .class_size = sizeof(SMMUBaseClass),
> .class_init = smmu_base_class_init,
> .abstract = true,
> };
>
> Have a look at the instance struct:
>
> struct SMMUv3State {
> SMMUState smmu_state;
>
> Starts with the supertype's instance struct, as is proper.
>
> uint32_t features;
> [more ...]
> };
>
> Here's the supertype's instance struct:
>
> struct SMMUState {
> /* <private> */
> SysBusDevice dev;
>
> Again, starts with the supertype's instance struct.
>
> const char *mrtypename;
> [more...]
> };
>
> This is a sysbus device, not a PCI device. Monkey-patching dc->bus_type
> from TYPE_SYSTEM_BUS to TYPE_PCIE_BUS won't change that. All it
> accomplishes is making the qdev core believe it plugs into a PCIE bus.
> This can only end in tears.
>
> In fact, when I build with the entire series applied (so the device can
> actually be used with -device), the result dies within seconds of my ad
> hoc testing:
>
> $ qemu-system-aarch64 -nodefaults -S -display none -monitor stdio -M
> virt -device pxb-pcie,id=pcie.1,bus_nr=2 -device arm-smmuv3,bus=pcie.1
> QEMU 10.0.50 monitor - type 'help' for more information
> qemu-system-aarch64: -device arm-smmuv3,bus=pcie.1: warning:
> SMMUv3 device only supported with pcie.0 for DT
> (qemu) info qtree
> bus: main-system-bus
> type System
> dev: pxb-host, id ""
> x-config-reg-migration-enabled = true
> bypass-iommu = false
> bus: pcie.1
> type pxb-pcie-bus
> dev: arm-smmuv3, id ""
> gpio-out "sysbus-irq" 4
> stage = "nested"
> bus_num = 0 (0x0)
> Segmentation fault (core dumped)
Ah..My bad. I didn't test this. Thanks for taking time to test this.
>
> Backtrace:
>
> #0 0x00005555557d8521 in pcibus_dev_print
> (mon=0x55555826d0e0, dev=0x5555590ad360, indent=8)
> at ../hw/pci/pci-hmp-cmds.c:140
> #1 0x0000555555eac0a0 in bus_print_dev
> (bus=<optimized out>, mon=<optimized out>, dev=0x5555590ad360,
> indent=8)
> at ../system/qdev-monitor.c:773
> #2 qdev_print (mon=<optimized out>, dev=<optimized out>, indent=8)
> at ../system/qdev-monitor.c:805
> #3 qbus_print
> (mon=mon@entry=0x55555826d0e0,
> bus=bus@entry=0x5555590ac4a0, indent=6,
> indent@entry=4, details=details@entry=true) at ../system/qdev-
> monitor.c:821
> #4 0x0000555555eabd92 in qbus_print
> (mon=0x55555826d0e0, bus=<optimized out>, indent=2, details=true)
> at ../system/qdev-monitor.c:824
> #5 0x0000555555979789 in handle_hmp_command_exec
> (cmd=<optimized out>, mon=0x55555826d0e0, qdict=0x55555907d8e0)
> at ../monitor/hmp.c:1106
> #6 handle_hmp_command_exec
> (mon=0x55555826d0e0, cmd=0x55555769d220
> <hmp_info_cmds+2560>, qdict=0x55555907d8e0) at ../monitor/hmp.c:1098
> #7 handle_hmp_command (mon=mon@entry=0x55555826d0e0,
> cmdline=<optimized out>,
> cmdline@entry=0x555558657490 "info qtree") at
> ../monitor/hmp.c:1158
> #8 0x000055555597983c in monitor_command_cb
>
> Crash line is
>
> int class = pci_get_word(d->config + PCI_CLASS_DEVICE);
>
> Debugger shows that d->config is invalid. This is hardly surprising!
>
> The qdev core is trying to print information on the "arm_smmuv3" device
> here. It's working with a DeviceState. Your monkey-patching convinced
> it it's a PCIDevice, so it duly calls pcibus_dev_print() to print PCI
> device information. pcibus_dev_print() casts the DeviceState * to
> PCIDevice *. This assumes the device's instance actually starts with
> PCIDevice. It actually starts with SysBusDevice.
>
> Unsurprisingly, and "make check" also fails:
>
> >>> MESON_TEST_ITERATION=1
> MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print
> _stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-
> aarch64 QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-
> daemon.sh
> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1
> PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3
> UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:prin
> t_stacktrace=1 MALLOC_PERTURB_=22 /work/armbru/qemu/bld-
> arm/tests/qtest/test-hmp --tap -k
>
> ―――――――――――――――――――――――――――――――――――
> ―― ✀
> ―――――――――――――――――――――――――――――――――――
> ――
> stderr:
> qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus:
> Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc-
> >bus_type)' failed.
> Broken pipe
> ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
> signal 6 (Aborted) (core dumped)
>
> and
>
> >>> MESON_TEST_ITERATION=1
> MSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:print
> _stacktrace=1 RUST_BACKTRACE=1 QTEST_QEMU_BINARY=./qemu-system-
> aarch64 MALLOC_PERTURB_=86 QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/work/armbru/qemu/tests/dbus-vmstate-
> daemon.sh
> ASAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1
> PYTHON=/work/armbru/qemu/bld-arm/pyvenv/bin/python3
> UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1:print_summary=1:prin
> t_stacktrace=1 /work/armbru/qemu/bld-arm/tests/qtest/qom-test --tap -k
>
> ―――――――――――――――――――――――――――――――――――
> ―― ✀
> ―――――――――――――――――――――――――――――――――――
> ――
> stderr:
> qemu-system-aarch64: ../hw/core/qdev.c:113: qdev_set_parent_bus:
> Assertion `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc-
> >bus_type)' failed.
> Broken pipe
> ../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
> signal 6 (Aborted) (core dumped)
>
> Please make sure "make check" passes before posting patches for review.
> If you need help getting there, post them marked RFC and with the bad
> test results right in the cover letter.
Sure. Will run "make check" in future.
> > }
> >
> > static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 177f3dd22c..3bae4e374f 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -56,6 +56,7 @@
> > #include "qemu/cutils.h"
> > #include "qemu/error-report.h"
> > #include "qemu/module.h"
> > +#include "hw/pci/pci_bus.h"
> > #include "hw/pci-host/gpex.h"
> > #include "hw/virtio/virtio-pci.h"
> > #include "hw/core/sysbus-fdt.h"
> > @@ -1442,7 +1443,7 @@ static void create_smmu(const
> VirtMachineState *vms,
> > }
> > object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
> > &error_abort);
> > - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > + qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
> > sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> > for (i = 0; i < NUM_SMMU_IRQS; i++) {
> > sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>
> What are you trying to accomplish?
>
> I *guess* you're trying to change the "arm-smmuv3" device to be a PCI
> device. Correct?
Not really. SMMUv3 is a platform device and it is normally associated with a
PCIe RC.
Currently Qemu has a machine wide SMMUv3(-machine virt, iommu=smmuv3) and
is associated by default with the Pcie.0 bus. This series as explained in cover
letter adds
support to user-creatable SMMUv3 devices and requires a way to associate a PCIe
RC.
This was initially done in the RFC as below,
-device arm-smmuv3,pci-bus=pcie.0
But there were feedback received that it is more intuitive and easy for libvirt
to use
the Qemu device default "bus" property to achieve this,
-device arm-smmuv3,bus=pcie.0
And hence the change to,
dc->bus_type = TYPE_PCIE_BUS;
Please see the discussion on this we had in this thread,
https://lore.kernel.org/qemu-devel/f1b91034-be2a-493e-9229-c164e0895...@redhat.com/
So from your reply, it is obvious that we can't use the default "bus" as used
this patch and
probably the only way is to use a different property name like,
-device arm-smmuv3,primary-bus=pcie.0
Please let me know if there is another way to achieve the same with default
"bus".
Thanks again for taking the time to run the above tests and the explanation.
Thanks,
Shameer
> The only way to do that is making it a subtype of PCIDevice, i.e. change
> the parent chain from
>
> arm-smmuv3 -> arm-smmu -> sys-bus-device -> device -> object
>
> to something like
>
> arm-smmuv3 -> ... -> pci-device -> device -> object
>
> Note you cannot have different subtypes of the same supertype (say
> "arm-smmu") plug into different buses. If you need a common device core
> to plug into different buses, things get more complicated. Here's how
> the "serial-FOO" devices do it:
>
> * "serial-mm", a subtype of "sys-bus-device", thus plugs into system bus
> * "serial-isa", a subtype of "isa-device", thus plugs into ISA bus
> * "serial-pci", a subtype of "pci-device", thus plugs into PCI bus
>
> They all *contain* the core "serial" device, which is a subtype of
> "device", and thus does not plug into any bus. "Contain" is "has a",
> not "subtype of".
>
> [...]
>
