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

[Jordan Niethe <jniethe5@gmail.com>]

On Wed, 2022-08-24 at 22:04 +1000, Michael Ellerman wrote:
> Jordan Niethe <jniethe5@gmail.com> writes:
> > On Tue, 2022-08-23 at 21:59 +1000, Michael Ellerman wrote:
> > > The semi-recent changes to MSR handling when entering RTAS (firmware)
> > > cause crashes on IBM Cell machines. An example trace:
> ...
> > > 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.
> 
> > > +	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.
> 
> > >  	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?

> 
> > 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.
> 
> > >  	li      r0,0
> > >  	mtmsrd  r0,1                    /* disable RI before using SRR0/1 */
> > 
> > Reviewed-by: Jordan Niethe <jniethe5@gmail.com>
> 
> Thanks.
> 
> cheers