MIPS: fix mips_cache fallback without __builtin_mips_cache

PATCH New Matthias Schiffer 3 participants 5 messages Mar 5, 2016 – Mar 7, 2016 MIPS

Share & tools

Matthias Schiffer

author

2016-03-05 03:15 UTC #0 lore raw

PATCH 1/1 MIPS: fix mips_cache fallback without __builtin_mips_cache

The "R" constraint supplies the address of an variable in a register. Use "r" instead and adjust asm to supply the content of addr in a register instead.

Fixes 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")

Signed-off-by Matthias Schiffer <[email protected]>

Cc Paul Burton <[email protected]>

Cc Daniel Schwierzeck <[email protected]>

---

Hi,
I've noticed this when reading the code to understand how the cache instruction is used. I'm not sure if this bug had any practical consequences, or if nowadays all relevant compilers have __builtin_mips_cache anyways.

Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot ML.

Matthias

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

arch/mips/include/asm/cacheops.h +1 -1

index a3b07c6..002b839 100644
@@ -16,7 +16,7 @@ static inline void mips_cache(int op, const volatile void *addr)
 #ifdef __GCC_HAVE_BUILTIN_MIPS_CACHE
 	__builtin_mips_cache(op, addr);
 #else
-	__asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));
+	__asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(addr));
 #endif
 }

Signature · 1 line

2.7.2

Daniel Schwierzeck

2016-03-06 19:38 UTC #1 lore raw

PATCH 1/1 MIPS: fix mips_cache fallback without __builtin_mips_cache

Am 05.03.2016 um 04:15 schrieb Matthias Schiffer:

Quoted 16 lines

The "R" constraint supplies the address of an variable in a register. Use "r" instead and adjust asm to supply the content of addr in a register instead.
Fixes 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")
Signed-off-by Matthias Schiffer <[email protected]>
Cc Paul Burton <[email protected]>
Cc Daniel Schwierzeck <[email protected]>
---
Hi,
I've noticed this when reading the code to understand how the cache instruction is used. I'm not sure if this bug had any practical consequences, or if nowadays all relevant compilers have __builtin_mips_cache anyways.

Quoted 4 lines

Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot ML.
Matthias

I've disabled the builtin code and compared dissaemblies with and without your patch. Without your patch, gcc adds an additional store instruction before each cache instruction.

E.g. for flush_dcache_range():

18: afa20008 sw v0,8(sp)
1c: bfb50008 cache 0x15,8(sp)

vs.

14: bc550000 cache 0x15,0(v0)

The cache operation works anyway, but with your patch better code is generated.

Quoted 11 lines


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

arch/mips/include/asm/cacheops.h +0 -0

index a3b07c6..002b839 100644

arch/mips/include/asm/cacheops.h static inline void mips_cache(int op, const volatile void *addr) · line 22

Quoted 8 lines

arch/mips/include/asm/cacheops.h +1 -1

@@ -16,7 +16,7 @@ static inline void mips_cache(int op, const volatile void *addr)
 #ifdef __GCC_HAVE_BUILTIN_MIPS_CACHE
 	__builtin_mips_cache(op, addr);
 #else
-	__asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));
+	__asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(addr));
 #endif
 }

applied to u-boot-mips/next, thanks!

Signature · 1 line

- Daniel

Matthias Schiffer

author

2016-03-06 19:53 UTC #2 lore raw

PATCH 1/1 MIPS: fix mips_cache fallback without __builtin_mips_cache

Quoted 28 lines · Daniel Schwierzeck

Am 05.03.2016 um 04:15 schrieb Matthias Schiffer:
Quoted 16 lines
The "R" constraint supplies the address of an variable in a register. Use "r" instead and adjust asm to supply the content of addr in a register instead.
Fixes 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")
Signed-off-by Matthias Schiffer <[email protected]>
Cc Paul Burton <[email protected]>
Cc Daniel Schwierzeck <[email protected]>
---
Hi,
I've noticed this when reading the code to understand how the cache instruction is used. I'm not sure if this bug had any practical consequences, or if nowadays all relevant compilers have __builtin_mips_cache anyways.
Quoted 4 lines
Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot ML.
Matthias
I've disabled the builtin code and compared dissaemblies with and without your patch. Without your patch, gcc adds an additional store instruction before each cache instruction.
E.g. for flush_dcache_range():

