iommu/intel-iommu: fix pasid table size encoding
PATCH 1/1 iommu/intel-iommu: fix pasid table size encoding
Different encodings are used to represent supported PASID bits and number of PASID table entries.
The current code assigns ecap_pss directly to extended context table entry PTS which is wrong and could result in writing non-zero bits to the reserved fields. IOMMU fault reason 11 will be reported when reserved bits are nonzero. This patch converts ecap_pss to extend context entry pts encoding based on VT-d spec. Chapter 9.4 as follows:
- number of PASID bits = ecap_pss + 1
- number of PASID table entries = 2^(pts + 5)
Software assigned limit of pasid_max value is also respected to match the allocation limitation of PASID table.
--- drivers/iommu/intel-iommu.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
drivers/iommu/intel-iommu.c +26 -1
@@ -5173,6 +5173,29 @@ static void intel_iommu_remove_device(struct device *dev) }#ifdef CONFIG_INTEL_IOMMU_SVM +#define MAX_NR_PASID_BITS (20) +static inline unsigned long intel_iommu_get_pts (struct intel_iommu *iommu) +{ +unsigned long pts; + +/* +* Convert ecap_pss to extend context entry pts encoding, also +* respect the soft pasid_max value set by the iommu. +* - number of PASID bits = ecap_pss + 1 +* - number of PASID table entries = 2^(pts + 5) +* Therefore, pts = ecap_pss - 4 +* e.g. KBL ecap_pss = 0x13, PASID has 20 bits, pts = 15 +*/ +if (ecap_pss (iommu->ecap) <5 ) +return 0 ; + + pts = (ecap_pss (iommu->ecap) -4 ); + +/* pasid_max is encoded as actual number of entries not the bits */ +return min (find_first_bit ((unsigned long *)&iommu->pasid_max, +MAX_NR_PASID_BITS ) -5 , pts); +} +int intel_iommu_enable_pasid (struct intel_iommu *iommu,struct intel_svm_dev *sdev) {struct device_domain_info *info;
@@ -5205,7 +5228,9 @@ int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sdif (!(ctx_lo &CONTEXT_PASIDE )) { context[1 ].hi = (u64 )virt_to_phys (iommu->pasid_state_table); - context[1 ].lo = (u64 )virt_to_phys (iommu->pasid_table) |ecap_pss (iommu->ecap); + context[1 ].lo = (u64 )virt_to_phys (iommu->pasid_table) | +intel_iommu_get_pts (iommu); +wmb ();/* CONTEXT_TT_MULTI_LEVEL and CONTEXT_TT_DEV_IOTLB are both * extended to permit requests-with-PASID if the PASIDE bit
Signature · 1 line
2.7.4
Hi David,
Any thoughts on this one? Without this patch Kabylake would fail with IOMMU error when svm is initialized.
Thanks,
Jacob
Quoted 67 lines · On Thu, 1 Dec 2016 13:50:26 -0800 Jacob Pan <[email protected]>…
Different encodings are used to represent supported PASID bits and number of PASID table entries.
The current code assignsecap_pssdirectly to extended context table entry PTS which is wrong and could result in writing non-zero bits to the reserved fields. IOMMU fault reason 11 will be reported when reserved bits are nonzero. This patch convertsecap_pssto extend context entry pts encoding based on VT-d spec. Chapter 9.4 as follows:
- number of PASID bits =ecap_pss+ 1
- number of PASID table entries = 2^(pts + 5)
Software assigned limit ofpasid_maxvalue is also respected to match the allocation limitation of PASID table.Signed-off-by Jacob Pan <[email protected]>--- drivers/iommu/intel-iommu.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)drivers/iommu/intel-iommu.c +6
-0@@ -5173,6 +5173,29 @@ static void intel_iommu_remove_device(struct device *dev) }#ifdef CONFIG_INTEL_IOMMU_SVM +#define MAX_NR_PASID_BITS (20) +static inline unsigned long intel_iommu_get_pts (struct intel_iommu *iommu) +{ +unsigned long pts; + +/* +* Convert ecap_pss to extend context entry pts encoding, also
drivers/iommu/intel-iommu.c +12
-0+* respect the soft pasid_max value set by the iommu. +* - number of PASID bits = ecap_pss + 1 +* - number of PASID table entries = 2^(pts + 5) +* Therefore, pts = ecap_pss - 4 +* e.g. KBL ecap_pss = 0x13, PASID has 20 bits, pts = 15 +*/ +if (ecap_pss (iommu->ecap) <5 ) +return 0 ; + + pts = (ecap_pss (iommu->ecap) -4 ); + +/* pasid_max is encoded as actual number of entries not the bits */
drivers/iommu/intel-iommu.c +4
-0+return min (find_first_bit ((unsigned long *)&iommu->pasid_max, +MAX_NR_PASID_BITS ) -5 , pts); +} + int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct
intel_svm_dev*sdev) {drivers/iommu/intel-iommu.c +1
-1struct device_domain_info *info;@@ -5205,7 +5228,9 @@ int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sdif (!(ctx_lo &CONTEXT_PASIDE )) { context[1 ].hi = (u64 )virt_to_phys (iommu->pasid_state_table); - context[1 ].lo = (u64 )virt_to_phys (iommu->pasid_table) |ecap_pss (iommu->ecap); + context[1].lo =(u64)virt_to_phys(iommu->pasid_table) |
drivers/iommu/intel-iommu.c +2
-0+intel_iommu_get_pts (iommu); +wmb ();/* CONTEXT_TT_MULTI_LEVEL and CONTEXT_TT_DEV_IOTLB
Quoted 3 lines
are both
* extended to permit requests-with-PASID if the
PASIDE bit
[Jacob Pan]
Joerg Roedel
maintainerHi Jacob,
Quoted 19 lines · Jacob Pan
drivers/iommu/intel-iommu.c +11
-0@@ -5173,6 +5173,29 @@ static void intel_iommu_remove_device(struct device *dev) }#ifdef CONFIG_INTEL_IOMMU_SVM +#define MAX_NR_PASID_BITS (20) +static inline unsigned long intel_iommu_get_pts (struct intel_iommu *iommu) +{ +unsigned long pts; + +/* +* Convert ecap_pss to extend context entry pts encoding, also +* respect the soft pasid_max value set by the iommu. +* - number of PASID bits = ecap_pss + 1 +* - number of PASID table entries = 2^(pts + 5) +* Therefore, pts = ecap_pss - 4
drivers/iommu/intel-iommu.c static void intel_iommu_remove_device(struct device *dev) · line 5196
Quoted 10 lines
drivers/iommu/intel-iommu.c +10
-0+* e.g. KBL ecap_pss = 0x13, PASID has 20 bits, pts = 15 +*/ +if (ecap_pss (iommu->ecap) <5 ) +return 0 ; + + pts = (ecap_pss (iommu->ecap) -4 ); + +/* pasid_max is encoded as actual number of entries not the bits */ +return min (find_first_bit ((unsigned long *)&iommu->pasid_max, +MAX_NR_PASID_BITS ) -5 , pts);
Iommu->max_pasid already depends on ecap_pss(), so I think it is better to just calculate the pts value as ffs(iommu->max_pasid) - 5. This way you don't need an extra function and have a simpler calculation.
Joerg
Quoted 33 lines · On Tue, 6 Dec 2016 17:31:11 +0100 Joerg Roedel <[email protected]> wrote:
Hi Jacob,
Quoted 30 lines · Jacob Pan
(quoted diff) +0
-0
b/drivers/iommu/intel-iommu.cindex 27596e6..f112aa9 100644drivers/iommu/intel-iommu.c +6
-0@@ -5173,6 +5173,29 @@ static void intel_iommu_remove_device(struct device *dev) }#ifdef CONFIG_INTEL_IOMMU_SVM +#define MAX_NR_PASID_BITS (20) +static inline unsigned long intel_iommu_get_pts (struct intel_iommu *iommu) +{ +unsigned long pts; + +/* +* Convert ecap_pss to extend context entry pts encoding, also
drivers/iommu/intel-iommu.c +12
-0+* respect the soft pasid_max value set by the iommu. +* - number of PASID bits = ecap_pss + 1 +* - number of PASID table entries = 2^(pts + 5) +* Therefore, pts = ecap_pss - 4 +* e.g. KBL ecap_pss = 0x13, PASID has 20 bits, pts = 15 +*/ +if (ecap_pss (iommu->ecap) <5 ) +return 0 ; + + pts = (ecap_pss (iommu->ecap) -4 ); + +/* pasid_max is encoded as actual number of entries not the bits */
drivers/iommu/intel-iommu.c +1
-0+ return min(find_first_bit((unsigned long
Quoted 9 lines
Quoted 3 lines
*)&iommu->pasid_max,
drivers/iommu/intel-iommu.c +1
-0+ MAX_NR_PASID_BITS) - 5,pts);
Iommu->max_pasidalready depends onecap_pss(), so I think it is better to just calculate the pts value asffs(iommu->max_pasid)- 5. This way you don't need an extra function and have a simpler calculation.
Good point. I will make the change. Still keeping the lower bond sanity check to prevent insanely small pss.
I initially did the patch w/o knowing David's pasid_max patch in rc7. I was under the impression that pasid_max is a temp solution until we remove the requirement of contiguous memory allocation.
Thanks,
Jacob