Re: [PATCH v6 09/19] vfio/iommufd: Implement HostIOMMUDeviceClass::realize() handler

[Eric Auger]

On 6/4/24 04:58, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement
>> HostIOMMUDeviceClass::realize() handler
>>
>> Hi Zhenzhong,
>>
>> On 6/3/24 08:10, Zhenzhong Duan wrote:
>>> It calls iommufd_backend_get_device_info() to get host IOMMU
>>> related information and translate it into HostIOMMUDeviceCaps
>>> for query with .get_cap().
>>>
>>> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to
>>> (aw_bits - 1).
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>  include/hw/i386/intel_iommu.h |  1 +
>>>  hw/vfio/iommufd.c             | 37
>> +++++++++++++++++++++++++++++++++++
>>>  2 files changed, 38 insertions(+)
>>>
>>> diff --git a/include/hw/i386/intel_iommu.h
>> b/include/hw/i386/intel_iommu.h
>>> index 7fa0a695c8..7d694b0813 100644
>>> --- a/include/hw/i386/intel_iommu.h
>>> +++ b/include/hw/i386/intel_iommu.h
>>> @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState,
>> INTEL_IOMMU_DEVICE)
>>>  #define VTD_HOST_AW_48BIT           48
>>>  #define VTD_HOST_ADDRESS_WIDTH      VTD_HOST_AW_39BIT
>>>  #define VTD_HAW_MASK(aw)            ((1ULL << (aw)) - 1)
>>> +#define VTD_MGAW_FROM_CAP(cap)      ((cap >> 16) & 0x3fULL)
>>>
>>>  #define DMAR_REPORT_F_INTR          (1)
>>>
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index e4a507d55c..9d2e95e20e 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -25,6 +25,7 @@
>>>  #include "qemu/cutils.h"
>>>  #include "qemu/chardev_open.h"
>>>  #include "pci.h"
>>> +#include "hw/i386/intel_iommu_internal.h"
>>>
>>>  static int iommufd_cdev_map(const VFIOContainerBase *bcontainer,
>> hwaddr iova,
>>>                              ram_addr_t size, void *vaddr, bool readonly)
>>> @@ -619,6 +620,41 @@ static void
>> vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>>>      vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
>>>  };
>>>
>>> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void
>> *opaque,
>>> +                                      Error **errp)
>>> +{
>>> +    VFIODevice *vdev = opaque;
>>> +    HostIOMMUDeviceCaps *caps = &hiod->caps;
>>> +    enum iommu_hw_info_type type;
>>> +    union {
>>> +        struct iommu_hw_info_vtd vtd;
>>> +    } data;
>>> +
>>> +    if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>>> +                                         &type, &data, sizeof(data), 
>>> errp)) {
>>> +        return false;
>>> +    }
>>> +
>>> +    caps->type = type;
>>> +
>>> +    switch (type) {
>>> +    case IOMMU_HW_INFO_TYPE_INTEL_VTD:
>>> +        caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1;
>> Please can you remind me of why you can't reuse the iova_ranges method.
>> isn't it generic enough?
> Yes, iova_ranges method is only for iova_ranges, we want to make
> HostIOMMUDevice.get_cap() a common interface.
>
> When we want to pass iova_ranges, we can add HOST_IOMMU_DEVICE_CAP_IOVA_RANGES
> and HostIOMMUDevice.iova_ranges.

I rather meant that iova_ranges is part of VFIOContainerBase and you
could reuse the technics used in hiod_legacy_vfio_realize, relying on a
common helper instead of using

VTD_MGAW_FROM_CAP(data.vtd.cap_reg). Doesn't it work? 


>
>>> +        break;
>>> +    case IOMMU_HW_INFO_TYPE_NONE:
>> so what about other types?
> There is no other types for now. When there is, we can easily add the code

Thanks

Eric

>
> case IOMMU_HW_INFO_TYPE_ARM_SMMU:
>     caps->aw_bits = xxx;
>
> Thanks
> Zhenzhong
>
>> Eric
>>> +        break;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
>>> +
>>> +    hiodc->realize = hiod_iommufd_vfio_realize;
>>> +};
>>> +
>>>  static const TypeInfo types[] = {
>>>      {
>>>          .name = TYPE_VFIO_IOMMU_IOMMUFD,
>>> @@ -627,6 +663,7 @@ static const TypeInfo types[] = {
>>>      }, {
>>>          .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO,
>>>          .parent = TYPE_HOST_IOMMU_DEVICE_IOMMUFD,
>>> +        .class_init = hiod_iommufd_vfio_class_init,
>>>      }
>>>  };
>>>