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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on
>IOMMU_GET_HW_INFO failure
>
>On 09/07/2024 12:45, Joao Martins wrote:
>> 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().
>>
>
>Sorry ignore the previous snip, it was the wrong version, see below instead.
>
>diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>index c2f158e60386..987dd9779f94 100644
>--- a/hw/vfio/iommufd.c
>+++ b/hw/vfio/iommufd.c
>@@ -631,6 +631,10 @@ static bool
>hiod_iommufd_vfio_realize(HostIOMMUDevice
>*hiod, void *opaque,
>
>     hiod->agent = opaque;
>
>+    if (vfio_is_mdev(vdev)) {
>+        return true;
>+    }
>+

Not necessary to create a dummy object.
What about bypassing object_new(ops->hiod_typename) in vfio_attach_device()?

>     if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>                                          &type, &data, sizeof(data), errp)) {
>         return false;
>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)) {

Yes.

>         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);
>+    }

Yes.

Thanks
Zhenzhong
> }