io-pgtable-arm + drm/msm: Extend iova fault debugging

drivers/iommu/io-pgtable-arm.c +19 -21

index f5d9fd1f45bf..b4bc358740e0 100644
@@ -747,33 +747,31 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
 }
 
 struct io_pgtable_walk_data {
-	struct iommu_dirty_bitmap	*dirty;
+	void				*data;
+	int (*visit)(struct io_pgtable_walk_data *walk_data, int lvl,
+		     arm_lpae_iopte pte, size_t size);
 	unsigned long			flags;
 	u64				addr;
 	const u64			end;
 };
 
-static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
-				       struct io_pgtable_walk_data *walk_data,
-				       arm_lpae_iopte *ptep,
-				       int lvl);
+static int __arm_lpae_iopte_walk(struct arm_lpae_io_pgtable *data,
+				 struct io_pgtable_walk_data *walk_data,
+				 arm_lpae_iopte *ptep,
+				 int lvl);
 
-static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
-				  struct io_pgtable_walk_data *walk_data,
-				  arm_lpae_iopte *ptep, int lvl)
+static int io_pgtable_visit(struct arm_lpae_io_pgtable *data,
+			    struct io_pgtable_walk_data *walk_data,
+			    arm_lpae_iopte *ptep, int lvl)
 {
 	struct io_pgtable *iop = &data->iop;
 	arm_lpae_iopte pte = READ_ONCE(*ptep);
 
 	if (iopte_leaf(pte, lvl, iop->fmt)) {
 		size_t size = ARM_LPAE_BLOCK_SIZE(lvl, data);
-
-		if (iopte_writeable_dirty(pte)) {
-			iommu_dirty_bitmap_record(walk_data->dirty,
-						  walk_data->addr, size);
-			if (!(walk_data->flags & IOMMU_DIRTY_NO_CLEAR))
-				iopte_set_writeable_clean(ptep);
-		}
+		int ret = walk_data->visit(walk_data, lvl, pte, size);
+		if (ret)
+			return ret;
 		walk_data->addr += size;
 		return 0;
 	}

@@ -782,13 +780,13 @@ static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
 		return -EINVAL;
 
 	ptep = iopte_deref(pte, data);
-	return __arm_lpae_iopte_walk_dirty(data, walk_data, ptep, lvl + 1);
+	return __arm_lpae_iopte_walk(data, walk_data, ptep, lvl + 1);
 }
 
-static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
-				       struct io_pgtable_walk_data *walk_data,
-				       arm_lpae_iopte *ptep,
-				       int lvl)
+static int __arm_lpae_iopte_walk(struct arm_lpae_io_pgtable *data,
+				 struct io_pgtable_walk_data *walk_data,
+				 arm_lpae_iopte *ptep,
+				 int lvl)
 {
 	u32 idx;
 	int max_entries, ret;

@@ -803,7 +801,7 @@ static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
 
 	for (idx = ARM_LPAE_LVL_IDX(walk_data->addr, lvl, data);
 	     (idx < max_entries) && (walk_data->addr < walk_data->end); ++idx) {
-		ret = io_pgtable_visit_dirty(data, walk_data, ptep + idx, lvl);
+		ret = io_pgtable_visit(data, walk_data, ptep + idx, lvl);
 		if (ret)
 			return ret;
 	}

@@ -811,6 +809,20 @@ static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
 	return 0;
 }

Quoted 83 lines · Rob Clark

drivers/iommu/io-pgtable-arm.c +19 -21

index f5d9fd1f45bf..b4bc358740e0 100644
@@ -747,33 +747,31 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
 }
 
 struct io_pgtable_walk_data {
-     struct iommu_dirty_bitmap       *dirty;
+     void                            *data;
+     int (*visit)(struct io_pgtable_walk_data *walk_data, int lvl,
+                  arm_lpae_iopte pte, size_t size);
      unsigned long                   flags;
      u64                             addr;
      const u64                       end;
 };
 
-static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
-                                    struct io_pgtable_walk_data *walk_data,
-                                    arm_lpae_iopte *ptep,
-                                    int lvl);
+static int __arm_lpae_iopte_walk(struct arm_lpae_io_pgtable *data,
+                              struct io_pgtable_walk_data *walk_data,
+                              arm_lpae_iopte *ptep,
+                              int lvl);
 
