> -----Original Message-----
> From: Eric Auger <eric.au...@redhat.com>
> Sent: Monday, May 5, 2025 11:12 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
> qemu-devel@nongnu.org
> Cc: 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 5/6] hw/arm/virt: Add support for smmuv3 device
>
>
>
> On 5/2/25 12:27 PM, Shameer Kolothum wrote:
> I would change the title into something like "Allow -device arm-smmuv3
> instantiation"
Ok.
> > Allow cold-plug of smmuv3 device to virt if there is no machine
> > wide legacy smmuv3 or a virtio-iommu is specified.
> >
> > Device tree support for new smmuv3 dev is limited to the case where
> > it is associated with the default pcie.0 RC.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.th...@huawei.com>
> > ---
> > hw/arm/virt.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++
> > hw/core/sysbus-fdt.c | 3 +++
> > 2 files changed, 51 insertions(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index e178282d71..f6ff584bac 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1449,6 +1449,31 @@ static void create_smmuv3_dt_bindings(const
> VirtMachineState *vms, hwaddr base,
> > g_free(node);
> > }
> >
> > +static void create_smmuv3_dev_dtb(VirtMachineState *vms,
> > + DeviceState *dev)
> > +{
> > + PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms-
> >platform_bus_dev);
> > + SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
> > + int irq = platform_bus_get_irqn(pbus, sbdev, 0);
> > + hwaddr base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
> > + MachineState *ms = MACHINE(vms);
> > + PCIBus *bus;
> > +
> > + bus = PCI_BUS(object_property_get_link(OBJECT(dev), "primary-bus",
> > + &error_abort));
> > + if (strcmp("pcie.0", bus->qbus.name)) {
> > + warn_report("SMMUv3 device only supported with pcie.0 for DT");
> > + return;
> > + }
> > + base += vms->memmap[VIRT_PLATFORM_BUS].base;
> > + irq += vms->irqmap[VIRT_PLATFORM_BUS];
> > +
> > + vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
> > + create_smmuv3_dt_bindings(vms, base, SMMU_IO_LEN, irq);
> > + qemu_fdt_setprop_cells(ms->fdt, vms->pciehb_nodename, "iommu-
> map",
> > + 0x0, vms->iommu_phandle, 0x0, 0x10000);
> > +}
> > +
> > static void create_smmu(const VirtMachineState *vms,
> > PCIBus *bus)
> > {
> > @@ -2949,6 +2974,13 @@ static void
> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > qlist_append_str(reserved_regions, resv_prop_str);
> > qdev_prop_set_array(dev, "reserved-regions", reserved_regions);
> > g_free(resv_prop_str);
> > + } else if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> > + if (vms->legacy_smmuv3_present || vms->iommu ==
> VIRT_IOMMU_VIRTIO) {
> > + error_setg(errp, "virt machine already has %s set. "
> > + "Doesn't support incompatible iommus",
> > + (vms->legacy_smmuv3_present) ?
> > + "iommu=smmuv3" : "virtio-iommu");
> what about bypass mode?
Yeah. Bypass is handled only in IORT ACPI code(that too silently!). I will add
a check to explicitly block specifying a SMMUv3 dev with RC bypass mode.
> > + }
> > }
> > }
> >
> > @@ -2972,6 +3004,21 @@ static void
> virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> > virtio_md_pci_plug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev),
> errp);
> > }
> >
> > + if (object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3)) {
> > + if (!vms->legacy_smmuv3_present && vms->platform_bus_dev) {
> this answers my previous comment ;-)
Yes.
> > + VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> > +
> > + create_smmuv3_dev_dtb(vms, dev);
> > + if (vms->iommu != VIRT_IOMMU_SMMUV3) {
> > + vms->iommu = VIRT_IOMMU_SMMUV3;
> > + }
> > + if (!vmc->no_nested_smmu) {
> > + object_property_set_str(OBJECT(dev), "stage", "nested",
> > + &error_fatal);
> > + }
> > + }
> > + }
> > +
> > if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> > PCIDevice *pdev = PCI_DEVICE(dev);
> >
> > @@ -3174,6 +3221,7 @@ static void virt_machine_class_init(ObjectClass
> *oc, const void *data)
> > machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
> > machine_class_allow_dynamic_sysbus_dev(mc,
> TYPE_VFIO_PLATFORM);
> > machine_class_allow_dynamic_sysbus_dev(mc,
> TYPE_UEFI_VARS_SYSBUS);
> > + machine_class_allow_dynamic_sysbus_dev(mc, TYPE_ARM_SMMUV3);
> > #ifdef CONFIG_TPM
> > machine_class_allow_dynamic_sysbus_dev(mc,
> TYPE_TPM_TIS_SYSBUS);
> > #endif
> > diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
> > index c339a27875..d778c0f559 100644
> > --- a/hw/core/sysbus-fdt.c
> > +++ b/hw/core/sysbus-fdt.c
> > @@ -31,6 +31,7 @@
> > #include "qemu/error-report.h"
> > #include "system/device_tree.h"
> > #include "system/tpm.h"
> > +#include "hw/arm/smmuv3.h"
> > #include "hw/platform-bus.h"
> > #include "hw/vfio/vfio-platform.h"
> > #include "hw/vfio/vfio-calxeda-xgmac.h"
> > @@ -513,6 +514,8 @@ static const BindingEntry bindings[] = {
> > #ifdef CONFIG_LINUX
> > TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC,
> add_calxeda_midway_xgmac_fdt_node),
> > TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
> > + /* No generic DT support for smmuv3 dev. Support added for arm virt
> only */
> > + TYPE_BINDING(TYPE_ARM_SMMUV3, no_fdt_node),
> Can't it live outside the CONFIG_LINUX?
Yes, It could. Will change.
Thanks,
Shameer
