On 10/07/2024 03:53, Duan, Zhenzhong wrote:
>
>
>> -----Original Message-----
>> From: Joao Martins <joao.m.mart...@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.mart...@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.d...@intel.com>
>>>>>> Fixes: 930589520128 ("vfio/iommufd: Implement
>>>>>> HostIOMMUDeviceClass::realize() handler")
>>>>>> Signed-off-by: Joao Martins <joao.m.mart...@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?
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)) {
?
[0]
https://lore.kernel.org/qemu-devel/4e85db04-fbaa-4a6b-b133-59170c471...@oracle.com/
