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
