RE: [RFC PATCH v3 06/15] hw/arm/smmuv3-accel: Restrict accelerated SMMUv3 to vfio-pci endpoints with iommufd

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

> -----Original Message-----
> From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
> Sent: Wednesday, July 16, 2025 7:26 AM
> To: Nicolin Chen <nicolinc@nvidia.com>
> Cc: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org; eric.auger@redhat.com;
> peter.maydell@linaro.org; jgg@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; shameerkolothum@gmail.com
> Subject: RE: [RFC PATCH v3 06/15] hw/arm/smmuv3-accel: Restrict
> accelerated SMMUv3 to vfio-pci endpoints with iommufd
> 

...

> >> >+static bool smmuv3_accel_pdev_allowed(PCIDevice *pdev, bool
> >*vfio_pci)
> >> >+{
> >> >+
> >> >+    if (object_dynamic_cast(OBJECT(pdev), TYPE_PCI_BRIDGE) ||
> >> >+        object_dynamic_cast(OBJECT(pdev), "pxb-pcie") ||
> >> >+        object_dynamic_cast(OBJECT(pdev), "gpex-root")) {
> >> >+        return true;
> >> >+    } else if ((object_dynamic_cast(OBJECT(pdev), TYPE_VFIO_PCI) &&
> >> >+        object_property_find(OBJECT(pdev), "iommufd"))) {
> >>
> >> Will this always return true?
> >
> >It won't if a vfio-pci device doesn't have the "iommufd" property?
> 
> IIUC, iommufd property is always there, just value not filled for legacy
> container case.
> What about checking VFIOPCIDevice.vbasedev.iommufd?

That's right. The property is always there. But instead of accessing VFIOPCIDevice
in SMMUv3 code, I think we can use object_property_get_link(obj, "iommufd", &error_abort)
instead?
 
> >
> >> >+        *vfio_pci = true;
> >> >+        return true;
> >> >+    }
> >> >+    return false;
> >
> >Then, it returns "false" here.
> >
> >> > static AddressSpace *smmuv3_accel_find_add_as(PCIBus *bus, void
> >> >*opaque,
> >> >                                               int devfn)
> >> > {
> >> >+    PCIDevice *pdev = pci_find_device(bus, pci_bus_num(bus), devfn);
> >> >     SMMUState *bs = opaque;
> >> >+    bool vfio_pci = false;
> >> >     SMMUPciBus *sbus;
> >> >     SMMUv3AccelDevice *accel_dev;
> >> >     SMMUDevice *sdev;
> >> >
> >> >+    if (pdev && !smmuv3_accel_pdev_allowed(pdev, &vfio_pci)) {
> >> >+        error_report("Device(%s) not allowed. Only PCIe root complex
> >> >devices "
> >> >+                     "or PCI bridge devices or vfio-pci endpoint
> >devices
> >> >with "
> >> >+                     "iommufd as backend is allowed with
> >> >arm-smmuv3,accel=on",
> >> >+                     pdev->name);
> >> >+        exit(1);
> >>
> >> Seems aggressive for a hotplug, could we fail hotplug instead of kill
> QEMU?
> >
> >Hotplug will unlikely be supported well, as it would introduce
> >too much complication.
> >
> >With iommufd, a vIOMMU object is allocated per device (vfio). If
> >the device fd (cdev) is not yet given to the QEMU. It isn't able
> >to allocate a vIOMMU object when creating a VM.
> >
> >While a vIOMMU object can be allocated at a later stage once the
> >device is hotplugged. But things like IORT mappings aren't able
> >to get refreshed since the OS is likely already booted. Even an
> >IOMMU capability sync via the hw_info ioctl will be difficult to
> >do at the runtime post the guest iommu driver's initialization.
> >
> >I am not 100% sure. But I think Intel model could have a similar
> >problem if the guest boots with zero cold-plugged device and then
> >hot-plugs a PASID-capable device at the runtime, when the guest-
> >level IOMMU driver is already inited?
> 
> For vtd we define a property for each capability we care about.
> When hotplug a device, we get hw_info through ioctl and compare
> host's capability with virtual vtd's property setting, if incompatible,
> we fail the hotplug.
> 
> In old implementation we sync host iommu caps into virtual vtd's cap,
> but that's Naked by maintainer. The suggested way is to define property
> for each capability we care and do compatibility check.
> 
> There is a "pasid" property in virtual vtd, only when it's true, the PASID-
> capable
> device can work with pasid.

Thanks for this information. I think probably we need to take a look at this as
this doesn't have a dependency on cold-plug device to be present for SMMUv3.
Will go through intel vtd implementation.

Thanks,
Shameer