Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on IOMMU_GET_HW_INFO failure

[Joao Martins]

On 09/07/2024 09:56, Joao Martins wrote:
> On 09/07/2024 04:43, Duan, Zhenzhong wrote:
>> Hi Joao,
>>
>>> -----Original Message-----
>>> From: Joao Martins <joao.m.martins@oracle.com>
>>> Subject: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on
>>> IOMMU_GET_HW_INFO failure
>>>
>>> mdevs aren't "physical" devices and when asking for backing IOMMU info, it
>>> fails the entire provisioning of the guest. Fix that by filling caps info
>>> when IOMMU_GET_HW_INFO succeeds plus discarding the error we would
>>> get into
>>> iommufd_backend_get_device_info().
>>>
>>> Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> Fixes: 930589520128 ("vfio/iommufd: Implement
>>> HostIOMMUDeviceClass::realize() handler")
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>> hw/vfio/iommufd.c | 12 +++++-------
>>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index c2f158e60386..a4d23f488b01 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -631,15 +631,13 @@ static bool
>>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>>
>>>     hiod->agent = opaque;
>>>
>>> -    if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>>> -                                         &type, &data, sizeof(data), 
>>> errp)) {
>>> -        return false;
>>> +    if (iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>>> +                                         &type, &data, sizeof(data), 
>>> NULL)) {
>>
>> This will make us miss the real error. What about bypassing host IOMMU device
>> creation for mdev as it's not "physical device", passing corresponding host 
>> IOMMU
>> device to vIOMMU make no sense.
> 
> Yeap -- This was my second alternative.
> 
> I can add an helper for vfio_is_mdev()) and just call
> iommufd_backend_get_device_info() if !vfio_is_mdev().  I am assuming you meant
> to skip the initialization of HostIOMMUDeviceCaps::caps as I think that
> initializing hiod still makes sense as we are still using a
> TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO somewhat?
> 
Something like this is what I've done with this patch, see below. I think it
matches what you suggested? Naturally there's a precedent patch that introduces
vfio_is_mdev().

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index a4d23f488b01..c0a019dffdb6 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -631,8 +631,9 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod,
void *opaque,

     hiod->agent = opaque;

-    if (iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
-                                         &type, &data, sizeof(data), NULL)) {
+    if (!vfio_is_mdev(vdev) &&
+        iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
+                                        &type, &data, sizeof(data), errp)) {
         hiod->name = g_strdup(vdev->name);
         caps->type = type;
         caps->aw_bits = vfio_device_get_aw_bits(vdev);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d95aa6b65788..f092c1537999 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3115,7 +3115,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)

     vfio_bars_register(vdev);

-    if (!pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
+    if (!is_mdev && !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
         error_prepend(errp, "Failed to set iommu_device: ");
         goto out_teardown;
     }
@@ -3238,7 +3238,9 @@ out_deregister:
         timer_free(vdev->intx.mmap_timer);
     }
 out_unset_idev:
-    pci_device_unset_iommu_device(pdev);
+    if (!is_mdev) {
+        pci_device_unset_iommu_device(pdev);
+    }
 out_teardown:
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
@@ -3268,6 +3270,7 @@ static void vfio_exitfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = VFIO_PCI(pdev);
     VFIODevice *vbasedev = &vdev->vbasedev;
+    bool is_mdev = vfio_is_mdev(vbasedev);

     vfio_unregister_req_notifier(vdev);
     vfio_unregister_err_notifier(vdev);
@@ -3283,7 +3286,9 @@ static void vfio_exitfn(PCIDevice *pdev)
     vfio_pci_disable_rp_atomics(vdev);
     vfio_bars_exit(vdev);
     vfio_migration_exit(vbasedev);
-    pci_device_unset_iommu_device(pdev);
+    if (!is_mdev) {
+        pci_device_unset_iommu_device(pdev);
+    }
 }