[PATCH] powerpc: Better setup of boot page TLB entry

[Trent Piepho]

On Sat, 22 Nov 2008, Milton Miller wrote:
> On Thu Nov 20 at 06:14:30 EST in 2008, Trent Piepho wrote:
>>  -       li      r7,0
>>  -       lis     r6,PAGE_OFFSET at h
>>  -       ori     r6,r6,PAGE_OFFSET at l
>>  -       rlwimi  r6,r7,0,20,31
>>  +       lis     r6,MAS2_VAL(PAGE_OFFSET, BOOKE_PAGESZ_64M, M_IF_SMP)@h
>>  +       ori     r6,r6,MAS2_VAL(PAGE_OFFSET, BOOKE_PAGESZ_64M, M_IF_SMP)@l
>
> I'm fine with this part, even if the expression is a bit long.  You might 
> consider using LOAD_REG_IMMEDIATE from asm/ppc_asm.h to avoid duplicating the 
> expression.

LOAD_REG_IMMEDIATE isn't used at all in that file, while lis/ori is used
many times.  In fact, there only one call of LOAD_REG_IMMEDIATE in all of
arch/powerpc/kernel/*.S.  So I think lis/ori is more easily recognized. 
And to be honest, I find switching syntax from assembly language
instructions to C style macros that generate instructions to be
aesthetically ugly.

It would be nice if the assembler provided a "liw" macro instruction that
would load an immediate.  When the assembler knows the immediate value, it
could even generate shorter sequences in some cases, like when the upper
16 bits are all zero.

>>  /* 7. Jump to KERNELBASE mapping */
>>  -       lis     r6,KERNELBASE at h
>>  -       ori     r6,r6,KERNELBASE at l
>>  -       rlwimi  r6,r7,0,20,31
>>  +       lis     r6,(KERNELBASE & ~0xfff)@h
>>  +       ori     r6,r6,(KERNELBASE & ~0xfff)@l
>
> Why do you need to mask off the bottom bits of KERNEL_BASE?  Just trying to 
> keep the instruction effect identical?

Yes, so it was clear I wasn't changing what the code did here.  And to
make it clear we only wanted the page number from KERNEL_BASE.  It's an
obvious expression and a compile time constant, merely saving a few
characters in the source doesn't seem worth much.  I realize it's
unnecessary since those bits get masked off in the wlwimi a few
instructions later.

Really all I wanted to fix the was memory coherency on SMP bug.  But the
code for MAS2 was stupid, so I had to change that to fix the bug in a
non-ugly way.  But then r7 didn't need to be zeroed and the (unnecessary)
"rlwimi r6,r7,0,20,31" would no longer be doing what's it's supposed to,
so I fixed that too.

> First of all, if its not aligned, then its likely other parts of the kernel 
> will break.  We could put a BUILD_BUG_ON somewhere (in c) if that check is 
> required.

I seems like "Require KERNEL_BASE to be page aligned and modify code to
depend on said requirement" belongs in another patch.

>>  -       addi    r6,r6,24
>>  +       addi    r6,r6,(2f - 1b)
>
> and while doing assembler math is better than the hand computed 24, how about 
> doing li r9,2f at l and just inserting that into r6?  Unless you expect step 8 
> to cross a page from the 1b label above.  But if you are that close to a page 
> boundary than assuming 1b is in the page at KERNEL_BASE would seem to be 
> suspect.
>
> For that matter, just do LOAD_ADDR(2f) or LOAD_REG_IMMEDIATE(2f), which would 
> give the same result, as KERNEL_BASE should be reflected the linked address 
> of the kernel (I assume that you are not doing dynamic runtime relocation 
> like ppc64 on the code up to here, otherwise the previous suggestion still 
> works).

I'm not sure if this code can be relocated or not.  If it isn't now, using
non-position independent code won't make it any easier to make it
relocatable.  It looks like the "bl 1f ; 1: mflr" sequence is used 13
times in arch/powerpc/kernel/*.S, I wonder if all of them are unnecessary?