On 06/09/2024 11:56 am, Will Deacon wrote:
On Thu, Sep 05, 2024 at 05:27:28PM +0100, Robin Murphy wrote:
On 05/09/2024 4:53 pm, Will Deacon wrote:
On Thu, Sep 05, 2024 at 05:49:56AM -0700, Rob Clark wrote:
From: Rob Clark <robdclark@xxxxxxxxxxxx>
This reverts commit 85b715a334583488ad7fbd3001fe6fd617b7d4c0.
It was causing gpu smmu faults on x1e80100.
I _think_ what is causing this is the change in ordering of
__arm_lpae_clear_pte() (dma_sync_single_for_device() on the pgtable
memory) and io_pgtable_tlb_flush_walk(). I'm not entirely sure how
this patch is supposed to work correctly in the face of other
concurrent translations (to buffers unrelated to the one being
unmapped(), because after the io_pgtable_tlb_flush_walk() we can have
stale data read back into the tlb.
Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
---
drivers/iommu/io-pgtable-arm.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
Please can you try the diff below, instead?
Given that the GPU driver's .tlb_add_page is a no-op, I can't see this
making a difference. In fact, given that msm_iommu_pagetable_unmap() still
does a brute-force iommu_flush_iotlb_all() after io-pgtable returns, and in
fact only recently made .tlb_flush_walk start doing anything either for the
sake of the map path, I'm now really wondering how this patch has had any
effect at all... :/
Hmm, yup. Looks like Rob has come back to say the problem lies elsewhere
anyway.
One thing below though...
Will
--->8
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 0e67f1721a3d..0a32e9499e2c 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -672,7 +672,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
/* Clear the remaining entries */
__arm_lpae_clear_pte(ptep, &iop->cfg, i);
- if (gather && !iommu_iotlb_gather_queued(gather))
+ if (!iommu_iotlb_gather_queued(gather))
Note that this would reintroduce the latent issue which was present
originally, wherein iommu_iotlb_gather_queued(NULL) is false, but if we
actually allow a NULL gather to be passed to io_pgtable_tlb_add_page() it
may end up being dereferenced (e.g. in arm-smmu-v3).
I think there is still something to fix here. arm_lpae_init_pte() can
pass a NULL gather to __arm_lpae_unmap() and I don't think skipping the
invalidation is correct in that case. Either the drivers need to handle
that or we shouldn't be passing NULL.
What do you think?
The subtlety there is that in that case it's always a non-leaf PTE, so
all that goes back to the driver is io_pgtable_tlb_flush_walk() and the
gather is never used.
Thanks,
Robin.
