[commit] r1043 - trunk/openbios-devel/arch/ppc/qemu

[repository service]

Author: agraf
Date: Fri Jun 17 02:55:02 2011
New Revision: 1043
URL: http://tracker.coreboot.org/trac/openbios/changeset/1043

Log:
ppc: fix mtmsr for SF setting

When running openbios-ppc32 on a ppc64 VM, we need to unset SF on every
interrupt to ensure that things still work.

So far we've been using mtmsr for this operation, but according to the
spec mtmsr doesn't set any bit above 32bit, so we were merely exploiting
a bug in qemu before.

This patch adds a runtime check on MSR_SF to see if we're running on a 64-bit
capable CPU. If so, we use mtmsrd, which can set the high 32bits of MSR.

CC: Andreas Färber <[e-mail]>
CC: Segher Boessenkool <[e-mail]>
Signed-off-by: Alexander Graf <[e-mail]>

---

v1 -> v2:

  - use segher's awesome MSR_SF detection

Modified:
   trunk/openbios-devel/arch/ppc/qemu/start.S

Modified: trunk/openbios-devel/arch/ppc/qemu/start.S
==============================================================================
--- trunk/openbios-devel/arch/ppc/qemu/start.S	Fri Jun 17 02:55:00 2011	(r1042)
+++ trunk/openbios-devel/arch/ppc/qemu/start.S	Fri Jun 17 02:55:02 2011	(r1043)
@@ -37,9 +37,16 @@
 
 #define EXCEPTION_PREAMBLE_TEMPLATE \
 	mtsprg1	r1 ;				/* scratch */ \
+	mfcr	r1 ; \
+	mtsprg2	r1 ;				/* scratch */ \
+	lis	r1, 0x8000 ;			/* r1=0x80000000 */ \
+	add.	r1,r1,r1 ;			/* r1=r1+r1 (high 32bit !0) */ \
+	beq	1f; \
+	\
 	mfmsr	r1 ;				/* unset MSR_SF */ \
 	clrlwi	r1,r1,0 ; \
-	mtmsr	r1 ; \
+	mtmsrd	r1 ; \
+1: \
 	mfsprg0	r1 ;				/* exception stack in sprg0 */ \
 .ifc ULONG_SIZE, 8 ; \
 	addi	r1,r1,-(40 * ULONG_SIZE) ;	/* push exception frame */ \
@@ -85,7 +92,7 @@
  \
 	mflr	r0 ; \
 	stl	r0,(13 * ULONG_SIZE)(r1) ; \
-	mfcr	r0 ; \
+	mfsprg2	r0 ; \
 	stl	r0,(14 * ULONG_SIZE)(r1) ; \
 	mfctr	r0 ; \
 	stl	r0,(15 * ULONG_SIZE)(r1) ; \
@@ -301,14 +308,12 @@
 ILLEGAL_VECTOR( 0x200 )
 
 VECTOR( 0x300, "DSI" ):
-	EXCEPTION_PREAMBLE
-	b	call_dsi_exception
+	b	real_dsi
 
 ILLEGAL_VECTOR( 0x380 )
 
 VECTOR( 0x400, "ISI" ):
-	EXCEPTION_PREAMBLE
-	b	call_isi_exception
+	b	real_isi
 
 ILLEGAL_VECTOR( 0x480 )
 
@@ -359,6 +364,14 @@
 
 #endif
 
+real_dsi:
+	EXCEPTION_PREAMBLE
+	b	call_dsi_exception
+
+real_isi:
+	EXCEPTION_PREAMBLE
+	b	call_isi_exception
+
 GLOBL(__vectors_end):
 
 /************************************************************************/
@@ -368,10 +381,21 @@
 GLOBL(_entry):
 
 #ifdef CONFIG_PPC_64BITSUPPORT
-	/* clear MSR, disable MMU */
-
 	li	r0,0