-static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
-                               struct io_pgtable_walk_data *walk_data,
-                               arm_lpae_iopte *ptep, int lvl)
+static int io_pgtable_visit(struct arm_lpae_io_pgtable *data,
+                         struct io_pgtable_walk_data *walk_data,
+                         arm_lpae_iopte *ptep, int lvl)
 {
      struct io_pgtable *iop = &data->iop;
      arm_lpae_iopte pte = READ_ONCE(*ptep);
 
      if (iopte_leaf(pte, lvl, iop->fmt)) {
              size_t size = ARM_LPAE_BLOCK_SIZE(lvl, data);
-
-             if (iopte_writeable_dirty(pte)) {
-                     iommu_dirty_bitmap_record(walk_data->dirty,
-                                               walk_data->addr, size);
-                     if (!(walk_data->flags & IOMMU_DIRTY_NO_CLEAR))
-                             iopte_set_writeable_clean(ptep);
-             }
+             int ret = walk_data->visit(walk_data, lvl, pte, size);
+             if (ret)
+                     return ret;
              walk_data->addr += size;
              return 0;
      }

@@ -782,13 +780,13 @@ static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
              return -EINVAL;
 
      ptep = iopte_deref(pte, data);
-     return __arm_lpae_iopte_walk_dirty(data, walk_data, ptep, lvl + 1);
+     return __arm_lpae_iopte_walk(data, walk_data, ptep, lvl + 1);
 }
 
-static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
-                                    struct io_pgtable_walk_data *walk_data,
-                                    arm_lpae_iopte *ptep,
-                                    int lvl)
+static int __arm_lpae_iopte_walk(struct arm_lpae_io_pgtable *data,
+                              struct io_pgtable_walk_data *walk_data,
+                              arm_lpae_iopte *ptep,
+                              int lvl)
 {
      u32 idx;
      int max_entries, ret;

@@ -803,7 +801,7 @@ static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
 
      for (idx = ARM_LPAE_LVL_IDX(walk_data->addr, lvl, data);
           (idx < max_entries) && (walk_data->addr < walk_data->end); ++idx) {
-             ret = io_pgtable_visit_dirty(data, walk_data, ptep + idx, lvl);
+             ret = io_pgtable_visit(data, walk_data, ptep + idx, lvl);
              if (ret)
                      return ret;
      }

@@ -811,6 +809,20 @@ static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
      return 0;
 }
 

drivers/iommu/io-pgtable-arm.c static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data, · line 820

Quoted 9 lines

drivers/iommu/io-pgtable-arm.c +9 -0

+static int visit_dirty(struct io_pgtable_walk_data *walk_data, int lvl,
+                    arm_lpae_iopte pte, size_t size)
+{
+     struct iommu_dirty_bitmap *dirty = walk_data->data;
+
+     if (iopte_writeable_dirty(pte)) {
+             iommu_dirty_bitmap_record(dirty, walk_data->addr, size);
+             if (!(walk_data->flags & IOMMU_DIRTY_NO_CLEAR))
+                     iopte_set_writeable_clean(&pte);

Are you sure that's correct? I suspect we really want to update the actual page-table in this case, so we probably want to pass the pointer in instead of the pte value.

Will

From: Rob Clark <[email protected]>

Re-use the generic pgtable walk path.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 73 +++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 37 deletions(-)

drivers/iommu/io-pgtable-arm.c +35 -36

index b4bc358740e0..5fa1274a665a 100644
@@ -710,42 +710,6 @@ static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops, unsigned long iov
 				data->start_level, ptep);
 }
 
