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

[Joao Martins]

On 10/07/2024 10:54, 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 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:
> 

/me facepalm.

Makes sense!

I read your comment in my head as "What about by passing
object_new(ops->hiod_typename)", when it was 'bypassing' that you wrote.

> --- 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/
>