+
+	lis	r3, 0x8000			/* r1=0x80000000 */
+	add.	r3,r3,r3			/* r1=r1+r1 (high 32bit !0) */
+	beq	no_64bit			/* only true when !MSR_SF */
+
+	/* clear MSR, disable MMU, SF */
+	mtmsrd	r0
+	b	real_entry
+
+no_64bit:
+	/* clear MSR, disable MMU */
 	mtmsr	r0
+
+real_entry:
 #endif
 
 	/* copy exception vectors */

[Andreas Färber]

Am 17.06.2011 um 02:55 schrieb repository service:
> Author: agraf
> Date: Fri Jun 17 02:55:02 2011
> New Revision: 1043
> URL: http://tracker.coreboot.org/trac/openbios/changeset/1043
>
> Log:
> ppc: fix mtmsr for SF setting
>
> When running openbios-ppc32 on a ppc64 VM, we need to unset SF on  
> every
> interrupt to ensure that things still work.
>
> So far we've been using mtmsr for this operation, but according to the
> spec mtmsr doesn't set any bit above 32bit, so we were merely  
> exploiting
> a bug in qemu before.
>
> This patch adds a runtime check on MSR_SF to see if we're running on  
> a 64-bit
> capable CPU. If so, we use mtmsrd, which can set the high 32bits of  
> MSR.
>
> CC: Andreas Färber <[e-mail]>
> CC: Segher Boessenkool <[e-mail]>
> Signed-off-by: Alexander Graf <[e-mail]>
>
> ---
>
> v1 -> v2:
>
> - use segher's awesome MSR_SF detection
>
> Modified:
>   trunk/openbios-devel/arch/ppc/qemu/start.S
>
> Modified: trunk/openbios-devel/arch/ppc/qemu/start.S
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- trunk/openbios-devel/arch/ppc/qemu/start.S	Fri Jun 17 02:55:00  
> 2011	(r1042)
> +++ trunk/openbios-devel/arch/ppc/qemu/start.S	Fri Jun 17 02:55:02  
> 2011	(r1043)
> @@ -37,9 +37,16 @@
>
> #define EXCEPTION_PREAMBLE_TEMPLATE \
>   mtsprg1	r1 ;				/* scratch */ \
> +	mfcr	r1 ; \
> +	mtsprg2	r1 ;				/* scratch */ \
> +	lis	r1, 0x8000 ;			/* r1=0x80000000 */ \
> +	add.	r1,r1,r1 ;			/* r1=r1+r1 (high 32bit !0) */ \
> +	beq	1f; \
> +	\

This is more readable than the previous version, thanks.
> mfmsr	r1 ;				/* unset MSR_SF */ \
>   clrlwi	r1,r1,0 ; \
> -	mtmsr	r1 ; \
> +	mtmsrd	r1 ; \
> +1: \
>   mfsprg0	r1 ;				/* exception stack in sprg0 */ \
> .ifc ULONG_SIZE, 8 ; \
>   addi	r1,r1,-(40 * ULONG_SIZE) ;	/* push exception frame */ \
> @@ -85,7 +92,7 @@
>  \
>   mflr	r0 ; \
>   stl	r0,(13 * ULONG_SIZE)(r1) ; \
> -	mfcr	r0 ; \
> +	mfsprg2	r0 ; \
>   stl	r0,(14 * ULONG_SIZE)(r1) ; \
>   mfctr	r0 ; \
>   stl	r0,(15 * ULONG_SIZE)(r1) ; \
> @@ -301,14 +308,12 @@
> ILLEGAL_VECTOR( 0x200 )
>
> VECTOR( 0x300, "DSI" ):
> -	EXCEPTION_PREAMBLE
> -	b	call_dsi_exception
> +	b	real_dsi
>
> ILLEGAL_VECTOR( 0x380 )
>
> VECTOR( 0x400, "ISI" ):
> -	EXCEPTION_PREAMBLE
> -	b	call_isi_exception
> +	b	real_isi
>
> ILLEGAL_VECTOR( 0x480 )
>
> @@ -359,6 +364,14 @@
>
> #endif
>
> +real_dsi:
> +	EXCEPTION_PREAMBLE
> +	b	call_dsi_exception
> +
> +real_isi:
> +	EXCEPTION_PREAMBLE
> +	b	call_isi_exception

