Re: [PATCH rfcv1 4/6] vfio: initialize IOMMUFDDevice and pass to vIOMMU

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

Hi Zhenzhong,

On 1/15/24 11:13, Zhenzhong Duan wrote:
> Initialize IOMMUFDDevice in vfio and pass to vIOMMU, so that vIOMMU
> could get hw IOMMU information.
>
> In VFIO legacy backend mode, we still pass a NULL IOMMUFDDevice to vIOMMU,
> in case vIOMMU needs some processing for VFIO legacy backend mode.
>
> Originally-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-common.h |  2 ++
>  hw/vfio/iommufd.c             |  2 ++
>  hw/vfio/pci.c                 | 24 +++++++++++++++++++-----
>  3 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 9b7ef7d02b..fde0d0ca60 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -31,6 +31,7 @@
>  #endif
>  #include "sysemu/sysemu.h"
>  #include "hw/vfio/vfio-container-base.h"
> +#include "sysemu/iommufd_device.h"
>  
>  #define VFIO_MSG_PREFIX "vfio %s: "
>  
> @@ -126,6 +127,7 @@ typedef struct VFIODevice {
>      bool dirty_tracking;
>      int devid;
>      IOMMUFDBackend *iommufd;
> +    IOMMUFDDevice idev;
This looks duplicate of existing fields:
idev.dev_id is same as above devid. by the way let's try to use the same
devid everywhere.
idev.iommufd is same as above iommufd if != NULL.
So we should at least rationalize.
>  } VFIODevice;
>  
>  struct VFIODeviceOps {
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 9bfddc1360..cbd035f148 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -309,6 +309,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>      VFIOContainerBase *bcontainer;
>      VFIOIOMMUFDContainer *container;
>      VFIOAddressSpace *space;
> +    IOMMUFDDevice *idev = &vbasedev->idev;
>      struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
>      int ret, devfd;
>      uint32_t ioas_id;
> @@ -428,6 +429,7 @@ found_container:
>      QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next);
>      QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
>  
> +    iommufd_device_init(idev, sizeof(*idev), container->be, vbasedev->devid);
>      trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev->num_irqs,
>                                     vbasedev->num_regions, vbasedev->flags);
>      return 0;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d7fe06715c..2c3a5d267b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3107,11 +3107,21 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>  
>      vfio_bars_register(vdev);
>  
> -    ret = vfio_add_capabilities(vdev, errp);
> +    if (vbasedev->iommufd) {
> +        ret = pci_device_set_iommu_device(pdev, &vbasedev->idev, errp);
> +    } else {
> +        ret = pci_device_set_iommu_device(pdev, 0, errp);
> +    }
>      if (ret) {
> +        error_prepend(errp, "Failed to set iommu_device: ");
at the moment it is rather an IOMMUFD device.
>          goto out_teardown;
>      }
>  
> +    ret = vfio_add_capabilities(vdev, errp);
> +    if (ret) {
> +        goto out_unset_idev;
> +    }
> +
>      if (vdev->vga) {
>          vfio_vga_quirk_setup(vdev);
>      }
> @@ -3128,7 +3138,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>              error_setg(errp,
>                         "cannot support IGD OpRegion feature on hotplugged "
>                         "device");
> -            goto out_teardown;
> +            goto out_unset_idev;
>          }
>  
>          ret = vfio_get_dev_region_info(vbasedev,
> @@ -3137,13 +3147,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          if (ret) {
>              error_setg_errno(errp, -ret,
>                               "does not support requested IGD OpRegion feature");
> -            goto out_teardown;
> +            goto out_unset_idev;
>          }
>  
>          ret = vfio_pci_igd_opregion_init(vdev, opregion, errp);
>          g_free(opregion);
>          if (ret) {
> -            goto out_teardown;
> +            goto out_unset_idev;
>          }
>      }
>  
> @@ -3229,6 +3239,8 @@ out_deregister:
>      if (vdev->intx.mmap_timer) {
>          timer_free(vdev->intx.mmap_timer);
>      }
> +out_unset_idev:
> +    pci_device_unset_iommu_device(pdev);
>  out_teardown:
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);
> @@ -3257,6 +3269,7 @@ static void vfio_instance_finalize(Object *obj)
>  static void vfio_exitfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
>  
>      vfio_unregister_req_notifier(vdev);
>      vfio_unregister_err_notifier(vdev);
> @@ -3271,7 +3284,8 @@ static void vfio_exitfn(PCIDevice *pdev)
>      vfio_teardown_msi(vdev);
>      vfio_pci_disable_rp_atomics(vdev);
>      vfio_bars_exit(vdev);
> -    vfio_migration_exit(&vdev->vbasedev);
> +    vfio_migration_exit(vbasedev);
> +    pci_device_unset_iommu_device(pdev);
>  }
>  
>  static void vfio_pci_reset(DeviceState *dev)
Thanks

Eric