On Mon, 14 Jul 2025 16:59:34 +0100
Shameer Kolothum <shameerali.kolothum.th...@huawei.com> 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)
> -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>
One questions inline plus trivial stuff. I'm not yet up to speed with
all the iommufd stuff so this is rather superficial for now.
> +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);
No unwinding of iommufd_backened_alloc_viommu?
Looks like we just leak it until destruction of the fd.
Maybe add a comment for those like me who aren't all that familiar with
this stuff and see an alloc with no matching free.
> + 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",
> + smmu_get_sid(sdev));
> + return false;
> + } else {
No need for else as other path already returned.
> + 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;
> + }
> +
> + 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;
> +}
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index f3386bd7ae..c4537ca1d6 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -66,6 +66,10 @@ smmuv3_notify_flag_del(const char *iommu) "DEL
> SMMUNotifier node for iommu mr=%s
> smmuv3_inv_notifiers_iova(const char *name, int asid, int vmid, uint64_t
> iova, uint8_t tg, uint64_t num_pages, int stage) "iommu mr=%s asid=%d vmid=%d
> iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" stage=%d"
> smmu_reset_exit(void) ""
>
> +#smmuv3-accel.c
> +smmuv3_accel_set_iommu_device(int devfn, uint32_t sid) "devfn=0x%x
> (sid=0x%x)"
> +smmuv3_accel_unset_iommu_device(int devfn, uint32_t sid) "devfn=0x%x
> (sid=0x%x"
bracket?
