Put cast inside macro instead of all the callers

PATCH In review Andrew Sharp 4 participants 7 messages Oct 31, 2007 – Nov 8, 2007 MIPS

Share & tools

Andrew Sharp

author

2007-10-31 21:12 UTC #0 lore raw

PATCH 1/1 Put cast inside macro instead of all the callers

Resend: I tried sending this a couple of days ago but haven't seen it. Wondering if it got stuck in a spam filter or our lovely exchange server or something.

Since all the callers of the PHYS_TO_XKPHYS macro call with a constant, put the cast to LL inside the macro where it really should be rather than in all the callers. This makes macros like PHYS_TO_XKSEG_UNCACHED work without gcc whining.

Hopefully this will apply ok.

Signed-off-by Andrew Sharp <[email protected]>

---
 arch/mips/lib/uncached.c     |   12 ++++++------
 include/asm-mips/addrspace.h |    2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

arch/mips/lib/uncached.c +6 -6

index 2388f7f..bbca1ea 100644
@@ -45,9 +45,9 @@ unsigned long __init run_uncached(void *func)
 	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
 		usp = CKSEG1ADDR(sp);
 #ifdef CONFIG_64BIT
-	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
-		 (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
-		usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0, 0) &&
+		 (long long)sp < (long long)PHYS_TO_XKPHYS(8, 0))
+		usp = PHYS_TO_XKPHYS(K_CALG_UNCACHED,
 				     XKPHYS_TO_PHYS((long long)sp));
 #endif
 	else {

@@ -57,9 +57,9 @@ unsigned long __init run_uncached(void *func)
 	if (lfunc >= (long)CKSEG0 && lfunc < (long)CKSEG2)
 		ufunc = CKSEG1ADDR(lfunc);
 #ifdef CONFIG_64BIT
-	else if ((long long)lfunc >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
-		 (long long)lfunc < (long long)PHYS_TO_XKPHYS(8LL, 0))
-		ufunc = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+	else if ((long long)lfunc >= (long long)PHYS_TO_XKPHYS(0, 0) &&
+		 (long long)lfunc < (long long)PHYS_TO_XKPHYS(8, 0))
+		ufunc = PHYS_TO_XKPHYS(K_CALG_UNCACHED,
 				       XKPHYS_TO_PHYS((long long)lfunc));
 #endif
 	else {

include/asm-mips/addrspace.h +1 -1

index 964c5ed..1bc23e8 100644
@@ -127,7 +127,7 @@
 #define PHYS_TO_XKSEG_CACHED(p)		PHYS_TO_XKPHYS(K_CALG_COH_SHAREABLE,(p))
 #define XKPHYS_TO_PHYS(p)		((p) & TO_PHYS_MASK)
 #define PHYS_TO_XKPHYS(cm,a)		(_CONST64_(0x8000000000000000) | \
-					 ((cm)<<59) | (a))
+					 (_CONST64_(cm)<<59) | (a))
 
 #if defined (CONFIG_CPU_R4300)						\
     || defined (CONFIG_CPU_R4X00)					\

Signature · 1 line

1.4.4.4

Ulrich Eckhardt

2007-11-01 16:07 UTC #1 lore raw

I'm by far not a MIPS expert, but I'm puzzled by the code and how it uses signed integers for addresses. I just added some comments below, but I'm not sure if they are valid. Thank you for any clarification!

Quoted 4 lines · Andrew Sharp

Since all the callers of the PHYS_TO_XKPHYS macro call with a constant, put the cast to LL inside the macro where it really should be rather than in all the callers. This makes macros like PHYS_TO_XKSEG_UNCACHED work without gcc whining.

I'm not sure if this is always a compile-time constant so that you can adorn it with a LL. However, note that this is not a cast, a cast is at runtime.

arch/mips/lib/uncached.c unsigned long __init run_uncached(void *func) · line 51

Quoted 10 lines

arch/mips/lib/uncached.c +3 -3

 	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
 		usp = CKSEG1ADDR(sp);
 #ifdef CONFIG_64BIT
-	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
-		 (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
-		usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0, 0) &&
+		 (long long)sp < (long long)PHYS_TO_XKPHYS(8, 0))
+		usp = PHYS_TO_XKPHYS(K_CALG_UNCACHED,
 				     XKPHYS_TO_PHYS((long long)sp));

