[PATCH v2 0/2] TEGRA GART cleanup and new debug option

[PATCH v2 0/2] TEGRA GART cleanup and new debug option

[Dmitry Osipenko]

This is a continuation of the "iommu/tegra: gart: Couple corrections"
patchset that was send out quite some time ago and reviewed by Thierry Reding
recently.

Change log:

v2:
	- Dropped "Don't unnecessarily allocate registers context" patch
	  as it is not entirely correct.

	- Replaced "Correct number of unmapped bytes" and "Check whether page
	  is already mapped" patches with a new GART debug Kconfig option
	  patch.

Dmitry Osipenko (2):
  iommu/tegra: gart: Optionally check for overwriting of page mappings
  iommu/tegra: gart: Move PFN validation out of spinlock

 drivers/iommu/Kconfig      |  9 +++++++++
 drivers/iommu/tegra-gart.c | 20 +++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

-- 
2.14.1

[PATCH v2 1/2] iommu/tegra: gart: Add optional debug checking for overwriting of page mappings

[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.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/Kconfig      |  9 +++++++++
 drivers/iommu/tegra-gart.c | 16 +++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f3a21343e636..851156a4896d 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -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
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index b62f790ad1ba..7e5df0ebcdfc 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -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;
+
 	spin_lock_irqsave(&gart->pte_lock, flags);
 	gart_set_pte(gart, iova, 0);
 	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
-	return 0;
+
+	return bytes;
 }
 
 static phys_addr_t gart_iommu_iova_to_phys(struct iommu_domain *domain,
-- 
2.14.1

[PATCH v2 1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

[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.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/Kconfig      |  9 +++++++++
 drivers/iommu/tegra-gart.c | 16 +++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f3a21343e636..851156a4896d 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -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
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index b62f790ad1ba..bc4cb200fa03 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -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);
@@ -295,6 +304,10 @@ static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	struct gart_device *gart = gart_domain->gart;
 	unsigned long flags;
 
+	/* don't unmap page entries to achieve better performance */
+	if (!IS_ENABLED(TEGRA_IOMMU_GART_DEBUG))
+		return 0;
+
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return 0;
 
@@ -302,7 +315,8 @@ static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	gart_set_pte(gart, iova, 0);
 	FLUSH_GART_REGS(gart);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
-	return 0;
+
+	return bytes;
 }
 
 static phys_addr_t gart_iommu_iova_to_phys(struct iommu_domain *domain,
-- 
2.14.1

[PATCH v2 2/2] iommu/tegra: gart: Move PFN validation out of spinlock

[Dmitry Osipenko]

Validation of page frame number doesn't require protection with a spinlock,
let's move it out of spinlock for consistency.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-gart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index bc4cb200fa03..75e6ab621300 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -276,13 +276,13 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return -EINVAL;
 
-	spin_lock_irqsave(&gart->pte_lock, flags);
 	pfn = __phys_to_pfn(pa);
 	if (!pfn_valid(pfn)) {
 		dev_err(gart->dev, "Invalid page: %pa\n", &pa);
-		spin_unlock_irqrestore(&gart->pte_lock, flags);
 		return -EINVAL;
 	}
+
+	spin_lock_irqsave(&gart->pte_lock, flags);
 	if (IS_ENABLED(TEGRA_IOMMU_GART_DEBUG)) {
 		pte = gart_read_pte(gart, iova);
 		if (pte & GART_ENTRY_PHYS_ADDR_VALID) {
-- 
2.14.1

Re: [PATCH v2 1/2] iommu/tegra: gart: Add optional debug checking for overwriting of page mappings

[Dmitry Osipenko]

On 04.10.2017 04:02, Dmitry Osipenko wrote:
> 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 <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/Kconfig      |  9 +++++++++
>  drivers/iommu/tegra-gart.c | 16 +++++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index f3a21343e636..851156a4896d 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -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
> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> index b62f790ad1ba..7e5df0ebcdfc 100644
> --- a/drivers/iommu/tegra-gart.c
> +++ b/drivers/iommu/tegra-gart.c
> @@ -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;
> +
>  	spin_lock_irqsave(&gart->pte_lock, flags);
>  	gart_set_pte(gart, iova, 0);
>  	FLUSH_GART_REGS(gart);
>  	spin_unlock_irqrestore(&gart->pte_lock, flags);
> -	return 0;
> +
> +	return bytes;
>  }
>  
>  static phys_addr_t gart_iommu_iova_to_phys(struct iommu_domain *domain,
> 

Please ignore this email, it was sent out by an accident. And please let me know
if I should re-send the patchset.

Re: [PATCH v2 1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

[Joerg Roedel]

On Wed, Oct 04, 2017 at 04:02:31AM +0300, Dmitry Osipenko wrote:
> 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?


	Joerg

Re: [PATCH v2 1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

[Dmitry Osipenko]

On 10.10.2017 12:53, Joerg Roedel wrote:
> On Wed, Oct 04, 2017 at 04:02:31AM +0300, Dmitry Osipenko wrote:
>> 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.

Re: [PATCH v2 1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

[Joerg Roedel]

On Thu, Oct 12, 2017 at 05:27:26PM +0300, Dmitry Osipenko wrote:
> 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.


Thanks,

	Joerg

Re: [PATCH v2 1/2] iommu/tegra: gart: Optionally check for overwriting of page mappings

[Dmitry Osipenko]

On 13.10.2017 11:38, Joerg Roedel wrote:
> On Thu, Oct 12, 2017 at 05:27:26PM +0300, Dmitry Osipenko wrote:
>> 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.
> 

Certainly iommu_map_sg doesn't perform itself any debug checks that I'm
proposing to add to the GART.

Yet we don't use GART in the mainline, right now you may take a look at the WIP
patches here:

https://github.com/grate-driver/linux/commit/9853371164a7f1b5698caee476e7cffe1b446afa

https://github.com/grate-driver/linux/commit/ea1fca4ac932464e7907a7ada8ea2698cab8db65

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