[OpenBIOS] ppc64 MMU issues

[Andreas Färber]

Am 09.11.2010 um 12:53 schrieb Alexander Graf:

> On 08.11.2010, at 23:18, Andreas Färber wrote:
>
>> Am 08.11.2010 um 23:03 schrieb Alexander Graf:
>>
>>> On 08.11.2010, at 22:48, Andreas Färber wrote:
>>>
>>>> Am 08.11.2010 um 10:34 schrieb Mark Cave-Ayland:
>>>>
>>>>> Andreas Färber wrote:
>>>>>
>>>>>> Since r945 everything except for the trampoline issue should be  
>>>>>> in SVN.
>>>>>
>>>>> Cool! :)
>>>>
>>>> Ah, no! Forgot about the exception handlers (preamble,  
>>>> epilogue). :( I have local patches that #ifdef the whole  
>>>> definitions and provide alternative ones for ppc64. But I'd  
>>>> rather supply a 2-step patch to drop the template macros and use  
>>>> #ifdef'ery inside the macro definition for ppc64/ppc differences,  
>>>> given that long-term we won't need the second version.
>>>
>>> Are you sure you need that? Most of the stuff should be generic.  
>>> Please take a look at Linux's PPC_LL macro for example. The stack  
>>> layout varies slightly, but that should also be reasonably easy to  
>>> catch.
>>
>> Problematic parts are the ULONG_SIZE comparisons for the number of  
>> registers that we can't #ifdef away inside an CPP macro
>
> You're talking about this piece of code, right?
>
> .ifc ULONG_SIZE, 8 ; \
>        addi    r1,r1,-(40 * ULONG_SIZE) ;      /* push exception  
> frame */ \
> .else ; \
>        addi    r1,r1,-(20 * ULONG_SIZE) ;      /* push exception  
> frame */ \
> .endif ; \
> \
>
>
> Just replace it with
>
> // PPC32
>
> #define EXCEPTION_STACK_LEN (20 * 4)
>
> // PPC64
>
> #define EXCEPTION_STACK_LEN (40 * 8)
>
> // code
>
> addi  r1, r1,-EXCEPTION_STACK_LEN

No, no, no! This is exactly the type of differences I wanted to rip  
out, but if I do so now I would break ppc64 support.
On __powerpc64__ this doesn't need to be 40, and it's not just the  
above immediate but the whole .ifc around the 20 additional register  
stores/loads:

#define EXCEPTION_PREAMBLE_TEMPLATE \
	...
	stl	r11,(11 * ULONG_SIZE)(r1) ; \
	stl	r12,(12 * ULONG_SIZE)(r1) ; \
.ifc ULONG_SIZE, 8 ; \
	stl	r13,(17 * ULONG_SIZE)(r1) ; \
	stl	r14,(18 * ULONG_SIZE)(r1) ; \
	...

.macro EXCEPTION_PREAMBLE
	EXCEPTION_PREAMBLE_TEMPLATE
.endm
...
.macro EXCEPTION_PREAMBLE_64
	EXCEPTION_PREAMBLE_TEMPLATE
.endm

My problem is the reuse of the _TEMPLATE macros. It becomes a matter  
of macros such as those you suggest if we can get rid of that.

> Btw, are you sure this code is correct? Usually the stack frame  
> includes the old r1 value as well. In __kvmppc_vcpu_entry (arch/ 
> powerpc/kvm/book3s_interrupts.S) I have working code that basically  
> does an ABI compliant function in asm:
>
>        /* Save host state to the stack */
>        PPC_STLU r1, -SWITCH_FRAME_SIZE(r1)

Haven't checked. I spotted an instance where two of the three stacks  
have a potential stack frame overlap though.

>> , plus the m*msr
>
>        mfmsr   r1 ;                            /* unset MSR_SF */ \
>        clrlwi  r1,r1,0 ; \
>        mtmsr   r1 ; \
>
> This one? Yeah, my awesome broken code. Just replace it with:
>
> // PPC32
>
> #define MFMSRD mfmsr
> #define MTMSRD mtmsr
>
> // PPC64
>
> #define MFMSRD mfmsrd
> #define MTMSRD mtmsrd
>
> // code
>
> MFMSRD r1
> MTMSRD r1
>
>
> The clearing is not necessary. If it is, qemu or kvm are broken.  
> According to the spec, mfmsr and mtmsr only handle the first 32 bits  
> of MSR.

I've been working without on ppc64, MSR looked okay.

>> and the trailing/leading stack manipulation (16 in SVR4 vs. 48 in  
>> PowerOpen ABI).
>
> Not sure I understand exactly what you mean here :).

addi r1, r1, -x
addi r1, r1,  x
:)

Btw I still don't understand what ABI this is supposed to be. If I can  
trust Hollis' ppc assembler article and the Mono and QEMU ppc64 ports,  
then Linux/ppc64 uses function descriptors. But our ppc64 assembler  
code calling setup_mmu() and entry() works without a function  
descriptor dereference...

>> Also rfid, both here and for the FPU vector.
>
> // PPC32
>
> #define RFID rfi
>
> // PPC64
>
> #define RFID rfid
>
> should work, no?

Yeah, if that's the naming convention we want to go with.

> Btw, you should probably define those helpers somewhere generic  
> depending on #ifdef __powerpc64__
>
>> Note that the other ppc targets don't use assembler macros, just  
>> preprocessor macros, so we have kind of a double-macro situation. ;)
>
> Yes, please don't use assembler macros. If I did that anywhere, I  
> did it wrong :). preprocessor macros are the way to go! :)

Both have their advantages imo when used correctly.

Andreas