I'd say this code is broken in way too many aspects:
1. A plethora of casts. PHYS_TO_XKPHYS() should return a physical address (i.e. 32 or 64 bits unsigned integer) already, so casting its result should not be necessary.
2. Using a signed integer of undefined size for an address. At least use an explicit 64 bit unsigned integer (__u64).
3. The use of signed types makes me wonder about intended overflow semantics. Just for the record, signed overflow in C causes undefined behaviour, no diagnostic required, and recent GCC even assume that no overflow occurs as an optimisation!

include/asm-mips/addrspace.h line 130

Quoted 5 lines

#define PHYS_TO_XKSEG_CACHED(p) PHYS_TO_XKPHYS(K_CALG_COH_SHAREABLE,(p))
#define XKPHYS_TO_PHYS(p) ((p) & TO_PHYS_MASK)
#define PHYS_TO_XKPHYS(cm,a) (_CONST64_(0x8000000000000000) | \
include/asm-mips/addrspace.h +1 -1
-					 ((cm)<<59) | (a))
+					 (_CONST64_(cm)<<59) | (a))

This macro will always(!!!) generate a negative number, is that intended?

Uli
- slightly puzzled -

Signature · 10 lines

Sator Laser GmbH
Geschäftsführer: Michael Wöhrmann, Amtsgericht Hamburg HR B62 932

**************************************************************************************
           Visit our website at <http://www.satorlaser.de/>
**************************************************************************************
Diese E-Mail einschließlich sämtlicher Anhänge ist nur für den Adressaten bestimmt und kann vertrauliche Informationen enthalten. Bitte benachrichtigen Sie den Absender umgehend, falls Sie nicht der beabsichtigte Empfänger sein sollten. Die E-Mail ist in diesem Fall zu löschen und darf weder gelesen, weitergeleitet, veröffentlicht oder anderweitig benutzt werden.
E-Mails können durch Dritte gelesen werden und Viren sowie nichtautorisierte Änderungen enthalten. Sator Laser GmbH ist für diese Folgen nicht verantwortlich.

**************************************************************************************

Andrew Sharp

author

2007-11-01 17:23 UTC #2 lore raw

Quoted 5 lines · On Thu, 1 Nov 2007 17:04:01 +0100 Ulrich Eckhardt <[email protected]>…

I'm by far not a MIPS expert, but I'm puzzled by the code and how it uses signed integers for addresses. I just added some comments below, but I'm not sure if they are valid. Thank you for any clarification!
On Wednesday 31 October 2007, Andrew Sharp wrote:

Quoted 8 lines

Quoted 4 lines
Since all the callers of the PHYS_TO_XKPHYS macro call with a constant, put the cast to LL inside the macro where it really should be rather than in all the callers. This makes macros like PHYS_TO_XKSEG_UNCACHED work without gcc whining.
I'm not sure if this is always a compile-time constant so that you can adorn it with a LL. However, note that this is not a cast, a cast is at runtime.

It is always a constant.

Quoted 25 lines

Quoted 12 lines
arch/mips/lib/uncached.c +0 -1
 	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
 		usp = CKSEG1ADDR(sp);
 #ifdef CONFIG_64BIT
-	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL,
0) &&
arch/mips/lib/uncached.c +1 -2
-		 (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
-		usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0, 0)
&&
arch/mips/lib/uncached.c +2 -0
+		 (long long)sp < (long long)PHYS_TO_XKPHYS(8, 0))
+		usp = PHYS_TO_XKPHYS(K_CALG_UNCACHED,
 				     XKPHYS_TO_PHYS((long
Quoted 1 line
long)sp));
I'd say this code is broken in way too many aspects:
1. A plethora of casts. PHYS_TO_XKPHYS() should return a physical address (i.e. 32 or 64 bits unsigned integer) already, so casting its result should not be necessary.
2. Using a signed integer of undefined size for an address. At least use an explicit 64 bit unsigned integer (__u64).
3. The use of signed types makes me wonder about intended overflow semantics. Just for the record, signed overflow in C causes undefined behaviour, no diagnostic required, and recent GCC even assume that no overflow occurs as an optimisation!

