TEGRA GART cleanup and new debug option

Due to a bug in IOVA allocator, page mapping could accidentally overwritten. We can catch this case by checking 'VALID' bit of GART's page entry prior to mapping of a page. Since that check introduces a noticeable performance impact, it should be enabled explicitly by a new CONFIG_TEGRA_IOMMU_GART_DEBUG option.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/Kconfig      |  9 +++++++++
 drivers/iommu/tegra-gart.c | 16 +++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

drivers/iommu/Kconfig +9 -0

index f3a21343e636..851156a4896d 100644
@@ -242,6 +242,15 @@ config TEGRA_IOMMU_GART
 	  space through the GART (Graphics Address Relocation Table)
 	  hardware included on Tegra SoCs.
 
+config TEGRA_IOMMU_GART_DEBUG
+	bool "Debug Tegra GART IOMMU"
+	depends on TEGRA_IOMMU_GART
+	help
+	  Properly unmap pages and check whether page is already mapped
+	  during of mapping in expense of performance. This allows to
+	  catch double page remappings, caused by a bug in the IOVA
+	  allocator for example.
+
 config TEGRA_IOMMU_SMMU
 	bool "NVIDIA Tegra SMMU Support"
 	depends on ARCH_TEGRA

drivers/iommu/tegra-gart.c +13 -0

index b62f790ad1ba..7e5df0ebcdfc 100644
@@ -271,6 +271,7 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	struct gart_device *gart = gart_domain->gart;
 	unsigned long flags;
 	unsigned long pfn;
+	unsigned long pte;
 
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return -EINVAL;

@@ -282,6 +283,14 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		spin_unlock_irqrestore(&gart->pte_lock, flags);
 		return -EINVAL;
 	}
+	if (IS_ENABLED(TEGRA_IOMMU_GART_DEBUG)) {
+		pte = gart_read_pte(gart, iova);
+		if (pte & GART_ENTRY_PHYS_ADDR_VALID) {
+			spin_unlock_irqrestore(&gart->pte_lock, flags);
+			dev_err(gart->dev, "Page entry is used already\n");
+			return -EBUSY;
+		}
+	}
 	gart_set_pte(gart, iova, GART_PTE(pfn));
 	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);

@@ -298,11 +307,16 @@ static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return 0;
 
+	/* don't unmap page entries to achieve better performance */
+	if (!IS_ENABLED(TEGRA_IOMMU_GART_DEBUG))
+		return 0;
+

Due to a bug in IOVA allocator, page mapping could accidentally overwritten. We can catch this case by checking 'VALID' bit of GART's page entry prior to mapping of a page. Since that check introduces a noticeable performance impact, it should be enabled explicitly by a new CONFIG_TEGRA_IOMMU_GART_DEBUG option.

Quoted 5 lines · Dmitry Osipenko

Due to a bug in IOVA allocator, page mapping could accidentally overwritten. We can catch this case by checking 'VALID' bit of GART's page entry prior to mapping of a page. Since that check introduces a noticeable performance impact, it should be enabled explicitly by a new CONFIG_TEGRA_IOMMU_GART_DEBUG option.

Which bug in the IOVA allocator are you talking about?

I'm not talking about any specific bug, but in general if allocator re-maps already mapped region or unmaps the wrong-and-used region. I had those bug-cases during of development of the 'scattered' graphics allocations for Tegra20.

Quoted 3 lines · Dmitry Osipenko

I'm not talking about any specific bug, but in general if allocator re-maps already mapped region or unmaps the wrong-and-used region. I had those bug-cases during of development of the 'scattered' graphics allocations for Tegra20.

The dma-iommu code does not re-map already mapped regions and it doesn't unmap wrong regions. If it does it should be reported and fixed.

So if you hit any bug there, please report it so that it can be fixed.