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

[Michael Ellerman <mpe@ellerman.id.au>]

Jordan Niethe <jniethe5@gmail.com> writes:
> 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?

Some folks are very comfortable with rldicl and probably prefer the
former, but I'm not sure there's many of those people around anymore :)

I think the extrdi is a bit more readable.

You could use MSR_TS_T_LG to avoid the - 1? All those uses have a
comment about it being 2 bits already.

cheers