Re: [PATCH 2/2] powerpc/rtas: Fix RTAS MSR[HV] handling for Cell

On Wed, 2022-08-24 at 22:04 +1000, Michael Ellerman wrote:

Jordan Niethe [off-list ref] writes:

quoted

On Tue, 2022-08-23 at 21:59 +1000, Michael Ellerman wrote:

quoted

The semi-recent changes to MSR handling when entering RTAS (firmware)
cause crashes on IBM Cell machines. An example trace:

...

quoted

quoted

diff --git a/arch/powerpc/kernel/rtas_entry.S b/arch/powerpc/kernel/rtas_entry.S
index 9a434d42e660..6ce95ddadbcd 100644
--- a/arch/powerpc/kernel/rtas_entry.S
+++ b/arch/powerpc/kernel/rtas_entry.S

@@ -109,8 +109,12 @@ _GLOBAL(enter_rtas)
 	 * its critical regions (as specified in PAPR+ section 7.2.1). MSR[S]
 	 * is not impacted by RFI_TO_KERNEL (only urfid can unset it). So if
 	 * MSR[S] is set, it will remain when entering RTAS.
+	 * If we're in HV mode, RTAS must also run in HV mode, so extract MSR_HV
+	 * from the saved MSR value and insert into the value RTAS will use.
 	 */

Interestingly it looks like these are the first uses of these extended
mnemonics in the kernel?

We used to have at least one use I know of in TM code, but it's since
been converted to C.

quoted

quoted

+	extrdi	r0, r6, 1, 63 - MSR_HV_LG

Or in non-mnemonic form...
rldicl  r0, r6, 64 - MSR_HV_LG, 63

It's rldicl all the way down.

quoted

quoted

 	LOAD_REG_IMMEDIATE(r6, MSR_ME | MSR_RI)
+	insrdi	r6, r0, 1, 63 - MSR_HV_LG

Or in non-mnemonic form...
rldimi	r6, r0, MSR_HV_LG, 63 - MSR_HV_LG

I think the extended mnemonics are slightly more readable than the
open-coded versions?

Yeah definitely. I was just noting the plain instruction as I think we
have some existing patterns that may be potential candidates for conversion to the
extended version. Like in exceptions-64s.S

	rldicl. r0, r12, (64-MSR_TS_LG), (64-2) 
	to 
	extrdi. r0, r12, 2, 63 - MSR_TS_LG - 1

Would it be worth changing these?

quoted

It is ok to use r0 as a scratch register as it is loaded with 0 afterwards anyway.

I originally used r7, but r0 is more obviously safe.

quoted

quoted

 	li      r0,0
 	mtmsrd  r0,1                    /* disable RI before using SRR0/1 */

Reviewed-by: Jordan Niethe <redacted>

Thanks.

cheers