io-pgtable-arm + drm/msm: Extend iova fault debugging
Subsystems (6)
DRM drivers Arm Qualcomm mailing list IOMMU DRM driver for Qualcomm display hardware +2 more subsystems
v2 cover io-pgtable-arm + drm/msm: Extend iova fault debugging
From: Rob Clark <[email protected]>
This series extends io-pgtable-arm with a method to retrieve the page table entries traversed in the process of address translation, and then beefs up drm/msm gpu devcore dump to include this (and additional info) in the devcore dump.
The motivation is tracking down an obscure iova fault triggered crash on the address of the IB1 cmdstream. This is one of the few places where the GPU address written into the cmdstream is soley under control of the kernel mode driver, so I don't think it can be a userspace bug. The logged cmdstream from the devcore's I've looked at look correct, and the TTBR0 read back from arm-smmu agrees with the kernel emitted cmdstream. Unfortunately it happens infrequently enough (something like once per 1000hrs of usage, from what I can tell from our telemetry) that actually reproducing it with an instrumented debug kernel is not an option. So further spiffying out the devcore dumps and hoping we can spot a clue is the plan I'm shooting for.
See https://gitlab.freedesktop.org/drm/msm/-/issues/8 for more info on the issue I'm trying to debug.
v2: Fix an armv7/32b build error in the last patch
Rob Clark (3):
iommu/io-pgtable-arm: Add way to debug pgtable walk drm/msm: Show all smmu info for iova fault devcore dumps drm/msm: Extend gpu devcore dumps with pgtbl info
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 35 +++++++++++++++++----- drivers/gpu/drm/msm/msm_gpu.c | 10 +++++++ drivers/gpu/drm/msm/msm_gpu.h | 10 ++++++- drivers/gpu/drm/msm/msm_iommu.c | 17 +++++++++++ drivers/gpu/drm/msm/msm_mmu.h | 2 ++ drivers/iommu/io-pgtable-arm.c | 40 ++++++++++++++++++++----- include/linux/io-pgtable.h | 9 ++++++ 8 files changed, 107 insertions(+), 18 deletions(-)
Signature · 1 line
2.31.1
PATCH v2 1/3 iommu/io-pgtable-arm: Add way to debug pgtable walk
From: Rob Clark <[email protected]>
Add an io-pgtable method to retrieve the raw PTEs that would be traversed for a given iova access.
Signed-off-by Rob Clark <[email protected]>
--- drivers/iommu/io-pgtable-arm.c | 40 +++++++++++++++++++++++++++------- include/linux/io-pgtable.h | 9 ++++++++ 2 files changed, 41 insertions(+), 8 deletions(-)
drivers/iommu/io-pgtable-arm.c +32 -8
@@ -700,38 +700,61 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,return arm_lpae_unmap_pages (ops, iova, size,1 , gather); } -static phys_addr_t arm_lpae_iova_to_phys (struct io_pgtable_ops *ops, -unsigned long iova) +static int arm_lpae_pgtable_walk (struct io_pgtable_ops *ops,unsigned long iova, +void *_ptes,int *num_ptes) {struct arm_lpae_io_pgtable *data =io_pgtable_ops_to_data (ops);arm_lpae_iopte pte, *ptep = data->pgd; +arm_lpae_iopte *ptes = _ptes; +int max_ptes = *num_ptes;int lvl = data->start_level; + *num_ptes =0 ; +do { +if (*num_ptes >= max_ptes) +return -ENOSPC ; +/* Valid IOPTE pointer? */ if (!ptep) -return 0 ; +return -EFAULT ;/* Grab the IOPTE we're interested in */ ptep +=ARM_LPAE_LVL_IDX (iova, lvl, data); pte =READ_ONCE (*ptep); + ptes[(*num_ptes)++] = pte; +/* Valid entry? */ if (!pte) -return 0 ; +return -EFAULT ;/* Leaf entry? */ if (iopte_leaf (pte, lvl, data->iop.fmt)) - goto found_translation; +return 0 ;/* 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 ; +return -EFAULT ; +} + +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, ptes[ARM_LPAE_MAX_LEVELS ]; +int lvl, num_ptes =ARM_LPAE_MAX_LEVELS ; +int ret; + + ret =arm_lpae_pgtable_walk (ops, iova, ptes, &num_ptes); +if (ret) +return 0 ; + + pte = ptes[num_ptes -1 ]; + lvl = num_ptes -1 + data->start_level; -found_translation: iova &= (ARM_LPAE_BLOCK_SIZE (lvl, data) -1 );return iopte_to_paddr (pte, data) | iova; }
@@ -816,6 +839,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg) .unmap = arm_lpae_unmap, .unmap_pages = arm_lpae_unmap_pages, .iova_to_phys = arm_lpae_iova_to_phys, + .pgtable_walk = arm_lpae_pgtable_walk, };return data;
include/linux/io-pgtable.h +9 -0
@@ -148,6 +148,13 @@ struct io_pgtable_cfg {* @unmap: Unmap a physically contiguous memory region. * @unmap_pages: Unmap a range of virtually contiguous pages of the same size. * @iova_to_phys: Translate iova to physical address. +* @pgtable_walk: Return details of a page table walk for a given iova. +* This returns the array of PTEs in a format that is +* specific to the page table format. The number of +* PTEs can be format specific. The num_ptes parameter +* on input specifies the size of the ptes array, and +* on output the number of PTEs filled in (which depends +* on the number of PTEs walked to resolve the iova) * * These functions map directly onto the iommu_ops member functions with * the same names.
@@ -165,6 +172,8 @@ struct io_pgtable_ops { struct iommu_iotlb_gather *gather);phys_addr_t (*iova_to_phys)(struct io_pgtable_ops *ops,unsigned long iova); +int (*pgtable_walk)(struct io_pgtable_ops *ops,unsigned long iova, +void *ptes,int *num_ptes); };/**
Signature · 1 line
2.31.1
Will Deacon
maintainerRe: [PATCH v2 1/3] iommu/io-pgtable-arm: Add way to debug pgtable walk
Quoted 98 lines · Rob Clark
From: Rob Clark <[email protected]>
Add an io-pgtable method to retrieve the raw PTEs that would be traversed for a given iova access.
Signed-off-by Rob Clark <[email protected]>--- drivers/iommu/io-pgtable-arm.c | 40 +++++++++++++++++++++++++++------- include/linux/io-pgtable.h | 9 ++++++++ 2 files changed, 41 insertions(+), 8 deletions(-)drivers/iommu/io-pgtable-arm.c +32
-8@@ -700,38 +700,61 @@ static size_t arm_lpae_unmap(struct io_pgtable_ops *ops, unsigned long iova,return arm_lpae_unmap_pages (ops, iova, size,1 , gather); } -static phys_addr_t arm_lpae_iova_to_phys (struct io_pgtable_ops *ops, -unsigned long iova) +static int arm_lpae_pgtable_walk (struct io_pgtable_ops *ops,unsigned long iova, +void *_ptes,int *num_ptes) {struct arm_lpae_io_pgtable *data =io_pgtable_ops_to_data (ops);arm_lpae_iopte pte, *ptep = data->pgd; +arm_lpae_iopte *ptes = _ptes; +int max_ptes = *num_ptes;int lvl = data->start_level; + *num_ptes =0 ; +do { +if (*num_ptes >= max_ptes) +return -ENOSPC ; +/* Valid IOPTE pointer? */ if (!ptep) -return 0 ; +return -EFAULT ;/* Grab the IOPTE we're interested in */ ptep +=ARM_LPAE_LVL_IDX (iova, lvl, data); pte =READ_ONCE (*ptep); + ptes[(*num_ptes)++] = pte; +/* Valid entry? */ if (!pte) -return 0 ; +return -EFAULT ;/* Leaf entry? */ if (iopte_leaf (pte, lvl, data->iop.fmt)) - goto found_translation; +return 0 ;/* 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 ; +return -EFAULT ; +} + +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, ptes[ARM_LPAE_MAX_LEVELS ]; +int lvl, num_ptes =ARM_LPAE_MAX_LEVELS ; +int ret; + + ret =arm_lpae_pgtable_walk (ops, iova, ptes, &num_ptes); +if (ret) +return 0 ; + + pte = ptes[num_ptes -1 ]; + lvl = num_ptes -1 + data->start_level; -found_translation: iova &= (ARM_LPAE_BLOCK_SIZE (lvl, data) -1 );return iopte_to_paddr (pte, data) | iova; }@@ -816,6 +839,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg) .unmap = arm_lpae_unmap, .unmap_pages = arm_lpae_unmap_pages, .iova_to_phys = arm_lpae_iova_to_phys, + .pgtable_walk = arm_lpae_pgtable_walk, };return data;include/linux/io-pgtable.h +0
-0
include/linux/io-pgtable.h struct io_pgtable_cfg { · line 157
Quoted 10 lines
include/linux/io-pgtable.h +7
-0@@ -148,6 +148,13 @@ struct io_pgtable_cfg {* @unmap: Unmap a physically contiguous memory region. * @unmap_pages: Unmap a range of virtually contiguous pages of the same size. * @iova_to_phys: Translate iova to physical address. +* @pgtable_walk: Return details of a page table walk for a given iova. +* This returns the array of PTEs in a format that is +* specific to the page table format. The number of +* PTEs can be format specific. The num_ptes parameter +* on input specifies the size of the ptes array, and +* on output the number of PTEs filled in (which depends +* on the number of PTEs walked to resolve the iova)
I think this would be a fair bit cleaner if the interface instead took a callback function to invoke at each page-table level. It would be invoked with the pte value and the level. Depending on its return value the walk could be terminated early. That would also potentially scale to walking ranges of iovas as well if we ever need it and it may be more readily implementable by other formats too.
include/linux/io-pgtable.h struct io_pgtable_cfg { · line 160
Quoted 3 lines
include/linux/io-pgtable.h +0
-0* * These functions map directly onto the iommu_ops member functions with * the same names.
This bit of the comment is no longer true with your change.
Will
PATCH v2 2/3 drm/msm: Show all smmu info for iova fault devcore dumps
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
Signed-off-by Rob Clark <[email protected]>
--- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 25 +++++++++++++++++-------- drivers/gpu/drm/msm/msm_gpu.h | 2 +- 3 files changed, 19 insertions(+), 10 deletions(-)
drivers/gpu/drm/msm/adreno/a6xx_gpu.c +1 -1
@@ -1282,7 +1282,7 @@ static int a6xx_fault_handler(void *arg, unsigned long iova, int flags, void *da/* Turn off the hangcheck timer to keep it from bothering us */ del_timer (&gpu->hangcheck_timer); - gpu->fault_info.ttbr0 = info->ttbr0; + gpu->fault_info.smmu_info = *info; gpu->fault_info.iova = iova; gpu->fault_info.flags = flags; gpu->fault_info.type = type;
drivers/gpu/drm/msm/adreno/adreno_gpu.c +17 -8
@@ -685,19 +685,28 @@ void adreno_show(struct msm_gpu *gpu, struct msm_gpu_state *state, adreno_gpu->rev.major, adreno_gpu->rev.minor, adreno_gpu->rev.patchid);/* -* If this is state collected due to iova fault, so fault related info +* If this is state collected due to iova fault, show fault related +* info * -* TTBR0 would not be zero, so this is a good way to distinguish +* TTBR0 would not be zero in this case, so this is a good way to +* distinguish */ -if (state->fault_info.ttbr0) { +if (state->fault_info.smmu_info.ttbr0) {const struct msm_gpu_fault_info *info = &state->fault_info; +const struct adreno_smmu_fault_info *smmu_info = &info->smmu_info;drm_puts (p,"fault-info:\n" ); -drm_printf (p," - ttbr0=%.16llx\n" , info->ttbr0); -drm_printf (p," - iova=%.16lx\n" , info->iova); -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); +drm_printf (p," - far: %.16llx\n" , smmu_info->far); +drm_printf (p," - ttbr0: %.16llx\n" , smmu_info->ttbr0); +drm_printf (p," - contextidr: %.8x\n" , smmu_info->contextidr); +drm_printf (p," - fsr: %.8x\n" , smmu_info->fsr); +drm_printf (p," - fsynr0: %.8x\n" , smmu_info->fsynr0); +drm_printf (p," - fsynr1: %.8x\n" , smmu_info->fsynr1); +drm_printf (p," - cbfrsynra: %.8x\n" , smmu_info->cbfrsynra); +drm_printf (p," - iova: %.16lx\n" , info->iova); +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); }drm_printf (p,"rbbm-status: 0x%08x\n" , state->rbbm_status);
drivers/gpu/drm/msm/msm_gpu.h +1 -1
@@ -73,7 +73,7 @@ struct msm_gpu_funcs {/* Additional state for iommu faults: */ struct msm_gpu_fault_info { -u64 ttbr0; +struct adreno_smmu_fault_info smmu_info;unsigned long iova;int flags;const char *type;
Signature · 1 line
2.31.1
PATCH v2 3/3 drm/msm: Extend gpu devcore dumps with pgtbl info
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
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]>
---
v2: Fix build error on 32b/armv7
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++++++ drivers/gpu/drm/msm/msm_gpu.c | 10 ++++++++++ drivers/gpu/drm/msm/msm_gpu.h | 8 ++++++++ drivers/gpu/drm/msm/msm_iommu.c | 17 +++++++++++++++++ drivers/gpu/drm/msm/msm_mmu.h | 2 ++ 5 files changed, 47 insertions(+)
drivers/gpu/drm/msm/adreno/adreno_gpu.c +10 -0
@@ -707,6 +707,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 +* 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); +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 ]); }drm_printf (p,"rbbm-status: 0x%08x\n" , state->rbbm_status);
drivers/gpu/drm/msm/msm_gpu.c +10 -0
@@ -284,6 +284,16 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,if (submit) {int i, nr =0 ; +if (state->fault_info.smmu_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, +ARRAY_SIZE (info->ptes)); + } +/* count # of buffers to dump: */ for (i =0 ; i < submit->nr_bos; i++)if (should_dump (submit, i))
drivers/gpu/drm/msm/msm_gpu.h +8 -0
@@ -78,6 +78,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 +17 -0
@@ -116,6 +116,23 @@ int msm_iommu_pagetable_params(struct msm_mmu *mmu,return 0 ; } +int msm_iommu_pagetable_walk (struct msm_mmu *mmu,unsigned long iova, +u64 *ptes,int num_ptes) +{ +struct msm_iommu_pagetable *pagetable; + +if (mmu->type !=MSM_MMU_IOMMU_PAGETABLE ) +return -EINVAL ; + + pagetable =to_pagetable (mmu); + +if (!pagetable->pgtbl_ops->pgtable_walk) +return -EINVAL ; + +return pagetable->pgtbl_ops->pgtable_walk (pagetable->pgtbl_ops, iova, + ptes, &num_ptes); +} +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 -0
@@ -58,5 +58,7 @@ void msm_gpummu_params(struct msm_mmu *mmu, dma_addr_t *pt_base,int msm_iommu_pagetable_params (struct msm_mmu *mmu,phys_addr_t *ttbr,int *asid); +int msm_iommu_pagetable_walk (struct msm_mmu *mmu,unsigned long iova, +u64 *ptes,int num_ptes);#endif /* __MSM_MMU_H__ */
Signature · 1 line
2.31.1