Re: [RFC PATCH v3 10/15] hw/arm/smmuv3-accel: Allocate a vDEVICE object for device

[Eric Auger <eric.auger@redhat.com>]

Hi Shameer,

On 7/14/25 5:59 PM, Shameer Kolothum wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
>
> Allocate and associate a vDEVICE object for the Guest device
> with the vIOMMU. This will help the kernel to do the
> vSID --> sid translation whenever required (eg: device specific
I am not sure I get this. Do you mean translation between the vSID and
the pSID?

> invalidations).
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/smmuv3-accel.c    | 25 +++++++++++++++++++++++++
>  hw/arm/smmuv3-accel.h    |  1 +
>  include/system/iommufd.h |  5 +++++
>  3 files changed, 31 insertions(+)
>
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 74bf20cfaf..f1584dd775 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -93,6 +93,23 @@ void smmuv3_accel_install_nested_ste(SMMUState *bs, SMMUDevice *sdev, int sid)
>          return;
>      }
>  
> +    if (!accel_dev->vdev && accel_dev->idev) {
> +        IOMMUFDVdev *vdev;
> +        uint32_t vdev_id;
> +        SMMUViommu *viommu = accel_dev->viommu;
> +
> +        iommufd_backend_alloc_vdev(viommu->core.iommufd, accel_dev->idev->devid,
> +                                   viommu->core.viommu_id, sid, &vdev_id,
> +                                   &error_abort);
error_abort vs error handling
> +        vdev = g_new(IOMMUFDVdev, 1);
> +        vdev->vdev_id = vdev_id;
> +        vdev->dev_id = sid;
> +        accel_dev->vdev = vdev;
> +        host_iommu_device_iommufd_attach_hwpt(accel_dev->idev,
> +                                              accel_dev->viommu->bypass_hwpt_id,
> +                                              &error_abort);
> +    }
> +
>      ret = smmu_find_ste(sdev->smmu, sid, &ste, &event);
>      if (ret) {
>          error_report("failed to find STE for sid 0x%x", sid);
> @@ -287,6 +304,7 @@ static void smmuv3_accel_unset_iommu_device(PCIBus *bus, void *opaque,
>      SMMUPciBus *sbus = g_hash_table_lookup(bs->smmu_pcibus_by_busptr, bus);
>      SMMUv3AccelDevice *accel_dev;
>      SMMUViommu *viommu;
> +    IOMMUFDVdev *vdev;
>      SMMUDevice *sdev;
>  
>      if (!sbus) {
> @@ -310,6 +328,13 @@ static void smmuv3_accel_unset_iommu_device(PCIBus *bus, void *opaque,
>      trace_smmuv3_accel_unset_iommu_device(devfn, smmu_get_sid(sdev));
>  
>      viommu = s->s_accel->viommu;
> +    vdev = accel_dev->vdev;
> +    if (vdev) {
> +        iommufd_backend_free_id(viommu->iommufd, vdev->vdev_id);
> +        g_free(vdev);
> +        accel_dev->vdev = NULL;
> +    }
> +
>      if (QLIST_EMPTY(&viommu->device_list)) {
>          iommufd_backend_free_id(viommu->iommufd, viommu->bypass_hwpt_id);
>          iommufd_backend_free_id(viommu->iommufd, viommu->abort_hwpt_id);
> diff --git a/hw/arm/smmuv3-accel.h b/hw/arm/smmuv3-accel.h
> index 06e81b630d..21028e60c8 100644
> --- a/hw/arm/smmuv3-accel.h
> +++ b/hw/arm/smmuv3-accel.h
> @@ -40,6 +40,7 @@ typedef struct SMMUv3AccelDevice {
>      HostIOMMUDeviceIOMMUFD *idev;
>      SMMUS1Hwpt  *s1_hwpt;
>      SMMUViommu *viommu;
> +    IOMMUFDVdev  *vdev;
>      QLIST_ENTRY(SMMUv3AccelDevice) next;
>  } SMMUv3AccelDevice;
>  
> diff --git a/include/system/iommufd.h b/include/system/iommufd.h
> index b7ad2cf10c..8de559d448 100644
> --- a/include/system/iommufd.h
> +++ b/include/system/iommufd.h
> @@ -44,6 +44,11 @@ typedef struct IOMMUFDViommu {
>      uint32_t viommu_id;
>  } IOMMUFDViommu;
>  
> +typedef struct IOMMUFDVdev {
> +    uint32_t vdev_id;
> +    uint32_t dev_id;
> +} IOMMUFDVdev;
Given the jungle of devices and dev_ids we have, I think we need to have
either proper doc comments explaining what the objects abstracts and role

Thanks

Eric
> +
>  bool iommufd_backend_connect(IOMMUFDBackend *be, Error **errp);
>  void iommufd_backend_disconnect(IOMMUFDBackend *be);
>