[v1,2/4] iommu/tegra: gart: Check whether page is already mapped

Message ID a19ba1ca835aa2526d2c0492e6b0d0b35889596a.1499270277.git.digetx@gmail.com
State New
Headers show

Commit Message

Dmitry Osipenko July 5, 2017, 4:29 p.m.
Due to a bug, multiple devices may try to map the same IOVA region. We can
catch that case by checking that 'VALID' bit of the GART's page entry is
unset prior to mapping of the page.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/iommu/tegra-gart.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Thierry Reding Sept. 26, 2017, 11:06 a.m. | #1
On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
> Due to a bug, multiple devices may try to map the same IOVA region. We can
> catch that case by checking that 'VALID' bit of the GART's page entry is
> unset prior to mapping of the page.

Due to what bug? Sounds to me like access to the GART should be
exclusive, so that only a single driver can ever access it.

Thierry
Dmitry Osipenko Sept. 26, 2017, 1:49 p.m. | #2
On 26.09.2017 14:06, Thierry Reding wrote:
> On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
>> Due to a bug, multiple devices may try to map the same IOVA region. We can
>> catch that case by checking that 'VALID' bit of the GART's page entry is
>> unset prior to mapping of the page.
> 
> Due to what bug? Sounds to me like access to the GART should be
> exclusive, so that only a single driver can ever access it.
> 

Actually, there are a lot of peripherals behind the GART. But yes, probably we
would use it exclusively for the GPU allocations.

In a case of the GPU allocations there could be a bug in the allocation code
(drm_mm_scan) that would cause re-mapping of the already mapped pages, we would
be able to catch such a bug.
Thierry Reding Sept. 26, 2017, 4:07 p.m. | #3
On Tue, Sep 26, 2017 at 04:49:52PM +0300, Dmitry Osipenko wrote:
> On 26.09.2017 14:06, Thierry Reding wrote:
> > On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
> >> Due to a bug, multiple devices may try to map the same IOVA region. We can
> >> catch that case by checking that 'VALID' bit of the GART's page entry is
> >> unset prior to mapping of the page.
> > 
> > Due to what bug? Sounds to me like access to the GART should be
> > exclusive, so that only a single driver can ever access it.
> > 
> 
> Actually, there are a lot of peripherals behind the GART. But yes, probably we
> would use it exclusively for the GPU allocations.
> 
> In a case of the GPU allocations there could be a bug in the allocation code
> (drm_mm_scan) that would cause re-mapping of the already mapped pages, we would
> be able to catch such a bug.

Sounds to me like something that should be opt-in. Given that we could
potentially map a lot of memory, the additional read seems like it could
incur a significant performance penalty.

How about protecting this by some Kconfig symbol (or debug module
parameter) and adding a WARN instead of failing the mapping operation?

The reason is that we really should be using the GART from a single
driver because it is a shared memory region. To avoid drivers from
treading on each others' feet the allocations must be managed by some
central entity. So either something like DMA API should manage it, or
we hardwire it to Tegra DRM explicitly.

And if a central allocator is responsible for all mappings through the
GART, then we should assume that it isn't buggy, and therefore the extra
check of the PTEs should not be needed in most cases, hence we can get a
performance boost by disabling the check by default.

Of course if there is not much performance impact, maybe it is okay to
keep it always enabled.

Thierry
Dmitry Osipenko Sept. 26, 2017, 4:57 p.m. | #4
On 26.09.2017 19:07, Thierry Reding wrote:
> On Tue, Sep 26, 2017 at 04:49:52PM +0300, Dmitry Osipenko wrote:
>> On 26.09.2017 14:06, Thierry Reding wrote:
>>> On Wed, Jul 05, 2017 at 07:29:46PM +0300, Dmitry Osipenko wrote:
>>>> Due to a bug, multiple devices may try to map the same IOVA region. We can
>>>> catch that case by checking that 'VALID' bit of the GART's page entry is
>>>> unset prior to mapping of the page.
>>>
>>> Due to what bug? Sounds to me like access to the GART should be
>>> exclusive, so that only a single driver can ever access it.
>>>
>>
>> Actually, there are a lot of peripherals behind the GART. But yes, probably we
>> would use it exclusively for the GPU allocations.
>>
>> In a case of the GPU allocations there could be a bug in the allocation code
>> (drm_mm_scan) that would cause re-mapping of the already mapped pages, we would
>> be able to catch such a bug.
> 
> Sounds to me like something that should be opt-in. Given that we could
> potentially map a lot of memory, the additional read seems like it could
> incur a significant performance penalty.
> 
> How about protecting this by some Kconfig symbol (or debug module
> parameter) and adding a WARN instead of failing the mapping operation?
> 
> The reason is that we really should be using the GART from a single
> driver because it is a shared memory region. To avoid drivers from
> treading on each others' feet the allocations must be managed by some
> central entity. So either something like DMA API should manage it, or
> we hardwire it to Tegra DRM explicitly.
> 
> And if a central allocator is responsible for all mappings through the
> GART, then we should assume that it isn't buggy, and therefore the extra
> check of the PTEs should not be needed in most cases, hence we can get a
> performance boost by disabling the check by default.
> 
> Of course if there is not much performance impact, maybe it is okay to
> keep it always enabled.
> 

I had a similar thought about making this checking and the "proper" unmapping
optional, but decided that it is not very worthy to implement it now because
caching of mappings helps to get rid of all IOVA expenses. The "checking" is
nearly "free" in terms of performance, it's the unmapping that causes measurable
performance impact. Maybe the improper unmapping was made intentionally for
better performance.

Given that you've made suggestion to make the checking optional, I'm going to
update this patchset, adding a new "debug" Kconfig option.

Patch

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 1557a6a9a438..54699e341110 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,12 @@  static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		spin_unlock_irqrestore(&gart->pte_lock, flags);
 		return -EINVAL;
 	}
+	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);