-static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
-					 unsigned long iova)
-{
-	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
-	arm_lpae_iopte pte, *ptep = data->pgd;
-	int lvl = data->start_level;
-
-	do {
-		/* Valid IOPTE pointer? */
-		if (!ptep)
-			return 0;
-
-		/* Grab the IOPTE we're interested in */
-		ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
-		pte = READ_ONCE(*ptep);
-
-		/* Valid entry? */
-		if (!pte)
-			return 0;
-
-		/* Leaf entry? */
-		if (iopte_leaf(pte, lvl, data->iop.fmt))
-			goto found_translation;
-
-		/* Take it to the next level */
-		ptep = iopte_deref(pte, data);
-	} while (++lvl < ARM_LPAE_MAX_LEVELS);
-
-	/* Ran out of page tables to walk */
-	return 0;
-
-found_translation:
-	iova &= (ARM_LPAE_BLOCK_SIZE(lvl, data) - 1);
-	return iopte_to_paddr(pte, data) | iova;
-}
-
 struct io_pgtable_walk_data {
 	void				*data;
 	int (*visit)(struct io_pgtable_walk_data *walk_data, int lvl,

@@ -760,6 +724,41 @@ static int __arm_lpae_iopte_walk(struct arm_lpae_io_pgtable *data,
 				 arm_lpae_iopte *ptep,
 				 int lvl);
 
+struct iova_to_phys_data {
+	arm_lpae_iopte pte;
+	int lvl;
+};
+
+static int visit_iova_to_phys(struct io_pgtable_walk_data *walk_data, int lvl,
+			      arm_lpae_iopte pte, size_t size)
+{
+	struct iova_to_phys_data *data = walk_data->data;
+	data->pte = pte;
+	data->lvl = lvl;
+	return 0;
+}
+
+static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
+					 unsigned long iova)
+{
+	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
+	struct iova_to_phys_data d;
+	struct io_pgtable_walk_data walk_data = {
+		.data = &d,
+		.visit = visit_iova_to_phys,
+		.addr = iova,
+		.end = iova + 1,
+	};
+	int ret;
+
+	ret = __arm_lpae_iopte_walk(data, &walk_data, data->pgd, data->start_level);
+	if (ret)
+		return 0;
+
+	iova &= (ARM_LPAE_BLOCK_SIZE(d.lvl, data) - 1);
+	return iopte_to_paddr(d.pte, data) | iova;
+}
+
 static int io_pgtable_visit(struct arm_lpae_io_pgtable *data,
 			    struct io_pgtable_walk_data *walk_data,
 			    arm_lpae_iopte *ptep, int lvl)

Quoted 102 lines · Rob Clark

From: Rob Clark <[email protected]>

Re-use the generic pgtable walk path.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 73 +++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 37 deletions(-)

drivers/iommu/io-pgtable-arm.c +35 -36

index b4bc358740e0..5fa1274a665a 100644
@@ -710,42 +710,6 @@ static size_t arm_lpae_unmap_pages(struct io_pgtable_ops *ops, unsigned long iov
                              data->start_level, ptep);
 }
 
