On 10/07/2024 10:54, 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 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:
>
/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-
>> 59170c471...@oracle.com/
>
