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

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

[Benjamin Herrenschmidt]

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@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
 	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
 			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
 	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)
 			/* 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;

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3756 bytes --]

Hi Benjamin,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.20-rc6 next-20181210]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Herrenschmidt/powerpc-Fix-bogus-usage-of-MSR_RI-on-BookE-and-40x/20181211-110017
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-ppa8548_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/sysdev/fsl_rio.c: In function 'fsl_rio_mcheck_exception':
>> arch/powerpc/sysdev/fsl_rio.c:115:17: error: 'MSR_RI' undeclared (first use in this function); did you mean 'MSR_BE'?
       regs->msr |= MSR_RI;
                    ^~~~~~
                    MSR_BE
   arch/powerpc/sysdev/fsl_rio.c:115:17: note: each undeclared identifier is reported only once for each function it appears in

vim +115 arch/powerpc/sysdev/fsl_rio.c

a52c8f52 Alexandre Bounine 2010-05-26   96  
ff33f182 Li Yang           2010-06-18   97  #ifdef CONFIG_E500
cce1f106 Shaohui Xie       2010-11-18   98  int fsl_rio_mcheck_exception(struct pt_regs *regs)
a52c8f52 Alexandre Bounine 2010-05-26   99  {
82a9a480 Scott Wood        2011-06-16  100  	const struct exception_table_entry *entry;
82a9a480 Scott Wood        2011-06-16  101  	unsigned long reason;
82a9a480 Scott Wood        2011-06-16  102  
82a9a480 Scott Wood        2011-06-16  103  	if (!rio_regs_win)
82a9a480 Scott Wood        2011-06-16  104  		return 0;
a52c8f52 Alexandre Bounine 2010-05-26  105  
a52c8f52 Alexandre Bounine 2010-05-26  106  	reason = in_be32((u32 *)(rio_regs_win + RIO_LTLEDCSR));
a52c8f52 Alexandre Bounine 2010-05-26  107  	if (reason & (RIO_LTLEDCSR_IER | RIO_LTLEDCSR_PRT)) {
a52c8f52 Alexandre Bounine 2010-05-26  108  		/* Check if we are prepared to handle this fault */
a52c8f52 Alexandre Bounine 2010-05-26  109  		entry = search_exception_tables(regs->nip);
a52c8f52 Alexandre Bounine 2010-05-26  110  		if (entry) {
a52c8f52 Alexandre Bounine 2010-05-26  111  			pr_debug("RIO: %s - MC Exception handled\n",
a52c8f52 Alexandre Bounine 2010-05-26  112  				 __func__);
a52c8f52 Alexandre Bounine 2010-05-26  113  			out_be32((u32 *)(rio_regs_win + RIO_LTLEDCSR),
a52c8f52 Alexandre Bounine 2010-05-26  114  				 0);
a52c8f52 Alexandre Bounine 2010-05-26 @115  			regs->msr |= MSR_RI;
61a92f70 Nicholas Piggin   2016-10-14  116  			regs->nip = extable_fixup(entry);
a52c8f52 Alexandre Bounine 2010-05-26  117  			return 1;
a52c8f52 Alexandre Bounine 2010-05-26  118  		}
a52c8f52 Alexandre Bounine 2010-05-26  119  	}
a52c8f52 Alexandre Bounine 2010-05-26  120  
cce1f106 Shaohui Xie       2010-11-18  121  	return 0;
a52c8f52 Alexandre Bounine 2010-05-26  122  }
cce1f106 Shaohui Xie       2010-11-18  123  EXPORT_SYMBOL_GPL(fsl_rio_mcheck_exception);
ff33f182 Li Yang           2010-06-18  124  #endif
a52c8f52 Alexandre Bounine 2010-05-26  125  

:::::: The code at line 115 was first introduced by commit
:::::: a52c8f521fed43bce53451d7dfddf2b42a2af689 rapidio, powerpc/85xx: Add MChk handler for SRIO port

:::::: TO: Alexandre Bounine <alexandre.bounine@idt.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 13138 bytes --]

Re: [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@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