-static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
-                                      unsigned long iova)
-{
-     struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
-     arm_lpae_iopte pte, *ptep = data->pgd;
-     int lvl = data->start_level;
-
-     do {
-             /* Valid IOPTE pointer? */
-             if (!ptep)
-                     return 0;
-
-             /* Grab the IOPTE we're interested in */
-             ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
-             pte = READ_ONCE(*ptep);
-
-             /* Valid entry? */
-             if (!pte)
-                     return 0;
-
-             /* Leaf entry? */
-             if (iopte_leaf(pte, lvl, data->iop.fmt))
-                     goto found_translation;
-
-             /* Take it to the next level */
-             ptep = iopte_deref(pte, data);
-     } while (++lvl < ARM_LPAE_MAX_LEVELS);
-
-     /* Ran out of page tables to walk */
-     return 0;
-
-found_translation:
-     iova &= (ARM_LPAE_BLOCK_SIZE(lvl, data) - 1);
-     return iopte_to_paddr(pte, data) | iova;
-}
-
 struct io_pgtable_walk_data {
      void                            *data;
      int (*visit)(struct io_pgtable_walk_data *walk_data, int lvl,

@@ -760,6 +724,41 @@ static int __arm_lpae_iopte_walk(struct arm_lpae_io_pgtable *data,
                               arm_lpae_iopte *ptep,
                               int lvl);
 
+struct iova_to_phys_data {
+     arm_lpae_iopte pte;
+     int lvl;
+};
+
+static int visit_iova_to_phys(struct io_pgtable_walk_data *walk_data, int lvl,
+                           arm_lpae_iopte pte, size_t size)
+{
+     struct iova_to_phys_data *data = walk_data->data;
+     data->pte = pte;
+     data->lvl = lvl;
+     return 0;
+}
+
+static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
+                                      unsigned long iova)
+{
+     struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
+     struct iova_to_phys_data d;
+     struct io_pgtable_walk_data walk_data = {
+             .data = &d,
+             .visit = visit_iova_to_phys,
+             .addr = iova,
+             .end = iova + 1,
+     };
+     int ret;
+
+     ret = __arm_lpae_iopte_walk(data, &walk_data, data->pgd, data->start_level);
+     if (ret)
+             return 0;
+
+     iova &= (ARM_LPAE_BLOCK_SIZE(d.lvl, data) - 1);
+     return iopte_to_paddr(d.pte, data) | iova;
+}
+
 static int io_pgtable_visit(struct arm_lpae_io_pgtable *data,
                          struct io_pgtable_walk_data *walk_data,
                          arm_lpae_iopte *ptep, int lvl)

@@ -776,7 +775,7 @@ static int io_pgtable_visit(struct arm_lpae_io_pgtable *data,
              return 0;
      }
 

drivers/iommu/io-pgtable-arm.c static int io_pgtable_visit(struct arm_lpae_io_pgtable *data, · line 778

Quoted 2 lines

drivers/iommu/io-pgtable-arm.c +1 -1

-     if (WARN_ON(!iopte_table(pte, lvl)))
+     if (WARN_ON(!iopte_table(pte, lvl) && !selftest_running))

Why do you care about the selftest here?

Will

In the case of iova fault triggered devcore dumps, include additional debug information based on what we think is the current page tables, including the TTBR0 value (which should match what we have in adreno_smmu_fault_info unless things have gone horribly wrong), and the pagetable entries traversed in the process of resolving the faulting iova.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++++++
 drivers/gpu/drm/msm/msm_gpu.c           |  9 +++++++++
 drivers/gpu/drm/msm/msm_gpu.h           |  8 ++++++++
 drivers/gpu/drm/msm/msm_iommu.c         | 22 ++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_mmu.h           |  3 ++-
 5 files changed, 51 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/adreno/adreno_gpu.c +0 -0

index 1c6626747b98..3848b5a64351 100644

drivers/gpu/drm/msm/adreno/adreno_gpu.c +0 -0

 	}
 
 	drm_printf(p, "rbbm-status: 0x%08x\n", state->rbbm_status);

drivers/gpu/drm/msm/msm_gpu.c +9 -0

index 3666b42b4ecd..bf2f8b2a7ccc 100644
@@ -281,6 +281,15 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
 	if (submit) {
 		int i;
 
+		if (state->fault_info.ttbr0) {
+			struct msm_gpu_fault_info *info = &state->fault_info;
+			struct msm_mmu *mmu = submit->aspace->mmu;
+
+			msm_iommu_pagetable_params(mmu, &info->pgtbl_ttbr0,
+						   &info->asid);
+			msm_iommu_pagetable_walk(mmu, info->iova, info->ptes);
+		}
+
 		state->bos = kcalloc(submit->nr_bos,
 			sizeof(struct msm_gpu_state_bo), GFP_KERNEL);
 

drivers/gpu/drm/msm/msm_gpu.h +8 -0

index 1f02bb9956be..82e838ba8c80 100644
@@ -101,6 +101,14 @@ struct msm_gpu_fault_info {
 	int flags;
 	const char *type;
 	const char *block;
+
+	/* Information about what we think/expect is the current SMMU state,
+	 * for example expected_ttbr0 should match smmu_info.ttbr0 which
+	 * was read back from SMMU registers.
+	 */
+	phys_addr_t pgtbl_ttbr0;
+	u64 ptes[4];
+	int asid;
 };
 
 /**

drivers/gpu/drm/msm/msm_iommu.c +22 -0

index 2a94e82316f9..3e692818ba1f 100644
@@ -195,6 +195,28 @@ struct iommu_domain_geometry *msm_iommu_get_geometry(struct msm_mmu *mmu)
 	return &iommu->domain->geometry;
 }
 
+int
+msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova, uint64_t ptes[4])
+{
+	struct msm_iommu_pagetable *pagetable;
+	struct arm_lpae_io_pgtable_walk_data wd = {};
+
+	if (mmu->type != MSM_MMU_IOMMU_PAGETABLE)
+		return -EINVAL;
+
+	pagetable = to_pagetable(mmu);
+
+	if (!pagetable->pgtbl_ops->pgtable_walk)
+		return -EINVAL;
+
+	pagetable->pgtbl_ops->pgtable_walk(pagetable->pgtbl_ops, iova, &wd);
+
+	for (int i = 0; i < ARRAY_SIZE(wd.ptes); i++)
+		ptes[i] = wd.ptes[i];
+
+	return 0;
+}
+
 static const struct msm_mmu_funcs pagetable_funcs = {
 		.map = msm_iommu_pagetable_map,
 		.unmap = msm_iommu_pagetable_unmap,

drivers/gpu/drm/msm/msm_mmu.h +2 -1

index 88af4f490881..96e509bd96a6 100644
@@ -53,7 +53,8 @@ static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
 struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent);
 
 int msm_iommu_pagetable_params(struct msm_mmu *mmu, phys_addr_t *ttbr,
-		int *asid);
+			       int *asid);
+int msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova, uint64_t ptes[4]);
 struct iommu_domain_geometry *msm_iommu_get_geometry(struct msm_mmu *mmu);
 
 #endif /* __MSM_MMU_H__ */

Signature · 1 line

2.46.0

On Tue, Aug 20, 2024 at 10:16:47AM -0700, Rob Clark wrote: > From: Rob Clark <[email protected]>

Quoted 27 lines

In the case of iova fault triggered devcore dumps, include additional debug information based on what we think is the current page tables, including the TTBR0 value (which should match what we have in adreno_smmu_fault_info unless things have gone horribly wrong), and the pagetable entries traversed in the process of resolving the faulting iova.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++++++
 drivers/gpu/drm/msm/msm_gpu.c           |  9 +++++++++
 drivers/gpu/drm/msm/msm_gpu.h           |  8 ++++++++
 drivers/gpu/drm/msm/msm_iommu.c         | 22 ++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_mmu.h           |  3 ++-
 5 files changed, 51 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/adreno/adreno_gpu.c +2 -0

index 1c6626747b98..3848b5a64351 100644
@@ -864,6 +864,16 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
              drm_printf(p, "  - dir=%s\n", info->flags & IOMMU_FAULT_WRITE ? "WRITE" : "READ");
              drm_printf(p, "  - type=%s\n", info->type);
              drm_printf(p, "  - source=%s\n", info->block);
+
+             /* Information extracted from what we think are the current

drivers/gpu/drm/msm/adreno/adreno_gpu.c void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state, · line 873

Quoted 5 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +5 -0

+              * pgtables.  Hopefully the TTBR0 matches what we've extracted
+              * from the SMMU registers in smmu_info!
+              */
+             drm_puts(p, "pgtable-fault-info:\n");
+             drm_printf(p, "  - ttbr0: %.16llx\n", (u64)info->pgtbl_ttbr0);

"0x" prefix? Otherwise, it is a bit confusing when the below one is decimal.

drivers/gpu/drm/msm/adreno/adreno_gpu.c void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state, · line 876

Quoted 3 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +3 -0

+             drm_printf(p, "  - asid: %d\n", info->asid);
+             drm_printf(p, "  - ptes: %.16llx %.16llx %.16llx %.16llx\n",
+                        info->ptes[0], info->ptes[1], info->ptes[2], info->ptes[3]);

Does crashdec decodes this?

-Akhil.

Quoted 92 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +0 -0

      }
 
      drm_printf(p, "rbbm-status: 0x%08x\n", state->rbbm_status);

drivers/gpu/drm/msm/msm_gpu.c +9 -0

index 3666b42b4ecd..bf2f8b2a7ccc 100644
@@ -281,6 +281,15 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
      if (submit) {
              int i;
 
+             if (state->fault_info.ttbr0) {
+                     struct msm_gpu_fault_info *info = &state->fault_info;
+                     struct msm_mmu *mmu = submit->aspace->mmu;
+
+                     msm_iommu_pagetable_params(mmu, &info->pgtbl_ttbr0,
+                                                &info->asid);
+                     msm_iommu_pagetable_walk(mmu, info->iova, info->ptes);
+             }
+
              state->bos = kcalloc(submit->nr_bos,
                      sizeof(struct msm_gpu_state_bo), GFP_KERNEL);
 

drivers/gpu/drm/msm/msm_gpu.h +8 -0

index 1f02bb9956be..82e838ba8c80 100644
@@ -101,6 +101,14 @@ struct msm_gpu_fault_info {
      int flags;
      const char *type;
      const char *block;
+
+     /* Information about what we think/expect is the current SMMU state,
+      * for example expected_ttbr0 should match smmu_info.ttbr0 which
+      * was read back from SMMU registers.
+      */
+     phys_addr_t pgtbl_ttbr0;
+     u64 ptes[4];
+     int asid;
 };
 
 /**

drivers/gpu/drm/msm/msm_iommu.c +22 -0

index 2a94e82316f9..3e692818ba1f 100644
@@ -195,6 +195,28 @@ struct iommu_domain_geometry *msm_iommu_get_geometry(struct msm_mmu *mmu)
      return &iommu->domain->geometry;
 }
 
+int
+msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova, uint64_t ptes[4])
+{
+     struct msm_iommu_pagetable *pagetable;
+     struct arm_lpae_io_pgtable_walk_data wd = {};
+
+     if (mmu->type != MSM_MMU_IOMMU_PAGETABLE)
+             return -EINVAL;
+
+     pagetable = to_pagetable(mmu);
+
+     if (!pagetable->pgtbl_ops->pgtable_walk)
+             return -EINVAL;
+
+     pagetable->pgtbl_ops->pgtable_walk(pagetable->pgtbl_ops, iova, &wd);
+
+     for (int i = 0; i < ARRAY_SIZE(wd.ptes); i++)
+             ptes[i] = wd.ptes[i];
+
+     return 0;
+}
+
 static const struct msm_mmu_funcs pagetable_funcs = {
              .map = msm_iommu_pagetable_map,
              .unmap = msm_iommu_pagetable_unmap,

drivers/gpu/drm/msm/msm_mmu.h +2 -2

index 88af4f490881..96e509bd96a6 100644
@@ -53,7 +53,8 @@ static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
 struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent);
 
 int msm_iommu_pagetable_params(struct msm_mmu *mmu, phys_addr_t *ttbr,
-             int *asid);
+                            int *asid);
+int msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova, uint64_t ptes[4]);
 struct iommu_domain_geometry *msm_iommu_get_geometry(struct msm_mmu *mmu);
 
 #endif /* __MSM_MMU_H__ */
--

2.46.0

Quoted 29 lines · Akhil P Oommen

On Tue, Aug 20, 2024 at 10:16:47AM -0700, Rob Clark wrote: > From: Rob Clark <[email protected]>

Quoted 27 lines

In the case of iova fault triggered devcore dumps, include additional debug information based on what we think is the current page tables, including the TTBR0 value (which should match what we have in adreno_smmu_fault_info unless things have gone horribly wrong), and the pagetable entries traversed in the process of resolving the faulting iova.

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++++++
 drivers/gpu/drm/msm/msm_gpu.c           |  9 +++++++++
 drivers/gpu/drm/msm/msm_gpu.h           |  8 ++++++++
 drivers/gpu/drm/msm/msm_iommu.c         | 22 ++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_mmu.h           |  3 ++-
 5 files changed, 51 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/adreno/adreno_gpu.c +2 -0

index 1c6626747b98..3848b5a64351 100644
@@ -864,6 +864,16 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
              drm_printf(p, "  - dir=%s\n", info->flags & IOMMU_FAULT_WRITE ? "WRITE" : "READ");
              drm_printf(p, "  - type=%s\n", info->type);
              drm_printf(p, "  - source=%s\n", info->block);
+
+             /* Information extracted from what we think are the current

Quoted 8 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state, · line 873

Quoted 5 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +5 -0

+              * pgtables.  Hopefully the TTBR0 matches what we've extracted
+              * from the SMMU registers in smmu_info!
+              */
+             drm_puts(p, "pgtable-fault-info:\n");
+             drm_printf(p, "  - ttbr0: %.16llx\n", (u64)info->pgtbl_ttbr0);

"0x" prefix? Otherwise, it is a bit confusing when the below one is decimal.

mixed feelings, the extra 0x is annoying when pasting into calc which is a simple way to get binary decoding

OTOH none of this is machine decoded so I guess we could change it

Quoted 5 lines

Quoted 4 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +3 -0

+             drm_printf(p, "  - asid: %d\n", info->asid);
+             drm_printf(p, "  - ptes: %.16llx %.16llx %.16llx %.16llx\n",
+                        info->ptes[0], info->ptes[1], info->ptes[2], info->ptes[3]);

Quoted 1 line

Does crashdec decodes this?

No, it just passed thru for human eyeballs

crashdec _does_ have some logic to flag buffers that are "near" the faulting iova to help identify if the fault is an underflow/overflow (which has been, along with the pte trail, useful to debug some issues)

BR,
-R

Quoted 94 lines

-Akhil.

Quoted 92 lines

drivers/gpu/drm/msm/adreno/adreno_gpu.c +0 -0

      }
 
      drm_printf(p, "rbbm-status: 0x%08x\n", state->rbbm_status);

drivers/gpu/drm/msm/msm_gpu.c +9 -0

index 3666b42b4ecd..bf2f8b2a7ccc 100644
@@ -281,6 +281,15 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
      if (submit) {
              int i;
 
+             if (state->fault_info.ttbr0) {
+                     struct msm_gpu_fault_info *info = &state->fault_info;
+                     struct msm_mmu *mmu = submit->aspace->mmu;
+
+                     msm_iommu_pagetable_params(mmu, &info->pgtbl_ttbr0,
+                                                &info->asid);
+                     msm_iommu_pagetable_walk(mmu, info->iova, info->ptes);
+             }
+
              state->bos = kcalloc(submit->nr_bos,
                      sizeof(struct msm_gpu_state_bo), GFP_KERNEL);
 

drivers/gpu/drm/msm/msm_gpu.h +8 -0

index 1f02bb9956be..82e838ba8c80 100644
@@ -101,6 +101,14 @@ struct msm_gpu_fault_info {
      int flags;
      const char *type;
      const char *block;
+
+     /* Information about what we think/expect is the current SMMU state,
+      * for example expected_ttbr0 should match smmu_info.ttbr0 which
+      * was read back from SMMU registers.
+      */
+     phys_addr_t pgtbl_ttbr0;
+     u64 ptes[4];
+     int asid;
 };
 
 /**

drivers/gpu/drm/msm/msm_iommu.c +22 -0

index 2a94e82316f9..3e692818ba1f 100644
@@ -195,6 +195,28 @@ struct iommu_domain_geometry *msm_iommu_get_geometry(struct msm_mmu *mmu)
      return &iommu->domain->geometry;
 }
 
+int
+msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova, uint64_t ptes[4])
+{
+     struct msm_iommu_pagetable *pagetable;
+     struct arm_lpae_io_pgtable_walk_data wd = {};
+
+     if (mmu->type != MSM_MMU_IOMMU_PAGETABLE)
+             return -EINVAL;
+
+     pagetable = to_pagetable(mmu);
+
+     if (!pagetable->pgtbl_ops->pgtable_walk)
+             return -EINVAL;
+
+     pagetable->pgtbl_ops->pgtable_walk(pagetable->pgtbl_ops, iova, &wd);
+
+     for (int i = 0; i < ARRAY_SIZE(wd.ptes); i++)
+             ptes[i] = wd.ptes[i];
+
+     return 0;
+}
+
 static const struct msm_mmu_funcs pagetable_funcs = {
              .map = msm_iommu_pagetable_map,
              .unmap = msm_iommu_pagetable_unmap,

drivers/gpu/drm/msm/msm_mmu.h +2 -2

index 88af4f490881..96e509bd96a6 100644
@@ -53,7 +53,8 @@ static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
 struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent);
 
 int msm_iommu_pagetable_params(struct msm_mmu *mmu, phys_addr_t *ttbr,
-             int *asid);
+                            int *asid);
+int msm_iommu_pagetable_walk(struct msm_mmu *mmu, unsigned long iova, uint64_t ptes[4]);
 struct iommu_domain_geometry *msm_iommu_get_geometry(struct msm_mmu *mmu);
 
 #endif /* __MSM_MMU_H__ */
--

2.46.0