[PATCH] Put cast inside macro instead of all the callers

[PATCH] Put cast inside macro instead of all the callers

[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.

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 <andy.sharp@onstor.com>
---
 arch/mips/lib/uncached.c     |   12 ++++++------
 include/asm-mips/addrspace.h |    2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/mips/lib/uncached.c b/arch/mips/lib/uncached.c
index 2388f7f..bbca1ea 100644
--- a/arch/mips/lib/uncached.c
+++ b/arch/mips/lib/uncached.c
@@ -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 {
diff --git a/include/asm-mips/addrspace.h b/include/asm-mips/addrspace.h
index 964c5ed..1bc23e8 100644
--- a/include/asm-mips/addrspace.h
+++ b/include/asm-mips/addrspace.h
@@ -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)					\
-- 
1.4.4.4

Re: [PATCH] Put cast inside macro instead of all the callers

[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!

On Wednesday 31 October 2007, Andrew Sharp wrote:
> 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.

>  	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!

>  #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))

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

Uli
- slightly puzzled -

-- 
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.

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

Re: [PATCH] Put cast inside macro instead of all the callers

[Ralf Baechle]

On Wed, Oct 31, 2007 at 02:11:24PM -0700, Andrew Sharp wrote:

> 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.

> 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

Re: [PATCH] Put cast inside macro instead of all the callers

[Andrew Sharp]

On Thu, 1 Nov 2007 17:04:01 +0100 Ulrich Eckhardt
<eckhardt@satorlaser.com> wrote:

> 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:
> > 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.

> >  	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!
> 
> >  #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))
> 
> 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,

a

Re: [PATCH] Put cast inside macro instead of all the callers

[Ralf Baechle]

On Thu, Nov 01, 2007 at 05:04:01PM +0100, Ulrich Eckhardt wrote:

> 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.

> On Wednesday 31 October 2007, Andrew Sharp wrote:
> > 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.

> >  	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.

No argument about the beauty of the whole thing.

> 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.

> 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.

> >  #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))
> 
> 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

Re: [PATCH] Put cast inside macro instead of all the callers

[Ulrich Eckhardt]

On Thursday 01 November 2007, Ulrich Eckhardt wrote:
[...]
> - slightly puzzled -

Andrew, Ralf, thanks for the clarifications!

Uli

-- 
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.

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

Re: [PATCH] Put cast inside macro instead of all the callers

[Maciej W. Rozycki]

On Thu, 1 Nov 2007, Ralf Baechle wrote:

> > 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...

> > >  	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.
> 
> 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