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)
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:print_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:print_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.
> }
>
> 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?
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".
[...]