Quoted 10 lines

include/asm-mips/addrspace.h line 130
Quoted 7 lines
#define PHYS_TO_XKSEG_CACHED(p)
PHYS_TO_XKPHYS(K_CALG_COH_SHAREABLE,(p)) #define
XKPHYS_TO_PHYS(p) ((p) & TO_PHYS_MASK) #define
PHYS_TO_XKPHYS(cm,a) (_CONST64_(0x8000000000000000)
| \
include/asm-mips/addrspace.h +1 -1
-					 ((cm)<<59) | (a))
+					 (_CONST64_(cm)<<59) | (a))
This macro will always(!!!) generate a negative number, is that intended?

Well, it's an address, not a number. Does that help? The point of the macro is to convert physical addresses to a selectable type of virtual address, of which mips has several.

Cheers,

Ralf Baechle

2007-11-01 17:47 UTC #3 lore raw

In reply to Ulrich Eckhardt

Quoted 3 lines · Ulrich Eckhardt

I'm by far not a MIPS expert, but I'm puzzled by the code and how it uses signed integers for addresses. I just added some comments below, but I'm not sure if they are valid. Thank you for any clarification!

When going from 32-bit to 64-bit MIPS did sign-extend register values and addresses, that is for example 0x80000000 became 0xffffffff80000000. That is the code sequence which on 32-bit is used to load the first address actually happens to load the 2nd value on a 64-bit machine. Which is an extremly elegant solution on the hardware level but at a few places software need to know. The code tries to make clever use of sign extension by the compiler to avoid multiple constants.

Quoted 8 lines · Andrew Sharp

Quoted 4 lines
Since all the callers of the PHYS_TO_XKPHYS macro call with a constant, put the cast to LL inside the macro where it really should be rather than in all the callers. This makes macros like PHYS_TO_XKSEG_UNCACHED work without gcc whining.
I'm not sure if this is always a compile-time constant so that you can adorn it with a LL. However, note that this is not a cast, a cast is at runtime.

No. The compiler can evaluate the cast of a constant value at compile time and that exactly is what the code is exploiting here.

Quoted 7 lines

arch/mips/lib/uncached.c +1 -3

 	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
 		usp = CKSEG1ADDR(sp);
 #ifdef CONFIG_64BIT
-	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
-		 (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
-		usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0, 0) &&

Quoted 8 lines

arch/mips/lib/uncached.c unsigned long __init run_uncached(void *func) · line 51
Quoted 3 lines
arch/mips/lib/uncached.c +2 -0
+		 (long long)sp < (long long)PHYS_TO_XKPHYS(8, 0))
+		usp = PHYS_TO_XKPHYS(K_CALG_UNCACHED,
 				     XKPHYS_TO_PHYS((long long)sp));
I'd say this code is broken in way too many aspects:
1. A plethora of casts. PHYS_TO_XKPHYS() should return a physical address (i.e. 32 or 64 bits unsigned integer) already, so casting its result should not be necessary.

No argument about the beauty of the whole thing.

Quoted 2 lines

2. Using a signed integer of undefined size for an address. At least use an explicit 64 bit unsigned integer (__u64).

long long is 64-bit on Linux.

Quoted 4 lines

3. The use of signed types makes me wonder about intended overflow semantics. Just for the record, signed overflow in C causes undefined behaviour, no diagnostic required, and recent GCC even assume that no overflow occurs as an optimisation!

There is no overflow possible here.

Quoted 7 lines

include/asm-mips/addrspace.h line 130
Quoted 5 lines
#define PHYS_TO_XKSEG_CACHED(p) PHYS_TO_XKPHYS(K_CALG_COH_SHAREABLE,(p))
#define XKPHYS_TO_PHYS(p) ((p) & TO_PHYS_MASK)
#define PHYS_TO_XKPHYS(cm,a) (_CONST64_(0x8000000000000000) | \
include/asm-mips/addrspace.h +1 -1
-					 ((cm)<<59) | (a))
+					 (_CONST64_(cm)<<59) | (a))
This macro will always(!!!) generate a negative number, is that intended?

Sort of. Most users don't care, the address is just an address for them. Note that the Linux idea of "all virtual addresses can be represented in an unsigned long" and the MIPS concept of sign extending addresses conflict at times, so sometimes addresses want to be handled with alot of care.

