Re: [PATCH v3 3/3] powerpc: Define and use MSR_RI only on non booke/40x

[Christophe Leroy]

Le 25/08/2021 à 06:54, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@xxxxxxxxxx> writes:
> > 40x and BOOKE don't have MSR_RI.
> >
> > Define MSR_RI only for platforms where it exists. For the other ones,
> > defines it as BUILD_BUG for C and do not define it for ASM.
> >
> > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> > ---
> > v3: Fixes kvm_emul.S and include <linux/bug.h> in <asm/reg.h>
> > ---
> >   arch/powerpc/include/asm/reg.h       |  5 +++++
> >   arch/powerpc/include/asm/reg_booke.h |  6 +++---
> >   arch/powerpc/kernel/head_32.h        |  4 ++++
> >   arch/powerpc/kernel/kvm_emul.S       | 13 +++++++++++++
> >   arch/powerpc/kernel/process.c        |  2 +-
> >   arch/powerpc/lib/sstep.c             |  2 +-
> >   6 files changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> > index be85cf156a1f..b270b570fb51 100644
> > --- a/arch/powerpc/include/asm/reg.h
> > +++ b/arch/powerpc/include/asm/reg.h
> > @@ -109,7 +109,12 @@
> >   #ifndef MSR_PMM
> >   #define MSR_PMM		__MASK(MSR_PMM_LG)	/* Performance monitor */
> >   #endif
> > +#if !defined(CONFIG_BOOKE) && !defined(CONFIG_40x)
> >   #define MSR_RI		__MASK(MSR_RI_LG)	/* Recoverable Exception */
>
> This breaks 64-bit BookE, which is using MSR_RI in bookehv_interrupts.S.
>
> eg. ppc64_book3e_allmodconfig gives:
>
>    arch/powerpc/kvm/bookehv_interrupts.S: Assembler messages:
>    arch/powerpc/kvm/bookehv_interrupts.S:221: Error: invalid operands (*ABS* and *UND* sections) for `|'
>    etc.

Oops, I missed that one. Should be easy to fix though as this file is dedicated to BOOKE we can just 
remove MSR_RI from there.

> ISA v2.07B says MSR_RI is Book3S only, but looking at the e500mc manual
> it does have bit 62 defined as RI.

Oh !

So it makes the story different, even different than what we have in kernel today where everything 
is more or less based on CONFIG_BOOKE or CONFIG_40x.

> I can fix it with:
>
> +#if !(defined(CONFIG_BOOKE) && !defined(CONFIG_PPC_BOOK3E)) && !defined(CONFIG_40x)
>   #define MSR_RI         __MASK(MSR_RI_LG)       /* Recoverable Exception */

Why CONFIG_PPC_BOOK3E ? Shouldn't it be CONFIG_PPC_E500MC instead ?

> But that's getting really ugly, and we'd have to repeat it elsewhere.
>
> I think we need a kconfig symbol that captures which platforms should
> use MSR_RI, something like:
>
>    CONFIG PPC_MSR_RI
>      def_bool y
>      depends on !40x && (!BOOKE || PPC_BOOK3E)


Yes I think that would be the best.


> Or maybe we should just define MSR_RI to 0 for platforms that don't use
> it, to avoid so much ifdefing?

Well, I tried to take the same approach as Ben in the original patch 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/61ad3646674e6bf541a8f7fb420cdf556d0b2152.camel@xxxxxxxxxxxxxxxxxxx/
but with a smoother approach for C files to avoid ifdefs there.

However for ASM it is different, we can't use tricks like BUILD_BUG. When we had MSR_RI completely 
undefined on all booke it was rather easy, only a few #ifdefs needed in parts common to 3S and 3E. 
But if we bring back MSR_RI on the e500mc that becomes more complex, and I agree with you too many 
#ifdefs will be unfriendly.

On the other hand, setting MSR_RI to 0 will make all tests that check msr & MSR_RI fails during run. 
Isn't it worse than what we have today ?

Maybe the way out is to carrefully look at the situations where e500mc uses MSR_RI, because 
according to the reference manual it is limited to a few situations:

4.4 Recoverability and MSR[RI]
MSR[RI] is an MSR (and save/restore register) storage bit for compatibility with pre-Book E PowerPC
processors. When an interrupt occurs, the recoverable interrupt bit, MSR[RI] is unchanged by the 
interrupt
mechanism when a new MSR is established; however, when a machine check, error report or NMI
interrupt occurs, MSR[RI] is cleared.
If used properly, RI determines whether an interrupt that is taken at the machine check interrupt 
vector can
be safely returned from (that is, that architected state set by the interrupt mechanism has been 
safely stored
by software). RI should be set by software when all MSR values are first established. When an interrupt
occurs that is taken at the machine check interrupt vector, software should set RI when it has 
safely stored
MCSRR0 and MCSRR1. The associated MCSRR1 bit should be checked to determine whether the
interrupt occurred when another machine check interrupt was being processed and before state was
successfully saved. If MCSRR1[RI] is set, it is safe to return when processing is complete.

So, let's think about it.

By the way patch 2 of the series could go now, it doesn't introduces anything new, it only cleans up 
things a bit.

Christophe