>-----Original Message-----
>From: Eric Auger <eric.au...@redhat.com>
>Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement
>HostIOMMUDeviceClass::realize() handler
>
>
>Hi,
>On 6/4/24 09:51, Duan, Zhenzhong wrote:
>>
>>> -----Original Message-----
>>> From: Eric Auger <eric.au...@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.au...@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.d...@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
OK, will do.
Thanks
Zhenzhong
