ioremap_wc on arm64

PATCH New Jayachandran C 4 participants 12 messages May 22, 2017 – May 24, 2017 arm64

lore

Share & tools

lore

Jayachandran C

author

2017-05-22 07:01 UTC #0 lore raw

cover ioremap_wc on arm64

Quoted 1 line

From its definition, the device "gather" attribute seems to be a better

fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.

I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.

Comments are very welcome.

Thanks,
JC.

Jayachandran C (2):
arm64: add PROT_DEVICE_GRE for Device GRE mapping
arm64: switch ioremap_wc to use Device GRE

 arch/arm64/include/asm/io.h           | 2 +-
 arch/arm64/include/asm/pgtable-prot.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Signature · 1 line

2.7.4

Jayachandran C

author

2017-05-22 07:01 UTC #1 lore raw

PATCH 1/2 arm64: add PROT_DEVICE_GRE for Device GRE mapping

The MAIR is programmed with attribute "Device GRE" (0x0C) at index 3, but there is no corresponding page protection value defined. Make the page attribute available for use by defining PROT_DEVICE_GRE.

Signed-off-by Jayachandran C <[email protected]>

---
 arch/arm64/include/asm/pgtable-prot.h | 1 +
 1 file changed, 1 insertion(+)

arch/arm64/include/asm/pgtable-prot.h +1 -0

index 2142c77..216f9f9 100644
@@ -39,6 +39,7 @@
 
 #define PROT_DEVICE_nGnRnE	(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_ATTRINDX(MT_DEVICE_nGnRnE))
 #define PROT_DEVICE_nGnRE	(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_ATTRINDX(MT_DEVICE_nGnRE))
+#define PROT_DEVICE_GRE		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_ATTRINDX(MT_DEVICE_GRE))
 #define PROT_NORMAL_NC		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_ATTRINDX(MT_NORMAL_NC))
 #define PROT_NORMAL_WT		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_ATTRINDX(MT_NORMAL_WT))
 #define PROT_NORMAL		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_DIRTY | PTE_WRITE | PTE_ATTRINDX(MT_NORMAL))

Signature · 1 line

2.7.4

Jayachandran C

author

2017-05-22 07:01 UTC #2 lore raw

PATCH 2/2 arm64: switch ioremap_wc to use Device GRE

In reply to Jayachandran C

The device "Gather" attribute is defined in the ARMv8 ARM as:
"The Gathering attribute determines whether it is permissible for either:
* Multiple memory accesses of the same type, read or write, to the same memory location to be merged into a single transaction.
* Multiple memory accesses of the same type, read or write, to different memory locations to be merged into a single memory transaction on an interconnect."

This attribute is more appropriate for the write combining ioremap_wc() than the non-cached memory attribute used now, so update the ioremap_wc implementation to use it.

Signed-off-by Jayachandran C <[email protected]>

---
 arch/arm64/include/asm/io.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/include/asm/io.h +1 -1

index 35b2e50..b1c92e3 100644
@@ -168,7 +168,7 @@ extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
 
 #define ioremap(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
 #define ioremap_nocache(addr, size)	__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
-#define ioremap_wc(addr, size)		__ioremap((addr), (size), __pgprot(PROT_NORMAL_NC))
+#define ioremap_wc(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_GRE))
 #define ioremap_wt(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
 #define iounmap				__iounmap

Signature · 1 line

2.7.4

Catalin Marinas

maintainer

2017-05-22 08:56 UTC #3 lore raw

cover ioremap_wc on arm64

In reply to Jayachandran C

Quoted 8 lines · Jayachandran C

From its definition, the device "gather" attribute seems to be a better fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.
I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.

The reason we kept it as Normal NC is that Device_GRE does not allow unaligned accesses.

Signature · 1 line

Catalin

Jayachandran C

author

2017-05-22 11:53 UTC #4 lore raw

cover ioremap_wc on arm64

Quoted 6 lines · Catalin Marinas

Quoted 5 lines · Jayachandran C
From its definition, the device "gather" attribute seems to be a better fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.

Quoted 6 lines

Quoted 3 lines
I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.
The reason we kept it as Normal NC is that Device_GRE does not allow unaligned accesses.

There does not to be an expectation to have unaligned access on __iomem pointers. I also see that memremap can call ioremap_wc (which is a Normal mapping) or ioremap_wt(which is a Device mapping), so that is inconsistent as well.

Was this added for a specific use case? Also, do you think this patchset is acceptable?

Thanks,
JC.

Alexander Graf

2017-05-22 11:58 UTC #5 lore raw

cover ioremap_wc on arm64

Quoted 12 lines · Jayachandran C

