Re: [PATCH v6 18/19] intel_iommu: Implement [set|unset]_iommu_device() callbacks

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

On 6/4/24 10:48, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH v6 18/19] intel_iommu: Implement
>> [set|unset]_iommu_device() callbacks
>>
>>
>>
>> On 6/4/24 07:40, Duan, Zhenzhong wrote:
>>>> -----Original Message-----
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>> Subject: Re: [PATCH v6 18/19] intel_iommu: Implement
>>>> [set|unset]_iommu_device() callbacks
>>>>
>>>> Hi Zhenzhong,
>>>>
>>>> On 6/3/24 08:10, Zhenzhong Duan wrote:
>>>>> From: Yi Liu <yi.l.liu@intel.com>
>>>>>
>>>>> Implement [set|unset]_iommu_device() callbacks in Intel vIOMMU.
>>>>> In set call, a new structure VTDHostIOMMUDevice which holds
>>>>> a reference to HostIOMMUDevice is stored in hash table
>>>>> indexed by PCI BDF.
>>>> maybe precise that this is not the aliased one?
>>> Sure.
>>>
>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>> ---
>>>>>  hw/i386/intel_iommu_internal.h |  9 ++++
>>>>>  include/hw/i386/intel_iommu.h  |  2 +
>>>>>  hw/i386/intel_iommu.c          | 76
>>>> ++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 87 insertions(+)
>>>>>
>>>>> diff --git a/hw/i386/intel_iommu_internal.h
>>>> b/hw/i386/intel_iommu_internal.h
>>>>> index f8cf99bddf..b800d62ca0 100644
>>>>> --- a/hw/i386/intel_iommu_internal.h
>>>>> +++ b/hw/i386/intel_iommu_internal.h
>>>>> @@ -28,6 +28,7 @@
>>>>>  #ifndef HW_I386_INTEL_IOMMU_INTERNAL_H
>>>>>  #define HW_I386_INTEL_IOMMU_INTERNAL_H
>>>>>  #include "hw/i386/intel_iommu.h"
>>>>> +#include "sysemu/host_iommu_device.h"
>>>>>
>>>>>  /*
>>>>>   * Intel IOMMU register specification
>>>>> @@ -537,4 +538,12 @@ typedef struct VTDRootEntry VTDRootEntry;
>>>>>  #define VTD_SL_IGN_COM              0xbff0000000000000ULL
>>>>>  #define VTD_SL_TM                   (1ULL << 62)
>>>>>
>>>>> +
>>>>> +typedef struct VTDHostIOMMUDevice {
>>>>> +    IntelIOMMUState *iommu_state;
>>>> Why do you need the iommu_state?
>>> It is used in nesting series.
>>>
>>>>> +    PCIBus *bus;
>>>>> +    uint8_t devfn;
>>>>> +    HostIOMMUDevice *dev;
>>>>> +    QLIST_ENTRY(VTDHostIOMMUDevice) next;
>>>>> +} VTDHostIOMMUDevice;
>>>> How VTD specific is it?
>>> In nesting series, it has element iommu_state and errata
>>> which are VTD specific.
>> so at least I would add a comment in the commit message explaining this.
> I'd like to drop it and reintroduce it in nesting series.

OK then just justify the choice of introducing another struct  because
other VTD specific fields will be introduced later on

Eric
>
> Thanks
> Zhenzhong
>