I don't like the naming "real_xxx".

Have you checked that the serial output still has the same vector  
number (+4)?

Andreas
> +
> GLOBL(__vectors_end):
>
> / 
> ************************************************************************/
> @@ -368,10 +381,21 @@
> GLOBL(_entry):
>
> #ifdef CONFIG_PPC_64BITSUPPORT
> -	/* clear MSR, disable MMU */
> -
>   li	r0,0
> +
> +	lis	r3, 0x8000			/* r1=0x80000000 */
> +	add.	r3,r3,r3			/* r1=r1+r1 (high 32bit !0) */
> +	beq	no_64bit			/* only true when !MSR_SF */
> +
> +	/* clear MSR, disable MMU, SF */
> +	mtmsrd	r0
> +	b	real_entry
> +
> +no_64bit:
> +	/* clear MSR, disable MMU */
>   mtmsr	r0
> +
> +real_entry:
> #endif
>
> /* copy exception vectors */

[Alexander Graf]

On 17.06.2011, at 21:51, Andreas Färber wrote:
> Am 17.06.2011 um 02:55 schrieb repository service:
> > Author: agraf
> > Date: Fri Jun 17 02:55:02 2011
> > New Revision: 1043
> > URL: http://tracker.coreboot.org/trac/openbios/changeset/1043
> >
> > Log:
> > ppc: fix mtmsr for SF setting
> >
> > When running openbios-ppc32 on a ppc64 VM, we need to unset SF on every
> > interrupt to ensure that things still work.
> >
> > So far we've been using mtmsr for this operation, but according to the
> > spec mtmsr doesn't set any bit above 32bit, so we were merely exploiting
> > a bug in qemu before.
> >
> > This patch adds a runtime check on MSR_SF to see if we're running on a 64-bit
> > capable CPU. If so, we use mtmsrd, which can set the high 32bits of MSR.
> >
> > CC: Andreas Färber <[e-mail]>
> > CC: Segher Boessenkool <[e-mail]>
> > Signed-off-by: Alexander Graf <[e-mail]>
> >
> > ---
> >
> > v1 -> v2:
> >
> > - use segher's awesome MSR_SF detection
> >
> > Modified:
> >  trunk/openbios-devel/arch/ppc/qemu/start.S
> >
> > Modified: trunk/openbios-devel/arch/ppc/qemu/start.S
> > ==============================================================================
> > --- trunk/openbios-devel/arch/ppc/qemu/start.S	Fri Jun 17 02:55:00 2011	(r1042)
> > +++ trunk/openbios-devel/arch/ppc/qemu/start.S	Fri Jun 17 02:55:02 2011	(r1043)
> > @@ -37,9 +37,16 @@
> >
> > #define EXCEPTION_PREAMBLE_TEMPLATE \
> >   mtsprg1	r1 ;				/* scratch */ \
> > +	mfcr	r1 ; \
> > +	mtsprg2	r1 ;				/* scratch */ \
> > +	lis	r1, 0x8000 ;			/* r1=0x80000000 */ \
> > +	add.	r1,r1,r1 ;			/* r1=r1+r1 (high 32bit !0) */ \
> > +	beq	1f; \
> > +	\
>
> This is more readable than the previous version, thanks.

