[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Catalin Marinas]

ARMv6 onwards requires that there are no aliases to the same physical
location using different memory types (i.e. Normal vs Strongly Ordered).
Access to SO mappings when the unaligned accesses are handled in
hardware is also Unpredictable (pgprot_noncached() mappings in user
space).

The patch modifies the pgprot_noncached() for ARMv6+ architecture
versions to generate Normal uncached memory attributes rather than
Strongly Ordered. The patch also modifies the mandatory barriers to use
dmb() rather than being simple compiler barriers.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Abhijeet <adharmap@codeaurora.org>
Cc: Richard Woodruff <r-woodruff2@ti.com>
---
 arch/arm/include/asm/pgtable.h |   12 ++++++++++++
 arch/arm/include/asm/system.h  |   10 ++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 8429868..c3cd7d4 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -372,11 +372,23 @@ PTE_BIT_FUNC(mkyoung,   |= L_PTE_YOUNG);
 
 static inline pte_t pte_mkspecial(pte_t pte) { return pte; }
 
+#if __LINUX_ARM_ARCH__ < 6
 /*
  * Mark the prot value as uncacheable and unbufferable.
  */
 #define pgprot_noncached(prot) \
 	__pgprot((pgprot_val(prot) & ~L_PTE_MT_MASK) | L_PTE_MT_UNCACHED)
+#else
+/*
+ * Mark the prot value as uncacheable, bufferable (Normal uncached memory).
+ * ARMv6 onwards requires that there are no aliases to the same physical
+ * location using different memory types (i.e. Normal vs Strongly Ordered).
+ * Access to SO mappings when the unaligned accesses are handled in hardware
+ * is also Unpredictable (pgprot_noncached() mappings in user space).
+ */
+#define pgprot_noncached(prot) \
+	__pgprot((pgprot_val(prot) & ~L_PTE_MT_MASK) | L_PTE_MT_BUFFERABLE)
+#endif
 #define pgprot_writecombine(prot) \
 	__pgprot((pgprot_val(prot) & ~L_PTE_MT_MASK) | L_PTE_MT_BUFFERABLE)
 
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index d65b2f5..cc89848 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -139,9 +139,19 @@ extern unsigned int user_debug;
 #endif
 
 #ifndef CONFIG_SMP
+#if __LINUX_ARM_ARCH__ < 6
 #define mb()	do { if (arch_is_coherent()) dmb(); else barrier(); } while (0)
 #define rmb()	do { if (arch_is_coherent()) dmb(); else barrier(); } while (0)
 #define wmb()	do { if (arch_is_coherent()) dmb(); else barrier(); } while (0)
+#else
+/*
+ * pgprot_noncached() creates Normal uncached mappings, therefore mandatory
+ * barriers are needed.
+ */
+#define mb()		dmb()
+#define rmb()		dmb()
+#define wmb()		dmb()
+#endif
 #define smp_mb()	barrier()
 #define smp_rmb()	barrier()
 #define smp_wmb()	barrier()

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Russell King - ARM Linux]

On Fri, Oct 23, 2009 at 10:41:21AM +0100, Catalin Marinas wrote:
> ARMv6 onwards requires that there are no aliases to the same physical
> location using different memory types (i.e. Normal vs Strongly Ordered).
> Access to SO mappings when the unaligned accesses are handled in
> hardware is also Unpredictable (pgprot_noncached() mappings in user
> space).
> 
> The patch modifies the pgprot_noncached() for ARMv6+ architecture
> versions to generate Normal uncached memory attributes rather than
> Strongly Ordered. The patch also modifies the mandatory barriers to use
> dmb() rather than being simple compiler barriers.

It's not this simple - pgprot_noncached() is used for /dev/mem O_SYNC
mappings, which are used for device mappings.  This means you still
end up with the unpredictable issue.

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Catalin Marinas]

On Fri, 2009-10-23 at 11:06 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 23, 2009 at 10:41:21AM +0100, Catalin Marinas wrote:
> > ARMv6 onwards requires that there are no aliases to the same physical
> > location using different memory types (i.e. Normal vs Strongly Ordered).
> > Access to SO mappings when the unaligned accesses are handled in
> > hardware is also Unpredictable (pgprot_noncached() mappings in user
> > space).
> > 
> > The patch modifies the pgprot_noncached() for ARMv6+ architecture
> > versions to generate Normal uncached memory attributes rather than
> > Strongly Ordered. The patch also modifies the mandatory barriers to use
> > dmb() rather than being simple compiler barriers.
> 
> It's not this simple - pgprot_noncached() is used for /dev/mem O_SYNC
> mappings, which are used for device mappings.  This means you still
> end up with the unpredictable issue.

