Re: [PATCH 4/5] hw/arm/virt: Add support for smmuv3 device

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Tue, 15 Apr 2025 09:11:03 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> Allow cold-plug of smmuv3 device to virt If the machine wide smmuv3
> or a virtio-iommu is not specified.
> 
> Also restrict the usage if virt <= 9.2. This will prevent accidently
> creating a SMMUv3 device on machines prior to 9.2 and cause failure
> on migrating to machines with same version but has a legacy smmuv3
> device.

Hi,

As we discussed internally I'm not convinced we need to prevent this particular
way for a user to shoot themselves in the foot.

To be a problem they have to specifically request an old machine + the
device that didn't exist for that machine, then migrate to a real old
version of QEMU.  Agreed it is possible but I'm not sure we need to
prevent that particular crazy.

Jonathan


> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> ToDo: probably need to change virt <= 9.2 to 10.0 considering the 
> Qemu cycle we are at now.
> ---
>  hw/arm/virt.c         | 54 +++++++++++++++++++++++++++++++++++++++++++
>  hw/core/sysbus-fdt.c  |  3 +++
>  include/hw/arm/virt.h |  1 +
>  3 files changed, 58 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 729f192558..8d0ae79f4d 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"
> @@ -1445,6 +1446,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, 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)
>  {
> @@ -2944,6 +2970,18 @@ 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_DEV)) {
> +        VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> +        if (vmc->no_smmuv3_device) {
> +            error_setg(errp, "virt machine does not support arm-smmuv3-device");
> +        } else if ((vms->iommu == VIRT_IOMMU_VIRTIO) ||
> +                   (vms->iommu == VIRT_IOMMU_SMMUV3)) {
> +            error_setg(errp, "virt machine already has %s set."
> +                       "Doesn't support multiple incompatible iommus",
> +                       (vms->iommu == VIRT_IOMMU_VIRTIO) ?
> +                       "virtio-iommu" : "iommu=smmuv3");
> +        }
>      }
>  }
>  
> @@ -2967,6 +3005,19 @@ 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_DEV)) {
> +        VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +
> +        create_smmuv3_dev_dtb(vms, dev);
> +        if (vms->iommu != VIRT_IOMMU_SMMUV3_DEV) {
> +            vms->iommu = VIRT_IOMMU_SMMUV3_DEV;
> +        }
> +        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);
>  
> @@ -3169,6 +3220,7 @@ static void virt_machine_class_init(ObjectClass *oc, 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_DEV);
>  #ifdef CONFIG_TPM
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
>  #endif
> @@ -3418,8 +3470,10 @@ DEFINE_VIRT_MACHINE_AS_LATEST(10, 0)
>  
>  static void virt_machine_9_2_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>      virt_machine_10_0_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_9_2, hw_compat_9_2_len);
> +    vmc->no_smmuv3_device = true;
>  }
>  DEFINE_VIRT_MACHINE(9, 2)
>  
> diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c
> index e85066b905..ec90ed2c14 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"
> @@ -512,6 +513,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_DEV, no_fdt_node),
>      VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
>  #endif
>  #ifdef CONFIG_TPM
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 12395c7594..9f98345c92 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -136,6 +136,7 @@ struct VirtMachineClass {
>      bool no_tcg_lpa2;
>      bool no_ns_el2_virt_timer_irq;
>      bool no_nested_smmu;
> +    bool no_smmuv3_device;
>  };
>  
>  struct VirtMachineState {