| Message ID | 0d5b12183d5176dd702d29ad94c39c384e51c78f.1638208156.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v5,1/5] powerpc/inst: Refactor ___get_user_instr() | expand |
Hi Christophe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.16-rc3 next-20211129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from arch/powerpc/kernel/asm-offsets.c:71:
In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
*inst = ppc_inst(val);
^~~
arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
#define ppc_inst(x) (x)
^
arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
unsigned int val, suffix;
^
= 0
1 warning generated.
<stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
#warning syscall futex_waitv not implemented
^
1 warning generated.
arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs'
.stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x:
^
arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs'
.stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x:
^
make[2]: *** [arch/powerpc/kernel/vdso32/Makefile:55: arch/powerpc/kernel/vdso32/gettimeofday.o] Error 1
make[2]: Target 'include/generated/vdso32-offsets.h' not remade because of errors.
make[1]: *** [arch/powerpc/Makefile:421: vdso_prepare] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:219: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +/val +165 arch/powerpc/include/asm/inst.h
152
153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
154 {
155 unsigned int val, suffix;
156
157 if (unlikely(!is_kernel_addr((unsigned long)src)))
158 return -ERANGE;
159
160 __get_kernel_nofault(&val, src, u32, Efault);
161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
163 *inst = ppc_inst_prefix(val, suffix);
164 } else {
> 165 *inst = ppc_inst(val);
166 }
167 return 0;
168 Efault:
169 return -EFAULT;
170 }
171
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Le 29/11/2021 à 23:55, kernel test robot a écrit : > Hi Christophe, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on powerpc/next] > [also build test WARNING on v5.16-rc3 next-20211129] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next > config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config) > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install powerpc cross compiling tool for clang build > # apt-get install binutils-powerpc-linux-gnu > # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > # save the config file to linux build tree > mkdir build_dir > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > In file included from arch/powerpc/kernel/asm-offsets.c:71: > In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: >>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > *inst = ppc_inst(val); > ^~~ > arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' > #define ppc_inst(x) (x) > ^ > arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning > unsigned int val, suffix; > ^ > = 0 I can't understand what's wrong here. We have __get_kernel_nofault(&val, src, u32, Efault); if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { __get_kernel_nofault(&suffix, src + 1, u32, Efault); *inst = ppc_inst_prefix(val, suffix); } else { *inst = ppc_inst(val); } With #define __get_kernel_nofault(dst, src, type, err_label) \ __get_user_size_goto(*((type *)(dst)), \ (__force type __user *)(src), sizeof(type), err_label) And #define __get_user_size_goto(x, ptr, size, label) \ do { \ BUILD_BUG_ON(size > sizeof(x)); \ switch (size) { \ case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \ case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \ case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \ default: x = 0; BUILD_BUG(); \ } \ } while (0) And #define __get_user_asm_goto(x, addr, label, op) \ asm_volatile_goto( \ "1: "op"%U1%X1 %0, %1 # get_user\n" \ EX_TABLE(1b, %l2) \ : "=r" (x) \ : "m<>" (*addr) \ : \ : label) I see no possibility, no alternative path where val wouldn't be set. The asm clearly has *addr as an output param so it is always set. > 1 warning generated. > <stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings] > #warning syscall futex_waitv not implemented > ^ > 1 warning generated. > arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs' > .stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x: > ^ > arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs' > .stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x: How should we fix that ? Will clang support .stabs in the future ? > ^ > make[2]: *** [arch/powerpc/kernel/vdso32/Makefile:55: arch/powerpc/kernel/vdso32/gettimeofday.o] Error 1 > make[2]: Target 'include/generated/vdso32-offsets.h' not remade because of errors. > make[1]: *** [arch/powerpc/Makefile:421: vdso_prepare] Error 2 > make[1]: Target 'prepare' not remade because of errors. > make: *** [Makefile:219: __sub-make] Error 2 > make: Target 'prepare' not remade because of errors. > > > vim +/val +165 arch/powerpc/include/asm/inst.h > > 152 > 153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) > 154 { > 155 unsigned int val, suffix; > 156 > 157 if (unlikely(!is_kernel_addr((unsigned long)src))) > 158 return -ERANGE; > 159 > 160 __get_kernel_nofault(&val, src, u32, Efault); > 161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { > 162 __get_kernel_nofault(&suffix, src + 1, u32, Efault); > 163 *inst = ppc_inst_prefix(val, suffix); > 164 } else { > > 165 *inst = ppc_inst(val); > 166 } > 167 return 0; > 168 Efault: > 169 return -EFAULT; > 170 } > 171 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 29/11/2021 à 23:55, kernel test robot a écrit : >> Hi Christophe, >> >> I love your patch! Perhaps something to improve: >> >> [auto build test WARNING on powerpc/next] >> [also build test WARNING on v5.16-rc3 next-20211129] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use '--base' as documented in >> https://git-scm.com/docs/git-format-patch] >> >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config) >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) >> reproduce (this is a W=1 build): >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # install powerpc cross compiling tool for clang build >> # apt-get install binutils-powerpc-linux-gnu >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826 >> git remote add linux-review https://github.com/0day-ci/linux >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826 >> # save the config file to linux build tree >> mkdir build_dir >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot <lkp@intel.com> >> >> All warnings (new ones prefixed by >>): >> >> In file included from arch/powerpc/kernel/asm-offsets.c:71: >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] >> *inst = ppc_inst(val); >> ^~~ >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' >> #define ppc_inst(x) (x) >> ^ >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning >> unsigned int val, suffix; >> ^ >> = 0 > > I can't understand what's wrong here. > > We have > > __get_kernel_nofault(&val, src, u32, Efault); > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { > __get_kernel_nofault(&suffix, src + 1, u32, Efault); > *inst = ppc_inst_prefix(val, suffix); > } else { > *inst = ppc_inst(val); > } > > With > > #define __get_kernel_nofault(dst, src, type, err_label) \ > __get_user_size_goto(*((type *)(dst)), \ > (__force type __user *)(src), sizeof(type), err_label) > > > And > > #define __get_user_size_goto(x, ptr, size, label) \ > do { \ > BUILD_BUG_ON(size > sizeof(x)); \ > switch (size) { \ > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \ > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \ > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \ > default: x = 0; BUILD_BUG(); \ > } \ > } while (0) > > And > > #define __get_user_asm_goto(x, addr, label, op) \ > asm_volatile_goto( \ > "1: "op"%U1%X1 %0, %1 # get_user\n" \ > EX_TABLE(1b, %l2) \ > : "=r" (x) \ > : "m<>" (*addr) \ > : \ > : label) > > > I see no possibility, no alternative path where val wouldn't be set. The > asm clearly has *addr as an output param so it is always set. I guess clang can't convince itself of that? >> 1 warning generated. >> <stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings] >> #warning syscall futex_waitv not implemented >> ^ >> 1 warning generated. >> arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs' >> .stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x: >> ^ >> arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs' >> .stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x: > > How should we fix that ? Will clang support .stabs in the future ? I think we should drop any stabs annotations / support. AFAICT none of the toolchains we support produce it anymore. cheers
On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: > Christophe Leroy <christophe.leroy@csgroup.eu> writes: > > Le 29/11/2021 à 23:55, kernel test robot a écrit : > >> Hi Christophe, > >> > >> I love your patch! Perhaps something to improve: > >> > >> [auto build test WARNING on powerpc/next] > >> [also build test WARNING on v5.16-rc3 next-20211129] > >> [If your patch is applied to the wrong git tree, kindly drop us a note. > >> And when submitting patch, we suggest to use '--base' as documented in > >> https://git-scm.com/docs/git-format-patch] > >> > >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next > >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config) > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) > >> reproduce (this is a W=1 build): > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > >> chmod +x ~/bin/make.cross > >> # install powerpc cross compiling tool for clang build > >> # apt-get install binutils-powerpc-linux-gnu > >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > >> git remote add linux-review https://github.com/0day-ci/linux > >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > >> # save the config file to linux build tree > >> mkdir build_dir > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare > >> > >> If you fix the issue, kindly add following tag as appropriate > >> Reported-by: kernel test robot <lkp@intel.com> > >> > >> All warnings (new ones prefixed by >>): > >> > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > >> *inst = ppc_inst(val); > >> ^~~ > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' > >> #define ppc_inst(x) (x) > >> ^ > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning > >> unsigned int val, suffix; > >> ^ > >> = 0 > > > > I can't understand what's wrong here. > > > > We have > > > > __get_kernel_nofault(&val, src, u32, Efault); > > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { > > __get_kernel_nofault(&suffix, src + 1, u32, Efault); > > *inst = ppc_inst_prefix(val, suffix); > > } else { > > *inst = ppc_inst(val); > > } > > > > With > > > > #define __get_kernel_nofault(dst, src, type, err_label) \ > > __get_user_size_goto(*((type *)(dst)), \ > > (__force type __user *)(src), sizeof(type), err_label) > > > > > > And > > > > #define __get_user_size_goto(x, ptr, size, label) \ > > do { \ > > BUILD_BUG_ON(size > sizeof(x)); \ > > switch (size) { \ > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ > > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \ > > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \ > > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \ > > default: x = 0; BUILD_BUG(); \ > > } \ > > } while (0) > > > > And > > > > #define __get_user_asm_goto(x, addr, label, op) \ > > asm_volatile_goto( \ > > "1: "op"%U1%X1 %0, %1 # get_user\n" \ > > EX_TABLE(1b, %l2) \ > > : "=r" (x) \ > > : "m<>" (*addr) \ > > : \ > > : label) > > > > > > I see no possibility, no alternative path where val wouldn't be set. The > > asm clearly has *addr as an output param so it is always set. > > I guess clang can't convince itself of that? A simplified reproducer: $ cat test.c static inline int copy_inst_from_kernel_nofault(unsigned int *inst, unsigned int *src) { unsigned int val; asm goto("1: lwz %U1%X1 %0, %1 # get_user\n" ".section __ex_table,\"a\";" ".balign 4;" ".long (1b) - . ;" ".long (%l2) - . ;" ".previous" : "=r" (*(unsigned int *)(&val)) : "m<>" (*(unsigned int *)(src)) : : Efault); *inst = val; return 0; Efault: return -14; /* -EFAULT */ } $ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized] *inst = val; ^~~ test.c:4:18: note: initialize the variable 'val' to silence this warning unsigned int val; ^ = 0 1 warning generated. It certainly looks like there is something wrong with how clang is tracking the initialization of the variable because it looks to me like val is only used in the fallthrough path, which happens after it is initialized via lwz. Perhaps something is wrong with the logic of https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are being migrated from Bugzilla to GitHub Issues right now so I cannot file this upstream at the moment). Cheers, Nathan
On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: > > Christophe Leroy <christophe.leroy@csgroup.eu> writes: > > > Le 29/11/2021 à 23:55, kernel test robot a écrit : > > >> Hi Christophe, > > >> > > >> I love your patch! Perhaps something to improve: > > >> > > >> [auto build test WARNING on powerpc/next] > > >> [also build test WARNING on v5.16-rc3 next-20211129] > > >> [If your patch is applied to the wrong git tree, kindly drop us a note. > > >> And when submitting patch, we suggest to use '--base' as documented in > > >> https://git-scm.com/docs/git-format-patch] > > >> > > >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next > > >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config) > > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) > > >> reproduce (this is a W=1 build): > > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > >> chmod +x ~/bin/make.cross > > >> # install powerpc cross compiling tool for clang build > > >> # apt-get install binutils-powerpc-linux-gnu > > >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > > >> git remote add linux-review https://github.com/0day-ci/linux > > >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > > >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > > >> # save the config file to linux build tree > > >> mkdir build_dir > > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare > > >> > > >> If you fix the issue, kindly add following tag as appropriate > > >> Reported-by: kernel test robot <lkp@intel.com> > > >> > > >> All warnings (new ones prefixed by >>): > > >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > > >> *inst = ppc_inst(val); > > >> ^~~ > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' > > >> #define ppc_inst(x) (x) > > >> ^ > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning > > >> unsigned int val, suffix; > > >> ^ > > >> = 0 > > > > > > I can't understand what's wrong here. > > > > > > We have > > > > > > __get_kernel_nofault(&val, src, u32, Efault); > > > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { > > > __get_kernel_nofault(&suffix, src + 1, u32, Efault); > > > *inst = ppc_inst_prefix(val, suffix); > > > } else { > > > *inst = ppc_inst(val); > > > } > > > > > > With > > > > > > #define __get_kernel_nofault(dst, src, type, err_label) \ > > > __get_user_size_goto(*((type *)(dst)), \ > > > (__force type __user *)(src), sizeof(type), err_label) > > > > > > > > > And > > > > > > #define __get_user_size_goto(x, ptr, size, label) \ > > > do { \ > > > BUILD_BUG_ON(size > sizeof(x)); \ > > > switch (size) { \ > > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ > > > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \ > > > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \ > > > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \ > > > default: x = 0; BUILD_BUG(); \ > > > } \ > > > } while (0) > > > > > > And > > > > > > #define __get_user_asm_goto(x, addr, label, op) \ > > > asm_volatile_goto( \ > > > "1: "op"%U1%X1 %0, %1 # get_user\n" \ > > > EX_TABLE(1b, %l2) \ > > > : "=r" (x) \ > > > : "m<>" (*addr) \ > > > : \ > > > : label) > > > > > > > > > I see no possibility, no alternative path where val wouldn't be set. The > > > asm clearly has *addr as an output param so it is always set. > > > > I guess clang can't convince itself of that? > > A simplified reproducer: > > $ cat test.c > static inline int copy_inst_from_kernel_nofault(unsigned int *inst, > unsigned int *src) > { > unsigned int val; > > asm goto("1: lwz %U1%X1 %0, %1 # get_user\n" > ".section __ex_table,\"a\";" > ".balign 4;" > ".long (1b) - . ;" > ".long (%l2) - . ;" > ".previous" > : "=r" (*(unsigned int *)(&val)) > : "m<>" (*(unsigned int *)(src)) > : > : Efault); > > *inst = val; > return 0; > > Efault: > return -14; /* -EFAULT */ > } > > $ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c > test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > *inst = val; > ^~~ > test.c:4:18: note: initialize the variable 'val' to silence this warning > unsigned int val; > ^ > = 0 > 1 warning generated. > > It certainly looks like there is something wrong with how clang is > tracking the initialization of the variable because it looks to me like > val is only used in the fallthrough path, which happens after it is > initialized via lwz. Perhaps something is wrong with the logic of > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are > being migrated from Bugzilla to GitHub Issues right now so I cannot file > this upstream at the moment). > If I remove the casts of "val" the warning doesn't appear. I suspect that when I wrote that patch I forgot to remove those when checking. #include "Captain_Picard_facepalm.h" I'll look into it. -bw
On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote: > > On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: > > > Christophe Leroy <christophe.leroy@csgroup.eu> writes: > > > > Le 29/11/2021 à 23:55, kernel test robot a écrit : > > > >> Hi Christophe, > > > >> > > > >> I love your patch! Perhaps something to improve: > > > >> > > > >> [auto build test WARNING on powerpc/next] > > > >> [also build test WARNING on v5.16-rc3 next-20211129] > > > >> [If your patch is applied to the wrong git tree, kindly drop us a note. > > > >> And when submitting patch, we suggest to use '--base' as documented in > > > >> https://git-scm.com/docs/git-format-patch] > > > >> > > > >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > > > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next > > > >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/202111300652.0yDBNvyJ-lkp@intel.com/config) > > > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) > > > >> reproduce (this is a W=1 build): > > > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > > >> chmod +x ~/bin/make.cross > > > >> # install powerpc cross compiling tool for clang build > > > >> # apt-get install binutils-powerpc-linux-gnu > > > >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > > > >> git remote add linux-review https://github.com/0day-ci/linux > > > >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346 > > > >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826 > > > >> # save the config file to linux build tree > > > >> mkdir build_dir > > > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare > > > >> > > > >> If you fix the issue, kindly add following tag as appropriate > > > >> Reported-by: kernel test robot <lkp@intel.com> > > > >> > > > >> All warnings (new ones prefixed by >>): > > > >> > > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: > > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: > > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > > > >> *inst = ppc_inst(val); > > > >> ^~~ > > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' > > > >> #define ppc_inst(x) (x) > > > >> ^ > > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning > > > >> unsigned int val, suffix; > > > >> ^ > > > >> = 0 > > > > > > > > I can't understand what's wrong here. > > > > > > > > We have > > > > > > > > __get_kernel_nofault(&val, src, u32, Efault); > > > > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { > > > > __get_kernel_nofault(&suffix, src + 1, u32, Efault); > > > > *inst = ppc_inst_prefix(val, suffix); > > > > } else { > > > > *inst = ppc_inst(val); > > > > } > > > > > > > > With > > > > > > > > #define __get_kernel_nofault(dst, src, type, err_label) \ > > > > __get_user_size_goto(*((type *)(dst)), \ > > > > (__force type __user *)(src), sizeof(type), err_label) > > > > > > > > > > > > And > > > > > > > > #define __get_user_size_goto(x, ptr, size, label) \ > > > > do { \ > > > > BUILD_BUG_ON(size > sizeof(x)); \ > > > > switch (size) { \ > > > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \ > > > > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \ > > > > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \ > > > > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \ > > > > default: x = 0; BUILD_BUG(); \ > > > > } \ > > > > } while (0) > > > > > > > > And > > > > > > > > #define __get_user_asm_goto(x, addr, label, op) \ > > > > asm_volatile_goto( \ > > > > "1: "op"%U1%X1 %0, %1 # get_user\n" \ > > > > EX_TABLE(1b, %l2) \ > > > > : "=r" (x) \ > > > > : "m<>" (*addr) \ > > > > : \ > > > > : label) > > > > > > > > > > > > I see no possibility, no alternative path where val wouldn't be set. The > > > > asm clearly has *addr as an output param so it is always set. > > > > > > I guess clang can't convince itself of that? > > > > A simplified reproducer: > > > > $ cat test.c > > static inline int copy_inst_from_kernel_nofault(unsigned int *inst, > > unsigned int *src) > > { > > unsigned int val; > > > > asm goto("1: lwz %U1%X1 %0, %1 # get_user\n" > > ".section __ex_table,\"a\";" > > ".balign 4;" > > ".long (1b) - . ;" > > ".long (%l2) - . ;" > > ".previous" > > : "=r" (*(unsigned int *)(&val)) > > : "m<>" (*(unsigned int *)(src)) > > : > > : Efault); > > > > *inst = val; > > return 0; > > > > Efault: > > return -14; /* -EFAULT */ > > } > > > > $ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c > > test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > > *inst = val; > > ^~~ > > test.c:4:18: note: initialize the variable 'val' to silence this warning > > unsigned int val; > > ^ > > = 0 > > 1 warning generated. > > > > It certainly looks like there is something wrong with how clang is > > tracking the initialization of the variable because it looks to me like > > val is only used in the fallthrough path, which happens after it is > > initialized via lwz. Perhaps something is wrong with the logic of > > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are > > being migrated from Bugzilla to GitHub Issues right now so I cannot file > > this upstream at the moment). > > > If I remove the casts of "val" the warning doesn't appear. I suspect > that when I wrote that patch I forgot to remove those when checking. > #include "Captain_Picard_facepalm.h" > > I'll look into it. > Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")
Hi Christophe,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.16-rc3 next-20211201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-randconfig-r021-20211201 (https://download.01.org/0day-ci/archive/20211201/202112011435.ttoYYtbC-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 25eb7fa01d7ebbe67648ea03841cda55b4239ab2)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:143:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:145:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:147:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:149:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:71:
In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
arch/powerpc/include/asm/inst.h:161:41: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
^~~
arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
unsigned int val, suffix;
^
= 0
>> arch/powerpc/include/asm/inst.h:163:32: warning: variable 'suffix' is uninitialized when used here [-Wuninitialized]
*inst = ppc_inst_prefix(val, suffix);
^~~~~~
arch/powerpc/include/asm/inst.h:62:69: note: expanded from macro 'ppc_inst_prefix'
#define ppc_inst_prefix(x, y) ((ppc_inst_t){ .val = (x), .suffix = (y) })
^
arch/powerpc/include/asm/inst.h:155:26: note: initialize the variable 'suffix' to silence this warning
unsigned int val, suffix;
^
= 0
14 warnings generated.
<stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
#warning syscall futex_waitv not implemented
^
1 warning generated.
/usr/bin/ld: unrecognised emulation mode: elf64ppc
Supported emulations: elf_x86_64 elf32_x86_64 elf_i386 elf_iamcu elf_l1om elf_k1om i386pep i386pe
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [arch/powerpc/kernel/vdso64/Makefile:44: arch/powerpc/kernel/vdso64/vdso64.so.dbg] Error 1
make[2]: Target 'include/generated/vdso64-offsets.h' not remade because of errors.
make[1]: *** [arch/powerpc/Makefile:422: vdso_prepare] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:219: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.
vim +/suffix +163 arch/powerpc/include/asm/inst.h
152
153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
154 {
155 unsigned int val, suffix;
156
157 if (unlikely(!is_kernel_addr((unsigned long)src)))
158 return -ERANGE;
159
160 __get_kernel_nofault(&val, src, u32, Efault);
161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> 163 *inst = ppc_inst_prefix(val, suffix);
164 } else {
165 *inst = ppc_inst(val);
166 }
167 return 0;
168 Efault:
169 return -EFAULT;
170 }
171
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Bill Wendling <morbo@google.com> writes: > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote: >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: >> > > Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit : ... >> > > >> All warnings (new ones prefixed by >>): >> > > >> >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] >> > > >> *inst = ppc_inst(val); >> > > >> ^~~ >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' >> > > >> #define ppc_inst(x) (x) >> > > >> ^ >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning >> > > >> unsigned int val, suffix; >> > > >> ^ >> > > >> = 0 >> > > > >> > > > I can't understand what's wrong here. ... >> > > > >> > > > I see no possibility, no alternative path where val wouldn't be set. The >> > > > asm clearly has *addr as an output param so it is always set. >> > > >> > > I guess clang can't convince itself of that? ... >> > >> > It certainly looks like there is something wrong with how clang is >> > tracking the initialization of the variable because it looks to me like >> > val is only used in the fallthrough path, which happens after it is >> > initialized via lwz. Perhaps something is wrong with the logic of >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are >> > being migrated from Bugzilla to GitHub Issues right now so I cannot file >> > this upstream at the moment). >> > >> If I remove the casts of "val" the warning doesn't appear. I suspect >> that when I wrote that patch I forgot to remove those when checking. >> #include "Captain_Picard_facepalm.h" >> >> I'll look into it. >> > Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&") I guess for now I'll just squash this in as a workaround? diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h index 631436f3f5c3..5b591c51fec9 100644 --- a/arch/powerpc/include/asm/inst.h +++ b/arch/powerpc/include/asm/inst.h @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) if (unlikely(!is_kernel_addr((unsigned long)src))) return -ERANGE; +#ifdef CONFIG_CC_IS_CLANG + val = suffix = 0; +#endif __get_kernel_nofault(&val, src, u32, Efault); if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { __get_kernel_nofault(&suffix, src + 1, u32, Efault); cheers
On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: > Bill Wendling <morbo@google.com> writes: > > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote: > >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: > >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: > >> > > Christophe Leroy <christophe.leroy@csgroup.eu> writes: > >> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit : > ... > >> > > >> All warnings (new ones prefixed by >>): > >> > > >> > >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: > >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: > >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > >> > > >> *inst = ppc_inst(val); > >> > > >> ^~~ > >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' > >> > > >> #define ppc_inst(x) (x) > >> > > >> ^ > >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning > >> > > >> unsigned int val, suffix; > >> > > >> ^ > >> > > >> = 0 > >> > > > > >> > > > I can't understand what's wrong here. > ... > >> > > > > >> > > > I see no possibility, no alternative path where val wouldn't be set. The > >> > > > asm clearly has *addr as an output param so it is always set. > >> > > > >> > > I guess clang can't convince itself of that? > ... > >> > > >> > It certainly looks like there is something wrong with how clang is > >> > tracking the initialization of the variable because it looks to me like > >> > val is only used in the fallthrough path, which happens after it is > >> > initialized via lwz. Perhaps something is wrong with the logic of > >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are > >> > being migrated from Bugzilla to GitHub Issues right now so I cannot file > >> > this upstream at the moment). > >> > > >> If I remove the casts of "val" the warning doesn't appear. I suspect > >> that when I wrote that patch I forgot to remove those when checking. > >> #include "Captain_Picard_facepalm.h" > >> > >> I'll look into it. > >> > > Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&") > > I guess for now I'll just squash this in as a workaround? > > > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h > index 631436f3f5c3..5b591c51fec9 100644 > --- a/arch/powerpc/include/asm/inst.h > +++ b/arch/powerpc/include/asm/inst.h > @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) > if (unlikely(!is_kernel_addr((unsigned long)src))) > return -ERANGE; Could we add a version check to this and a link to our bug tracker: /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 > +#ifdef CONFIG_CC_IS_CLANG > + val = suffix = 0; > +#endif > __get_kernel_nofault(&val, src, u32, Efault); > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { > __get_kernel_nofault(&suffix, src + 1, u32, Efault); > > > > cheers
Le 07/12/2021 à 05:48, Nathan Chancellor a écrit : > On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: >> Bill Wendling <morbo@google.com> writes: >>> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote: >>>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: >>>>> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: >>>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes: >>>>>>> Le 29/11/2021 à 23:55, kernel test robot a écrit : >> ... >>>>>>>> All warnings (new ones prefixed by >>): >>>>>>>> >>>>>>>> In file included from arch/powerpc/kernel/asm-offsets.c:71: >>>>>>>> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: >>>>>>>>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] >>>>>>>> *inst = ppc_inst(val); >>>>>>>> ^~~ >>>>>>>> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' >>>>>>>> #define ppc_inst(x) (x) >>>>>>>> ^ >>>>>>>> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning >>>>>>>> unsigned int val, suffix; >>>>>>>> ^ >>>>>>>> = 0 >>>>>>> >>>>>>> I can't understand what's wrong here. >> ... >>>>>>> >>>>>>> I see no possibility, no alternative path where val wouldn't be set. The >>>>>>> asm clearly has *addr as an output param so it is always set. >>>>>> >>>>>> I guess clang can't convince itself of that? >> ... >>>>> >>>>> It certainly looks like there is something wrong with how clang is >>>>> tracking the initialization of the variable because it looks to me like >>>>> val is only used in the fallthrough path, which happens after it is >>>>> initialized via lwz. Perhaps something is wrong with the logic of >>>>> https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are >>>>> being migrated from Bugzilla to GitHub Issues right now so I cannot file >>>>> this upstream at the moment). >>>>> >>>> If I remove the casts of "val" the warning doesn't appear. I suspect >>>> that when I wrote that patch I forgot to remove those when checking. >>>> #include "Captain_Picard_facepalm.h" >>>> >>>> I'll look into it. >>>> >>> Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&") >> >> I guess for now I'll just squash this in as a workaround? >> >> >> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h >> index 631436f3f5c3..5b591c51fec9 100644 >> --- a/arch/powerpc/include/asm/inst.h >> +++ b/arch/powerpc/include/asm/inst.h >> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) >> if (unlikely(!is_kernel_addr((unsigned long)src))) >> return -ERANGE; > > Could we add a version check to this and a link to our bug tracker: > > /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ > #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 The robot reported the problem on: compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) Should it be CONFIG_CLANG_VERSION <= 140000 ? > >> +#ifdef CONFIG_CC_IS_CLANG >> + val = suffix = 0; >> +#endif >> __get_kernel_nofault(&val, src, u32, Efault); >> if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { >> __get_kernel_nofault(&suffix, src + 1, u32, Efault); >> Christophe
On Tue, Dec 07, 2021 at 05:45:08AM +0000, Christophe Leroy wrote: > > > Le 07/12/2021 à 05:48, Nathan Chancellor a écrit : > > On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: > >> Bill Wendling <morbo@google.com> writes: > >>> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote: > >>>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: > >>>>> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: > >>>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes: > >>>>>>> Le 29/11/2021 à 23:55, kernel test robot a écrit : > >> ... > >>>>>>>> All warnings (new ones prefixed by >>): > >>>>>>>> > >>>>>>>> In file included from arch/powerpc/kernel/asm-offsets.c:71: > >>>>>>>> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: > >>>>>>>>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] > >>>>>>>> *inst = ppc_inst(val); > >>>>>>>> ^~~ > >>>>>>>> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' > >>>>>>>> #define ppc_inst(x) (x) > >>>>>>>> ^ > >>>>>>>> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning > >>>>>>>> unsigned int val, suffix; > >>>>>>>> ^ > >>>>>>>> = 0 > >>>>>>> > >>>>>>> I can't understand what's wrong here. > >> ... > >>>>>>> > >>>>>>> I see no possibility, no alternative path where val wouldn't be set. The > >>>>>>> asm clearly has *addr as an output param so it is always set. > >>>>>> > >>>>>> I guess clang can't convince itself of that? > >> ... > >>>>> > >>>>> It certainly looks like there is something wrong with how clang is > >>>>> tracking the initialization of the variable because it looks to me like > >>>>> val is only used in the fallthrough path, which happens after it is > >>>>> initialized via lwz. Perhaps something is wrong with the logic of > >>>>> https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are > >>>>> being migrated from Bugzilla to GitHub Issues right now so I cannot file > >>>>> this upstream at the moment). > >>>>> > >>>> If I remove the casts of "val" the warning doesn't appear. I suspect > >>>> that when I wrote that patch I forgot to remove those when checking. > >>>> #include "Captain_Picard_facepalm.h" > >>>> > >>>> I'll look into it. > >>>> > >>> Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&") > >> > >> I guess for now I'll just squash this in as a workaround? > >> > >> > >> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h > >> index 631436f3f5c3..5b591c51fec9 100644 > >> --- a/arch/powerpc/include/asm/inst.h > >> +++ b/arch/powerpc/include/asm/inst.h > >> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) > >> if (unlikely(!is_kernel_addr((unsigned long)src))) > >> return -ERANGE; > > > > Could we add a version check to this and a link to our bug tracker: > > > > /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ > > #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 > > The robot reported the problem on: > > compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project > df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) > > Should it be CONFIG_CLANG_VERSION <= 140000 ? The robot tests clang from tip of tree, rebuilding every week or so. The fix is getting ready to land so it will be released in 14.0.0 final. We have always written tip of tree version checks with the expectation that if people are testing tip of tree clang, they are frequently rebuilding. If that is not true, they need to be using released/stable versions, otherwise the model is broken. If that is too problematic, we could add a version check to Kconfig (cannot think of a great name for the config off the top of my head) that checks for this issue and ifdef on that. That might be nice in case another instance of this crops up in the future. Cheers, Nathan > > > >> +#ifdef CONFIG_CC_IS_CLANG > >> + val = suffix = 0; > >> +#endif > >> __get_kernel_nofault(&val, src, u32, Efault); > >> if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { > >> __get_kernel_nofault(&suffix, src + 1, u32, Efault); > >> > > Christophe
Le 07/12/2021 à 07:41, Nathan Chancellor a écrit : > On Tue, Dec 07, 2021 at 05:45:08AM +0000, Christophe Leroy wrote: >> >> >> Le 07/12/2021 à 05:48, Nathan Chancellor a écrit : >>> On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: >>>> Bill Wendling <morbo@google.com> writes: >>>>> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote: >>>>>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: >>>>>>> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: >>>>>>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes: >>>>>>>>> Le 29/11/2021 à 23:55, kernel test robot a écrit : >>>> ... >>>>>>>>>> All warnings (new ones prefixed by >>): >>>>>>>>>> >>>>>>>>>> In file included from arch/powerpc/kernel/asm-offsets.c:71: >>>>>>>>>> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: >>>>>>>>>>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] >>>>>>>>>> *inst = ppc_inst(val); >>>>>>>>>> ^~~ >>>>>>>>>> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' >>>>>>>>>> #define ppc_inst(x) (x) >>>>>>>>>> ^ >>>>>>>>>> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning >>>>>>>>>> unsigned int val, suffix; >>>>>>>>>> ^ >>>>>>>>>> = 0 >>>>>>>>> >>>>>>>>> I can't understand what's wrong here. >>>> ... >>>>>>>>> >>>>>>>>> I see no possibility, no alternative path where val wouldn't be set. The >>>>>>>>> asm clearly has *addr as an output param so it is always set. >>>>>>>> >>>>>>>> I guess clang can't convince itself of that? >>>> ... >>>>>>> >>>>>>> It certainly looks like there is something wrong with how clang is >>>>>>> tracking the initialization of the variable because it looks to me like >>>>>>> val is only used in the fallthrough path, which happens after it is >>>>>>> initialized via lwz. Perhaps something is wrong with the logic of >>>>>>> https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are >>>>>>> being migrated from Bugzilla to GitHub Issues right now so I cannot file >>>>>>> this upstream at the moment). >>>>>>> >>>>>> If I remove the casts of "val" the warning doesn't appear. I suspect >>>>>> that when I wrote that patch I forgot to remove those when checking. >>>>>> #include "Captain_Picard_facepalm.h" >>>>>> >>>>>> I'll look into it. >>>>>> >>>>> Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&") >>>> >>>> I guess for now I'll just squash this in as a workaround? >>>> >>>> >>>> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h >>>> index 631436f3f5c3..5b591c51fec9 100644 >>>> --- a/arch/powerpc/include/asm/inst.h >>>> +++ b/arch/powerpc/include/asm/inst.h >>>> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) >>>> if (unlikely(!is_kernel_addr((unsigned long)src))) >>>> return -ERANGE; >>> >>> Could we add a version check to this and a link to our bug tracker: >>> >>> /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ >>> #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 >> >> The robot reported the problem on: >> >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project >> df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e) >> >> Should it be CONFIG_CLANG_VERSION <= 140000 ? > > The robot tests clang from tip of tree, rebuilding every week or so. The > fix is getting ready to land so it will be released in 14.0.0 final. We > have always written tip of tree version checks with the expectation that > if people are testing tip of tree clang, they are frequently rebuilding. > If that is not true, they need to be using released/stable versions, > otherwise the model is broken. > > If that is too problematic, we could add a version check to Kconfig > (cannot think of a great name for the config off the top of my head) > that checks for this issue and ifdef on that. That might be nice in > case another instance of this crops up in the future. > It's fine for me. I didn't know robot was using prereleases with the same name as the future release. Christophe
Nathan Chancellor <nathan@kernel.org> writes: > On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote: >> Bill Wendling <morbo@google.com> writes: >> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@google.com> wrote: >> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@kernel.org> wrote: >> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote: >> >> > > Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> >> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit : >> ... >> >> > > >> All warnings (new ones prefixed by >>): >> >> > > >> >> >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71: >> >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7: >> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized] >> >> > > >> *inst = ppc_inst(val); >> >> > > >> ^~~ >> >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst' >> >> > > >> #define ppc_inst(x) (x) >> >> > > >> ^ >> >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning >> >> > > >> unsigned int val, suffix; >> >> > > >> ^ >> >> > > >> = 0 >> >> > > > >> >> > > > I can't understand what's wrong here. >> ... >> >> > > > >> >> > > > I see no possibility, no alternative path where val wouldn't be set. The >> >> > > > asm clearly has *addr as an output param so it is always set. >> >> > > >> >> > > I guess clang can't convince itself of that? >> ... >> >> > >> >> > It certainly looks like there is something wrong with how clang is >> >> > tracking the initialization of the variable because it looks to me like >> >> > val is only used in the fallthrough path, which happens after it is >> >> > initialized via lwz. Perhaps something is wrong with the logic of >> >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are >> >> > being migrated from Bugzilla to GitHub Issues right now so I cannot file >> >> > this upstream at the moment). >> >> > >> >> If I remove the casts of "val" the warning doesn't appear. I suspect >> >> that when I wrote that patch I forgot to remove those when checking. >> >> #include "Captain_Picard_facepalm.h" >> >> >> >> I'll look into it. >> >> >> > Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&") >> >> I guess for now I'll just squash this in as a workaround? >> >> >> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h >> index 631436f3f5c3..5b591c51fec9 100644 >> --- a/arch/powerpc/include/asm/inst.h >> +++ b/arch/powerpc/include/asm/inst.h >> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) >> if (unlikely(!is_kernel_addr((unsigned long)src))) >> return -ERANGE; > > Could we add a version check to this and a link to our bug tracker: > > /* https://github.com/ClangBuiltLinux/linux/issues/1521 */ > #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000 Yep, thanks I'll use that. cheers
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h index 53a40faf362a..631436f3f5c3 100644 --- a/arch/powerpc/include/asm/inst.h +++ b/arch/powerpc/include/asm/inst.h @@ -4,6 +4,8 @@ #include <asm/ppc-opcode.h> #include <asm/reg.h> +#include <asm/disassemble.h> +#include <asm/uaccess.h> #define ___get_user_instr(gu_op, dest, ptr) \ ({ \ @@ -148,6 +150,23 @@ static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], ppc_inst_t x) __str; \ }) -int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src); +static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) +{ + unsigned int val, suffix; + + if (unlikely(!is_kernel_addr((unsigned long)src))) + return -ERANGE; + + __get_kernel_nofault(&val, src, u32, Efault); + if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { + __get_kernel_nofault(&suffix, src + 1, u32, Efault); + *inst = ppc_inst_prefix(val, suffix); + } else { + *inst = ppc_inst(val); + } + return 0; +Efault: + return -EFAULT; +} #endif /* _ASM_POWERPC_INST_H */ diff --git a/arch/powerpc/mm/maccess.c b/arch/powerpc/mm/maccess.c index 5abae96b2b46..ea821d0ffe16 100644 --- a/arch/powerpc/mm/maccess.c +++ b/arch/powerpc/mm/maccess.c @@ -11,20 +11,3 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size) { return is_kernel_addr((unsigned long)unsafe_src); } - -int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src) -{ - unsigned int val, suffix; - int err; - - err = copy_from_kernel_nofault(&val, src, sizeof(val)); - if (err) - return err; - if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) { - err = copy_from_kernel_nofault(&suffix, src + 1, sizeof(suffix)); - *inst = ppc_inst_prefix(val, suffix); - } else { - *inst = ppc_inst(val); - } - return err; -}
copy_inst_from_kernel_nofault() uses copy_from_kernel_nofault() to copy one or two 32bits words. This means calling an out-of-line function which itself calls back copy_from_kernel_nofault_allowed() then performs a generic copy with loops. Rewrite copy_inst_from_kernel_nofault() to do everything at a single place and use __get_kernel_nofault() directly to perform single accesses without loops. Allthough the generic function uses pagefault_disable(), it is not required on powerpc because do_page_fault() bails earlier when a kernel mode fault happens on a kernel address. As the function has now become very small, inline it. With this change, on an 8xx the time spent in the loop in ftrace_replace_code() is reduced by 23% at function tracer activation and 27% at nop tracer activation. The overall time to activate function tracer (measured with shell command 'time') is 570ms before the patch and 470ms after the patch. Even vmlinux size is reduced (by 152 instruction). Before the patch: 00000018 <copy_inst_from_kernel_nofault>: 18: 94 21 ff e0 stwu r1,-32(r1) 1c: 7c 08 02 a6 mflr r0 20: 38 a0 00 04 li r5,4 24: 93 e1 00 1c stw r31,28(r1) 28: 7c 7f 1b 78 mr r31,r3 2c: 38 61 00 08 addi r3,r1,8 30: 90 01 00 24 stw r0,36(r1) 34: 48 00 00 01 bl 34 <copy_inst_from_kernel_nofault+0x1c> 34: R_PPC_REL24 copy_from_kernel_nofault 38: 2c 03 00 00 cmpwi r3,0 3c: 40 82 00 0c bne 48 <copy_inst_from_kernel_nofault+0x30> 40: 81 21 00 08 lwz r9,8(r1) 44: 91 3f 00 00 stw r9,0(r31) 48: 80 01 00 24 lwz r0,36(r1) 4c: 83 e1 00 1c lwz r31,28(r1) 50: 38 21 00 20 addi r1,r1,32 54: 7c 08 03 a6 mtlr r0 58: 4e 80 00 20 blr After the patch (before inlining): 00000018 <copy_inst_from_kernel_nofault>: 18: 3d 20 b0 00 lis r9,-20480 1c: 7c 04 48 40 cmplw r4,r9 20: 7c 69 1b 78 mr r9,r3 24: 41 80 00 14 blt 38 <copy_inst_from_kernel_nofault+0x20> 28: 81 44 00 00 lwz r10,0(r4) 2c: 38 60 00 00 li r3,0 30: 91 49 00 00 stw r10,0(r9) 34: 4e 80 00 20 blr 38: 38 60 ff de li r3,-34 3c: 4e 80 00 20 blr 40: 38 60 ff f2 li r3,-14 44: 4e 80 00 20 blr Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- v4: Inline and remove pagefault_disable() v3: New --- arch/powerpc/include/asm/inst.h | 21 ++++++++++++++++++++- arch/powerpc/mm/maccess.c | 17 ----------------- 2 files changed, 20 insertions(+), 18 deletions(-)