Joao,
On 10/17/2023 4:54 PM, Joao Martins wrote:
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index af36c627022f..31b333cc6fe1 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
....
@@ -2156,11 +2160,17 @@ static inline u64 dma_max_address(void)
return ((1ULL << PM_LEVEL_SHIFT(amd_iommu_gpt_level)) - 1);
}
+static bool amd_iommu_hd_support(struct amd_iommu *iommu)
+{
+ return iommu && (iommu->features & FEATURE_HDSUP);
+}
+
You can use the newly introduced check_feature(u64 mask) to check the HD support.
It appears that the check_feature() is logically equivalent to
check_feature_on_all_iommus(); where this check is per-device/per-iommu check to
support potentially nature of different IOMMUs with different features. Being
per-IOMMU would allow you to have firmware to not advertise certain IOMMU
features on some devices while still supporting for others. I understand this is
not a thing in x86, but the UAPI supports it. Having said that, you still want
me to switch to check_feature() ?
So far, AMD does not have system w/ multiple IOMMUs, which have
different EFR/EFR2. However, the AMD IOMMU spec does not enforce
EFR/EFR2 of all IOMMU instances to be the same. There are certain
features, which require consistent support across all IOMMUs. That's why
we introduced the system-wide amd_iommu_efr / amd_iommu_efr2 to simpify
feature checking logic in the driver.
For EFR[HDSup], let's consider a VM with two VFIO pass-through devices
(dev_A and dev_B). Each device is on different IOMMU instance (IOMMU_A,
IOMMU_B), where only IOMMU_A has EFR[HDSUP]=1.
If we call do_iommu_domain_alloc(type, dev_A,
IOMMU_HWPT_ALLOC_ENFORCE_DIRTY), this should return a domain w/
dirty_ops set. Then, if we attach dev_B to the same domain, the
following check should return -EINVAL.
@@ -2252,6 +2268,9 @@ static int amd_iommu_attach_device(struct
iommu_domain *dom,
return 0;
dev_data->defer_attach = false;
+ if (dom->dirty_ops && iommu &&
+ !(iommu->features & FEATURE_HDSUP))
+ return -EINVAL;
which means dev_A and dev_B cannot be in the same VFIO domain.
In this case, since we can prevent devices on IOMMUs w/ different
EFR[HDSUP] bit to share the same domain, it should be safe to support
dirty tracking on such system, and it makes sense to just check the
per-IOMMU EFR value (i.e. iommu->features). If we decide to keep this,
we probably should put comment in the code to describe this.
Thanks,
Suravee