Ralf

Maciej W. Rozycki

2007-11-08 15:27 UTC #4 lore raw

Quoted 5 lines · Ralf Baechle

Quoted 2 lines
I'm not sure if this is always a compile-time constant so that you can adorn it with a LL. However, note that this is not a cast, a cast is at runtime.
No. The compiler can evaluate the cast of a constant value at compile time and that exactly is what the code is exploiting here.

Except that some versions of GCC "forget" to stop a warning here as
irrelevant after the cast, hence the need for the stupid "#ifdef", sigh...

Quoted 16 lines

Quoted 7 lines
Quoted 7 lines
arch/mips/lib/uncached.c +1 -3
 	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
 		usp = CKSEG1ADDR(sp);
 #ifdef CONFIG_64BIT
-	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
-		 (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
-		usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
+	else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0, 0) &&
Quoted 8 lines
arch/mips/lib/uncached.c unsigned long __init run_uncached(void *func) · line 51
Quoted 3 lines
arch/mips/lib/uncached.c +2 -0
+		 (long long)sp < (long long)PHYS_TO_XKPHYS(8, 0))
+		usp = PHYS_TO_XKPHYS(K_CALG_UNCACHED,
 				     XKPHYS_TO_PHYS((long long)sp));
I'd say this code is broken in way too many aspects:
1. A plethora of casts. PHYS_TO_XKPHYS() should return a physical address (i.e. 32 or 64 bits unsigned integer) already, so casting its result should not be necessary.

Quoted 1 line

No argument about the beauty of the whole thing.

The casts were an attempt by myself to shut up GCC warning about
comparisons giving a predictable result because of a limited size of the data type used. Of course this is no longer true with "long long", but GCC does not care and warns regardless.

A possible workaround would be using auxiliary variables of the "long
long" type, but I recall it making GCC produce worse code for some cases. I can check it again, since it has been a while, and if a recent version of GCC produces reasonable code, then I can try to recheck this approach.

Please note this code changes quite wildly depending on the exact
configuration, so chances are GCC may warn with one, but not another.

Maciej

Ulrich Eckhardt

2007-11-02 08:07 UTC #5 lore raw

In reply to Ulrich Eckhardt

On Thursday 01 November 2007, Ulrich Eckhardt wrote:
[...]

Quoted 1 line

- slightly puzzled -

Andrew, Ralf, thanks for the clarifications!

Uli

Signature · 10 lines

Sator Laser GmbH
Geschäftsführer: Michael Wöhrmann, Amtsgericht Hamburg HR B62 932

**************************************************************************************
           Visit our website at <http://www.satorlaser.de/>
**************************************************************************************
Diese E-Mail einschließlich sämtlicher Anhänge ist nur für den Adressaten bestimmt und kann vertrauliche Informationen enthalten. Bitte benachrichtigen Sie den Absender umgehend, falls Sie nicht der beabsichtigte Empfänger sein sollten. Die E-Mail ist in diesem Fall zu löschen und darf weder gelesen, weitergeleitet, veröffentlicht oder anderweitig benutzt werden.
E-Mails können durch Dritte gelesen werden und Viren sowie nichtautorisierte Änderungen enthalten. Sator Laser GmbH ist für diese Folgen nicht verantwortlich.

**************************************************************************************

Ralf Baechle

2007-11-01 17:17 UTC #6 lore raw

In reply to Andrew Sharp

Quoted 3 lines · Andrew Sharp

Resend: I tried sending this a couple of days ago but haven't seen it. Wondering if it got stuck in a spam filter or our lovely exchange server or something.

It seems the list's spam filter has developed some appetite for patches, I'm afraid. Generally please cc me on patches.

Quoted 6 lines

Since all the callers of the PHYS_TO_XKPHYS macro call with a constant, put the cast to LL inside the macro where it really should be rather than in all the callers. This makes macros like PHYS_TO_XKSEG_UNCACHED work without gcc whining.
Hopefully this will apply ok.

I'm afraid, no, the definition of PHYS_TO_XKPHYS did change 3 weeks ago ...

Anyway, I fixed that up and queued it for 2.6.25.

Thanks,

Ralf