Re: [PATCH v6 07/19] vfio/container: Implement HostIOMMUDeviceClass::realize() handler

[Eric Auger <eric.auger@redhat.com>]

Hi Zhenzhong,

On 6/4/24 04:45, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH v6 07/19] vfio/container: Implement
>> HostIOMMUDeviceClass::realize() handler
>>
>> Hi Zhenzhong,
>>
>> On 6/3/24 08:10, Zhenzhong Duan wrote:
>>> Utilize range_get_last_bit() to get host IOMMU address width and
>>> package it in HostIOMMUDeviceCaps for query with .get_cap().
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>  hw/vfio/container.c | 26 ++++++++++++++++++++++++++
>>>  1 file changed, 26 insertions(+)
>>>
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index c4fca2dfca..48800fe92f 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -1136,6 +1136,31 @@ static void
>> vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>>      vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>>  };
>>>
>>> +static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void
>> *opaque,
>>> +                                     Error **errp)
>>> +{
>>> +    VFIODevice *vdev = opaque;
>>> +    /* iova_ranges is a sorted list */
>>> +    GList *l = g_list_last(vdev->bcontainer->iova_ranges);
>>> +
>>> +    /* Only support query HOST_IOMMU_DEVICE_CAP_AW_BITS with
>> legacy backend */
>> I don't get the comment as HOST_IOMMU_DEVICE_CAP_AW_BITS support
>> seems
>> to be introduced in [PATCH v6 11/19] backends/iommufd: Implement
>> HostIOMMUDeviceClass::get_cap() handler
> Sorry about my poor English, I mean legacy backend only support
> HOST_IOMMU_DEVICE_CAP_AW_BITS, no other caps.
> May be only:
>
> /* Only support query HOST_IOMMU_DEVICE_CAP_AW_BITS */
no problem. Then I would put this comment in the commit msg instead.
Something like "the realize function populates the capabilities. For now
only the aw_bits caps is computed".


>
>>> +    if (l) {
>>> +        Range *range = l->data;
>>> +        hiod->caps.aw_bits = range_get_last_bit(range) + 1;
>>> +    } else {
>>> +        hiod->caps.aw_bits = 0xff;
>> why this value?
> 0xff means no limitation on aw_bits from host side. Aw_bits check
> should always pass. This could be a case that an old kernel doesn't
> support query iova ranges.
>
> Will add a define like:
>
> #define HOST_IOMMU_DEVICE_CAP_AW_BITS_MAX 0xff
Wouldn't 64 bits be a better choice? Also maybe add a comment explaining
that iova_ranges may be void for old kernels that do not support the query?

Eric
>
> Thanks
> Zhenzhong
>
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>>> +
>>> +    hioc->realize = hiod_legacy_vfio_realize;
>>> +};
>>> +
>>>  static const TypeInfo types[] = {
>>>      {
>>>          .name = TYPE_VFIO_IOMMU_LEGACY,
>>> @@ -1144,6 +1169,7 @@ static const TypeInfo types[] = {
>>>      }, {
>>>          .name = TYPE_HOST_IOMMU_DEVICE_LEGACY_VFIO,
>>>          .parent = TYPE_HOST_IOMMU_DEVICE,
>>> +        .class_init = hiod_legacy_vfio_class_init,
>>>      }
>>>  };
>>>
>> Thanks
>>
>> Eric