Hi Zhenzhong,
On 2/26/24 08:36, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.au...@redhat.com>
>> Subject: Re: [PATCH rfcv2 14/18] intel_iommu: Add a framework to check
>> and sync host IOMMU cap/ecap
>>
>>
>>
>> On 2/1/24 08:28, Zhenzhong Duan wrote:
>>> From: Yi Liu <yi.l....@intel.com>
>>>
>>> Add a framework to check and synchronize host IOMMU cap/ecap with
>>> vIOMMU cap/ecap.
>>>
>>> The sequence will be:
>>>
>>> vtd_cap_init() initializes iommu->cap/ecap.
>>> vtd_check_hdev() update iommu->cap/ecap based on host cap/ecap.
>>> iommu->cap_frozen set when machine create done, iommu->cap/ecap
>> become readonly.
>>> Implementation details for different backends will be in following patches.
>>>
>>> Signed-off-by: Yi Liu <yi.l....@intel.com>
>>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>> ---
>>> include/hw/i386/intel_iommu.h | 1 +
>>> hw/i386/intel_iommu.c | 41
>> ++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 41 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/i386/intel_iommu.h
>> b/include/hw/i386/intel_iommu.h
>>> index bbc7b96add..c71a133820 100644
>>> --- a/include/hw/i386/intel_iommu.h
>>> +++ b/include/hw/i386/intel_iommu.h
>>> @@ -283,6 +283,7 @@ struct IntelIOMMUState {
>>>
>>> uint64_t cap; /* The value of capability reg */
>>> uint64_t ecap; /* The value of extended capability
>>> reg */
>>> + bool cap_frozen; /* cap/ecap become read-only after
>>> frozen */
>>>
>>> uint32_t context_cache_gen; /* Should be in [1,MAX] */
>>> GHashTable *iotlb; /* IOTLB */
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index ffa1ad6429..7ed2b79669 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -3819,6 +3819,31 @@ VTDAddressSpace
>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>> return vtd_dev_as;
>>> }
>>>
>>> +static int vtd_check_legacy_hdev(IntelIOMMUState *s,
>>> + IOMMULegacyDevice *ldev,
>>> + Error **errp)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
>>> + IOMMUFDDevice *idev,
>>> + Error **errp)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice
>> *vtd_hdev,
>>> + Error **errp)
>>> +{
>>> + HostIOMMUDevice *base_dev = vtd_hdev->dev;
>>> +
>>> + if (base_dev->type == HID_LEGACY) {
>>> + return vtd_check_legacy_hdev(s, vtd_hdev->ldev, errp);
>>> + }
>>> + return vtd_check_iommufd_hdev(s, vtd_hdev->idev, errp);
>> Couldn't we have HostIOMMUDevice ops instead of having this check here?
> Hmm, not sure if this is deserved. If we define such ops, it has only one
> function
> check_hdev and we still need to check base_dev->type to assign different
> function to HostIOMMUDevice.ops.check_hdev in vtd_dev_set_iommu_device().
OK maybe this is over engineered. Drop that comment for now
Thanks
Eric
>
> Thanks
> Zhenzhong
