Message ID | 61ad3646674e6bf541a8f7fb420cdf556d0b2152.camel@kernel.crashing.org (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | powerpc: Fix bogus usage of MSR_RI on BookE and 40x | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
snowpatch_ozlabs/build-ppc64le | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64be | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64e | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-pmac32 | success | build succeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 4 checks, 73 lines checked |
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
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
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;
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(-)