A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org ---
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S =================================================================== --- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@
#ifdef CONFIG_PPC_64BITSUPPORT
+/* When running on ppc64, we cannot use rfi anymore. Let's try to + * catch which cpu we are running on and act accordingly. This is + * for 970s only. + */ +.macro do_rfi + mtsprg1 r3 + mfpvr r3 + rlwinm r3,r3,16,16,31 + cmplwi cr1,r3,0x0039 /* 970 CPUs */ + beq- cr1,0f + cmplwi cr1,r3,0x003C /* 970fx CPUs */ + beq- cr1,0f + cmplwi cr1,r3,0x0044 /* 970mp CPUs */ + beq- cr1,0f + mfsprg1 r3 + rfi +0: + mfsprg1 r3 + rfid +.endm + /* We're trying to use the same code for the ppc32 and ppc64 handlers here. * On ppc32 we only save/restore the registers, C considers volatile. * @@ -148,7 +169,7 @@ ll r31,(35 * ULONG_SIZE)(r1) ; \ .endif ; \ ll r1,(1 * ULONG_SIZE)(r1) ; /* restore stack at last */ \ - rfi + do_rfi
// PPC32
@@ -319,7 +340,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3 - RFI + do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 ) Index: openbios.git/arch/ppc/start.S =================================================================== --- openbios.git.orig/arch/ppc/start.S +++ openbios.git/arch/ppc/start.S @@ -25,6 +25,27 @@ #define ILLEGAL_VECTOR( v ) .org __vectors + v ; bl trap_error ; #define VECTOR( v, dummystr ) .org __vectors + v ; vector__##v
+/* When running on ppc64, we cannot use rfi anymore. Let's try to + * catch which cpu we are running on and act accordingly. This is + * for 970s only. + */ +.macro do_rfi + mtsprg1 r3 + mfpvr r3 + rlwinm r3,r3,16,16,31 + cmplwi cr1,r3,0x0039 /* 970 CPUs */ + beq- cr1,0f + cmplwi cr1,r3,0x003C /* 970fx CPUs */ + beq- cr1,0f + cmplwi cr1,r3,0x0044 /* 970mp CPUs */ + beq- cr1,0f + mfsprg1 r3 + rfi +0: + mfsprg1 r3 + rfid +.endm + #define EXCEPTION_PREAMBLE \ mtsprg1 r1 ; /* scratch */ \ mfsprg0 r1 ; /* exception stack in sprg0 */ \ @@ -252,7 +273,7 @@ exception_return: lwz r11,44(r1) lwz r12,48(r1) lwz r1,4(r1) // restore r1 - rfi + do_rfi
trap_error: mflr r3 @@ -287,7 +308,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3 - rfi + do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 )
On 20.06.16 09:34, Cédric Le Goater wrote:
A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S
--- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@
#ifdef CONFIG_PPC_64BITSUPPORT
+/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
Alex
- */
+.macro do_rfi
- mtsprg1 r3
- mfpvr r3
- rlwinm r3,r3,16,16,31
- cmplwi cr1,r3,0x0039 /* 970 CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x003C /* 970fx CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x0044 /* 970mp CPUs */
- beq- cr1,0f
- mfsprg1 r3
- rfi
+0:
- mfsprg1 r3
- rfid
+.endm
/* We're trying to use the same code for the ppc32 and ppc64 handlers here.
- On ppc32 we only save/restore the registers, C considers volatile.
@@ -148,7 +169,7 @@ ll r31,(35 * ULONG_SIZE)(r1) ; \ .endif ; \ ll r1,(1 * ULONG_SIZE)(r1) ; /* restore stack at last */ \
- rfi
- do_rfi
// PPC32
@@ -319,7 +340,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3
- RFI
- do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 ) Index: openbios.git/arch/ppc/start.S =================================================================== --- openbios.git.orig/arch/ppc/start.S +++ openbios.git/arch/ppc/start.S @@ -25,6 +25,27 @@ #define ILLEGAL_VECTOR( v ) .org __vectors + v ; bl trap_error ; #define VECTOR( v, dummystr ) .org __vectors + v ; vector__##v
+/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
- */
+.macro do_rfi
- mtsprg1 r3
- mfpvr r3
- rlwinm r3,r3,16,16,31
- cmplwi cr1,r3,0x0039 /* 970 CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x003C /* 970fx CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x0044 /* 970mp CPUs */
- beq- cr1,0f
- mfsprg1 r3
- rfi
+0:
- mfsprg1 r3
- rfid
+.endm
#define EXCEPTION_PREAMBLE \ mtsprg1 r1 ; /* scratch */ \ mfsprg0 r1 ; /* exception stack in sprg0 */ \ @@ -252,7 +273,7 @@ exception_return: lwz r11,44(r1) lwz r12,48(r1) lwz r1,4(r1) // restore r1
- rfi
- do_rfi
trap_error: mflr r3 @@ -287,7 +308,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3
- rfi
- do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 )
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S
--- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@
#ifdef CONFIG_PPC_64BITSUPPORT
+/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
I will try that. It would look better anyhow.
Thanks,
C.
Alex
- */
+.macro do_rfi
- mtsprg1 r3
- mfpvr r3
- rlwinm r3,r3,16,16,31
- cmplwi cr1,r3,0x0039 /* 970 CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x003C /* 970fx CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x0044 /* 970mp CPUs */
- beq- cr1,0f
- mfsprg1 r3
- rfi
+0:
- mfsprg1 r3
- rfid
+.endm
/* We're trying to use the same code for the ppc32 and ppc64 handlers here.
- On ppc32 we only save/restore the registers, C considers volatile.
@@ -148,7 +169,7 @@ ll r31,(35 * ULONG_SIZE)(r1) ; \ .endif ; \ ll r1,(1 * ULONG_SIZE)(r1) ; /* restore stack at last */ \
- rfi
- do_rfi
// PPC32
@@ -319,7 +340,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3
- RFI
- do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 ) Index: openbios.git/arch/ppc/start.S =================================================================== --- openbios.git.orig/arch/ppc/start.S +++ openbios.git/arch/ppc/start.S @@ -25,6 +25,27 @@ #define ILLEGAL_VECTOR( v ) .org __vectors + v ; bl trap_error ; #define VECTOR( v, dummystr ) .org __vectors + v ; vector__##v
+/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
- */
+.macro do_rfi
- mtsprg1 r3
- mfpvr r3
- rlwinm r3,r3,16,16,31
- cmplwi cr1,r3,0x0039 /* 970 CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x003C /* 970fx CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x0044 /* 970mp CPUs */
- beq- cr1,0f
- mfsprg1 r3
- rfi
+0:
- mfsprg1 r3
- rfid
+.endm
#define EXCEPTION_PREAMBLE \ mtsprg1 r1 ; /* scratch */ \ mfsprg0 r1 ; /* exception stack in sprg0 */ \ @@ -252,7 +273,7 @@ exception_return: lwz r11,44(r1) lwz r12,48(r1) lwz r1,4(r1) // restore r1
- rfi
- do_rfi
trap_error: mflr r3 @@ -287,7 +308,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3
- rfi
- do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 )
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S
--- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@
#ifdef CONFIG_PPC_64BITSUPPORT
+/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
C.
Alex
- */
+.macro do_rfi
- mtsprg1 r3
- mfpvr r3
- rlwinm r3,r3,16,16,31
- cmplwi cr1,r3,0x0039 /* 970 CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x003C /* 970fx CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x0044 /* 970mp CPUs */
- beq- cr1,0f
- mfsprg1 r3
- rfi
+0:
- mfsprg1 r3
- rfid
+.endm
/* We're trying to use the same code for the ppc32 and ppc64 handlers here.
- On ppc32 we only save/restore the registers, C considers volatile.
@@ -148,7 +169,7 @@ ll r31,(35 * ULONG_SIZE)(r1) ; \ .endif ; \ ll r1,(1 * ULONG_SIZE)(r1) ; /* restore stack at last */ \
- rfi
- do_rfi
// PPC32
@@ -319,7 +340,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3
- RFI
- do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 ) Index: openbios.git/arch/ppc/start.S =================================================================== --- openbios.git.orig/arch/ppc/start.S +++ openbios.git/arch/ppc/start.S @@ -25,6 +25,27 @@ #define ILLEGAL_VECTOR( v ) .org __vectors + v ; bl trap_error ; #define VECTOR( v, dummystr ) .org __vectors + v ; vector__##v
+/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
- */
+.macro do_rfi
- mtsprg1 r3
- mfpvr r3
- rlwinm r3,r3,16,16,31
- cmplwi cr1,r3,0x0039 /* 970 CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x003C /* 970fx CPUs */
- beq- cr1,0f
- cmplwi cr1,r3,0x0044 /* 970mp CPUs */
- beq- cr1,0f
- mfsprg1 r3
- rfi
+0:
- mfsprg1 r3
- rfid
+.endm
#define EXCEPTION_PREAMBLE \ mtsprg1 r1 ; /* scratch */ \ mfsprg0 r1 ; /* exception stack in sprg0 */ \ @@ -252,7 +273,7 @@ exception_return: lwz r11,44(r1) lwz r12,48(r1) lwz r1,4(r1) // restore r1
- rfi
- do_rfi
trap_error: mflr r3 @@ -287,7 +308,7 @@ VECTOR( 0x800, "FPU" ): ori r3,r3,0x2000 mtsrr1 r3 mfsprg1 r3
- rfi
- do_rfi
ILLEGAL_VECTOR( 0x900 ) ILLEGAL_VECTOR( 0xa00 )
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S
--- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@
#ifdef CONFIG_PPC_64BITSUPPORT
+/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Alex
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S
--- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@ #ifdef CONFIG_PPC_64BITSUPPORT +/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
Jeff/Stefan - completing the migration is becoming increasing important now, any news on an ETA?
ATB,
Mark.
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S
--- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@ #ifdef CONFIG_PPC_64BITSUPPORT +/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
OK. The first version applies fine. I pushed everything here :
https://github.com/legoater/openbios/commits/master
I am still looking at what Alex is asking for : a global variable doing live patching for rfi.
Cheers,
C.
Jeff/Stefan - completing the migration is becoming increasing important now, any news on an ETA?
ATB,
Mark.
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
A recent attempt to restrict the use of rfi to 64bit cpus in qemu broke 32bit OpenBIOS when run under a 970.
This is adding a dynamic check on the pvr to choose between rfi and rfid. Only the 970 family is supported.
Signed-off-by: Cédric Le Goater clg@kaod.org
Tested on qemu.
arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- arch/ppc/start.S | 25 +++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 4 deletions(-)
Index: openbios.git/arch/ppc/qemu/start.S
--- openbios.git.orig/arch/ppc/qemu/start.S +++ openbios.git/arch/ppc/qemu/start.S @@ -27,6 +27,27 @@ #ifdef CONFIG_PPC_64BITSUPPORT +/* When running on ppc64, we cannot use rfi anymore. Let's try to
- catch which cpu we are running on and act accordingly. This is
- for 970s only.
Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
OK. The first version applies fine. I pushed everything here :
https://github.com/legoater/openbios/commits/master
I am still looking at what Alex is asking for : a global variable doing live patching for rfi.
It's not both - it's either or :).
So either you define a global variable that we can just read from to determine whether we do rfi or rfid or we live patch. Looking at the code, it's probably easiest to live patch even. Just call a function from setup_mmu() that checks PVR and if it's 64bit, loops through (0x0 .. 0x1000) in 32bit steps and checks if *p == INST_RFI. If so, replace with INST_RFID and flush the cache.
Alex
On 06/20/2016 03:26 PM, Alexander Graf wrote:
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote: > A recent attempt to restrict the use of rfi to 64bit cpus in qemu > broke 32bit OpenBIOS when run under a 970. > > This is adding a dynamic check on the pvr to choose between rfi and > rfid. Only the 970 family is supported. > > Signed-off-by: Cédric Le Goater clg@kaod.org > --- > > Tested on qemu. > > arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- > arch/ppc/start.S | 25 +++++++++++++++++++++++-- > 2 files changed, 46 insertions(+), 4 deletions(-) > > Index: openbios.git/arch/ppc/qemu/start.S > =================================================================== > --- openbios.git.orig/arch/ppc/qemu/start.S > +++ openbios.git/arch/ppc/qemu/start.S > @@ -27,6 +27,27 @@ > #ifdef CONFIG_PPC_64BITSUPPORT > +/* When running on ppc64, we cannot use rfi anymore. Let's > try to > + * catch which cpu we are running on and act accordingly. This is > + * for 970s only. Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
OK. The first version applies fine. I pushed everything here :
https://github.com/legoater/openbios/commits/masterI am still looking at what Alex is asking for : a global variable doing live patching for rfi.
It's not both - it's either or :).
So either you define a global variable that we can just read from to determine whether we do rfi or rfid or we live patch. Looking at the code, it's probably easiest to live patch even. Just call a function from setup_mmu() that checks PVR and if it's 64bit, loops through (0x0 .. 0x1000) in 32bit steps and checks if *p == INST_RFI. If so, replace with INST_RFID and flush the cache.
... unless we're still running from ROM at that point. Then it gets tricky :).
Alex
On 06/20/2016 03:26 PM, Alexander Graf wrote:
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote: > A recent attempt to restrict the use of rfi to 64bit cpus in qemu > broke 32bit OpenBIOS when run under a 970. > > This is adding a dynamic check on the pvr to choose between rfi and > rfid. Only the 970 family is supported. > > Signed-off-by: Cédric Le Goater clg@kaod.org > --- > > Tested on qemu. > > arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- > arch/ppc/start.S | 25 +++++++++++++++++++++++-- > 2 files changed, 46 insertions(+), 4 deletions(-) > > Index: openbios.git/arch/ppc/qemu/start.S > =================================================================== > --- openbios.git.orig/arch/ppc/qemu/start.S > +++ openbios.git/arch/ppc/qemu/start.S > @@ -27,6 +27,27 @@ > #ifdef CONFIG_PPC_64BITSUPPORT > +/* When running on ppc64, we cannot use rfi anymore. Let's try to > + * catch which cpu we are running on and act accordingly. This is > + * for 970s only. Do you think you could move the check into the cpu init function and use a global variable / live patching instead? I don't remember if we need to handle any faults before we reach that path.
So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
OK. The first version applies fine. I pushed everything here :
https://github.com/legoater/openbios/commits/masterI am still looking at what Alex is asking for : a global variable doing live patching for rfi.
It's not both - it's either or :).
ah :) I need to grow my live patching skills.
So either you define a global variable that we can just read from to determine whether we do rfi or rfid
That is what I started doing and that was much bigger then v1.
or we live patch. Looking at the code, it's probably easiest to live patch even. Just call a function from setup_mmu() that checks PVR and if it's 64bit, loops through (0x0 .. 0x1000) in 32bit steps and checks if *p == INST_RFI. If so, replace with INST_RFID and flush the cache.
Indeed, the hunk below seems to do the trick :
@@ -524,6 +524,17 @@ setup_mmu(unsigned long ramsize) asm volatile("mtsrin %0,%1" :: "r" (sr_base + i), "r" (j)); } + if (is_ppc64()) { +#define RFI 0x4c000064 +#define RFID 0x4c000024 + unsigned long addr; + + for (addr = 0x2000; addr < 0x3000; addr += 4) { + unsigned long *ptr = (unsigned long *) addr; + if (*ptr == RFI) *ptr = RFID; + } + } + #endif
and that's permitted ? :)
flushing the cache does not seem necessary. I suppose because we did not go through this range of instructions yet, I am not sure how qemu handle the cache instructions. Need to check.
Thanks, I will bake a patch for it.
C.
Am 20.06.2016 um 17:42 schrieb Cédric Le Goater clg@kaod.org:
On 06/20/2016 03:26 PM, Alexander Graf wrote:
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote: > On 06/20/2016 09:39 AM, Alexander Graf wrote: >> On 20.06.16 09:34, Cédric Le Goater wrote: >> A recent attempt to restrict the use of rfi to 64bit cpus in qemu >> broke 32bit OpenBIOS when run under a 970. >> >> This is adding a dynamic check on the pvr to choose between rfi and >> rfid. Only the 970 family is supported. >> >> Signed-off-by: Cédric Le Goater clg@kaod.org >> --- >> >> Tested on qemu. >> >> arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- >> arch/ppc/start.S | 25 +++++++++++++++++++++++-- >> 2 files changed, 46 insertions(+), 4 deletions(-) >> >> Index: openbios.git/arch/ppc/qemu/start.S >> =================================================================== >> --- openbios.git.orig/arch/ppc/qemu/start.S >> +++ openbios.git/arch/ppc/qemu/start.S >> @@ -27,6 +27,27 @@ >> #ifdef CONFIG_PPC_64BITSUPPORT >> +/* When running on ppc64, we cannot use rfi anymore. Let's try to >> + * catch which cpu we are running on and act accordingly. This is >> + * for 970s only. > Do you think you could move the check into the cpu init function and use > a global variable / live patching instead? I don't remember if we need > to handle any faults before we reach that path. So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
OK. The first version applies fine. I pushed everything here :
https://github.com/legoater/openbios/commits/master
I am still looking at what Alex is asking for : a global variable doing live patching for rfi.
It's not both - it's either or :).
ah :) I need to grow my live patching skills.
So either you define a global variable that we can just read from to determine whether we do rfi or rfid
That is what I started doing and that was much bigger then v1.
or we live patch. Looking at the code, it's probably easiest to live patch even. Just call a function from setup_mmu() that checks PVR and if it's 64bit, loops through (0x0 .. 0x1000) in 32bit steps and checks if *p == INST_RFI. If so, replace with INST_RFID and flush the cache.
Indeed, the hunk below seems to do the trick :
@@ -524,6 +524,17 @@ setup_mmu(unsigned long ramsize) asm volatile("mtsrin %0,%1" :: "r" (sr_base + i), "r" (j)); }
- if (is_ppc64()) {
+#define RFI 0x4c000064 +#define RFID 0x4c000024
unsigned long addr;for (addr = 0x2000; addr < 0x3000; addr += 4) {unsigned long *ptr = (unsigned long *) addr;if (*ptr == RFI) *ptr = RFID;}- }
#endifand that's permitted ? :)
I don't see why not. But please make sure you run after the code got copied to phys 0 ;).
flushing the cache does not seem necessary. I suppose because we did not go through this range of instructions yet, I am not sure how qemu handle the cache instructions. Need to check.
It's not strictly necessary for tcg because that doesn't expose caching semantics and it's not really needed for pr kvm either because you patch a privileged inst with another privileged one. But it's "the correct thing to do" :).
Alex
Thanks, I will bake a patch for it.
C.
On 20.06.2016 17:42, Cédric Le Goater wrote:
On 06/20/2016 03:26 PM, Alexander Graf wrote:
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote: > On 20.06.16 09:34, Cédric Le Goater wrote: >> A recent attempt to restrict the use of rfi to 64bit cpus in qemu >> broke 32bit OpenBIOS when run under a 970. >> >> This is adding a dynamic check on the pvr to choose between rfi and >> rfid. Only the 970 family is supported. >> >> Signed-off-by: Cédric Le Goater clg@kaod.org >> --- >> >> Tested on qemu. >> >> arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- >> arch/ppc/start.S | 25 +++++++++++++++++++++++-- >> 2 files changed, 46 insertions(+), 4 deletions(-) >> >> Index: openbios.git/arch/ppc/qemu/start.S >> =================================================================== >> --- openbios.git.orig/arch/ppc/qemu/start.S >> +++ openbios.git/arch/ppc/qemu/start.S >> @@ -27,6 +27,27 @@ >> #ifdef CONFIG_PPC_64BITSUPPORT >> +/* When running on ppc64, we cannot use rfi anymore. Let's try to >> + * catch which cpu we are running on and act accordingly. This is >> + * for 970s only. > Do you think you could move the check into the cpu init function and use > a global variable / live patching instead? I don't remember if we need > to handle any faults before we reach that path. So we get an ISI really early, when returning from setup_mmu(), which is the first call in _entry.
Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
OK. The first version applies fine. I pushed everything here :
https://github.com/legoater/openbios/commits/masterI am still looking at what Alex is asking for : a global variable doing live patching for rfi.
It's not both - it's either or :).
ah :) I need to grow my live patching skills.
So either you define a global variable that we can just read from to determine whether we do rfi or rfid
That is what I started doing and that was much bigger then v1.
or we live patch. Looking at the code, it's probably easiest to live patch even. Just call a function from setup_mmu() that checks PVR and if it's 64bit, loops through (0x0 .. 0x1000) in 32bit steps and checks if *p == INST_RFI. If so, replace with INST_RFID and flush the cache.
Indeed, the hunk below seems to do the trick :
@@ -524,6 +524,17 @@ setup_mmu(unsigned long ramsize) asm volatile("mtsrin %0,%1" :: "r" (sr_base + i), "r" (j)); }
- if (is_ppc64()) {
+#define RFI 0x4c000064 +#define RFID 0x4c000024
unsigned long addr;for (addr = 0x2000; addr < 0x3000; addr += 4) {unsigned long *ptr = (unsigned long *) addr;if (*ptr == RFI) *ptr = RFID;
Could you please use "unsigned int" or "uint32_t" instead of unsigned long here? ... in case anybody every tries to compile this in 64-bit mode...
Thomas
On 06/20/2016 06:59 PM, Thomas Huth wrote:
On 20.06.2016 17:42, Cédric Le Goater wrote:
On 06/20/2016 03:26 PM, Alexander Graf wrote:
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote: > On 06/20/2016 09:39 AM, Alexander Graf wrote: >> On 20.06.16 09:34, Cédric Le Goater wrote: >>> A recent attempt to restrict the use of rfi to 64bit cpus in qemu >>> broke 32bit OpenBIOS when run under a 970. >>> >>> This is adding a dynamic check on the pvr to choose between rfi and >>> rfid. Only the 970 family is supported. >>> >>> Signed-off-by: Cédric Le Goater clg@kaod.org >>> --- >>> >>> Tested on qemu. >>> >>> arch/ppc/qemu/start.S | 25 +++++++++++++++++++++++-- >>> arch/ppc/start.S | 25 +++++++++++++++++++++++-- >>> 2 files changed, 46 insertions(+), 4 deletions(-) >>> >>> Index: openbios.git/arch/ppc/qemu/start.S >>> =================================================================== >>> --- openbios.git.orig/arch/ppc/qemu/start.S >>> +++ openbios.git/arch/ppc/qemu/start.S >>> @@ -27,6 +27,27 @@ >>> #ifdef CONFIG_PPC_64BITSUPPORT >>> +/* When running on ppc64, we cannot use rfi anymore. Let's try to >>> + * catch which cpu we are running on and act accordingly. This is >>> + * for 970s only. >> Do you think you could move the check into the cpu init function and use >> a global variable / live patching instead? I don't remember if we need >> to handle any faults before we reach that path. > So we get an ISI really early, when returning from setup_mmu(), which is > the first call in _entry. Ok, can we add the check there?
Bear in mind that the code for entry/context switching has been reworked pending commit whilst we switch the OpenBIOS repository over from SVN to git. The relevant commits are here: https://www.coreboot.org/pipermail/openbios/2016-May/009395.html.
So please consider OpenBIOS git master plus the patchset above to be the latest code.
OK. The first version applies fine. I pushed everything here :
https://github.com/legoater/openbios/commits/masterI am still looking at what Alex is asking for : a global variable doing live patching for rfi.
It's not both - it's either or :).
ah :) I need to grow my live patching skills.
So either you define a global variable that we can just read from to determine whether we do rfi or rfid
That is what I started doing and that was much bigger then v1.
or we live patch. Looking at the code, it's probably easiest to live patch even. Just call a function from setup_mmu() that checks PVR and if it's 64bit, loops through (0x0 .. 0x1000) in 32bit steps and checks if *p == INST_RFI. If so, replace with INST_RFID and flush the cache.
Indeed, the hunk below seems to do the trick :
@@ -524,6 +524,17 @@ setup_mmu(unsigned long ramsize) asm volatile("mtsrin %0,%1" :: "r" (sr_base + i), "r" (j)); }
- if (is_ppc64()) {
+#define RFI 0x4c000064 +#define RFID 0x4c000024
unsigned long addr;for (addr = 0x2000; addr < 0x3000; addr += 4) {unsigned long *ptr = (unsigned long *) addr;if (*ptr == RFI) *ptr = RFID;Could you please use "unsigned int" or "uint32_t" instead of unsigned long here? ... in case anybody every tries to compile this in 64-bit mode...
oh yes, this needs clean ups and more tests. I have your -prom-env jewel for that :)
Thanks,
C.
On Mon, 2016-06-20 at 17:42 +0200, Cédric Le Goater wrote:
flushing the cache does not seem necessary. I suppose because we did not go through this range of instructions yet, I am not sure how qemu handle the cache instructions. Need to check.
Thanks, I will bake a patch for it.
The cache might prefetch, you need to do the dcbf/sync/icbi/isync dance (it's not needed on modern processors but is on older ones so better safe than sorry in openbios).
You didn't see a problem also because qemu doesn't care ;-)
Cheers, Ben.
On 06/21/2016 04:18 AM, Benjamin Herrenschmidt wrote:
On Mon, 2016-06-20 at 17:42 +0200, Cédric Le Goater wrote:
flushing the cache does not seem necessary. I suppose because we did not go through this range of instructions yet, I am not sure how qemu handle the cache instructions. Need to check.
Thanks, I will bake a patch for it.
The cache might prefetch, you need to do the dcbf/sync/icbi/isync dance (it's not needed on modern processors but is on older ones so better safe than sorry in openbios).
You didn't see a problem also because qemu doesn't care ;-)
I see. I will add a flush_icache_range().
Thanks,
C.
On 21/06/16 07:09, Cédric Le Goater wrote:
On 06/21/2016 04:18 AM, Benjamin Herrenschmidt wrote:
On Mon, 2016-06-20 at 17:42 +0200, Cédric Le Goater wrote:
flushing the cache does not seem necessary. I suppose because we did not go through this range of instructions yet, I am not sure how qemu handle the cache instructions. Need to check.
Thanks, I will bake a patch for it.
The cache might prefetch, you need to do the dcbf/sync/icbi/isync dance (it's not needed on modern processors but is on older ones so better safe than sorry in openbios).
You didn't see a problem also because qemu doesn't care ;-)
I see. I will add a flush_icache_range().
Yes indeed. I think the other architectures are still playing catch-up here, however PPC already has a flush_icache_range() function in arch/ppc/qemu/start.S ready to go.
ATB,
Mark.
-
Alexander Graf -
Benjamin Herrenschmidt -
Cédric Le Goater -
Mark Cave-Ayland -
Thomas Huth