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

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

Hi,
On 6/4/24 09:51, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement
>> HostIOMMUDeviceClass::realize() handler
>>
>>
>>
>> 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?
> Get your point.
> Yes, It does work and should have same result.
> That means iommufd backend support two ways to get aw_bits.
>
> Only reason is I feel VTD_MGAW_FROM_CAP(data.vtd.cap_reg) is a bit simpler
> and there are other bits picked in nesting series, see:
>
>     case IOMMU_HW_INFO_TYPE_INTEL_VTD:
>         caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1;
>         caps->nesting = !!(data.vtd.ecap_reg & VTD_ECAP_NEST);
>         caps->fs1gp = !!(data.vtd.cap_reg & VTD_CAP_FS1GP);
>         caps->errata = data.vtd.flags & IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17;
>
> I'm fine to use iova_ranges to calculate aw_bits for iommufd backend if you prefer that.
Yes I think I would prefer because this technics also work for other
iommus and not only VTD. It also can rely on common code between legacy
and iommufd. The nesting series can bring the rest later

Eric
>
> Thanks
> Zhenzhong