Quoted 8 lines

18: afa20008 sw v0,8(sp)
1c: bfb50008 cache 0x15,8(sp)
vs.
14: bc550000 cache 0x15,0(v0)
The cache operation works anyway, but with your patch better code is generated.

If I understand this correctly, the code without my patch would rather invalidate the cache for the address 8(sp), which is part of the stack, and not the memory pointed at by v0.

Quoted 6 lines



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

Quoted 17 lines

Quoted 6 lines
arch/mips/include/asm/cacheops.h +0 -0
index a3b07c6..002b839 100644
arch/mips/include/asm/cacheops.h static inline void mips_cache(int op, const volatile void *addr) · line 22
Quoted 8 lines
arch/mips/include/asm/cacheops.h +1 -1
@@ -16,7 +16,7 @@ static inline void mips_cache(int op, const volatile void *addr)
 #ifdef __GCC_HAVE_BUILTIN_MIPS_CACHE
 	__builtin_mips_cache(op, addr);
 #else
-	__asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));
+	__asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(addr));
 #endif
 }
 
applied to u-boot-mips/next, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160306/b10e10a7/attachment.sig>

Daniel Schwierzeck

2016-03-06 20:36 UTC #3 lore raw

PATCH 1/1 MIPS: fix mips_cache fallback without __builtin_mips_cache

Am 06.03.2016 um 20:53 schrieb Matthias Schiffer:

Quoted 34 lines · Daniel Schwierzeck

Quoted 33 lines
Am 05.03.2016 um 04:15 schrieb Matthias Schiffer:
Quoted 16 lines
The "R" constraint supplies the address of an variable in a register. Use "r" instead and adjust asm to supply the content of addr in a register instead.
Fixes 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")
Signed-off-by Matthias Schiffer <[email protected]>
Cc Paul Burton <[email protected]>
Cc Daniel Schwierzeck <[email protected]>
---
Hi,
I've noticed this when reading the code to understand how the cache instruction is used. I'm not sure if this bug had any practical consequences, or if nowadays all relevant compilers have __builtin_mips_cache anyways.
Quoted 4 lines
Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot ML.
Matthias
I've disabled the builtin code and compared dissaemblies with and without your patch. Without your patch, gcc adds an additional store instruction before each cache instruction.
E.g. for flush_dcache_range():
18: afa20008 sw v0,8(sp)
1c: bfb50008 cache 0x15,8(sp)
vs.

Quoted 7 lines

Quoted 3 lines
14: bc550000 cache 0x15,0(v0)
The cache operation works anyway, but with your patch better code is generated.
If I understand this correctly, the code without my patch would rather invalidate the cache for the address 8(sp), which is part of the stack, and not the memory pointed at by v0.

"sw v0,8(sp)" stores the address in v0 on the stack (location is current stack pointer + 8 * 4 bytes). The cache op then loads this address from stack. Eventually the cache op always uses the address stored in v0 so there is/was no functional bug. But the extra copy to the stack causes a performance degradation.

Signature · 1 line

- Daniel

Matthew Fortune

2016-03-07 09:30 UTC #4 lore raw

PATCH 1/1 MIPS: fix mips_cache fallback without __builtin_mips_cache

In reply to Matthias Schiffer

Quoted 12 lines · Matthias Schiffer

Quoted 2 lines
I've disabled the builtin code and compared dissaemblies with and
without your patch. Without your patch, gcc adds an additional store instruction before each cache instruction.
Quoted 8 lines
E.g. for flush_dcache_range():
18: afa20008 sw v0,8(sp)
1c: bfb50008 cache 0x15,8(sp)
vs.

Quoted 8 lines

Quoted 3 lines
14: bc550000 cache 0x15,0(v0)
The cache operation works anyway, but with your patch better code is
generated.
If I understand this correctly, the code without my patch would rather invalidate the cache for the address 8(sp), which is part of the stack, and not the memory pointed at by v0.

Matthias is correct. These two code sequences will exhibit different overall behaviour. The latter is much more likely to be what you wanted from what I can see as the flush is to operate on the value of the 'addr' variable not the address of the addr variable.

The same behaviour could be achieved with:

__asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));

and

__asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(&addr));

Hope that's useful.

Matthew