>-----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(). Thanks Zhenzhong