RE: [PATCH v2 5/6] hw/arm/virt: Add support for smmuv3 device

[Shameerali Kolothum Thodi via <qemu-devel@nongnu.org>]

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Monday, May 5, 2025 11:12 AM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@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.thodi@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