If it is used for accessing device memory, why not create a
pgprot_device() and use this instead (maybe after checking for pfn_valid
to make sure we don't map the RAM as device memory)? Of course, it needs
some modifications to the mem.c driver which may be ARM specific, though
some other architecture might benefit as well.

>From drivers/char/mem.c:

	/*
	 * On ia64, we ignore O_SYNC because we cannot tolerate memory attribute aliases.
	 */

So ARM it's not the only architecture with this issue (I know you don't
like this argument :-)).

I can see MIPS has its own #ifdef in the mem.c driver as well.

-- 
Catalin

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Russell King - ARM Linux]

On Fri, Oct 23, 2009 at 11:22:54AM +0100, Catalin Marinas wrote:
> On Fri, 2009-10-23 at 11:06 +0100, Russell King - ARM Linux wrote:
> > On Fri, Oct 23, 2009 at 10:41:21AM +0100, Catalin Marinas wrote:
> > > ARMv6 onwards requires that there are no aliases to the same physical
> > > location using different memory types (i.e. Normal vs Strongly Ordered).
> > > Access to SO mappings when the unaligned accesses are handled in
> > > hardware is also Unpredictable (pgprot_noncached() mappings in user
> > > space).
> > > 
> > > The patch modifies the pgprot_noncached() for ARMv6+ architecture
> > > versions to generate Normal uncached memory attributes rather than
> > > Strongly Ordered. The patch also modifies the mandatory barriers to use
> > > dmb() rather than being simple compiler barriers.
> > 
> > It's not this simple - pgprot_noncached() is used for /dev/mem O_SYNC
> > mappings, which are used for device mappings.  This means you still
> > end up with the unpredictable issue.
> 
> If it is used for accessing device memory, why not create a
> pgprot_device() and use this instead (maybe after checking for pfn_valid
> to make sure we don't map the RAM as device memory)? Of course, it needs
> some modifications to the mem.c driver which may be ARM specific, though
> some other architecture might benefit as well.

People keep fiddling with /dev/mem, and it keeps having these problems.
I suggest we leave that area well alone - we can not properly solve the
/dev/mem problems.  Fixing it to use "memory" mappings will break attempts
to use it to access devices, and fixing it for "device" mappings will break
attempts to access memory.

No amount of fiddling with uncached_access() can fix that.


When it comes to the writecombine and DMA coherent mappings, these are
architecture private stuff.  I guess on ARMv6 and v7, there is no
difference between these two (since writecombine is already uncacheable
normal memory).

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Catalin Marinas]

