>-----Original Message-----
>From: Eric Auger <eric.au...@redhat.com>
>Subject: Re: [PATCH rfcv1 4/6] vfio: initialize IOMMUFDDevice and pass to
>vIOMMU
>
>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....@intel.com>
>> Signed-off-by: Nicolin Chen <nicol...@nvidia.com>
>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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.
Indeed, I'll remove devid and *iommufd. Thanks for suggestion.
>> } 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.
Will use "Failed to set IOMMUFD device: "
Thanks
Zhenzhong
>> 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
