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 10/07/2024 03:53, Duan, Zhenzhong wrote:
>>
>>
>>> -----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()?
>>
>Not sure I am parsing this. What dummy object you refer to here if it's not
>vbasedev::hiod that remains unused? Also in a suggestion by Cedric, and
>pre-seeding vbasedev::hiod during attach_device()[0]. So I will sort of do that
>already, but your comments means we are allocating a dummy object
>anyways too?

Yes, with your snip change, it's allocated by object_new(ops->hiod_typename) 
but not realized 
and never used else where.

>
>Or are you perhaps suggesting something like:
>
>@@ -1552,17 +1552,20 @@ bool vfio_attach_device(char *name,
>VFIODevice *vbasedev,
>
>     assert(ops);
>
>     if (!ops->attach_device(name, vbasedev, as, errp)) {
>         return false;
>     }
>
>     if (!vfio_mdev(vbasedev) &&
>        !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>errp)) {
>
>?

I mean bypass host IOMMU device thoroughly for mdev, like:

--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1548,6 +1548,10 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev,
         return false;
     }

+    if (vfio_is_mdev(vdev)) {
+        return true;
+    }
+
     hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
     if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) {
         object_unref(hiod);


>
>
>[0]
>https://lore.kernel.org/qemu-devel/4e85db04-fbaa-4a6b-b133-
>59170c471e24@oracle.com/