Quoted 11 lines · Catalin Marinas
Quoted 8 lines · Jayachandran C
From its definition, the device "gather" attribute seems to be a better
fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.
I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.
The reason we kept it as Normal NC is that Device_GRE does not allow unaligned accesses.

Quoted 7 lines

There does not to be an expectation to have unaligned access on __iomem pointers. I also see that memremap can call ioremap_wc (which is a Normal mapping) or ioremap_wt(which is a Device mapping), so that is inconsistent as well.
Was this added for a specific use case? Also, do you think this patchset is acceptable?

I think the patch description in patch 2/2 needs to be more explicit as to why mapping device memory as normal is a problem.

Alex

Will Deacon

2017-05-22 12:22 UTC #6 lore raw

cover ioremap_wc on arm64

In reply to Jayachandran C

Quoted 14 lines · Jayachandran C

Quoted 6 lines · Catalin Marinas
Quoted 5 lines · Jayachandran C
From its definition, the device "gather" attribute seems to be a better fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.
Quoted 6 lines
Quoted 3 lines
I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.
The reason we kept it as Normal NC is that Device_GRE does not allow unaligned accesses.

Quoted 7 lines

There does not to be an expectation to have unaligned access on __iomem pointers. I also see that memremap can call ioremap_wc (which is a Normal mapping) or ioremap_wt(which is a Device mapping), so that is inconsistent as well.
Was this added for a specific use case? Also, do you think this patchset is acceptable?

I think it was used for framebuffers. Note that using normal-NC also aligns with arch/arm/ and, since normal-NC is strictly a more relaxed memory type than Device-GRE, I'm really not keen making on this change.

Will

Jayachandran C

author

2017-05-22 14:21 UTC #7 lore raw

cover ioremap_wc on arm64

Quoted 20 lines · Will Deacon

Quoted 19 lines · Jayachandran C
Quoted 6 lines · Catalin Marinas
Quoted 5 lines · Jayachandran C
From its definition, the device "gather" attribute seems to be a better fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.
Quoted 6 lines
Quoted 3 lines
I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.
The reason we kept it as Normal NC is that Device_GRE does not allow unaligned accesses.
There does not to be an expectation to have unaligned access on __iomem pointers. I also see that memremap can call ioremap_wc (which is a Normal mapping) or ioremap_wt(which is a Device mapping), so that is inconsistent as well.

Quoted 6 lines

Quoted 2 lines
Was this added for a specific use case? Also, do you think this patchset is acceptable?
I think it was used for framebuffers. Note that using normal-NC also aligns with arch/arm/ and, since normal-NC is strictly a more relaxed memory type than Device-GRE, I'm really not keen making on this change.

If there is a hardware requirement that Normal-NC has to be implemented as more relaxed attribute than Device GRE, I can go back to the hardware team on this. I did not see any text in the ARM ARM requiring this.

Even if that is the case, having Normal attribute for ioremap just for ioremap_wc is inconsistent. The Device Gathering attribute by its definition is exactly what write combining requires. The memremap() API looks like a better way to expose Normal-NC mapping (with additional features like speculation etc.) if implemented correctly.

Also, patch 1/2 should be useful anyway - can that be picked up?

Thanks,
JC.

Catalin Marinas

maintainer

2017-05-22 15:49 UTC #8 lore raw

cover ioremap_wc on arm64

Quoted 24 lines · Jayachandran C

Quoted 23 lines · Will Deacon
Quoted 21 lines · Jayachandran C
Quoted 6 lines · Catalin Marinas
Quoted 5 lines · Jayachandran C
From its definition, the device "gather" attribute seems to be a better fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.
Quoted 6 lines
Quoted 3 lines
I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.
The reason we kept it as Normal NC is that Device_GRE does not allow unaligned accesses.
There does not to be an expectation to have unaligned access on __iomem pointers. I also see that memremap can call ioremap_wc (which is a Normal mapping) or ioremap_wt(which is a Device mapping), so that is inconsistent as well.
Was this added for a specific use case? Also, do you think this patchset is acceptable?

Quoted 7 lines

Quoted 3 lines
I think it was used for framebuffers. Note that using normal-NC also aligns with arch/arm/ and, since normal-NC is strictly a more relaxed memory type than Device-GRE, I'm really not keen making on this change.
If there is a hardware requirement that Normal-NC has to be implemented as more relaxed attribute than Device GRE, I can go back to the hardware team on this. I did not see any text in the ARM ARM requiring this.

