20 Jun
2016
20 Jun
'16
3:34 p.m.
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(a)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 )
20 Jun
20 Jun
3:39 p.m.
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(a)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 )
3:41 p.m.
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
I will try that. It would look better anyhow.
Thanks,
C.
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(a)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 )
>
5:32 p.m.
On 06/20/2016 09:39 AM, Alexander Graf wrote:
On 20.06.16 09:34, Cédric Le Goater wrote:
So we get an ISI really early, when returning from setup_mmu(), which is
the first call in _entry.
C.
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(a)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 )
>
5:33 p.m.
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
On 06/20/2016 09:39 AM, Alexander Graf wrote:
Ok, can we add the check there?
Alex
On 20.06.16 09:34, Cédric Le Goater wrote:
So we get an ISI really early,
when returning from setup_mmu(), which is
the first call in _entry. 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(a)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. 
5:49 p.m.
On 20/06/16 10:33, Alexander Graf wrote:
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
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 09:39 AM, Alexander Graf wrote:
Ok, can we add the check there?
On 20.06.16 09:34, Cédric Le Goater wrote:
So we get an ISI really early,
when returning from setup_mmu(), which is
the first call in _entry. 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(a)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.
8:59 p.m.
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
On 20/06/16 10:33, Alexander Graf wrote:
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.
On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
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. On 06/20/2016 09:39 AM, Alexander Graf wrote:
Ok, can we add the check there?
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(a)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. Jeff/Stefan - completing the migration is becoming
increasing important
now, any news on an ETA?
ATB,
Mark.
9:26 p.m.
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
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 20/06/16 10:33, Alexander Graf wrote:
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. On 06/20/2016 11:32 AM, Cédric Le Goater wrote:
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. 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(a)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?
9:27 p.m.
On 06/20/2016 03:26 PM, Alexander Graf wrote:
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
... unless we're still running from ROM at that point. Then it gets
tricky :).
Alex
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
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. On 20/06/16 10:33, Alexander Graf wrote:
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. 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(a)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.
11:42 p.m.
On 06/20/2016 03:26 PM, Alexander Graf wrote:
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
ah :) I need to grow my live patching skills.
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
It's not both - it's either or :). On 20/06/16 10:33, Alexander Graf wrote:
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. 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(a)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. 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.
21 Jun
21 Jun
12:36 a.m.
Am 20.06.2016 um 17:42 schrieb Cédric Le Goater
<clg(a)kaod.org>rg>:
I don't see why not. But please make sure you run after the code got copied to phys 0
;).
On 06/20/2016 03:26 PM, Alexander Graf wrote:
ah :) I need to grow my live patching skills.
On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
It's not both - it's either or :). 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(a)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. 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.
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.
12:59 a.m.
On 20.06.2016 17:42, Cédric Le Goater wrote:
On 06/20/2016 03:26 PM, Alexander Graf wrote:
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 02:59 PM, Cédric Le Goater wrote:
ah :) I need to grow my live patching skills.
On 06/20/2016 11:49 AM, Mark Cave-Ayland wrote:
It's not both - it's either or :). 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(a)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. 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;
5:32 a.m.
On 06/20/2016 06:59 PM, Thomas Huth wrote:
On 20.06.2016 17:42, Cédric Le Goater wrote:
oh yes, this needs clean ups and more tests. I have your -prom-env jewel for
that :)
Thanks,
C.
On 06/20/2016 03:26 PM, Alexander Graf wrote:
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... On 06/20/2016 02:59 PM, Cédric Le Goater wrote:
ah :) I need to grow my live patching skills.
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(a)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
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; 
10:18 a.m.
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.
2:09 p.m.
On 06/21/2016 04:18 AM, Benjamin Herrenschmidt wrote:
On Mon, 2016-06-20 at 17:42 +0200, Cédric Le Goater
wrote:
I see. I will add a flush_icache_range().
Thanks,
C.
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 ;-)
2:55 p.m.
On 21/06/16 07:09, Cédric Le Goater wrote:
On 06/21/2016 04:18 AM, Benjamin Herrenschmidt wrote:
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.
On Mon, 2016-06-20 at 17:42 +0200, Cédric Le
Goater wrote:
I see. I will add a flush_icache_range(). 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 ;-)
1505
days inactive
1506
days old
15 comments
5 participants
participants (5)
-
Alexander Graf -
Benjamin Herrenschmidt -
Cédric Le Goater -
Mark Cave-Ayland -
Thomas Huth