>-----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 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?
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-
>59170c471...@oracle.com/
