[PATCH] powerpc: Fix bogus usage of MSR_RI on BookE and 40x

[Christophe Leroy]

Le 11/12/2018 à 02:57, Benjamin Herrenschmidt a écrit :
> BookE and 40x processors lack the MSR:RI bit. However, we have a
> few common code places that rely on it.
> 
> This fixes it by not defining MSR_RI on those processor types and
> using the appropriate ifdef's in those locations.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> ---
>   arch/powerpc/include/asm/reg.h       | 2 ++
>   arch/powerpc/include/asm/reg_booke.h | 4 ++--
>   arch/powerpc/kernel/process.c        | 2 +-
>   arch/powerpc/kernel/traps.c          | 8 ++++++--
>   arch/powerpc/lib/sstep.c             | 2 ++
>   5 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index de52c31..41d0b2e 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -110,7 +110,9 @@
>   #ifndef MSR_PMM
>   #define MSR_PMM		__MASK(MSR_PMM_LG)	/* Performance monitor */
>   #endif
> +#if !defined(CONFIG_BOOKE) && !defined(CONFIG_4xx)
>   #define MSR_RI		__MASK(MSR_RI_LG)	/* Recoverable Exception */
> +#endif
>   #define MSR_LE		__MASK(MSR_LE_LG)	/* Little Endian */
>   
>   #define MSR_TM		__MASK(MSR_TM_LG)	/* Transactional Mem Available */
> diff --git a/arch/powerpc/include/asm/reg_booke.h b/arch/powerpc/include/asm/reg_booke.h
> index eb2a33d..06967f1 100644
> --- a/arch/powerpc/include/asm/reg_booke.h
> +++ b/arch/powerpc/include/asm/reg_booke.h
> @@ -46,10 +46,10 @@
>   #define MSR_USER32	(MSR_ | MSR_PR | MSR_EE)
>   #define MSR_USER64	(MSR_USER32 | MSR_64BIT)
>   #elif defined (CONFIG_40x)
> -#define MSR_KERNEL	(MSR_ME|MSR_RI|MSR_IR|MSR_DR|MSR_CE)
> +#define MSR_KERNEL	(MSR_ME|MSR_IR|MSR_DR|MSR_CE)
>   #define MSR_USER	(MSR_KERNEL|MSR_PR|MSR_EE)
>   #else
> -#define MSR_KERNEL	(MSR_ME|MSR_RI|MSR_CE)
> +#define MSR_KERNEL	(MSR_ME|MSR_CE)
>   #define MSR_USER	(MSR_KERNEL|MSR_PR|MSR_EE)
>   #endif
>   
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 96f3473..77679a7 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1359,7 +1359,7 @@ static struct regbit msr_bits[] = {
>   	{MSR_IR,	"IR"},
>   	{MSR_DR,	"DR"},
>   	{MSR_PMM,	"PMM"},
> -#ifndef CONFIG_BOOKE
> +#if !defined(CONFIG_BOOKE) && !defined(CONFIG_40x)
>   	{MSR_RI,	"RI"},
>   	{MSR_LE,	"LE"},
>   #endif
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 9a86572..2d00696 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -429,10 +429,11 @@ void system_reset_exception(struct pt_regs *regs)
>   	if (get_paca()->in_nmi > 1)
>   		nmi_panic(regs, "Unrecoverable nested System Reset");
>   #endif
> +#ifdef MSR_RI
>   	/* Must die if the interrupt is not recoverable */
>   	if (!(regs->msr & MSR_RI))
>   		nmi_panic(regs, "Unrecoverable System Reset");
> -
> +#endif

Could we have a helper in .h file instead of #ifdefs ?

For instance something like

#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
static inline bool exception_is_recoverable(u32 msr)
{
	return msr & MSR_RI;
}
#else
static inline bool exception_is_recoverable(u32 msr)
{
	return true;
}
#endif

Or reuse the function unrecoverable_excp() defined in xmon.c ?

>   	if (!nested)
>   		nmi_exit();
>   
> @@ -478,7 +479,9 @@ static inline int check_io_access(struct pt_regs *regs)
>   			printk(KERN_DEBUG "%s bad port %lx at %p\n",
>   			       (*nip & 0x100)? "OUT to": "IN from",
>   			       regs->gpr[rb] - _IO_BASE, nip);
> +#ifdef MSR_RI
>   			regs->msr |= MSR_RI;
> +#endif

Same here, a helper, something like that, to also be used in 
arch/powerpc/sysdev/fsl_rio.c ?

#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
static inline void exception_set_recoverable(struct pt_regs *regs)
{
	regs->msr |= MSR_RI;
}
#else
static inline void exception_set_recoverable(struct pt_regs *regs)
{
}
#endif

>   			regs->nip = extable_fixup(entry);
>   			return 1;
>   		}
> @@ -763,10 +766,11 @@ void machine_check_exception(struct pt_regs *regs)
>   	if (check_io_access(regs))
>   		goto bail;
>   
> +#ifdef MSR_RI
>   	/* Must die if the interrupt is not recoverable */
>   	if (!(regs->msr & MSR_RI))
>   		nmi_panic(regs, "Unrecoverable Machine check");
> -
> +#endif

Same

>   	if (!nested)
>   		nmi_exit();
>   
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index d81568f..c03c453 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -3059,9 +3059,11 @@ int emulate_step(struct pt_regs *regs, unsigned int instr)
>   
>   	case MTMSR:
>   		val = regs->gpr[op.reg];
> +#ifdef MSR_RI
>   		if ((val & MSR_RI) == 0)

Could be something like the following instead of this #ifdef stuff ?

		if (!IS_ENABLED(CONFIG_4xx) && !IS_ENABLED(CONFIG_BOOKE) && 
exception_is_recoverable(val))


>   			/* can't step mtmsr[d] that would clear MSR_RI */
>   			return -1;
> +#endif
>   		/* here op.val is the mask of bits to change */
>   		regs->msr = (regs->msr & ~op.val) | (val & op.val);
>   		goto instr_done;
> 


Christophe