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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH rfcv1 4/6] vfio: initialize IOMMUFDDevice and pass to
>vIOMMU
>
>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;
>>   } 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);
>
>
>AFAICT, pci_device_set_iommu_device() with a NULL IOMMUFDDevice will
>do
>nothing. Why call it ?

We will do something in nesting series, see 
https://github.com/yiliu1765/qemu/commit/7f0bb59575bb5cf38618ae950f68a8661676e881

Another choice is to call pci_device_set_iommu_device() no matter which backend
is used and check idev->iommufd in vtd_dev_set_iommu_device(). Is this better
for you?

Thanks
Zhenzhong