Yeah, I prefer that version by a lot as well :).
> > mfmsr	r1 ;				/* unset MSR_SF */ \
> >   clrlwi	r1,r1,0 ; \
> > -	mtmsr	r1 ; \
> > +	mtmsrd	r1 ; \
> > +1: \
> >   mfsprg0	r1 ;				/* exception stack in sprg0 */ \
> > .ifc ULONG_SIZE, 8 ; \
> >   addi	r1,r1,-(40 * ULONG_SIZE) ;	/* push exception frame */ \
> > @@ -85,7 +92,7 @@
> > \
> >   mflr	r0 ; \
> >   stl	r0,(13 * ULONG_SIZE)(r1) ; \
> > -	mfcr	r0 ; \
> > +	mfsprg2	r0 ; \
> >   stl	r0,(14 * ULONG_SIZE)(r1) ; \
> >   mfctr	r0 ; \
> >   stl	r0,(15 * ULONG_SIZE)(r1) ; \
> > @@ -301,14 +308,12 @@
> > ILLEGAL_VECTOR( 0x200 )
> >
> > VECTOR( 0x300, "DSI" ):
> > -	EXCEPTION_PREAMBLE
> > -	b	call_dsi_exception
> > +	b	real_dsi
> >
> > ILLEGAL_VECTOR( 0x380 )
> >
> > VECTOR( 0x400, "ISI" ):
> > -	EXCEPTION_PREAMBLE
> > -	b	call_isi_exception
> > +	b	real_isi
> >
> > ILLEGAL_VECTOR( 0x480 )
> >
> > @@ -359,6 +364,14 @@
> >
> > #endif
> >
> > +real_dsi:
> > +	EXCEPTION_PREAMBLE
> > +	b	call_dsi_exception
> > +
> > +real_isi:
> > +	EXCEPTION_PREAMBLE
> > +	b	call_isi_exception
>
> I don't like the naming "real_xxx".

Feel free to just commit a patch that changes the naming.
> Have you checked that the serial output still has the same vector number (+4)?

Which serial output? This is really only here because the exception preamble became > 0x80 bytes.


Alex

[Andreas Färber]

Am 18.06.2011 um 01:03 schrieb Alexander Graf:
> On 17.06.2011, at 21:51, Andreas Färber wrote:
> > Am 17.06.2011 um 02:55 schrieb repository service:
> > > +real_dsi:
> > > +	EXCEPTION_PREAMBLE
> > > +	b	call_dsi_exception
> > > +
> > > +real_isi:
> > > +	EXCEPTION_PREAMBLE
> > > +	b	call_isi_exception
> >
> > I don't like the naming "real_xxx".
>
> Feel free to just commit a patch that changes the naming.

The way I see it, we no longer need call_*si_exception (which are  
located in front of the vectors) or could name real_*  
call_*_exception, to avoid another redirection. Even drop space- 
optimized exception_return if we like.
> > Have you checked that the serial output still has the same vector  
> > number (+4)?
>
> Which serial output? This is really only here because the exception  
> preamble became > 0x80 bytes.

Sorry, doesn't apply here. I was thinking of unexpected_excep(), which  
outputs the vector number (unfortunately offset by 4), then loops  
endlessly. DSI and ISI don't go into that code path, obviously. ;)

Andreas

[Alexander Graf]

On 18.06.2011, at 13:11, Andreas Färber wrote:
> Am 18.06.2011 um 01:03 schrieb Alexander Graf:
> > On 17.06.2011, at 21:51, Andreas Färber wrote:
> > > Am 17.06.2011 um 02:55 schrieb repository service:
> > > > +real_dsi:
> > > > +	EXCEPTION_PREAMBLE
> > > > +	b	call_dsi_exception
> > > > +
> > > > +real_isi:
> > > > +	EXCEPTION_PREAMBLE
> > > > +	b	call_isi_exception
> > >
> > > I don't like the naming "real_xxx".
> >
> > Feel free to just commit a patch that changes the naming.
>
> The way I see it, we no longer need call_*si_exception (which are located in front of the vectors) or could name real_* call_*_exception, to avoid another redirection. Even drop space-optimized exception_return if we like.

Yup. Is it worth the hassle? :) Direct branches are fast, so the only thing it'd help with is code readability. Does the code become much more readable then? Not sure. Either way, I'm fairly indifferent on this - it's a very very minor issue IMHO. If you feel that it's unpretty, please just change it.


Alex