> -----Original Message-----
> From: Eric Auger <eric.au...@redhat.com>
> Sent: 05 September 2025 10:28
> To: qemu-...@nongnu.org; qemu-devel@nongnu.org; Shameer Kolothum
> <skolothum...@nvidia.com>
> Cc: peter.mayd...@linaro.org; Jason Gunthorpe <j...@nvidia.com>; Nicolin
> Chen <nicol...@nvidia.com>; ddut...@redhat.com; berra...@redhat.com;
> Nathan Chen <nath...@nvidia.com>; Matt Ochs <mo...@nvidia.com>;
> smost...@google.com; linux...@huawei.com; wangzh...@hisilicon.com;
> jiangkun...@huawei.com; jonathan.came...@huawei.com;
> zhangfei....@linaro.org; zhenzhong.d...@intel.com;
> shameerkolot...@gmail.com
> Subject: Re: [RFC PATCH v3 08/15] hw/arm/smmuv3-accel: Add
> set/unset_iommu_device callback
>
> External email: Use caution opening links or attachments
>
>
> Hi Shameer,
>
> On 7/14/25 5:59 PM, Shameer Kolothum wrote:
> > From: Nicolin Chen <nicol...@nvidia.com>
> >
> > Implement a set_iommu_device callback:
> > -If found an existing viommu reuse that.
> > (Devices behind the same physical SMMU should share an S2 HWPT)
> I failed to see where this is done below?
Right. I think this is from the previous series. I need to update this.
At present, we only allocate a viommu if one is not allocated already.
And during the viommu allocation a S2 hwpt_id is passed which is a
nested parent HWPT allocated by vfio/iommufd. All the devices that
gets attached to this accel smmuv3/viommu will share that parent S2.
> > -Else,
> > Allocate a viommu with the nested parent S2 hwpt allocated by VFIO.
> > Allocate bypass and abort hwpt.
> > -And add the dev to viommu device list
> >
> > Also add an unset_iommu_device to unwind/cleanup above.
> >
> > Signed-off-by: Nicolin Chen <nicol...@nvidia.com>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.th...@huawei.com>
> > ---
> > hw/arm/smmuv3-accel.c | 154
> +++++++++++++++++++++++++++++++++++++++
> > hw/arm/smmuv3-accel.h | 20 +++++
> > hw/arm/trace-events | 4 +
> > include/system/iommufd.h | 6 ++
> > 4 files changed, 184 insertions(+)
> >
> > diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c index
> > 66cd4f5ece..fe90d48675 100644
> > --- a/hw/arm/smmuv3-accel.c
> > +++ b/hw/arm/smmuv3-accel.c
> > @@ -7,6 +7,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "trace.h"
> > #include "qemu/error-report.h"
> >
> > #include "hw/arm/smmuv3.h"
> > @@ -17,6 +18,9 @@
> >
> > #include "smmuv3-accel.h"
> >
> > +#define SMMU_STE_VALID (1ULL << 0)
> > +#define SMMU_STE_CFG_BYPASS (1ULL << 3)
> > +
> > static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs,
> SMMUPciBus *sbus,
> > PCIBus *bus, int
> > devfn) { @@ -39,6 +43,154 @@ static SMMUv3AccelDevice
> > *smmuv3_accel_get_dev(SMMUState *bs, SMMUPciBus *sbus,
> > return accel_dev;
> > }
> >
> > +static bool
> > +smmuv3_accel_dev_alloc_viommu(SMMUv3AccelDevice *accel_dev,
> > + HostIOMMUDeviceIOMMUFD *idev, Error
> > +**errp) {
> > + struct iommu_hwpt_arm_smmuv3 bypass_data = {
> > + .ste = { SMMU_STE_CFG_BYPASS | SMMU_STE_VALID, 0x0ULL },
> > + };
> > + struct iommu_hwpt_arm_smmuv3 abort_data = {
> > + .ste = { SMMU_STE_VALID, 0x0ULL },
> > + };
> > + SMMUDevice *sdev = &accel_dev->sdev;
> > + SMMUState *bs = sdev->smmu;
> > + SMMUv3State *s = ARM_SMMUV3(bs);
> > + SMMUv3AccelState *s_accel = s->s_accel;
> > + uint32_t s2_hwpt_id = idev->hwpt_id;
> > + SMMUS2Hwpt *s2_hwpt;
> > + SMMUViommu *viommu;
> > + uint32_t viommu_id;
> > +
> > + if (s_accel->viommu) {
> > + accel_dev->viommu = s_accel->viommu;
> > + return true;
> > + }
> > +
> > + if (!iommufd_backend_alloc_viommu(idev->iommufd, idev->devid,
> > + IOMMU_VIOMMU_TYPE_ARM_SMMUV3,
> > + s2_hwpt_id, &viommu_id, errp)) {
> > + return false;
> > + }
> > +
> > + viommu = g_new0(SMMUViommu, 1);
> > + viommu->core.viommu_id = viommu_id;
> > + viommu->core.s2_hwpt_id = s2_hwpt_id;
> > + viommu->core.iommufd = idev->iommufd;
> > +
> > + if (!iommufd_backend_alloc_hwpt(idev->iommufd, idev->devid,
> > + viommu->core.viommu_id, 0,
> > + IOMMU_HWPT_DATA_ARM_SMMUV3,
> > + sizeof(abort_data), &abort_data,
> > + &viommu->abort_hwpt_id, errp)) {
> > + goto free_viommu;
> > + }
> > +
> > + if (!iommufd_backend_alloc_hwpt(idev->iommufd, idev->devid,
> > + viommu->core.viommu_id, 0,
> > + IOMMU_HWPT_DATA_ARM_SMMUV3,
> > + sizeof(bypass_data), &bypass_data,
> > + &viommu->bypass_hwpt_id, errp)) {
> > + goto free_abort_hwpt;
> > + }
> > +
> > + s2_hwpt = g_new(SMMUS2Hwpt, 1);
> > + s2_hwpt->iommufd = idev->iommufd;
> > + s2_hwpt->hwpt_id = s2_hwpt_id;
> > +
> > + viommu->iommufd = idev->iommufd;
> > + viommu->s2_hwpt = s2_hwpt;
> > +
> > + s_accel->viommu = viommu;
> > + accel_dev->viommu = viommu;
> > + return true;
> > +
> > +free_abort_hwpt:
> > + iommufd_backend_free_id(idev->iommufd, viommu->abort_hwpt_id);
> > +free_viommu:
> > + iommufd_backend_free_id(idev->iommufd, viommu->core.viommu_id);
> > + g_free(viommu);
> > + return false;
> > +}
> > +
> > +static bool smmuv3_accel_set_iommu_device(PCIBus *bus, void *opaque,
> int devfn,
> > + HostIOMMUDevice *hiod,
> > +Error **errp) {
> > + HostIOMMUDeviceIOMMUFD *idev =
> HOST_IOMMU_DEVICE_IOMMUFD(hiod);
> > + SMMUState *bs = opaque;
> > + SMMUv3State *s = ARM_SMMUV3(bs);
> > + SMMUv3AccelState *s_accel = s->s_accel;
> > + SMMUPciBus *sbus = smmu_get_sbus(bs, bus);
> > + SMMUv3AccelDevice *accel_dev = smmuv3_accel_get_dev(bs, sbus, bus,
> devfn);
> > + SMMUDevice *sdev = &accel_dev->sdev;
> > +
> > + if (!idev) {
> > + return true;
> > + }
> > +
> > + if (accel_dev->idev) {
> > + if (accel_dev->idev != idev) {
> > + error_report("Device 0x%x already has an associated
> > + idev",
> would not use "idev" as end-user will not easily understand what the beast is
Ok.
> > + smmu_get_sid(sdev));
> > + return false;
> if this is considered as an error why don't we set errp?
> > + } else {
> > + return true;
> > + }
> > + }
> > +
> > + if (!smmuv3_accel_dev_alloc_viommu(accel_dev, idev, errp)) {
> > + error_report("Device 0x%x: Unable to alloc viommu",
> smmu_get_sid(sdev));
> > + return false;
> same here
> > + }
> > +
> > + accel_dev->idev = idev;
> > + QLIST_INSERT_HEAD(&s_accel->viommu->device_list, accel_dev, next);
> > + trace_smmuv3_accel_set_iommu_device(devfn, smmu_get_sid(sdev));
> > + return true;
> > +}
> > +
> > +static void smmuv3_accel_unset_iommu_device(PCIBus *bus, void
> *opaque,
> > + int devfn) {
> > + SMMUState *bs = opaque;
> > + SMMUv3State *s = ARM_SMMUV3(bs);
> > + SMMUPciBus *sbus = g_hash_table_lookup(bs-
> >smmu_pcibus_by_busptr, bus);
> > + SMMUv3AccelDevice *accel_dev;
> > + SMMUViommu *viommu;
> > + SMMUDevice *sdev;
> > +
> > + if (!sbus) {
> > + return;
> > + }
> > +
> > + sdev = sbus->pbdev[devfn];
> > + if (!sdev) {
> > + return;
> > + }
> > +
> > + accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
> > + if (!host_iommu_device_iommufd_attach_hwpt(accel_dev->idev,
> > + accel_dev->idev->hwpt_id,
> > + NULL)) {
> > + error_report("Unable to attach dev to the default HW
> > + pagetable");
> also trace smmu_get_sid(sdev) to be consistent with other traces?
Ok.
Thanks,
Shameer
