[PATCH v2 0/2] vtd: Minor cleanup

[PATCH v2 0/2] vtd: Minor cleanup

[Zhenzhong Duan]

Hi,

This cleanup the vtd dump code to dump entry when it's present.
See patch log for details.

Thanks
Zhenzhong

Changelog:
v2:
- Add Fixes tag in patch1 (Baolu)
- Refine commit log in patch2 (Baolu)

Zhenzhong Duan (2):
  iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
  iommu/vt-d: Fix checks in pgtable_walk()

 drivers/iommu/intel/iommu.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()

[Zhenzhong Duan]

In dmar_fault_dump_ptes(), return value of phys_to_virt() is used for
checking if an entry is present. It's never NULL on x86 platform at least.
This makes some zero entries are dumped like below:

[  442.240357] DMAR: pasid dir entry: 0x000000012c83e001
[  442.246661] DMAR: pasid table entry[0]: 0x0000000000000000
[  442.253429] DMAR: pasid table entry[1]: 0x0000000000000000
[  442.260203] DMAR: pasid table entry[2]: 0x0000000000000000
[  442.266969] DMAR: pasid table entry[3]: 0x0000000000000000
[  442.273733] DMAR: pasid table entry[4]: 0x0000000000000000
[  442.280479] DMAR: pasid table entry[5]: 0x0000000000000000
[  442.287234] DMAR: pasid table entry[6]: 0x0000000000000000
[  442.293989] DMAR: pasid table entry[7]: 0x0000000000000000
[  442.300742] DMAR: PTE not present at level 2

Fix it by checking present bit of an entry before dump its content.