It depends what you mean by "relaxed". Normal NC allows unaligned accesses and also allows speculative loads (you don't get any ordering guarantees of the Device memory). If you mean skipping some transparent cache snooping, it may break other use-cases.

Quoted 5 lines

Even if that is the case, having Normal attribute for ioremap just for ioremap_wc is inconsistent. The Device Gathering attribute by its definition is exactly what write combining requires. The memremap() API looks like a better way to expose Normal-NC mapping (with additional features like speculation etc.) if implemented correctly.

I agree, memremap() is a better way but this was merged in 4.3 while ioremap_wc() had been around for a long time. If you want to change its semantics, you'd need to go through all its uses in the kernel and make sure they *only* use I/O accessors with such memory. At a quick grep, there are several places where the __iomem pointer attribute is dropped shortly after ioremap() and the pointer is accessed directly. That's where things will break with Device GRE memory since the compiler doesn't guarantee aligned accesses.

Quoted 1 line

Also, patch 1/2 should be useful anyway - can that be picked up?

The patch is harmless but are we going to have a user of Device_GRE?

Signature · 1 line

Catalin

Jayachandran C

author

2017-05-23 10:09 UTC #9 lore raw

cover ioremap_wc on arm64

Quoted 29 lines · Catalin Marinas

Quoted 28 lines · Jayachandran C
Quoted 26 lines · Will Deacon
Quoted 14 lines · Jayachandran C
Quoted 6 lines · Catalin Marinas
Quoted 5 lines · Jayachandran C
From its definition, the device "gather" attribute seems to be a better fit for implementing write combining mapping in ioremap_wc(). And on ThunderX2, Device GRE mapping has optimizations that makes it much faster than normal uncached mapping.
Quoted 6 lines
Quoted 3 lines
I am not sure of the reasoning behind the original decision to make ioremap_wc use "Normal Non-Cached" attribute, since all the other variants of ioremap use device attributes, and ioremap_wc looks like an exception.
The reason we kept it as Normal NC is that Device_GRE does not allow unaligned accesses.
Quoted 7 lines
There does not to be an expectation to have unaligned access on __iomem pointers. I also see that memremap can call ioremap_wc (which is a Normal mapping) or ioremap_wt(which is a Device mapping), so that is inconsistent as well.
Was this added for a specific use case? Also, do you think this patchset is acceptable?
I think it was used for framebuffers. Note that using normal-NC also aligns with arch/arm/ and, since normal-NC is strictly a more relaxed memory type than Device-GRE, I'm really not keen making on this change.

Quoted 8 lines

Quoted 3 lines
If there is a hardware requirement that Normal-NC has to be implemented as more relaxed attribute than Device GRE, I can go back to the hardware team on this. I did not see any text in the ARM ARM requiring this.
It depends what you mean by "relaxed". Normal NC allows unaligned accesses and also allows speculative loads (you don't get any ordering guarantees of the Device memory). If you mean skipping some transparent cache snooping, it may break other use-cases.

Ok. I was using Will's terminology here. This is not the main issue, but worth understanding. If there is a requirement in ARM spec that "Normal NC" must be "strictly more releaxed" than "Device GRE", I will have to point that out to our hardware team.

Quoted 6 lines

Quoted 5 lines
Even if that is the case, having Normal attribute for ioremap just for ioremap_wc is inconsistent. The Device Gathering attribute by its definition is exactly what write combining requires. The memremap() API looks like a better way to expose Normal-NC mapping (with additional features like speculation etc.) if implemented correctly.

Quoted 8 lines

I agree, memremap() is a better way but this was merged in 4.3 while ioremap_wc() had been around for a long time. If you want to change its semantics, you'd need to go through all its uses in the kernel and make sure they *only* use I/O accessors with such memory. At a quick grep, there are several places where the __iomem pointer attribute is dropped shortly after ioremap() and the pointer is accessed directly. That's where things will break with Device GRE memory since the compiler doesn't guarantee aligned accesses.

There is a difference in behavior with regard to unaligned access between ioremap_wc and ioremap already. I don't follow why unaligned access has to supported for ioremap_wc but not for ioremap - it would be much better to be consistent here.

Quoted 3 lines

Quoted 1 line
Also, patch 1/2 should be useful anyway - can that be picked up?
The patch is harmless but are we going to have a user of Device_GRE?

This patch would make the attribute in MAIR and the index MT_DEVICE_GRE useful for __ioremap. Otherwise I don't see the use in having the attribute at all. I can at least give this to people who want Device GRE mapping, which is the fastest way to access device memory on ThunderX2.

JC.

Catalin Marinas

maintainer

2017-05-23 14:08 UTC #10 lore raw

cover ioremap_wc on arm64

Quoted 22 lines · Jayachandran C

Quoted 12 lines · Catalin Marinas
Quoted 10 lines · Jayachandran C
Quoted 3 lines · Will Deacon
I think it was used for framebuffers. Note that using normal-NC also aligns with arch/arm/ and, since normal-NC is strictly a more relaxed memory type than Device-GRE, I'm really not keen making on this change.
Even if that is the case, having Normal attribute for ioremap just for ioremap_wc is inconsistent. The Device Gathering attribute by its definition is exactly what write combining requires. The memremap() API looks like a better way to expose Normal-NC mapping (with additional features like speculation etc.) if implemented correctly.
Quoted 8 lines
I agree, memremap() is a better way but this was merged in 4.3 while ioremap_wc() had been around for a long time. If you want to change its semantics, you'd need to go through all its uses in the kernel and make sure they *only* use I/O accessors with such memory. At a quick grep, there are several places where the __iomem pointer attribute is dropped shortly after ioremap() and the pointer is accessed directly. That's where things will break with Device GRE memory since the compiler doesn't guarantee aligned accesses.

Quoted 4 lines

There is a difference in behavior with regard to unaligned access between ioremap_wc and ioremap already. I don't follow why unaligned access has to supported for ioremap_wc but not for ioremap - it would be much better to be consistent here.

Please re-read my paragraph above. It's nice if both ioremap_wc() and ioremap() returned Device memory as long as you fix the ioremap_wc() misuses throughout the kernel.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Also, patch 1/2 should be useful anyway - can that be picked up?
The patch is harmless but are we going to have a user of Device_GRE?
This patch would make the attribute in MAIR and the index MT_DEVICE_GRE useful for __ioremap. Otherwise I don't see the use in having the attribute at all. I can at least give this to people who want Device GRE mapping, which is the fastest way to access device memory on ThunderX2.

Is this a driver that's going to be used on arm64 kernels only? I'm not that keen on __ioremap() since it's not a standard driver API, so you end up with #ifdef CONFIG_ARM64 in the driver code.

Signature · 1 line

Catalin

Jayachandran C

author

2017-05-24 13:33 UTC #11 lore raw

cover ioremap_wc on arm64

Quoted 23 lines · Catalin Marinas

Quoted 22 lines · Jayachandran C
Quoted 20 lines · Catalin Marinas
Quoted 10 lines · Jayachandran C
Quoted 3 lines · Will Deacon
I think it was used for framebuffers. Note that using normal-NC also aligns with arch/arm/ and, since normal-NC is strictly a more relaxed memory type than Device-GRE, I'm really not keen making on this change.
Even if that is the case, having Normal attribute for ioremap just for ioremap_wc is inconsistent. The Device Gathering attribute by its definition is exactly what write combining requires. The memremap() API looks like a better way to expose Normal-NC mapping (with additional features like speculation etc.) if implemented correctly.
I agree, memremap() is a better way but this was merged in 4.3 while ioremap_wc() had been around for a long time. If you want to change its semantics, you'd need to go through all its uses in the kernel and make sure they *only* use I/O accessors with such memory. At a quick grep, there are several places where the __iomem pointer attribute is dropped shortly after ioremap() and the pointer is accessed directly. That's where things will break with Device GRE memory since the compiler doesn't guarantee aligned accesses.

Quoted 6 lines

Quoted 4 lines
There is a difference in behavior with regard to unaligned access between ioremap_wc and ioremap already. I don't follow why unaligned access has to supported for ioremap_wc but not for ioremap - it would be much better to be consistent here.
Please re-read my paragraph above.

I had the same feeling after seeing your reply :)

Quoted 3 lines

It's nice if both ioremap_wc() and
ioremap() returned Device memory as long as you fix the ioremap_wc() misuses throughout the kernel.

ioremap and ioremap_wc is been used and abused in multiple places after dropping the __iomem annotation, and it is not an easy task to figure out where unaligned access is done on the pointer returned. I am told specifically that in ioremap_wc needs to support unaligned access as opposed to ioremap.

I am trying to figure out if there are specific cases you expect it to fail. Since arm64 is a fairly new architecture and it will be easy to get this correct now, rather than to let the status quo continue.

There is no "Gather" attribute on arm, so using "Normal uncached" for ioremap_wc is understandable. On arm64, given that there is an attribute specifically for write combining - you have decided to go with Normal uncached. I had hoped for a proper reason for that and not a vague "maybe framebuffer, go grep the kernel and figure out" response.

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
Quoted 1 line
Also, patch 1/2 should be useful anyway - can that be picked up?
The patch is harmless but are we going to have a user of Device_GRE?

Quoted 8 lines

Quoted 4 lines
This patch would make the attribute in MAIR and the index MT_DEVICE_GRE useful for __ioremap. Otherwise I don't see the use in having the attribute at all. I can at least give this to people who want Device GRE mapping, which is the fastest way to access device memory on ThunderX2.
Is this a driver that's going to be used on arm64 kernels only? I'm not that keen on __ioremap() since it's not a standard driver API, so you end up with #ifdef CONFIG_ARM64 in the driver code.

FWIW, __ioremap() is EXPORT_SYMBOL. Having the attribute will be useful.

JC.