On Fri, 2009-10-23 at 11:34 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 23, 2009 at 11:22:54AM +0100, Catalin Marinas wrote:
> > On Fri, 2009-10-23 at 11:06 +0100, Russell King - ARM Linux wrote:
> > > On Fri, Oct 23, 2009 at 10:41:21AM +0100, Catalin Marinas wrote:
> > > > The patch modifies the pgprot_noncached() for ARMv6+ architecture
> > > > versions to generate Normal uncached memory attributes rather than
> > > > Strongly Ordered. The patch also modifies the mandatory barriers to use
> > > > dmb() rather than being simple compiler barriers.
> > > 
> > > It's not this simple - pgprot_noncached() is used for /dev/mem O_SYNC
> > > mappings, which are used for device mappings.  This means you still
> > > end up with the unpredictable issue.
> > 
> > If it is used for accessing device memory, why not create a
> > pgprot_device() and use this instead (maybe after checking for pfn_valid
> > to make sure we don't map the RAM as device memory)? Of course, it needs
> > some modifications to the mem.c driver which may be ARM specific, though
> > some other architecture might benefit as well.
> 
> People keep fiddling with /dev/mem, and it keeps having these problems.
> I suggest we leave that area well alone - we can not properly solve the
> /dev/mem problems.  Fixing it to use "memory" mappings will break attempts
> to use it to access devices, and fixing it for "device" mappings will break
> attempts to access memory.
> 
> No amount of fiddling with uncached_access() can fix that.

You can also fix phys_mem_access_prot() to differentiate between
standard and device memory. You can simply define
__HAVE_PHYS_MEM_ACCESS_PROT and implement an ARM-specific function (we
probably don't even need to modify uncached_access()).

I can send you a patch on this (or update the one I posted) which
doesn't touch the mem.c file.

We really need to sort out this issue before we start to see people
complaining of random failures as a result aliased memory (no specific
case yet but I don't think it would be long).

> When it comes to the writecombine and DMA coherent mappings, these are
> architecture private stuff.  I guess on ARMv6 and v7, there is no
> difference between these two (since writecombine is already uncacheable
> normal memory).

Yes, the writecombine is fine.

But my patch is intended for dma_alloc_coherent which uses
pgprot_noncached(). There are also several drivers that make use of
pgprot_noncached(), especially framebuffer drivers which are not
ARM-specific.

-- 
Catalin

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Russell King - ARM Linux]

On Fri, Oct 23, 2009 at 11:59:51AM +0100, Catalin Marinas wrote:
> On Fri, 2009-10-23 at 11:34 +0100, Russell King - ARM Linux wrote:
> > On Fri, Oct 23, 2009 at 11:22:54AM +0100, Catalin Marinas wrote:
> > > On Fri, 2009-10-23 at 11:06 +0100, Russell King - ARM Linux wrote:
> > > > On Fri, Oct 23, 2009 at 10:41:21AM +0100, Catalin Marinas wrote:
> > > > > The patch modifies the pgprot_noncached() for ARMv6+ architecture
> > > > > versions to generate Normal uncached memory attributes rather than
> > > > > Strongly Ordered. The patch also modifies the mandatory barriers to use
> > > > > dmb() rather than being simple compiler barriers.
> > > > 
> > > > It's not this simple - pgprot_noncached() is used for /dev/mem O_SYNC
> > > > mappings, which are used for device mappings.  This means you still
> > > > end up with the unpredictable issue.
> > > 
> > > If it is used for accessing device memory, why not create a
> > > pgprot_device() and use this instead (maybe after checking for pfn_valid
> > > to make sure we don't map the RAM as device memory)? Of course, it needs
> > > some modifications to the mem.c driver which may be ARM specific, though
> > > some other architecture might benefit as well.
> > 
> > People keep fiddling with /dev/mem, and it keeps having these problems.
> > I suggest we leave that area well alone - we can not properly solve the
> > /dev/mem problems.  Fixing it to use "memory" mappings will break attempts
> > to use it to access devices, and fixing it for "device" mappings will break
> > attempts to access memory.
> > 
> > No amount of fiddling with uncached_access() can fix that.
> 
> You can also fix phys_mem_access_prot() to differentiate between
> standard and device memory. You can simply define
> __HAVE_PHYS_MEM_ACCESS_PROT and implement an ARM-specific function (we
> probably don't even need to modify uncached_access()).

I don't think you understand the problem.  /dev/mem can map *both* memory
and devices.  Fixing the mapping type for one doesn't solve the problem.
The only real way to fix it "properly" is to make /dev/mem lookup in a
table to find the proper mapping type for a specific physical address,
and get platforms to register these physical regions...

That said, /dev/mem is more or less legacy when it comes to frame buffers
(which are supposed to use the frame buffer stuff - but iirc X11 stuff
either took ages or just doesn't use the frame buffer stuff, especially
with PCI cards.)

> We really need to sort out this issue before we start to see people
> complaining of random failures as a result aliased memory (no specific
> case yet but I don't think it would be long).

That's a pipedream.  Changing the memory type of dma_alloc_coherent()
is going to produce random failures by itself, since device drivers
as currently written for ARM assume that writes to this memory are
strongly ordered.

The only way to avoid this is to audit all the ARM drivers first,
before changing this.  Not doing this will be a recipe for an unstable
system - which could be really difficult to debug - eg, if DMA ends
up scribbling over some random part of the kernel because the DMA
address hasn't been written to the descriptor.

> > When it comes to the writecombine and DMA coherent mappings, these are
> > architecture private stuff.  I guess on ARMv6 and v7, there is no
> > difference between these two (since writecombine is already uncacheable
> > normal memory).
> 
> Yes, the writecombine is fine.

I don't think you read what I said.  What I was implying is that on
v6 and v7, there is (and can be) no difference between
dma_alloc_coherent() and dma_alloc_writecombine() - see what I said
in parens where I detail the type of memory _writecombine() provides
you with, which is precisely what you're proposing the _coherent()
interface returns.

> But my patch is intended for dma_alloc_coherent which uses
> pgprot_noncached(). There are also several drivers that make use of
> pgprot_noncached(), especially framebuffer drivers which are not
> ARM-specific.

These other drivers are a separate problem, and need auditing to see
what type of mapping is actually required.  pgprot_noncached() has
been used both for things like frame buffers and for things like
device mappings.  To change it to suit one case is likely to break
the other case.

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Catalin Marinas]

On Fri, 2009-10-23 at 12:30 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 23, 2009 at 11:59:51AM +0100, Catalin Marinas wrote:
> > On Fri, 2009-10-23 at 11:34 +0100, Russell King - ARM Linux wrote:
> > > On Fri, Oct 23, 2009 at 11:22:54AM +0100, Catalin Marinas wrote:
> > > > On Fri, 2009-10-23 at 11:06 +0100, Russell King - ARM Linux wrote:
> > > > > On Fri, Oct 23, 2009 at 10:41:21AM +0100, Catalin Marinas wrote:
> > > > > > The patch modifies the pgprot_noncached() for ARMv6+ architecture
> > > > > > versions to generate Normal uncached memory attributes rather than
> > > > > > Strongly Ordered. The patch also modifies the mandatory barriers to use
> > > > > > dmb() rather than being simple compiler barriers.
> > > > > 
> > > > > It's not this simple - pgprot_noncached() is used for /dev/mem O_SYNC
> > > > > mappings, which are used for device mappings.  This means you still
> > > > > end up with the unpredictable issue.
> > > > 
> > > > If it is used for accessing device memory, why not create a
> > > > pgprot_device() and use this instead (maybe after checking for pfn_valid
> > > > to make sure we don't map the RAM as device memory)? Of course, it needs
> > > > some modifications to the mem.c driver which may be ARM specific, though
> > > > some other architecture might benefit as well.
> > > 
> > > People keep fiddling with /dev/mem, and it keeps having these problems.
> > > I suggest we leave that area well alone - we can not properly solve the
> > > /dev/mem problems.  Fixing it to use "memory" mappings will break attempts
> > > to use it to access devices, and fixing it for "device" mappings will break
> > > attempts to access memory.
> > > 
> > > No amount of fiddling with uncached_access() can fix that.
> > 
> > You can also fix phys_mem_access_prot() to differentiate between
> > standard and device memory. You can simply define
> > __HAVE_PHYS_MEM_ACCESS_PROT and implement an ARM-specific function (we
> > probably don't even need to modify uncached_access()).
> 
> I don't think you understand the problem.  /dev/mem can map *both* memory
> and devices.  Fixing the mapping type for one doesn't solve the problem.
> The only real way to fix it "properly" is to make /dev/mem lookup in a
> table to find the proper mapping type for a specific physical address,
> and get platforms to register these physical regions...

Something like below defined under arch/arm (if it's not RAM, we
probably don't want Normal cached memory anyway, so no check for
O_SYNC):

#if __LINUX_ARM_ARCH__ >= 6
pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
			      unsigned long size, pgprot_t vma_prot)
{
	if (!pfn_valid(pfn))
		return pgprot_device(vma_prot);
	else if (file->f_flags & O_SYNC)
		return pgprot_noncached(vma_prot);
	return vma_prot;
}
EXPORT_SYMBOL(phys_mem_access_prot);
#endif

> > We really need to sort out this issue before we start to see people
> > complaining of random failures as a result aliased memory (no specific
> > case yet but I don't think it would be long).
> 
> That's a pipedream.  Changing the memory type of dma_alloc_coherent()
> is going to produce random failures by itself, since device drivers
> as currently written for ARM assume that writes to this memory are
> strongly ordered.
> 
> The only way to avoid this is to audit all the ARM drivers first,
> before changing this.  

Yes, I don't say that's not needed (together with testing). If you want,
we can restrict this requirement to ARMv7 onwards only (that's where it
actually matters most) and there aren't many platforms around. The
driver fix is simple enough - adding mb() before starting DMA transfers
(I don't think this contradicts Documentation/memory-barriers.txt). I
can see some well written drivers already use wmb() in functions like
start_xmit().

Alternatively, we can place a DMB in readl/writel but I'm not sure about
the performance impact.

Of course, you can use a more complicated workaround and ensure that
coherent mappings first unmap the pages from the linear kernel mapping
but we end up using pages instead of sections.

> Not doing this will be a recipe for an unstable
> system - which could be really difficult to debug - eg, if DMA ends
> up scribbling over some random part of the kernel because the DMA
> address hasn't been written to the descriptor.

The current configuration will lead to unstable systems in the future
anyway (especially with processors doing aggressive prefetching via the
cached alias). That would be even more difficult to debug.

> > > When it comes to the writecombine and DMA coherent mappings, these are
> > > architecture private stuff.  I guess on ARMv6 and v7, there is no
> > > difference between these two (since writecombine is already uncacheable
> > > normal memory).
> > 
> > Yes, the writecombine is fine.
> 
> I don't think you read what I said.  What I was implying is that on
> v6 and v7, there is (and can be) no difference between
> dma_alloc_coherent() and dma_alloc_writecombine() - see what I said
> in parens where I detail the type of memory _writecombine() provides
> you with, which is precisely what you're proposing the _coherent()
> interface returns.

I agree, with my patch there is no difference between coherent and
writecombine. Why does it need to be one?

-- 
Catalin

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Russell King - ARM Linux]

On Fri, Oct 23, 2009 at 01:05:12PM +0100, Catalin Marinas wrote:
> On Fri, 2009-10-23 at 12:30 +0100, Russell King - ARM Linux wrote:
> > I don't think you understand the problem.  /dev/mem can map *both* memory
> > and devices.  Fixing the mapping type for one doesn't solve the problem.
> > The only real way to fix it "properly" is to make /dev/mem lookup in a
> > table to find the proper mapping type for a specific physical address,
> > and get platforms to register these physical regions...
> 
> Something like below defined under arch/arm (if it's not RAM, we
> probably don't want Normal cached memory anyway, so no check for
> O_SYNC):
> 
> #if __LINUX_ARM_ARCH__ >= 6
> pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> 			      unsigned long size, pgprot_t vma_prot)
> {
> 	if (!pfn_valid(pfn))
> 		return pgprot_device(vma_prot);
> 	else if (file->f_flags & O_SYNC)
> 		return pgprot_noncached(vma_prot);
> 	return vma_prot;
> }
> EXPORT_SYMBOL(phys_mem_access_prot);
> #endif

This WILL NOT WORK, as I've tried to explain.

For example, what about framebuffers which aren't part of system RAM?
With the above, you end up mapping them as device, but the kernel will
map them as normal memory.  So you still end up with the same physical
address mapped using two different types.

It is not as simple as you seem to think, which is the point I've been
*painfully* trying to get across to you, and apparantly keeps being
ignored.

> > That's a pipedream.  Changing the memory type of dma_alloc_coherent()
> > is going to produce random failures by itself, since device drivers
> > as currently written for ARM assume that writes to this memory are
> > strongly ordered.
> > 
> > The only way to avoid this is to audit all the ARM drivers first,
> > before changing this.  
> 
> Yes, I don't say that's not needed (together with testing). If you want,
> we can restrict this requirement to ARMv7 onwards only (that's where it
> actually matters most) and there aren't many platforms around. The
> driver fix is simple enough - adding mb() before starting DMA transfers
> (I don't think this contradicts Documentation/memory-barriers.txt). I
> can see some well written drivers already use wmb() in functions like
> start_xmit().

Indeed, and as I say this is what needs to happen first.  But there is no
option (as you seem to want) to do this without causing instability.
One way or another, we're going to get unstable systems - the choice
is whether we get them *now* by changing the memory returned by
dma_alloc_coherent() or *later* when we actually have platforms which
suffer a problem.

> Alternatively, we can place a DMB in readl/writel but I'm not sure about
> the performance impact.

If you do that, you might as well use strongly ordered memory for device
mappings - since you'll be causing the CPU to stall after each access.

> Of course, you can use a more complicated workaround and ensure that
> coherent mappings first unmap the pages from the linear kernel mapping
> but we end up using pages instead of sections.

Please put some thought into your statements before you make them.
This is impossible without getting rid of the section mappings for
the kernel's direct-mapped pages, and converting them to 4K page
mappings.  That means a lot of additional TLB pressure which we
_already_ know people will not be happy with.

There is no way in hell that this would be a sane solution.

> > I don't think you read what I said.  What I was implying is that on
> > v6 and v7, there is (and can be) no difference between
> > dma_alloc_coherent() and dma_alloc_writecombine() - see what I said
> > in parens where I detail the type of memory _writecombine() provides
> > you with, which is precisely what you're proposing the _coherent()
> > interface returns.
> 
> I agree, with my patch there is no difference between coherent and
> writecombine. Why does it need to be one?

Because writecombine is unsafe for older architectures to use for
coherent DMA - it isn't coherent.  Especially when you realise that
older architecture write buffers are buggy as hell by allowing writes
to buffered memory to be randomly reordered, in spite of documentation
to the contary.

If you have a shared ownership ring buffer, you need the writes for
updating the DMA pointers to hit it before the write to change the
ownership of the descriptor to device.  Reverse those two writes and
you end up transmitting junk.

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Catalin Marinas]

On Fri, 2009-10-23 at 13:36 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 23, 2009 at 01:05:12PM +0100, Catalin Marinas wrote:
> > Something like below defined under arch/arm (if it's not RAM, we
> > probably don't want Normal cached memory anyway, so no check for
> > O_SYNC):
> > 
> > #if __LINUX_ARM_ARCH__ >= 6
> > pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> > 			      unsigned long size, pgprot_t vma_prot)
> > {
> > 	if (!pfn_valid(pfn))
> > 		return pgprot_device(vma_prot);
> > 	else if (file->f_flags & O_SYNC)
> > 		return pgprot_noncached(vma_prot);
> > 	return vma_prot;
> > }
> > EXPORT_SYMBOL(phys_mem_access_prot);
> > #endif
> 
> This WILL NOT WORK, as I've tried to explain.
> 
> For example, what about framebuffers which aren't part of system RAM?
> With the above, you end up mapping them as device, but the kernel will
> map them as normal memory.  So you still end up with the same physical
> address mapped using two different types.

In this case we would need other ways of checking whether a page is RAM,
maybe having a per-platform phys_mem_access_prot function (similar to
what powerpc seems to do) which is aware of other RAM-like areas and
what drivers are used.

It's not simple but IMHO not impossible either.

> > Alternatively, we can place a DMB in readl/writel but I'm not sure about
> > the performance impact.
> 
> If you do that, you might as well use strongly ordered memory for device
> mappings - since you'll be causing the CPU to stall after each access.

I'm not sure that's guaranteed. The ARM ARM doesn't state any ordering
requirements between normal memory (cached or uncached) and strongly
ordered memory.

> > Of course, you can use a more complicated workaround and ensure that
> > coherent mappings first unmap the pages from the linear kernel mapping
> > but we end up using pages instead of sections.
> 
> Please put some thought into your statements before you make them.
> This is impossible without getting rid of the section mappings for
> the kernel's direct-mapped pages, and converting them to 4K page
> mappings.  That means a lot of additional TLB pressure which we
> _already_ know people will not be happy with.

I'm aware of this and I don't think its a viable solution. An
alternative is to allocate already unmapped pages (highmem) in
dma_alloc_coherent but I haven't explored this variant.

-- 
Catalin

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Catalin Marinas]

On Fri, 2009-10-23 at 13:36 +0100, Russell King - ARM Linux wrote:
> If you have a shared ownership ring buffer, you need the writes for
> updating the DMA pointers to hit it before the write to change the
> ownership of the descriptor to device.  Reverse those two writes and
> you end up transmitting junk.

But if we have arch_is_coherent() enabled (possibly with hardware
support like ARM's ACP), we need the drivers to have barriers anyway
since we would use normal cached memory (and ordering isn't guaranteed).
Of course, one has to go through the drivers used with a platform and
check them but we already have a precedent.

BTW, there are two other architectures (x86 and PowerPC) with a similar
restriction. Do they do anything about this in Linux?

PowerPC, 4.8.1 (page 36):

http://pcsostres.ac.upc.edu/cellsim/lib/exe/fetch.php/book_iii.powerpc_operating_environment_architecture.version_2.01.es-archpub3.pdf?id=additional_cell_documents&cache=cache

x86, 10.12.4 (page 10-44):

ftp://download.intel.com/support/processors/celeron/sb/25366820.pdf

-- 
Catalin

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Russell King - ARM Linux]

On Fri, Oct 23, 2009 at 05:54:21PM +0100, Catalin Marinas wrote:
> On Fri, 2009-10-23 at 13:36 +0100, Russell King - ARM Linux wrote:
> > If you have a shared ownership ring buffer, you need the writes for
> > updating the DMA pointers to hit it before the write to change the
> > ownership of the descriptor to device.  Reverse those two writes and
> > you end up transmitting junk.
> 
> But if we have arch_is_coherent() enabled (possibly with hardware
> support like ARM's ACP), we need the drivers to have barriers anyway
> since we would use normal cached memory (and ordering isn't guaranteed).
> Of course, one has to go through the drivers used with a platform and
> check them but we already have a precedent.

Actually, we do not.  arch_is_coherent() is for one ARM platform
(Intel IXP) which has their own DMA coherent support for PCI devices -
devices which quite possibly already have the necessary barriers.

> BTW, there are two other architectures (x86 and PowerPC) with a similar
> restriction. Do they do anything about this in Linux?

PPC doesn't appear to - they seem to live with multiple mappings for
the same space with different attributes.

x86 is fully cache coherent, and so doesn't have the DMA issue to
worry about - there's no need for dma_alloc_coherent() to be anything
but a call to get_free_pages().

ARM is more complex because we don't have any coherency hardware,
and the only way to safely get the required semantics for this memory
is by returning "strongly ordered" on ARMv5 and below.

In summary, the concern is:
1. The ARM support is currently setup to return strongly ordered memory.
2. Drivers are _currently_ written with this expectation in mind no
   matter what CPU architecture is in use.
3. Relaxing the returned memory type will destabilise systems,
   especially on ARMv6 and above.

As far as I'm concerned, we've spent far too long discussing this issue -
there's nothing really to discuss.  We need dma_alloc_coherent() to use
its own page protection modifier, which causes it to behave the same as
dma_alloc_writecombine() on ARMv7, and with the existing behaviour (for
the time being) on ARMv6 and below.  We should leave pgprot_noncached()
well alone until we know that its other places need to be changed.

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Woodruff, Richard]

> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Friday, October 23, 2009 12:12 PM

> As far as I'm concerned, we've spent far too long discussing this issue -
> there's nothing really to discuss.  We need dma_alloc_coherent() to use
> its own page protection modifier, which causes it to behave the same as
> dma_alloc_writecombine() on ARMv7, and with the existing behaviour (for
> the time being) on ARMv6 and below.  We should leave pgprot_noncached()
> well alone until we know that its other places need to be changed.

We do have a few ARMv7 versions in the pipeline at various stages. ARM has projected speculative access is going to be an issue and we do believe them.  Any tools which will help here and finding the problems will be useful.  Some of these probably will be internal hack patches.  However, once hardware is fully out there having some conditionals out there would help everyone out.

This week there was some odd coherency issue yet to be explained.  That eventual debug will look at this as one of the angles.

On CortexA8-OMAP3430 in the field, if L2-Speculative access is enabled using AuxCR, defiantly we did run into some issues.  In instance I know about the L2 speculative fetch had some collision with secure protection.  This happened across a SMI to monitor mode.  Perhaps it could have as easily been elsewhere like what will be up coming.

Regards,
Richard W.

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[David Brown]

On Fri, Oct 23, 2009 at 09:54:21AM -0700, Catalin Marinas wrote:

> But if we have arch_is_coherent() enabled (possibly with hardware
> support like ARM's ACP), we need the drivers to have barriers anyway
> since we would use normal cached memory (and ordering isn't guaranteed).
> Of course, one has to go through the drivers used with a platform and
> check them but we already have a precedent.

Our Scorpion target definitely needs barriers in the drivers.
There aren't any devices available externally, yet, but we can
test things internally.

David

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[David Brown]

On Fri, Oct 23, 2009 at 05:36:00AM -0700, Russell King - ARM Linux wrote:

> > Alternatively, we can place a DMB in readl/writel but I'm not sure about
> > the performance impact.
> 
> If you do that, you might as well use strongly ordered memory for device
> mappings - since you'll be causing the CPU to stall after each access.

Actually, the DMB is stronger than strongly ordered.  Strongly
ordered only guarantees ordering within somewhat arbitrarily
defined (and not completely settled) 1KB boundaries, for memory
mapped peripherals.

David

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Russell King - ARM Linux]

On Fri, Oct 23, 2009 at 06:12:27PM +0100, Russell King - ARM Linux wrote:
> As far as I'm concerned, we've spent far too long discussing this issue -
> there's nothing really to discuss.  We need dma_alloc_coherent() to use
> its own page protection modifier, which causes it to behave the same as
> dma_alloc_writecombine() on ARMv7, and with the existing behaviour (for
> the time being) on ARMv6 and below.  We should leave pgprot_noncached()
> well alone until we know that its other places need to be changed.

And here is the patch which does this.  Note that it also ensures that
NX is set for these pages on both ARMv6 and later.

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 201ccaa..1139768 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -304,13 +304,23 @@ PTE_BIT_FUNC(mkyoung,   |= L_PTE_YOUNG);
 
 static inline pte_t pte_mkspecial(pte_t pte) { return pte; }
 
+#define __pgprot_modify(prot,mask,bits)		\
+	__pgprot((pgprot_val(prot) & ~(mask)) | (bits))
+
 /*
  * Mark the prot value as uncacheable and unbufferable.
  */
 #define pgprot_noncached(prot) \
-	__pgprot((pgprot_val(prot) & ~L_PTE_MT_MASK) | L_PTE_MT_UNCACHED)
+	__pgprot_modify(prot, L_PTE_MT_MASK, L_PTE_MT_UNCACHED)
 #define pgprot_writecombine(prot) \
-	__pgprot((pgprot_val(prot) & ~L_PTE_MT_MASK) | L_PTE_MT_BUFFERABLE)
+	__pgprot_modify(prot, L_PTE_MT_MASK, L_PTE_MT_BUFFERABLE)
+#if __LINUX_ARM_ARCH__ >= 7
+#define pgprot_dmacoherent(prot) \
+	__pgprot_modify(prot, L_PTE_MT_MASK|L_PTE_EXEC, L_PTE_MT_BUFFERABLE)
+#else
+#define pgprot_dmacoherent(prot) \
+	__pgprot_modify(prot, L_PTE_MT_MASK|L_PTE_EXEC, L_PTE_MT_UNCACHED)
+#endif
 
 #define pmd_none(pmd)		(!pmd_val(pmd))
 #define pmd_present(pmd)	(pmd_val(pmd))
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index b30925f..7a976bb 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -328,7 +328,7 @@ dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *handle, gfp_t gf
 	}
 
 	return __dma_alloc(dev, size, handle, gfp,
-			   pgprot_noncached(pgprot_kernel));
+			   pgprot_dmacoherent(pgprot_kernel));
 }
 EXPORT_SYMBOL(dma_alloc_coherent);
 
@@ -379,7 +379,7 @@ static int dma_mmap(struct device *dev, struct vm_area_struct *vma,
 int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
 		      void *cpu_addr, dma_addr_t dma_addr, size_t size)
 {
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
 	return dma_mmap(dev, vma, cpu_addr, dma_addr, size);
 }
 EXPORT_SYMBOL(dma_mmap_coherent);

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Russell King - ARM Linux]

On Fri, Oct 23, 2009 at 02:30:31PM -0700, David Brown wrote:
> On Fri, Oct 23, 2009 at 05:36:00AM -0700, Russell King - ARM Linux wrote:
> 
> > > Alternatively, we can place a DMB in readl/writel but I'm not sure about
> > > the performance impact.
> > 
> > If you do that, you might as well use strongly ordered memory for device
> > mappings - since you'll be causing the CPU to stall after each access.
> 
> Actually, the DMB is stronger than strongly ordered.  Strongly
> ordered only guarantees ordering within somewhat arbitrarily
> defined (and not completely settled) 1KB boundaries, for memory
> mapped peripherals.

I don't believe so for ARMv6 and below.  The ARMv6 ARM (DDI0100I) is
quite explicit about the ordering there and contains no such exception
(I checked tonight).  See B2.5, figure B2-1 and its associated text.

ARMv7 is a different kettle of fish, where memory accesses are completely
unordered with respect to any other kind of access.

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[adharmap at codeaurora.org]

In addition we would need to change mb() rmb() wmb() to expand to dmb() for
arch = v7


Abhijeet Dharmapurikar

> On Fri, Oct 23, 2009 at 06:12:27PM +0100, Russell King - ARM Linux wrote:
>> As far as I'm concerned, we've spent far too long discussing this issue
>> -
>> there's nothing really to discuss.  We need dma_alloc_coherent() to use
>> its own page protection modifier, which causes it to behave the same as
>> dma_alloc_writecombine() on ARMv7, and with the existing behaviour (for
>> the time being) on ARMv6 and below.  We should leave pgprot_noncached()
>> well alone until we know that its other places need to be changed.
>
> And here is the patch which does this.  Note that it also ensures that
> NX is set for these pages on both ARMv6 and later.
>
> diff --git a/arch/arm/include/asm/pgtable.h
> b/arch/arm/include/asm/pgtable.h
> index 201ccaa..1139768 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -304,13 +304,23 @@ PTE_BIT_FUNC(mkyoung,   |= L_PTE_YOUNG);
>
>  static inline pte_t pte_mkspecial(pte_t pte) { return pte; }
>
> +#define __pgprot_modify(prot,mask,bits)		\
> +	__pgprot((pgprot_val(prot) & ~(mask)) | (bits))
> +
>  /*
>   * Mark the prot value as uncacheable and unbufferable.
>   */
>  #define pgprot_noncached(prot) \
> -	__pgprot((pgprot_val(prot) & ~L_PTE_MT_MASK) | L_PTE_MT_UNCACHED)
> +	__pgprot_modify(prot, L_PTE_MT_MASK, L_PTE_MT_UNCACHED)
>  #define pgprot_writecombine(prot) \
> -	__pgprot((pgprot_val(prot) & ~L_PTE_MT_MASK) | L_PTE_MT_BUFFERABLE)
> +	__pgprot_modify(prot, L_PTE_MT_MASK, L_PTE_MT_BUFFERABLE)
> +#if __LINUX_ARM_ARCH__ >= 7
> +#define pgprot_dmacoherent(prot) \
> +	__pgprot_modify(prot, L_PTE_MT_MASK|L_PTE_EXEC, L_PTE_MT_BUFFERABLE)
> +#else
> +#define pgprot_dmacoherent(prot) \
> +	__pgprot_modify(prot, L_PTE_MT_MASK|L_PTE_EXEC, L_PTE_MT_UNCACHED)
> +#endif
>
>  #define pmd_none(pmd)		(!pmd_val(pmd))
>  #define pmd_present(pmd)	(pmd_val(pmd))
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index b30925f..7a976bb 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -328,7 +328,7 @@ dma_alloc_coherent(struct device *dev, size_t size,
> dma_addr_t *handle, gfp_t gf
>  	}
>
>  	return __dma_alloc(dev, size, handle, gfp,
> -			   pgprot_noncached(pgprot_kernel));
> +			   pgprot_dmacoherent(pgprot_kernel));
>  }
>  EXPORT_SYMBOL(dma_alloc_coherent);
>
> @@ -379,7 +379,7 @@ static int dma_mmap(struct device *dev, struct
> vm_area_struct *vma,
>  int dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
>  		      void *cpu_addr, dma_addr_t dma_addr, size_t size)
>  {
> -	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
>  	return dma_mmap(dev, vma, cpu_addr, dma_addr, size);
>  }
>  EXPORT_SYMBOL(dma_mmap_coherent);
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH] Use Normal uncached memory rather than Strongly Ordered on ARMv6+

[Catalin Marinas]

On Fri, 2009-10-23 at 22:33 +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 23, 2009 at 06:12:27PM +0100, Russell King - ARM Linux wrote:
> > As far as I'm concerned, we've spent far too long discussing this issue -
> > there's nothing really to discuss.  We need dma_alloc_coherent() to use
> > its own page protection modifier, which causes it to behave the same as
> > dma_alloc_writecombine() on ARMv7, and with the existing behaviour (for
> > the time being) on ARMv6 and below.  We should leave pgprot_noncached()
> > well alone until we know that its other places need to be changed.
> 
> And here is the patch which does this.  Note that it also ensures that
> NX is set for these pages on both ARMv6 and later.

With Abhijeet's comment for mb(), I'm ok with the patch.

There are a few other explicit uses for pgprot_noncached() in the kernel
but I don't think they affect us yet. Some of them could probably be
changed to writecombine.

For /dev/mem, I still think we should do what IA-64 or PPC do - using
cached or writecombine if it is known to be RAM (yes, I know what you
said in the previous e-mails, it's not simple but I'll try to post
another patch for that).

-- 
Catalin