Fixes: 914ff7719e8a ("iommu/vt-d: Dump DMAR translation structure when DMA fault occurs")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 drivers/iommu/intel/iommu.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a564eeaf2375..8288b0ee7a61 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -733,12 +733,17 @@ void dmar_fault_dump_ptes(struct intel_iommu *iommu, u16 source_id,
 	u8 devfn = source_id & 0xff;
 	u8 bus = source_id >> 8;
 	struct dma_pte *pgtable;
+	u64 entry;
 
 	pr_info("Dump %s table entries for IOVA 0x%llx\n", iommu->name, addr);
 
 	/* root entry dump */
 	rt_entry = &iommu->root_entry[bus];
-	if (!rt_entry) {
+	entry = rt_entry->lo;
+	if (sm_supported(iommu) && devfn >= 0x80)
+		entry = rt_entry->hi;
+
+	if (!(entry & 1)) {
 		pr_info("root table entry is not present\n");
 		return;
 	}
@@ -766,28 +771,32 @@ void dmar_fault_dump_ptes(struct intel_iommu *iommu, u16 source_id,
 		goto pgtable_walk;
 	}
 
+	/* For request-without-pasid, get the pasid from context entry */
+	if (pasid == IOMMU_PASID_INVALID)
+		pasid = IOMMU_NO_PASID;
+
 	/* get the pointer to pasid directory entry */
 	dir = phys_to_virt(ctx_entry->lo & VTD_PAGE_MASK);
-	if (!dir) {
+	dir_index = pasid >> PASID_PDE_SHIFT;
+	pde = &dir[dir_index];
+
+	if (!pasid_pde_is_present(pde)) {
 		pr_info("pasid directory entry is not present\n");
 		return;
 	}
-	/* For request-without-pasid, get the pasid from context entry */
-	if (intel_iommu_sm && pasid == IOMMU_PASID_INVALID)
-		pasid = IOMMU_NO_PASID;
 
-	dir_index = pasid >> PASID_PDE_SHIFT;
-	pde = &dir[dir_index];
 	pr_info("pasid dir entry: 0x%016llx\n", pde->val);
 
 	/* get the pointer to the pasid table entry */
-	entries = get_pasid_table_from_pde(pde);
-	if (!entries) {
+	entries = phys_to_virt(READ_ONCE(pde->val) & PDE_PFN_MASK);
+	index = pasid & PASID_PTE_MASK;
+	pte = &entries[index];
+
+	if (!pasid_pte_is_present(pte)) {
 		pr_info("pasid table entry is not present\n");
 		return;
 	}
-	index = pasid & PASID_PTE_MASK;
-	pte = &entries[index];
+
 	for (i = 0; i < ARRAY_SIZE(pte->val); i++)
 		pr_info("pasid table entry[%d]: 0x%016llx\n", i, pte->val[i]);
 
-- 
2.34.1

[PATCH v2 2/2] iommu/vt-d: Fix checks in pgtable_walk()

[Zhenzhong Duan]

There are some issues in pgtable_walk():

1. Super page is dumped as non-present page
2. dma_pte_superpage() should not check against leaf page table entries
3. Pointer pte is never NULL so checking it is meaningless

Fix it by moving the check of super page after (level == 1) check and
removing pte check.

Fixes: 914ff7719e8a ("iommu/vt-d: Dump DMAR translation structure when DMA fault occurs")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 drivers/iommu/intel/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8288b0ee7a61..fec5cc1147f3 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -707,14 +707,14 @@ static void pgtable_walk(struct intel_iommu *iommu, unsigned long pfn,
 	while (1) {
 		offset = pfn_level_offset(pfn, level);
 		pte = &parent[offset];
-		if (!pte || (dma_pte_superpage(pte) || !dma_pte_present(pte))) {
+		if (!dma_pte_present(pte)) {
 			pr_info("PTE not present at level %d\n", level);
 			break;
 		}
 
 		pr_info("pte level: %d, pte value: 0x%016llx\n", level, pte->val);
 
-		if (level == 1)
+		if (level == 1 || dma_pte_superpage(pte))
 			break;
 
 		parent = phys_to_virt(dma_pte_addr(pte));
-- 
2.34.1

RE: [PATCH v2 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()

[Duan, Zhenzhong]

Just found I mixed the present bit processing with non-present entry in this patch,
I'll resend after fix, please ignore it. Sorry for the noise.

Thanks
Zhenzhong

>-----Original Message-----
>From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Subject: [PATCH v2 1/2] iommu/vt-d: Fix checks in dmar_fault_dump_ptes()
>
>In dmar_fault_dump_ptes(), return value of phys_to_virt() is used for
>checking if an entry is present. It's never NULL on x86 platform at least.
>This makes some zero entries are dumped like below:
>
>[  442.240357] DMAR: pasid dir entry: 0x000000012c83e001
>[  442.246661] DMAR: pasid table entry[0]: 0x0000000000000000
>[  442.253429] DMAR: pasid table entry[1]: 0x0000000000000000
>[  442.260203] DMAR: pasid table entry[2]: 0x0000000000000000
>[  442.266969] DMAR: pasid table entry[3]: 0x0000000000000000
>[  442.273733] DMAR: pasid table entry[4]: 0x0000000000000000
>[  442.280479] DMAR: pasid table entry[5]: 0x0000000000000000
>[  442.287234] DMAR: pasid table entry[6]: 0x0000000000000000
>[  442.293989] DMAR: pasid table entry[7]: 0x0000000000000000
>[  442.300742] DMAR: PTE not present at level 2
>
>Fix it by checking present bit of an entry before dump its content.
>
>Fixes: 914ff7719e8a ("iommu/vt-d: Dump DMAR translation structure when DMA
>fault occurs")
>Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>---
> drivers/iommu/intel/iommu.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>index a564eeaf2375..8288b0ee7a61 100644
>--- a/drivers/iommu/intel/iommu.c
>+++ b/drivers/iommu/intel/iommu.c
>@@ -733,12 +733,17 @@ void dmar_fault_dump_ptes(struct intel_iommu
>*iommu, u16 source_id,
> 	u8 devfn = source_id & 0xff;
> 	u8 bus = source_id >> 8;
> 	struct dma_pte *pgtable;
>+	u64 entry;
>
> 	pr_info("Dump %s table entries for IOVA 0x%llx\n", iommu->name, addr);
>
> 	/* root entry dump */
> 	rt_entry = &iommu->root_entry[bus];
>-	if (!rt_entry) {
>+	entry = rt_entry->lo;
>+	if (sm_supported(iommu) && devfn >= 0x80)
>+		entry = rt_entry->hi;
>+
>+	if (!(entry & 1)) {
> 		pr_info("root table entry is not present\n");
> 		return;
> 	}
>@@ -766,28 +771,32 @@ void dmar_fault_dump_ptes(struct intel_iommu
>*iommu, u16 source_id,
> 		goto pgtable_walk;
> 	}
>
>+	/* For request-without-pasid, get the pasid from context entry */
>+	if (pasid == IOMMU_PASID_INVALID)
>+		pasid = IOMMU_NO_PASID;
>+
> 	/* get the pointer to pasid directory entry */
> 	dir = phys_to_virt(ctx_entry->lo & VTD_PAGE_MASK);
>-	if (!dir) {
>+	dir_index = pasid >> PASID_PDE_SHIFT;
>+	pde = &dir[dir_index];
>+
>+	if (!pasid_pde_is_present(pde)) {
> 		pr_info("pasid directory entry is not present\n");
> 		return;
> 	}
>-	/* For request-without-pasid, get the pasid from context entry */
>-	if (intel_iommu_sm && pasid == IOMMU_PASID_INVALID)
>-		pasid = IOMMU_NO_PASID;
>
>-	dir_index = pasid >> PASID_PDE_SHIFT;
>-	pde = &dir[dir_index];
> 	pr_info("pasid dir entry: 0x%016llx\n", pde->val);
>
> 	/* get the pointer to the pasid table entry */
>-	entries = get_pasid_table_from_pde(pde);
>-	if (!entries) {
>+	entries = phys_to_virt(READ_ONCE(pde->val) & PDE_PFN_MASK);
>+	index = pasid & PASID_PTE_MASK;
>+	pte = &entries[index];
>+
>+	if (!pasid_pte_is_present(pte)) {
> 		pr_info("pasid table entry is not present\n");
> 		return;
> 	}
>-	index = pasid & PASID_PTE_MASK;
>-	pte = &entries[index];
>+
> 	for (i = 0; i < ARRAY_SIZE(pte->val); i++)
> 		pr_info("pasid table entry[%d]: 0x%016llx\n", i, pte->val[i]);
>
>--
>2.34.1