* Re: [PATCH v3 25/41] powerpc/book3s64/kuep: Store/restore userspace IAMR correctly on entry and exit from kernel
From: kernel test robot @ 2020-06-11 0:03 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
Cc: clang-built-linux, kbuild-all, bauerman, linuxram,
Aneesh Kumar K.V
In-Reply-To: <20200610095204.608183-26-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 9130 bytes --]
Hi "Aneesh,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20200610]
[cannot apply to v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Kernel-userspace-access-execution-prevention-with-hash-translation/20200610-191943
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r006-20200608 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project bc2b70982be8f5250cd0082a7190f8b417bd4dfe)
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
arch/powerpc/include/asm/book3s/64/kup.h:181:24: error: no member named 'kuap' in 'struct pt_regs'
mtspr(SPRN_AMR, regs->kuap);
~~~~ ^
arch/powerpc/include/asm/reg.h:1386:33: note: expanded from macro 'mtspr'
: "r" ((unsigned long)(v)) ^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
>> arch/powerpc/include/asm/book3s/64/kup.h:182:25: error: no member named 'kuep' in 'struct pt_regs'
mtspr(SPRN_IAMR, regs->kuep);
~~~~ ^
arch/powerpc/include/asm/reg.h:1386:33: note: expanded from macro 'mtspr'
: "r" ((unsigned long)(v)) ^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
arch/powerpc/include/asm/book3s/64/kup.h:194:22: error: no member named 'kuap' in 'struct pt_regs'
if (unlikely(regs->kuap != amr)) {
~~~~ ^
include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
arch/powerpc/include/asm/book3s/64/kup.h:196:26: error: no member named 'kuap' in 'struct pt_regs'
mtspr(SPRN_AMR, regs->kuap);
~~~~ ^
arch/powerpc/include/asm/reg.h:1386:33: note: expanded from macro 'mtspr'
: "r" ((unsigned long)(v)) ^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
arch/powerpc/include/asm/book3s/64/kup.h:293:14: error: no member named 'kuap' in 'struct pt_regs'
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
~~~~ ^
include/asm-generic/bug.h:122:25: note: expanded from macro 'WARN'
int __ret_warn_on = !!(condition); ^~~~~~~~~
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
arch/powerpc/include/asm/kup.h:56:20: error: redefinition of 'allow_user_access'
static inline void allow_user_access(void __user *to, const void __user *from,
^
arch/powerpc/include/asm/book3s/64/kup.h:254:29: note: previous definition is here
static __always_inline void allow_user_access(void __user *to, const void __user *from,
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
arch/powerpc/include/asm/kup.h:58:20: error: redefinition of 'prevent_user_access'
static inline void prevent_user_access(void __user *to, const void __user *from,
^
arch/powerpc/include/asm/book3s/64/kup.h:269:20: note: previous definition is here
static inline void prevent_user_access(void __user *to, const void __user *from,
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
arch/powerpc/include/asm/kup.h:60:29: error: redefinition of 'prevent_user_access_return'
static inline unsigned long prevent_user_access_return(void) { return 0UL; }
^
arch/powerpc/include/asm/book3s/64/kup.h:275:29: note: previous definition is here
static inline unsigned long prevent_user_access_return(void)
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
vim +182 arch/powerpc/include/asm/book3s/64/kup.h
174
175 static inline void kuap_restore_user_amr(struct pt_regs *regs)
176 {
177 if (!mmu_has_feature(MMU_FTR_PKEY))
178 return;
179
180 isync();
181 mtspr(SPRN_AMR, regs->kuap);
> 182 mtspr(SPRN_IAMR, regs->kuep);
183 /*
184 * No isync required here because we are about to rfi
185 * back to previous context before any user accesses
186 * would be made, which is a CSI.
187 */
188 }
189 static inline void kuap_restore_kernel_amr(struct pt_regs *regs,
190 unsigned long amr)
191 {
192 if (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)) {
193
194 if (unlikely(regs->kuap != amr)) {
195 isync();
196 mtspr(SPRN_AMR, regs->kuap);
197 /*
198 * No isync required here because we are about to rfi
199 * back to previous context before any user accesses
200 * would be made, which is a CSI.
201 */
202 }
203 }
204 /*
205 * No need to restore IAMR when returning to kernel space.
206 */
207 }
208
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37467 bytes --]
^ permalink raw reply
* Re: [PATCH v3 24/41] powerpc/book3s64/pkeys: Store/restore userspace AMR correctly on entry and exit from kernel
From: kernel test robot @ 2020-06-10 22:29 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
Cc: clang-built-linux, kbuild-all, bauerman, linuxram,
Aneesh Kumar K.V
In-Reply-To: <20200610095204.608183-25-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 13507 bytes --]
Hi "Aneesh,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20200610]
[cannot apply to v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Kernel-userspace-access-execution-prevention-with-hash-translation/20200610-191943
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r006-20200608 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project bc2b70982be8f5250cd0082a7190f8b417bd4dfe)
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
>> arch/powerpc/include/asm/book3s/64/kup.h:142:24: error: no member named 'kuap' in 'struct pt_regs'
mtspr(SPRN_AMR, regs->kuap);
~~~~ ^
arch/powerpc/include/asm/reg.h:1386:33: note: expanded from macro 'mtspr'
: "r" ((unsigned long)(v)) ^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
arch/powerpc/include/asm/book3s/64/kup.h:155:22: error: no member named 'kuap' in 'struct pt_regs'
if (unlikely(regs->kuap != amr)) {
~~~~ ^
include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
arch/powerpc/include/asm/book3s/64/kup.h:157:26: error: no member named 'kuap' in 'struct pt_regs'
mtspr(SPRN_AMR, regs->kuap);
~~~~ ^
arch/powerpc/include/asm/reg.h:1386:33: note: expanded from macro 'mtspr'
: "r" ((unsigned long)(v)) ^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
In file included from arch/powerpc/include/asm/kup.h:18:
arch/powerpc/include/asm/book3s/64/kup.h:251:14: error: no member named 'kuap' in 'struct pt_regs'
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
~~~~ ^
include/asm-generic/bug.h:122:25: note: expanded from macro 'WARN'
int __ret_warn_on = !!(condition); ^~~~~~~~~
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:56:20: error: redefinition of 'allow_user_access'
static inline void allow_user_access(void __user *to, const void __user *from,
^
arch/powerpc/include/asm/book3s/64/kup.h:212:29: note: previous definition is here
static __always_inline void allow_user_access(void __user *to, const void __user *from,
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:58:20: error: redefinition of 'prevent_user_access'
static inline void prevent_user_access(void __user *to, const void __user *from,
^
arch/powerpc/include/asm/book3s/64/kup.h:227:20: note: previous definition is here
static inline void prevent_user_access(void __user *to, const void __user *from,
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:60:29: error: redefinition of 'prevent_user_access_return'
static inline unsigned long prevent_user_access_return(void) { return 0UL; }
^
arch/powerpc/include/asm/book3s/64/kup.h:233:29: note: previous definition is here
static inline unsigned long prevent_user_access_return(void)
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:61:20: error: redefinition of 'restore_user_access'
static inline void restore_user_access(unsigned long flags) { }
^
arch/powerpc/include/asm/book3s/64/kup.h:242:20: note: previous definition is here
static inline void restore_user_access(unsigned long flags)
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:15:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/crypto/hash.h:11:
In file included from include/linux/crypto.h:21:
In file included from include/linux/uaccess.h:11:
In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:63:1: error: redefinition of 'bad_kuap_fault'
bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
^
arch/powerpc/include/asm/book3s/64/kup.h:248:1: note: previous definition is here
bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:87:11: warning: array index 3 is past the end of the array (which contains 1 element) [-Warray-bounds]
return (set->sig[3] | set->sig[2] |
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:87:25: warning: array index 2 is past the end of the array (which contains 1 element) [-Warray-bounds]
return (set->sig[3] | set->sig[2] |
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:88:4: warning: array index 1 is past the end of the array (which contains 1 element) [-Warray-bounds]
set->sig[1] | set->sig[0]) == 0;
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:90:11: warning: array index 1 is past the end of the array (which contains 1 element) [-Warray-bounds]
return (set->sig[1] | set->sig[0]) == 0;
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:103:11: warning: array index 3 is past the end of the array (which contains 1 element) [-Warray-bounds]
return (set1->sig[3] == set2->sig[3]) &&
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:103:27: warning: array index 3 is past the end of the array (which contains 1 element) [-Warray-bounds]
return (set1->sig[3] == set2->sig[3]) &&
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:104:5: warning: array index 2 is past the end of the array (which contains 1 element) [-Warray-bounds]
(set1->sig[2] == set2->sig[2]) &&
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
^
In file included from arch/powerpc/kernel/asm-offsets.c:14:
In file included from include/linux/compat.h:17:
In file included from include/linux/fs.h:34:
In file included from include/linux/percpu-rwsem.h:7:
In file included from include/linux/rcuwait.h:6:
In file included from include/linux/sched/signal.h:6:
include/linux/signal.h:104:21: warning: array index 2 is past the end of the array (which contains 1 element) [-Warray-bounds]
(set1->sig[2] == set2->sig[2]) &&
^ ~
arch/powerpc/include/uapi/asm/signal.h:18:2: note: array 'sig' declared here
unsigned long sig[_NSIG_WORDS];
vim +142 arch/powerpc/include/asm/book3s/64/kup.h
135
136 static inline void kuap_restore_user_amr(struct pt_regs *regs)
137 {
138 if (!mmu_has_feature(MMU_FTR_PKEY))
139 return;
140
141 isync();
> 142 mtspr(SPRN_AMR, regs->kuap);
143 /*
144 * No isync required here because we are about to rfi
145 * back to previous context before any user accesses
146 * would be made, which is a CSI.
147 */
148 }
149
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37467 bytes --]
^ permalink raw reply
* RE: Re: [RESEND PATCH v5 2/5] arm64/crash_core: Export TCR_EL1.T1SZ in vmcoreinfo
From: Bharat Gooty @ 2020-06-10 16:49 UTC (permalink / raw)
To: Scott Branden, Bhupesh Sharma, Amit Kachhap
Cc: Mark Rutland, Kazuhito Hagio, Bharat Gooty, Ard Biesheuvel,
Catalin Marinas, x86, Linux Doc Mailing List,
Linux Kernel Mailing List, Ray Jui, linuxppc-dev, James Morse,
Dave Anderson, bhupesh linux, Will Deacon, kexec mailing list,
linux-arm-kernel, Steve Capper
In-Reply-To: <d401b003-af3e-c525-ba00-0de48486b7a0@broadcom.com>
Sorry, error message was not posted. Following is the error message
crash: cannot determine VA_BITS_ACTUAL
-----Original Message-----
From: Bharat Gooty [mailto:bharat.gooty@broadcom.com]
Sent: Wednesday, June 10, 2020 10:18 PM
To: Scott Branden; 'Bhupesh Sharma'; 'Amit Kachhap'
Cc: 'Mark Rutland'; 'x86@kernel.org'; 'Will Deacon'; 'Linux Doc Mailing
List'; 'Catalin Marinas'; 'Ard Biesheuvel'; 'kexec mailing list'; 'Linux
Kernel Mailing List'; 'Kazuhito Hagio'; 'James Morse'; 'Dave Anderson';
'bhupesh linux'; 'linuxppc-dev@lists.ozlabs.org'; 'linux-arm-kernel'; 'Steve
Capper'; Ray Jui
Subject: RE: Re: [RESEND PATCH v5 2/5] arm64/crash_core: Export TCR_EL1.T1SZ
in vmcoreinfo
Hello Bhupesh,
V6 patch set on Linux 5.7, did not help.
I have applied makedump file
http://lists.infradead.org/pipermail/kexec/2019-November/023963.html changes
also (makedump-1.6.6). Tried to apply it on makedumpfile 1.6.7. Patch set_2
failed. Would like to know, if you have V5 patch set for makedump file
changes. With makedump 1.6.6, able to collect the vmore file.
I used latest crash utility
(https://www.redhat.com/archives/crash-utility/2019-November/msg00014.html
changes are present)
When I used crash utility, following is the error:
Thanks,
-Bharat
-----Original Message-----
From: Scott Branden [mailto:scott.branden@broadcom.com]
Sent: Thursday, April 30, 2020 4:34 AM
To: Bhupesh Sharma; Amit Kachhap
Cc: Mark Rutland; x86@kernel.org; Will Deacon; Linux Doc Mailing List;
Catalin Marinas; Ard Biesheuvel; kexec mailing list; Linux Kernel Mailing
List; Kazuhito Hagio; James Morse; Dave Anderson; bhupesh linux;
linuxppc-dev@lists.ozlabs.org; linux-arm-kernel; Steve Capper; Ray Jui;
Bharat Gooty
Subject: Re: Re: [RESEND PATCH v5 2/5] arm64/crash_core: Export TCR_EL1.T1SZ
in vmcoreinfo
Hi Bhupesh,
On 2020-02-23 10:25 p.m., Bhupesh Sharma wrote:
> Hi Amit,
>
> On Fri, Feb 21, 2020 at 2:36 PM Amit Kachhap <amit.kachhap@arm.com> wrote:
>> Hi Bhupesh,
>>
>> On 1/13/20 5:44 PM, Bhupesh Sharma wrote:
>>> Hi James,
>>>
>>> On 01/11/2020 12:30 AM, Dave Anderson wrote:
>>>> ----- Original Message -----
>>>>> Hi Bhupesh,
>>>>>
>>>>> On 25/12/2019 19:01, Bhupesh Sharma wrote:
>>>>>> On 12/12/2019 04:02 PM, James Morse wrote:
>>>>>>> On 29/11/2019 19:59, Bhupesh Sharma wrote:
>>>>>>>> vabits_actual variable on arm64 indicates the actual VA space size,
>>>>>>>> and allows a single binary to support both 48-bit and 52-bit VA
>>>>>>>> spaces.
>>>>>>>>
>>>>>>>> If the ARMv8.2-LVA optional feature is present, and we are running
>>>>>>>> with a 64KB page size; then it is possible to use 52-bits of
>>>>>>>> address
>>>>>>>> space for both userspace and kernel addresses. However, any kernel
>>>>>>>> binary that supports 52-bit must also be able to fall back to
>>>>>>>> 48-bit
>>>>>>>> at early boot time if the hardware feature is not present.
>>>>>>>>
>>>>>>>> Since TCR_EL1.T1SZ indicates the size offset of the memory region
>>>>>>>> addressed by TTBR1_EL1 (and hence can be used for determining the
>>>>>>>> vabits_actual value) it makes more sense to export the same in
>>>>>>>> vmcoreinfo rather than vabits_actual variable, as the name of the
>>>>>>>> variable can change in future kernel versions, but the
>>>>>>>> architectural
>>>>>>>> constructs like TCR_EL1.T1SZ can be used better to indicate
>>>>>>>> intended
>>>>>>>> specific fields to user-space.
>>>>>>>>
>>>>>>>> User-space utilities like makedumpfile and crash-utility, need to
>>>>>>>> read/write this value from/to vmcoreinfo
>>>>>>> (write?)
>>>>>> Yes, also write so that the vmcoreinfo from an (crashing) arm64
>>>>>> system can
>>>>>> be used for
>>>>>> analysis of the root-cause of panic/crash on say an x86_64 host using
>>>>>> utilities like
>>>>>> crash-utility/gdb.
>>>>> I read this as as "User-space [...] needs to write to vmcoreinfo".
>>> That's correct. But for writing to vmcore dump in the kdump kernel, we
>>> need to read the symbols from the vmcoreinfo in the primary kernel.
>>>
>>>>>>>> for determining if a virtual address lies in the linear map range.
>>>>>>> I think this is a fragile example. The debugger shouldn't need to
>>>>>>> know
>>>>>>> this.
>>>>>> Well that the current user-space utility design, so I am not sure we
>>>>>> can
>>>>>> tweak that too much.
>>>>>>
>>>>>>>> The user-space computation for determining whether an address lies
>>>>>>>> in
>>>>>>>> the linear map range is the same as we have in kernel-space:
>>>>>>>>
>>>>>>>> #define __is_lm_address(addr) (!(((u64)addr) &
>>>>>>>> BIT(vabits_actual -
>>>>>>>> 1)))
>>>>>>> This was changed with 14c127c957c1 ("arm64: mm: Flip kernel VA
>>>>>>> space"). If
>>>>>>> user-space
>>>>>>> tools rely on 'knowing' the kernel memory layout, they must have to
>>>>>>> constantly be fixed
>>>>>>> and updated. This is a poor argument for adding this to something
>>>>>>> that
>>>>>>> ends up as ABI.
>>>>>> See above. The user-space has to rely on some ABI/guaranteed
>>>>>> hardware-symbols which can be
>>>>>> used for 'determining' the kernel memory layout.
>>>>> I disagree. Everything and anything in the kernel will change. The
>>>>> ABI rules apply to
>>>>> stuff exposed via syscalls and kernel filesystems. It does not apply
>>>>> to kernel internals,
>>>>> like the memory layout we used yesterday. 14c127c957c1 is a case in
>>>>> point.
>>>>>
>>>>> A debugger trying to rely on this sort of thing would have to play
>>>>> catchup whenever it
>>>>> changes.
>>>> Exactly. That's the whole point.
>>>>
>>>> The crash utility and makedumpfile are not in the same league as other
>>>> user-space tools.
>>>> They have always had to "play catchup" precisely because they depend
>>>> upon kernel internals,
>>>> which constantly change.
>>> I agree with you and DaveA here. Software user-space debuggers are
>>> dependent on kernel internals (which can change from time-to-time) and
>>> will have to play catch-up (which has been the case since the very
>>> start).
>>>
>>> Unfortunately we don't have any clear ABI for software debugging tools -
>>> may be something to look for in future.
>>>
>>> A case in point is gdb/kgdb, which still needs to run with KASLR
>>> turned-off (nokaslr) for debugging, as it confuses gdb which resolve
>>> kernel symbol address from symbol table of vmlinux. But we can
>>> work-around the same in makedumpfile/crash by reading the 'kaslr_offset'
>>> value. And I have several users telling me now they cannot use gdb on
>>> KASLR enabled kernel to debug panics, but can makedumpfile + crash
>>> combination to achieve the same.
>>>
>>> So, we should be looking to fix these utilities which are broken since
>>> the 52-bit changes for arm64. Accordingly, I will try to send the v6
>>> soon while incorporating the comments posted on the v5.
>> Any update on the next v6 version. Since this patch series is fixing the
>> current broken kdump so need this series to add some more fields in
>> vmcoreinfo for Pointer Authentication work.
> Sorry for the delay. I was caught up in some other urgent arm64
> user-space issues.
> I am preparing the v6 now and hopefully will be able to post it out
> for review later today.
Did v6 get sent out?
>
> Thanks,
> Bhupesh
>
>
Regards,
Scott
^ permalink raw reply
* RE: Re: [RESEND PATCH v5 2/5] arm64/crash_core: Export TCR_EL1.T1SZ in vmcoreinfo
From: Bharat Gooty @ 2020-06-10 16:47 UTC (permalink / raw)
To: Scott Branden, Bhupesh Sharma, Amit Kachhap
Cc: Mark Rutland, Kazuhito Hagio, Ard Biesheuvel, Catalin Marinas,
x86, Linux Doc Mailing List, Linux Kernel Mailing List, Ray Jui,
linuxppc-dev, James Morse, Dave Anderson, bhupesh linux,
Will Deacon, kexec mailing list, linux-arm-kernel, Steve Capper
In-Reply-To: <d401b003-af3e-c525-ba00-0de48486b7a0@broadcom.com>
Hello Bhupesh,
V6 patch set on Linux 5.7, did not help.
I have applied makedump file
http://lists.infradead.org/pipermail/kexec/2019-November/023963.html changes
also (makedump-1.6.6). Tried to apply it on makedumpfile 1.6.7. Patch set_2
failed. Would like to know, if you have V5 patch set for makedump file
changes. With makedump 1.6.6, able to collect the vmore file.
I used latest crash utility
(https://www.redhat.com/archives/crash-utility/2019-November/msg00014.html
changes are present)
When I used crash utility, following is the error:
Thanks,
-Bharat
-----Original Message-----
From: Scott Branden [mailto:scott.branden@broadcom.com]
Sent: Thursday, April 30, 2020 4:34 AM
To: Bhupesh Sharma; Amit Kachhap
Cc: Mark Rutland; x86@kernel.org; Will Deacon; Linux Doc Mailing List;
Catalin Marinas; Ard Biesheuvel; kexec mailing list; Linux Kernel Mailing
List; Kazuhito Hagio; James Morse; Dave Anderson; bhupesh linux;
linuxppc-dev@lists.ozlabs.org; linux-arm-kernel; Steve Capper; Ray Jui;
Bharat Gooty
Subject: Re: Re: [RESEND PATCH v5 2/5] arm64/crash_core: Export TCR_EL1.T1SZ
in vmcoreinfo
Hi Bhupesh,
On 2020-02-23 10:25 p.m., Bhupesh Sharma wrote:
> Hi Amit,
>
> On Fri, Feb 21, 2020 at 2:36 PM Amit Kachhap <amit.kachhap@arm.com> wrote:
>> Hi Bhupesh,
>>
>> On 1/13/20 5:44 PM, Bhupesh Sharma wrote:
>>> Hi James,
>>>
>>> On 01/11/2020 12:30 AM, Dave Anderson wrote:
>>>> ----- Original Message -----
>>>>> Hi Bhupesh,
>>>>>
>>>>> On 25/12/2019 19:01, Bhupesh Sharma wrote:
>>>>>> On 12/12/2019 04:02 PM, James Morse wrote:
>>>>>>> On 29/11/2019 19:59, Bhupesh Sharma wrote:
>>>>>>>> vabits_actual variable on arm64 indicates the actual VA space size,
>>>>>>>> and allows a single binary to support both 48-bit and 52-bit VA
>>>>>>>> spaces.
>>>>>>>>
>>>>>>>> If the ARMv8.2-LVA optional feature is present, and we are running
>>>>>>>> with a 64KB page size; then it is possible to use 52-bits of
>>>>>>>> address
>>>>>>>> space for both userspace and kernel addresses. However, any kernel
>>>>>>>> binary that supports 52-bit must also be able to fall back to
>>>>>>>> 48-bit
>>>>>>>> at early boot time if the hardware feature is not present.
>>>>>>>>
>>>>>>>> Since TCR_EL1.T1SZ indicates the size offset of the memory region
>>>>>>>> addressed by TTBR1_EL1 (and hence can be used for determining the
>>>>>>>> vabits_actual value) it makes more sense to export the same in
>>>>>>>> vmcoreinfo rather than vabits_actual variable, as the name of the
>>>>>>>> variable can change in future kernel versions, but the
>>>>>>>> architectural
>>>>>>>> constructs like TCR_EL1.T1SZ can be used better to indicate
>>>>>>>> intended
>>>>>>>> specific fields to user-space.
>>>>>>>>
>>>>>>>> User-space utilities like makedumpfile and crash-utility, need to
>>>>>>>> read/write this value from/to vmcoreinfo
>>>>>>> (write?)
>>>>>> Yes, also write so that the vmcoreinfo from an (crashing) arm64
>>>>>> system can
>>>>>> be used for
>>>>>> analysis of the root-cause of panic/crash on say an x86_64 host using
>>>>>> utilities like
>>>>>> crash-utility/gdb.
>>>>> I read this as as "User-space [...] needs to write to vmcoreinfo".
>>> That's correct. But for writing to vmcore dump in the kdump kernel, we
>>> need to read the symbols from the vmcoreinfo in the primary kernel.
>>>
>>>>>>>> for determining if a virtual address lies in the linear map range.
>>>>>>> I think this is a fragile example. The debugger shouldn't need to
>>>>>>> know
>>>>>>> this.
>>>>>> Well that the current user-space utility design, so I am not sure we
>>>>>> can
>>>>>> tweak that too much.
>>>>>>
>>>>>>>> The user-space computation for determining whether an address lies
>>>>>>>> in
>>>>>>>> the linear map range is the same as we have in kernel-space:
>>>>>>>>
>>>>>>>> #define __is_lm_address(addr) (!(((u64)addr) &
>>>>>>>> BIT(vabits_actual -
>>>>>>>> 1)))
>>>>>>> This was changed with 14c127c957c1 ("arm64: mm: Flip kernel VA
>>>>>>> space"). If
>>>>>>> user-space
>>>>>>> tools rely on 'knowing' the kernel memory layout, they must have to
>>>>>>> constantly be fixed
>>>>>>> and updated. This is a poor argument for adding this to something
>>>>>>> that
>>>>>>> ends up as ABI.
>>>>>> See above. The user-space has to rely on some ABI/guaranteed
>>>>>> hardware-symbols which can be
>>>>>> used for 'determining' the kernel memory layout.
>>>>> I disagree. Everything and anything in the kernel will change. The
>>>>> ABI rules apply to
>>>>> stuff exposed via syscalls and kernel filesystems. It does not apply
>>>>> to kernel internals,
>>>>> like the memory layout we used yesterday. 14c127c957c1 is a case in
>>>>> point.
>>>>>
>>>>> A debugger trying to rely on this sort of thing would have to play
>>>>> catchup whenever it
>>>>> changes.
>>>> Exactly. That's the whole point.
>>>>
>>>> The crash utility and makedumpfile are not in the same league as other
>>>> user-space tools.
>>>> They have always had to "play catchup" precisely because they depend
>>>> upon kernel internals,
>>>> which constantly change.
>>> I agree with you and DaveA here. Software user-space debuggers are
>>> dependent on kernel internals (which can change from time-to-time) and
>>> will have to play catch-up (which has been the case since the very
>>> start).
>>>
>>> Unfortunately we don't have any clear ABI for software debugging tools -
>>> may be something to look for in future.
>>>
>>> A case in point is gdb/kgdb, which still needs to run with KASLR
>>> turned-off (nokaslr) for debugging, as it confuses gdb which resolve
>>> kernel symbol address from symbol table of vmlinux. But we can
>>> work-around the same in makedumpfile/crash by reading the 'kaslr_offset'
>>> value. And I have several users telling me now they cannot use gdb on
>>> KASLR enabled kernel to debug panics, but can makedumpfile + crash
>>> combination to achieve the same.
>>>
>>> So, we should be looking to fix these utilities which are broken since
>>> the 52-bit changes for arm64. Accordingly, I will try to send the v6
>>> soon while incorporating the comments posted on the v5.
>> Any update on the next v6 version. Since this patch series is fixing the
>> current broken kdump so need this series to add some more fields in
>> vmcoreinfo for Pointer Authentication work.
> Sorry for the delay. I was caught up in some other urgent arm64
> user-space issues.
> I am preparing the v6 now and hopefully will be able to post it out
> for review later today.
Did v6 get sent out?
>
> Thanks,
> Bhupesh
>
>
Regards,
Scott
^ permalink raw reply
* Re: [PATCH v5 2/4] riscv: Introduce CONFIG_RELOCATABLE
From: Jerome Forissier @ 2020-06-10 14:10 UTC (permalink / raw)
To: Alexandre Ghiti, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Anup Patel, Atish Patra, Zong Li, linux-kernel, linuxppc-dev,
linux-riscv
Cc: Anup Patel
In-Reply-To: <20200607075949.665-3-alex@ghiti.fr>
On 6/7/20 9:59 AM, Alexandre Ghiti wrote:
[...]
> +config RELOCATABLE
> + bool
> + depends on MMU
> + help
> + This builds a kernel as a Position Independent Executable (PIE),
> + which retains all relocation metadata required to relocate the
> + kernel binary at runtime to a different virtual address than the
> + address it was linked at.
> + Since RISCV uses the RELA relocation format, this requires a
> + relocation pass at runtime even if the kernel is loaded at the
> + same address it was linked at.
Is this true? I thought that the GNU linker would write the "proper"
values by default, contrary to the LLVM linker (ld.lld) which would need
a special flag: --apply-dynamic-relocs (by default the relocated places
are set to zero). At least, it is my experience with Aarch64 on a
different project. So, sorry if I'm talking nonsense here -- I have not
looked at the details.
--
Jerome
^ permalink raw reply
* [PATCH 3/3] powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_1
From: Murilo Opsfelder Araujo @ 2020-06-10 21:51 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
Cc: Murilo Opsfelder Araujo, Murilo Opsfelder Araujo, Paul Mackerras,
Aneesh Kumar K . V, Alistair Popple, Jordan Niethe, Daniel Axtens
In-Reply-To: <20200610215114.167544-1-muriloo@linux.ibm.com>
Macro ISA_V3_1 was defined but never used. Use it instead of literal.
Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 9d6e833da2bd..a0edeb391e3e 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -678,7 +678,7 @@ static void __init cpufeatures_setup_start(u32 isa)
cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_ARCH_3_00;
}
- if (isa >= 3100) {
+ if (isa >= ISA_V3_1) {
cur_cpu_spec->cpu_features |= CPU_FTR_ARCH_31;
cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_ARCH_3_1;
}
--
2.25.4
^ permalink raw reply related
* [PATCH 2/3] powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_0B
From: Murilo Opsfelder Araujo @ 2020-06-10 21:51 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
Cc: Murilo Opsfelder Araujo, Murilo Opsfelder Araujo, Paul Mackerras,
Aneesh Kumar K . V, Alistair Popple, Jordan Niethe, Daniel Axtens
In-Reply-To: <20200610215114.167544-1-muriloo@linux.ibm.com>
Macro ISA_V3_0B was defined but never used. Use it instead of
literal.
Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 1b7da8b5ce0d..9d6e833da2bd 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -673,7 +673,7 @@ static void __init cpufeatures_setup_start(u32 isa)
{
pr_info("setup for ISA %d\n", isa);
- if (isa >= 3000) {
+ if (isa >= ISA_V3_0B) {
cur_cpu_spec->cpu_features |= CPU_FTR_ARCH_300;
cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_ARCH_3_00;
}
--
2.25.4
^ permalink raw reply related
* [PATCH 0/3] powerpc/dt_cpu_ftrs: Make use of ISA_V3_* macros
From: Murilo Opsfelder Araujo @ 2020-06-10 21:51 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
Cc: Murilo Opsfelder Araujo, Murilo Opsfelder Araujo, Paul Mackerras,
Aneesh Kumar K . V, Alistair Popple, Jordan Niethe, Daniel Axtens
The first patch removes unused macro ISA_V2_07B. The second and third
patches make use of macros ISA_V3_0B and ISA_V3_1, respectively,
instead their corresponding literals.
Murilo Opsfelder Araujo (3):
powerpc/dt_cpu_ftrs: Remove unused macro ISA_V2_07B
powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_0B
powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_1
arch/powerpc/kernel/dt_cpu_ftrs.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--
2.25.4
^ permalink raw reply
* [PATCH 1/3] powerpc/dt_cpu_ftrs: Remove unused macro ISA_V2_07B
From: Murilo Opsfelder Araujo @ 2020-06-10 21:51 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
Cc: Murilo Opsfelder Araujo, Murilo Opsfelder Araujo, Paul Mackerras,
Aneesh Kumar K . V, Alistair Popple, Jordan Niethe, Daniel Axtens
In-Reply-To: <20200610215114.167544-1-muriloo@linux.ibm.com>
Macro ISA_V2_07B is defined but not used anywhere else in the code.
Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
---
arch/powerpc/kernel/dt_cpu_ftrs.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 3a409517c031..1b7da8b5ce0d 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -24,7 +24,6 @@
/* Device-tree visible constants follow */
-#define ISA_V2_07B 2070
#define ISA_V3_0B 3000
#define ISA_V3_1 3100
--
2.25.4
^ permalink raw reply related
* Re: [PATCH 2/3] powerpc/pci: unmap legacy INTx interrupts of passthrough IO adapters
From: Cédric Le Goater @ 2020-06-10 18:10 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <CAOSf1CHZ4+vEHotKzPDu2czVDBBM_oerxcCRS5QOFxsMbSknKQ@mail.gmail.com>
On 5/27/20 2:57 AM, Oliver O'Halloran wrote:
> On Wed, Apr 29, 2020 at 5:51 PM Cédric Le Goater <clg@kaod.org> wrote:
>>
>> When a passthrough IO adapter is removed from a pseries machine using
>> hash MMU and the XIVE interrupt mode, the POWER hypervisor, pHyp,
>> expects the guest OS to have cleared all page table entries related to
>> the adapter. If some are still present, the RTAS call which isolates
>> the PCI slot returns error 9001 "valid outstanding translations" and
>> the removal of the IO adapter fails.
>>
>> INTx interrupt numbers need special care because Linux maps the
>> interrupts automatically in the Linux interrupt number space if they
>> are presented in the device tree node describing the IO adapter. These
>> interrupts are not un-mapped automatically and in case of an hot-plug
>> adapter, the PCI hot-plug layer needs to handle the cleanup to make
>> sure that all the page table entries of the XIVE ESB pages are
>> cleared.
>>
>> Cc: "Oliver O'Halloran" <oohall@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>> arch/powerpc/kernel/pci-hotplug.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
>> index bf83f76563a3..9e9c6befd7ea 100644
>> --- a/arch/powerpc/kernel/pci-hotplug.c
>> +++ b/arch/powerpc/kernel/pci-hotplug.c
>> @@ -57,6 +57,8 @@ void pcibios_release_device(struct pci_dev *dev)
>> struct pci_controller *phb = pci_bus_to_host(dev->bus);
>> struct pci_dn *pdn = pci_get_pdn(dev);
>>
>> + irq_dispose_mapping(dev->irq);
>
> What does the original mapping? Powerpc arch code or the PCI core?
> Tearing down the mapping in pcibios_release_device() seems a bit fishy
> to me since the PCI core has already torn down the device state at
> that point. If the release is delayed it's possible that another
> pci_dev has mapped the IRQ before we get here, but maybe that's ok.
How's that below ? INTx mappings are cleared only when the PHB is removed.
It applies to all platforms but we could limit the removal to PHB hotplug
on pseries.
C.
From 10794159567552355f87e86e24002641c54e7ab5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Wed, 10 Jun 2020 19:55:24 +0200
Subject: [PATCH] powerpc/pci: unmap legacy INTx interrupts of passthrough IO
adapters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor, pHyp,
expects the guest OS to have cleared all page table entries related to
the adapter. If some are still present, the RTAS call which isolates
the PCI slot returns error 9001 "valid outstanding translations" and
the removal of the IO adapter fails.
INTx interrupt numbers need special care because Linux maps the
interrupts automatically in the Linux interrupt number space. These
interrupts are not un-mapped automatically and in case of an hot-plug
adapter, the PCI hot-plug layer needs to handle the cleanup to make
sure that all the page table entries of the ESB pages are cleared.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
arch/powerpc/include/asm/pci-bridge.h | 4 +++
arch/powerpc/kernel/pci-common.c | 47 +++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
index b92e81b256e5..9960dd249079 100644
--- a/arch/powerpc/include/asm/pci-bridge.h
+++ b/arch/powerpc/include/asm/pci-bridge.h
@@ -48,6 +48,8 @@ struct pci_controller_ops {
/*
* Structure of a PCI controller (host bridge)
+ *
+ * @intx: legacy INTx mappings
*/
struct pci_controller {
struct pci_bus *bus;
@@ -127,6 +129,8 @@ struct pci_controller {
void *private_data;
struct npu *npu;
+
+ unsigned int intx[PCI_NUM_INTX];
};
/* These are used for config access before all the PCI probing
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index be108616a721..795a9b49e0d6 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -353,6 +353,48 @@ struct pci_controller *pci_find_controller_for_domain(int domain_nr)
return NULL;
}
+static void pci_intx_register(struct pci_dev *pdev, int virq)
+{
+ struct pci_controller *phb = pci_bus_to_host(pdev->bus);
+ int i;
+
+ for (i = 0; i < PCI_NUM_INTX; i++) {
+ /*
+ * Look for an empty or an equivalent slot, IRQs can be
+ * shared
+ */
+ if (phb->intx[i] == virq || !phb->intx[i]) {
+ phb->intx[i] = virq;
+ break;
+ }
+ }
+
+ if (i == PCI_NUM_INTX)
+ pr_err("PCI:%s INTx all mapped\n", pci_name(pdev));
+}
+
+/*
+ * Clearing the mapped INTx interrupts will also clear the underlying
+ * mappings of the ESB pages of the interrupts when under XIVE. It is
+ * a requirement of PowerVM to clear all memory mappings before
+ * removing a PHB.
+ */
+static void pci_intx_dispose(struct pci_controller *phb)
+{
+ int i;
+
+ for (i = 0; i < PCI_NUM_INTX; i++)
+ irq_dispose_mapping(phb->intx[i]);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+ pr_debug("PCI: Clearing PHB %04x:%02x...\n",
+ pci_domain_nr(bus), bus->number);
+ pci_intx_dispose(pci_bus_to_host(bus));
+}
+EXPORT_SYMBOL_GPL(pcibios_remove_bus);
+
/*
* Reads the interrupt pin to determine if interrupt is use by card.
* If the interrupt is used, then gets the interrupt line from the
@@ -401,6 +443,11 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)
pci_dev->irq = virq;
+ /*
+ * Record all INTx mappings to clear them if the PHB is
+ * dynamically removed.
+ */
+ pci_intx_register(pci_dev, virq);
return 0;
}
--
2.25.4
^ permalink raw reply related
* Re: [PATCH v3 25/41] powerpc/book3s64/kuep: Store/restore userspace IAMR correctly on entry and exit from kernel
From: kernel test robot @ 2020-06-10 18:47 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
Cc: Aneesh Kumar K.V, kbuild-all, bauerman, linuxram
In-Reply-To: <20200610095204.608183-26-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 8059 bytes --]
Hi "Aneesh,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20200610]
[cannot apply to scottwood/next mpe/next v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Kernel-userspace-access-execution-prevention-with-hash-translation/20200610-191943
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r036-20200607 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
In file included from arch/powerpc/include/asm/processor.h:9,
from arch/powerpc/include/asm/thread_info.h:40,
from include/linux/thread_info.h:38,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h: In function 'kuap_restore_user_amr':
arch/powerpc/include/asm/book3s/64/kup.h:181:22: error: 'struct pt_regs' has no member named 'kuap'
181 | mtspr(SPRN_AMR, regs->kuap);
| ^~
arch/powerpc/include/asm/reg.h:1386:33: note: in definition of macro 'mtspr'
1386 | : "r" ((unsigned long)(v)) | ^
>> arch/powerpc/include/asm/book3s/64/kup.h:182:23: error: 'struct pt_regs' has no member named 'kuep'
182 | mtspr(SPRN_IAMR, regs->kuep);
| ^~
arch/powerpc/include/asm/reg.h:1386:33: note: in definition of macro 'mtspr'
1386 | : "r" ((unsigned long)(v)) | ^
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h: In function 'kuap_restore_kernel_amr':
arch/powerpc/include/asm/book3s/64/kup.h:194:20: error: 'struct pt_regs' has no member named 'kuap'
194 | if (unlikely(regs->kuap != amr)) {
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
In file included from arch/powerpc/include/asm/processor.h:9,
from arch/powerpc/include/asm/thread_info.h:40,
from include/linux/thread_info.h:38,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:196:24: error: 'struct pt_regs' has no member named 'kuap'
196 | mtspr(SPRN_AMR, regs->kuap);
| ^~
arch/powerpc/include/asm/reg.h:1386:33: note: in definition of macro 'mtspr'
1386 | : "r" ((unsigned long)(v)) | ^
In file included from arch/powerpc/include/asm/bug.h:109,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h: In function 'bad_kuap_fault':
arch/powerpc/include/asm/book3s/64/kup.h:293:12: error: 'struct pt_regs' has no member named 'kuap'
293 | (regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
| ^~
include/asm-generic/bug.h:122:25: note: in definition of macro 'WARN'
122 | int __ret_warn_on = !!(condition); | ^~~~~~~~~
In file included from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/kup.h: At top level:
arch/powerpc/include/asm/kup.h:56:20: error: redefinition of 'allow_user_access'
56 | static inline void allow_user_access(void __user *to, const void __user *from,
| ^~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/kup.h:18,
from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:254:29: note: previous definition of 'allow_user_access' was here
254 | static __always_inline void allow_user_access(void __user *to, const void __user *from,
| ^~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/kup.h:58:20: error: redefinition of 'prevent_user_access'
58 | static inline void prevent_user_access(void __user *to, const void __user *from,
| ^~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/kup.h:18,
from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:269:20: note: previous definition of 'prevent_user_access' was here
vim +182 arch/powerpc/include/asm/book3s/64/kup.h
174
175 static inline void kuap_restore_user_amr(struct pt_regs *regs)
176 {
177 if (!mmu_has_feature(MMU_FTR_PKEY))
178 return;
179
180 isync();
181 mtspr(SPRN_AMR, regs->kuap);
> 182 mtspr(SPRN_IAMR, regs->kuep);
183 /*
184 * No isync required here because we are about to rfi
185 * back to previous context before any user accesses
186 * would be made, which is a CSI.
187 */
188 }
189 static inline void kuap_restore_kernel_amr(struct pt_regs *regs,
190 unsigned long amr)
191 {
192 if (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)) {
193
194 if (unlikely(regs->kuap != amr)) {
195 isync();
196 mtspr(SPRN_AMR, regs->kuap);
197 /*
198 * No isync required here because we are about to rfi
199 * back to previous context before any user accesses
200 * would be made, which is a CSI.
201 */
202 }
203 }
204 /*
205 * No need to restore IAMR when returning to kernel space.
206 */
207 }
208
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33846 bytes --]
^ permalink raw reply
* Re: [PATCH v3 24/41] powerpc/book3s64/pkeys: Store/restore userspace AMR correctly on entry and exit from kernel
From: kernel test robot @ 2020-06-10 17:36 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, mpe
Cc: Aneesh Kumar K.V, kbuild-all, bauerman, linuxram
In-Reply-To: <20200610095204.608183-25-aneesh.kumar@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 10620 bytes --]
Hi "Aneesh,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20200610]
[cannot apply to scottwood/next mpe/next v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Kernel-userspace-access-execution-prevention-with-hash-translation/20200610-191943
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r036-20200607 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
In file included from arch/powerpc/include/asm/processor.h:9,
from arch/powerpc/include/asm/thread_info.h:40,
from include/linux/thread_info.h:38,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h: In function 'kuap_restore_user_amr':
>> arch/powerpc/include/asm/book3s/64/kup.h:142:22: error: 'struct pt_regs' has no member named 'kuap'
142 | mtspr(SPRN_AMR, regs->kuap);
| ^~
arch/powerpc/include/asm/reg.h:1386:33: note: in definition of macro 'mtspr'
1386 | : "r" ((unsigned long)(v)) | ^
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h: In function 'kuap_restore_kernel_amr':
arch/powerpc/include/asm/book3s/64/kup.h:155:20: error: 'struct pt_regs' has no member named 'kuap'
155 | if (unlikely(regs->kuap != amr)) {
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
In file included from arch/powerpc/include/asm/processor.h:9,
from arch/powerpc/include/asm/thread_info.h:40,
from include/linux/thread_info.h:38,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:157:24: error: 'struct pt_regs' has no member named 'kuap'
157 | mtspr(SPRN_AMR, regs->kuap);
| ^~
arch/powerpc/include/asm/reg.h:1386:33: note: in definition of macro 'mtspr'
1386 | : "r" ((unsigned long)(v)) | ^
In file included from arch/powerpc/include/asm/bug.h:109,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/compat.h:10,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h: In function 'bad_kuap_fault':
arch/powerpc/include/asm/book3s/64/kup.h:251:12: error: 'struct pt_regs' has no member named 'kuap'
251 | (regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
| ^~
include/asm-generic/bug.h:122:25: note: in definition of macro 'WARN'
122 | int __ret_warn_on = !!(condition); | ^~~~~~~~~
In file included from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/kup.h: At top level:
>> arch/powerpc/include/asm/kup.h:56:20: error: redefinition of 'allow_user_access'
56 | static inline void allow_user_access(void __user *to, const void __user *from,
| ^~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/kup.h:18,
from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:212:29: note: previous definition of 'allow_user_access' was here
212 | static __always_inline void allow_user_access(void __user *to, const void __user *from,
| ^~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
>> arch/powerpc/include/asm/kup.h:58:20: error: redefinition of 'prevent_user_access'
58 | static inline void prevent_user_access(void __user *to, const void __user *from,
| ^~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/kup.h:18,
from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:227:20: note: previous definition of 'prevent_user_access' was here
227 | static inline void prevent_user_access(void __user *to, const void __user *from,
| ^~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
>> arch/powerpc/include/asm/kup.h:60:29: error: redefinition of 'prevent_user_access_return'
60 | static inline unsigned long prevent_user_access_return(void) { return 0UL; }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/kup.h:18,
from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:233:29: note: previous definition of 'prevent_user_access_return' was here
233 | static inline unsigned long prevent_user_access_return(void)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
>> arch/powerpc/include/asm/kup.h:61:20: error: redefinition of 'restore_user_access'
61 | static inline void restore_user_access(unsigned long flags) { }
| ^~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/kup.h:18,
from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:242:20: note: previous definition of 'restore_user_access' was here
242 | static inline void restore_user_access(unsigned long flags)
| ^~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
>> arch/powerpc/include/asm/kup.h:63:1: error: redefinition of 'bad_kuap_fault'
63 | bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
| ^~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/kup.h:18,
from arch/powerpc/include/asm/uaccess.h:9,
from include/linux/uaccess.h:11,
from include/linux/crypto.h:21,
from include/crypto/hash.h:11,
from include/linux/uio.h:10,
from include/linux/socket.h:8,
from include/linux/compat.h:15,
from arch/powerpc/kernel/asm-offsets.c:14:
arch/powerpc/include/asm/book3s/64/kup.h:248:1: note: previous definition of 'bad_kuap_fault' was here
248 | bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
| ^~~~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:100: arch/powerpc/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [Makefile:1141: prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:180: sub-make] Error 2
vim +142 arch/powerpc/include/asm/book3s/64/kup.h
135
136 static inline void kuap_restore_user_amr(struct pt_regs *regs)
137 {
138 if (!mmu_has_feature(MMU_FTR_PKEY))
139 return;
140
141 isync();
> 142 mtspr(SPRN_AMR, regs->kuap);
143 /*
144 * No isync required here because we are about to rfi
145 * back to previous context before any user accesses
146 * would be made, which is a CSI.
147 */
148 }
149
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33846 bytes --]
^ permalink raw reply
* Re: [PATCH v11 5/6] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
From: Dan Williams @ 2020-06-10 15:11 UTC (permalink / raw)
To: Vaibhav Jain
Cc: Santosh Sivaraj, kbuild-all, kernel test robot, linux-nvdimm,
Aneesh Kumar K . V, Linux Kernel Mailing List, Steven Rostedt,
clang-built-linux, Oliver O'Halloran, linuxppc-dev
In-Reply-To: <87k10fw29r.fsf@linux.ibm.com>
On Wed, Jun 10, 2020 at 5:10 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > On Tue, Jun 9, 2020 at 10:54 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
> >>
> >> Thanks Dan for the consideration and taking time to look into this.
> >>
> >> My responses below:
> >>
> >> Dan Williams <dan.j.williams@intel.com> writes:
> >>
> >> > On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <lkp@intel.com> wrote:
> >> >>
> >> >> Hi Vaibhav,
> >> >>
> >> >> Thank you for the patch! Perhaps something to improve:
> >> >>
> >> >> [auto build test WARNING on powerpc/next]
> >> >> [also build test WARNING on linus/master v5.7 next-20200605]
> >> >> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
> >> >> [if your patch is applied to the wrong git tree, please drop us a note to help
> >> >> improve the system. BTW, we also suggest to use '--base' option to specify the
> >> >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> >> >>
> >> >> url: https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
> >> >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> >> >> config: powerpc-randconfig-r016-20200607 (attached as .config)
> >> >> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
> >> >> 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
> >> >> # save the attached .config to linux build tree
> >> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
> >> >>
> >> >> 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 >>, old ones prefixed by <<):
> >> >>
> >> >> In file included from <built-in>:1:
> >> >> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> >> >> struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
> >> >
> >> > Hi Vaibhav,
> >> >
> >> [.]
> >> > This looks like it's going to need another round to get this fixed. I
> >> > don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
> >> > 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
> >> > payload that is the 'pdsm' specifics. As the code has it now it's
> >> > defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
> >> > is pointing out a real 'struct' organization problem.
> >> >
> >> > Given the soak time needed in -next after the code is finalized this
> >> > there's no time to do another round of updates and still make the v5.8
> >> > merge window.
> >>
> >> Agreed that this looks bad, a solution will probably need some more
> >> review cycles resulting in this series missing the merge window.
> >>
> >> I am investigating into the possible solutions for this reported issue
> >> and made few observations:
> >>
> >> I see command pkg for Intel, Hpe, Msft and Hyperv families using a
> >> similar layout of embedding nd_cmd_pkg at the head of the
> >> command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.
> >>
> >> struct nd_pdsm_cmd_pkg {
> >> struct nd_cmd_pkg hdr;
> >> /* other members */
> >> };
> >>
> >> struct ndn_pkg_msft {
> >> struct nd_cmd_pkg gen;
> >> /* other members */
> >> };
> >> struct nd_pkg_intel {
> >> struct nd_cmd_pkg gen;
> >> /* other members */
> >> };
> >> struct ndn_pkg_hpe1 {
> >> struct nd_cmd_pkg gen;
> >> /* other members */
> [.]
> >
> > In those cases the other members are a union and there is no second
> > variable length array. Perhaps that is why those definitions are not
> > getting flagged? I'm not seeing anything in ndctl build options that
> > would explicitly disable this warning, but I'm not sure if the ndctl
> > build environment is missing this build warning by accident.
>
> I tried building ndctl master with clang-10 with CC=clang and
> CFLAGS="". Seeing the same warning messages reported for all command
> package struct for existing command families.
>
> ./hpe1.h:334:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> struct nd_cmd_pkg gen;
> ^
> ./msft.h:59:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> struct nd_cmd_pkg gen;
> ^
> ./hyperv.h:34:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> struct nd_cmd_pkg gen;
> ^
Good to know, but ugh now I'm just realizing this warning is only
coming from clang and not gcc. Frankly I'm not as concerned about
clang warnings and I should have been more careful looking at the
source of this warning.
> >
> > Those variable size payloads are also not being used in any code paths
> > that would look at the size of the command payload, like the kernel
> > ioctl() path. The payload validation code needs static sizes and the
> > payload parsing code wants to cast the payload to a known type. I
> > don't think you can use the same struct definition for both those
> > cases which is why the ndctl parsing code uses the union layout, but
> > the kernel command marshaling code does strict layering.
> Even if I switch to union layout and replacing the flexible array 'payload'
> at end to a fixed size array something like below, I still see
> '-Wgnu-variable-sized-type-not-at-end' warning reported by clang:
>
> union nd_pdsm_cmd_payload {
> struct nd_papr_pdsm_health health;
> __u8 buf[ND_PDSM_PAYLOAD_MAX_SIZE];
> };
>
> struct nd_pdsm_cmd_pkg {
> struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
> __s32 cmd_status; /* Out: Sub-cmd status returned back */
> __u16 reserved[2]; /* Ignored and to be used in future */
> union nd_pdsm_cmd_payload payload;
> } __attribute__((packed));
Even though this is a clang warning, I'm still not sure it's a good
idea to copy the ndctl approach into the kernel. Could you perhaps
handle this the way that drivers/acpi/nfit/intel.c handles submitting
commands through the ND_CMD_CALL interface, i.e. by just defining the
command locally like this (from intel_security_flags()):
struct {
struct nd_cmd_pkg pkg;
struct nd_intel_get_security_state cmd;
} nd_cmd = {
.pkg = {
.nd_command = NVDIMM_INTEL_GET_SECURITY_STATE,
.nd_family = NVDIMM_FAMILY_INTEL,
.nd_size_out =
sizeof(struct nd_intel_get_security_state),
.nd_fw_size =
sizeof(struct nd_intel_get_security_state),
},
};
That way it's clear that the payload is 'struct
nd_intel_get_security_state' without needing to have a pre-existing
definition. For parsing in the ioctl path I think it's clearer to cast
the payload to the local pdsm structure for the command.
>
>
> >
> >> };
> >>
> >> Even though other command families implement similar command-package
> >> layout they were not flagged (yet) as they are (I am guessing) serviced
> >> in vendor acpi drivers rather than in kernel like in case of papr-scm
> >> command family.
> >
> > I sincerely hope there are no vendor acpi kernel drivers outside of
> > the upstream one.
> Apologies if I was not clear. Was referring to nvdimm vendor uefi
> drivers which ultimately service the DSM commands. Since CMD_CALL serves
> as a conduit to send the command payload to these vendor drivers,
> libnvdimm never needs to peek into the nd_cmd_pkg.payload
> field. Consequently nfit module never hit this warning in kernel before.
Ah, understood, and no, that's not the root reason this problem is not
present in the kernel. The expectation is that any payload that the
kernel would need to consider should probably have a kernel specific
translation defined. For example,
ND_CMD_GET_CONFIG_SIZE
ND_CMD_GET_CONFIG_DATA
ND_CMD_SET_CONFIG_DATA
...are payloads that the kernel needs to understand. However instead
of supporting each way to read / write the label area the expectation
is that all drivers just parse this common kernel payload and
translate it if necessary. For example ND_CMD_{GET,SET}_CONFIG_DATA is
optionally translated to the Intel DSMs, generic ACPI _LSR/_LSW, or
papr_scm_meta_{get,set}.
Outside of validating command numbers the expectation is that the
kernel does not validate/consume the contents of the ND_CMD_CALL
payload, it passes it to the backend where ACPI DSM or pdsm protocol
takes over.
>
> >
> >>
> >> So, I think this issue is not just specific to papr-scm command family
> >> introduced in this patch series but rather across all other command
> >> families. Every other command family assumes 'struct nd_cmd_pkg_hdr' to
> >> be embeddable and puts it at the beginning of their corresponding
> >> command-packages. And its only a matter of time when someone tries
> >> filtering/handling of vendor specific commands in nfit module when they
> >> hit similar issue.
> >>
> >> Possible Solutions:
> >>
> >> * One way would be to redefine 'struct nd_cmd_pkg' to mark field
> >> 'nd_payload[]' from a flexible array to zero sized array as
> >> 'nd_payload[0]'.
> >
> > I just went through a round of removing the usage of buf[0] in ndctl
> > since gcc10 now warns about that too.
> >
> >> This should make 'struct nd_cmd_pkg' embeddable and
> >> clang shouldn't report 'gnu-variable-sized-type-not-at-end'
> >> warning. Also I think this change shouldn't introduce any ABI change.
> >>
> >> * Another way to solve this issue might be to redefine 'struct
> >> nd_pdsm_cmd_pkg' to below removing the 'struct nd_cmd_pkg' member. This
> >> struct should immediately follow the 'struct nd_cmd_pkg' command package
> >> when sent to libnvdimm:
> >>
> >> struct nd_pdsm_cmd_pkg {
> >> __s32 cmd_status; /* Out: Sub-cmd status returned back */
> >> __u16 reserved[2]; /* Ignored and to be used in future */
> >> __u8 payload[];
> >> };
> >>
> >> This should remove the flexible member nc_cmd_pkg.nd_payload from the
> >> struct with just one remaining at the end. However this would make
> >> accessing the [in|out|fw]_size members of 'struct nd_cmd_pkg'
> >> difficult for the pdsm servicing functions.
> >>
> >>
> >> Any other solution that you think, may solve this issue ?
> >
> > The union might help, but per the above I think only for parsing the
> > command at which point I don't think the kernel needs a unified
> > structure defining both the generic envelope and the end-point
> > specific payload at once.
>
> As I tested above, switching to union too will not solve the clang
> warning.
>
> Having a unified structure for a command family defining both
> generic envelop and end-point specific payload, is what I see all the
> existing command families doing.
>
> However if I split 'struct nd_pdsm_cmd_pkg' to not have an embedded
> 'struct nd_cmd_pkg' then it goes opposite to what existing command family
> implementations.
>
> So to me it looks like no clear way to address this :-(
>
> Another non-conventional way to fix this might be to suppress this clang
> warning messages by adding "CFLAGS_papr_scm.o +=
> -Wno-gnu-variable-sized-type-not-at-end" to papr_scm Makefile.
No, I don't think it's appropriate to customize clang warnings. Have a
look at splitting parsing vs local command submission following the
approach taken in drivers/acpi/nfit/intel.c.
> Current implementation 'struct nd_cmd_pkg' clearly depends on gcc
> extension of having a flexible payload array which is allowed to be not
> necessarily placed at the end of a containing struct. So the problem can be
> attributed to difference in compiler implementations between GCC and
> Clang rather than how 'struct nd_pdsm_cmd_pkg' and 'struct nd_cmd_pkg'
> are laid out.
>
> --
> Cheers
> ~ Vaibhav
^ permalink raw reply
* Re: [PATCH? v2] powerpc: Hard wire PT_SOFTE value to 1 in gpr_get() too
From: Oleg Nesterov @ 2020-06-10 15:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Madhavan Srinivasan, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, Jan Kratochvil, linux-kernel
In-Reply-To: <20190917143753.GA12300@redhat.com>
Hi,
looks like this patch was forgotten.
Do you think this should be fixed or should we document that
PTRACE_GETREGS is not consistent with PTRACE_PEEKUSER on ppc64?
On 09/17, Oleg Nesterov wrote:
>
> I don't have a ppc machine, this patch wasn't even compile tested,
> could you please review?
>
> The commit a8a4b03ab95f ("powerpc: Hard wire PT_SOFTE value to 1 in
> ptrace & signals") changed ptrace_get_reg(PT_SOFTE) to report 0x1,
> but PTRACE_GETREGS still copies pt_regs->softe as is.
>
> This is not consistent and this breaks
> http://sourceware.org/systemtap/wiki/utrace/tests/user-regs-peekpoke
>
> Reported-by: Jan Kratochvil <jan.kratochvil@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> arch/powerpc/kernel/ptrace.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 8c92feb..291acfb 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -363,11 +363,36 @@ static int gpr_get(struct task_struct *target, const struct user_regset *regset,
> BUILD_BUG_ON(offsetof(struct pt_regs, orig_gpr3) !=
> offsetof(struct pt_regs, msr) + sizeof(long));
>
> +#ifdef CONFIG_PPC64
> + if (!ret)
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + &target->thread.regs->orig_gpr3,
> + offsetof(struct pt_regs, orig_gpr3),
> + offsetof(struct pt_regs, softe));
> +
> + if (!ret) {
> + unsigned long softe = 0x1;
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &softe,
> + offsetof(struct pt_regs, softe),
> + offsetof(struct pt_regs, softe) +
> + sizeof(softe));
> + }
> +
> + BUILD_BUG_ON(offsetof(struct pt_regs, trap) !=
> + offsetof(struct pt_regs, softe) + sizeof(long));
> +
> + if (!ret)
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + &target->thread.regs->trap,
> + offsetof(struct pt_regs, trap),
> + sizeof(struct user_pt_regs));
> +#else
> if (!ret)
> ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> &target->thread.regs->orig_gpr3,
> offsetof(struct pt_regs, orig_gpr3),
> sizeof(struct user_pt_regs));
> +#endif
> if (!ret)
> ret = user_regset_copyout_zero(&pos, &count, &kbuf, &ubuf,
> sizeof(struct user_pt_regs), -1);
> --
> 2.5.0
>
^ permalink raw reply
* Re: [PATCH v2 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR
From: Christophe Leroy @ 2020-06-10 12:41 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20200403093529.43587-1-npiggin@gmail.com>
Hi Nick
Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
> There is no need to allow user accesses when probing kernel addresses.
You should have a look at
https://github.com/torvalds/linux/commit/fa94111d94354de76c47fea6e1187d1ee91e23a7
At seems to implement a generic way of achieving what you are trying to
do here.
Christophe
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2:
> - Enable for all powerpc (suggested by Christophe)
> - Fold helper function together (Christophe)
> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>
> arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
> arch/powerpc/lib/Makefile | 2 +-
> arch/powerpc/lib/maccess.c | 34 ++++++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+), 9 deletions(-)
> create mode 100644 arch/powerpc/lib/maccess.c
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 2f500debae21..670910df3cc7 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -341,8 +341,8 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
> }
> #endif /* __powerpc64__ */
>
> -static inline unsigned long raw_copy_from_user(void *to,
> - const void __user *from, unsigned long n)
> +static inline unsigned long
> +raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
> {
> unsigned long ret;
> if (__builtin_constant_p(n) && (n <= 8)) {
> @@ -351,19 +351,19 @@ static inline unsigned long raw_copy_from_user(void *to,
> switch (n) {
> case 1:
> barrier_nospec();
> - __get_user_size(*(u8 *)to, from, 1, ret);
> + __get_user_size_allowed(*(u8 *)to, from, 1, ret);
> break;
> case 2:
> barrier_nospec();
> - __get_user_size(*(u16 *)to, from, 2, ret);
> + __get_user_size_allowed(*(u16 *)to, from, 2, ret);
> break;
> case 4:
> barrier_nospec();
> - __get_user_size(*(u32 *)to, from, 4, ret);
> + __get_user_size_allowed(*(u32 *)to, from, 4, ret);
> break;
> case 8:
> barrier_nospec();
> - __get_user_size(*(u64 *)to, from, 8, ret);
> + __get_user_size_allowed(*(u64 *)to, from, 8, ret);
> break;
> }
> if (ret == 0)
> @@ -371,9 +371,18 @@ static inline unsigned long raw_copy_from_user(void *to,
> }
>
> barrier_nospec();
> - allow_read_from_user(from, n);
> ret = __copy_tofrom_user((__force void __user *)to, from, n);
> - prevent_read_from_user(from, n);
> + return ret;
> +}
> +
> +static inline unsigned long
> +raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> +{
> + unsigned long ret;
> +
> + allow_read_from_user(to, n);
> + ret = raw_copy_from_user_allowed(to, from, n);
> + prevent_read_from_user(to, n);
> return ret;
> }
>
> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index b8de3be10eb4..77af10ad0b0d 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
> CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
> endif
>
> -obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
> +obj-y += alloc.o code-patching.o feature-fixups.o pmem.o maccess.o
>
> ifndef CONFIG_KASAN
> obj-y += string.o memcmp_$(BITS).o
> diff --git a/arch/powerpc/lib/maccess.c b/arch/powerpc/lib/maccess.c
> new file mode 100644
> index 000000000000..ce5465db1e2d
> --- /dev/null
> +++ b/arch/powerpc/lib/maccess.c
> @@ -0,0 +1,34 @@
> +#include <linux/mm.h>
> +#include <linux/uaccess.h>
> +
> +/*
> + * Override the generic weak linkage functions to avoid changing KUP state via
> + * the generic user access functions, as this is accessing kernel addresses.
> + */
> +long probe_kernel_read(void *dst, const void *src, size_t size)
> +{
> + long ret;
> + mm_segment_t old_fs = get_fs();
> +
> + set_fs(KERNEL_DS);
> + pagefault_disable();
> + ret = raw_copy_from_user_allowed(dst, (__force const void __user *)src, size);
> + pagefault_enable();
> + set_fs(old_fs);
> +
> + return ret ? -EFAULT : 0;
> +}
> +
> +long probe_kernel_write(void *dst, const void *src, size_t size)
> +{
> + long ret;
> + mm_segment_t old_fs = get_fs();
> +
> + set_fs(KERNEL_DS);
> + pagefault_disable();
> + ret = raw_copy_to_user_allowed((__force void __user *)dst, src, size);
> + pagefault_enable();
> + set_fs(old_fs);
> +
> + return ret ? -EFAULT : 0;
> +}
>
^ permalink raw reply
* Re: [PATCH v11 5/6] ndctl/papr_scm, uapi: Add support for PAPR nvdimm specific methods
From: Vaibhav Jain @ 2020-06-10 12:09 UTC (permalink / raw)
To: Dan Williams
Cc: Santosh Sivaraj, kbuild-all, kernel test robot, linux-nvdimm,
Aneesh Kumar K . V, Linux Kernel Mailing List, Steven Rostedt,
clang-built-linux, Oliver O'Halloran, linuxppc-dev
In-Reply-To: <CAPcyv4jfeBoFCdg2sKP5ExpTTQ_+LyrJewTupcrTgh-qWykNxw@mail.gmail.com>
Dan Williams <dan.j.williams@intel.com> writes:
> On Tue, Jun 9, 2020 at 10:54 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>>
>> Thanks Dan for the consideration and taking time to look into this.
>>
>> My responses below:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > On Mon, Jun 8, 2020 at 5:16 PM kernel test robot <lkp@intel.com> wrote:
>> >>
>> >> Hi Vaibhav,
>> >>
>> >> Thank you for the patch! Perhaps something to improve:
>> >>
>> >> [auto build test WARNING on powerpc/next]
>> >> [also build test WARNING on linus/master v5.7 next-20200605]
>> >> [cannot apply to linux-nvdimm/libnvdimm-for-next scottwood/next]
>> >> [if your patch is applied to the wrong git tree, please drop us a note to help
>> >> improve the system. BTW, we also suggest to use '--base' option to specify the
>> >> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>> >>
>> >> url: https://github.com/0day-ci/linux/commits/Vaibhav-Jain/powerpc-papr_scm-Add-support-for-reporting-nvdimm-health/20200607-211653
>> >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
>> >> config: powerpc-randconfig-r016-20200607 (attached as .config)
>> >> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
>> >> 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
>> >> # save the attached .config to linux build tree
>> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
>> >>
>> >> 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 >>, old ones prefixed by <<):
>> >>
>> >> In file included from <built-in>:1:
>> >> >> ./usr/include/asm/papr_pdsm.h:69:20: warning: field 'hdr' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>> >> struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
>> >
>> > Hi Vaibhav,
>> >
>> [.]
>> > This looks like it's going to need another round to get this fixed. I
>> > don't think 'struct nd_pdsm_cmd_pkg' should embed a definition of
>> > 'struct nd_cmd_pkg'. An instance of 'struct nd_cmd_pkg' carries a
>> > payload that is the 'pdsm' specifics. As the code has it now it's
>> > defined as a superset of 'struct nd_cmd_pkg' and the compiler warning
>> > is pointing out a real 'struct' organization problem.
>> >
>> > Given the soak time needed in -next after the code is finalized this
>> > there's no time to do another round of updates and still make the v5.8
>> > merge window.
>>
>> Agreed that this looks bad, a solution will probably need some more
>> review cycles resulting in this series missing the merge window.
>>
>> I am investigating into the possible solutions for this reported issue
>> and made few observations:
>>
>> I see command pkg for Intel, Hpe, Msft and Hyperv families using a
>> similar layout of embedding nd_cmd_pkg at the head of the
>> command-pkg. struct nd_pdsm_cmd_pkg is following the same pattern.
>>
>> struct nd_pdsm_cmd_pkg {
>> struct nd_cmd_pkg hdr;
>> /* other members */
>> };
>>
>> struct ndn_pkg_msft {
>> struct nd_cmd_pkg gen;
>> /* other members */
>> };
>> struct nd_pkg_intel {
>> struct nd_cmd_pkg gen;
>> /* other members */
>> };
>> struct ndn_pkg_hpe1 {
>> struct nd_cmd_pkg gen;
>> /* other members */
[.]
>
> In those cases the other members are a union and there is no second
> variable length array. Perhaps that is why those definitions are not
> getting flagged? I'm not seeing anything in ndctl build options that
> would explicitly disable this warning, but I'm not sure if the ndctl
> build environment is missing this build warning by accident.
I tried building ndctl master with clang-10 with CC=clang and
CFLAGS="". Seeing the same warning messages reported for all command
package struct for existing command families.
./hpe1.h:334:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
struct nd_cmd_pkg gen;
^
./msft.h:59:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
struct nd_cmd_pkg gen;
^
./hyperv.h:34:20: warning: field 'gen' with variable sized type 'struct nd_cmd_pkg' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
struct nd_cmd_pkg gen;
^
>
> Those variable size payloads are also not being used in any code paths
> that would look at the size of the command payload, like the kernel
> ioctl() path. The payload validation code needs static sizes and the
> payload parsing code wants to cast the payload to a known type. I
> don't think you can use the same struct definition for both those
> cases which is why the ndctl parsing code uses the union layout, but
> the kernel command marshaling code does strict layering.
Even if I switch to union layout and replacing the flexible array 'payload'
at end to a fixed size array something like below, I still see
'-Wgnu-variable-sized-type-not-at-end' warning reported by clang:
union nd_pdsm_cmd_payload {
struct nd_papr_pdsm_health health;
__u8 buf[ND_PDSM_PAYLOAD_MAX_SIZE];
};
struct nd_pdsm_cmd_pkg {
struct nd_cmd_pkg hdr; /* Package header containing sub-cmd */
__s32 cmd_status; /* Out: Sub-cmd status returned back */
__u16 reserved[2]; /* Ignored and to be used in future */
union nd_pdsm_cmd_payload payload;
} __attribute__((packed));
>
>> };
>>
>> Even though other command families implement similar command-package
>> layout they were not flagged (yet) as they are (I am guessing) serviced
>> in vendor acpi drivers rather than in kernel like in case of papr-scm
>> command family.
>
> I sincerely hope there are no vendor acpi kernel drivers outside of
> the upstream one.
Apologies if I was not clear. Was referring to nvdimm vendor uefi
drivers which ultimately service the DSM commands. Since CMD_CALL serves
as a conduit to send the command payload to these vendor drivers,
libnvdimm never needs to peek into the nd_cmd_pkg.payload
field. Consequently nfit module never hit this warning in kernel before.
>
>>
>> So, I think this issue is not just specific to papr-scm command family
>> introduced in this patch series but rather across all other command
>> families. Every other command family assumes 'struct nd_cmd_pkg_hdr' to
>> be embeddable and puts it at the beginning of their corresponding
>> command-packages. And its only a matter of time when someone tries
>> filtering/handling of vendor specific commands in nfit module when they
>> hit similar issue.
>>
>> Possible Solutions:
>>
>> * One way would be to redefine 'struct nd_cmd_pkg' to mark field
>> 'nd_payload[]' from a flexible array to zero sized array as
>> 'nd_payload[0]'.
>
> I just went through a round of removing the usage of buf[0] in ndctl
> since gcc10 now warns about that too.
>
>> This should make 'struct nd_cmd_pkg' embeddable and
>> clang shouldn't report 'gnu-variable-sized-type-not-at-end'
>> warning. Also I think this change shouldn't introduce any ABI change.
>>
>> * Another way to solve this issue might be to redefine 'struct
>> nd_pdsm_cmd_pkg' to below removing the 'struct nd_cmd_pkg' member. This
>> struct should immediately follow the 'struct nd_cmd_pkg' command package
>> when sent to libnvdimm:
>>
>> struct nd_pdsm_cmd_pkg {
>> __s32 cmd_status; /* Out: Sub-cmd status returned back */
>> __u16 reserved[2]; /* Ignored and to be used in future */
>> __u8 payload[];
>> };
>>
>> This should remove the flexible member nc_cmd_pkg.nd_payload from the
>> struct with just one remaining at the end. However this would make
>> accessing the [in|out|fw]_size members of 'struct nd_cmd_pkg'
>> difficult for the pdsm servicing functions.
>>
>>
>> Any other solution that you think, may solve this issue ?
>
> The union might help, but per the above I think only for parsing the
> command at which point I don't think the kernel needs a unified
> structure defining both the generic envelope and the end-point
> specific payload at once.
As I tested above, switching to union too will not solve the clang
warning.
Having a unified structure for a command family defining both
generic envelop and end-point specific payload, is what I see all the
existing command families doing.
However if I split 'struct nd_pdsm_cmd_pkg' to not have an embedded
'struct nd_cmd_pkg' then it goes opposite to what existing command family
implementations.
So to me it looks like no clear way to address this :-(
Another non-conventional way to fix this might be to suppress this clang
warning messages by adding "CFLAGS_papr_scm.o +=
-Wno-gnu-variable-sized-type-not-at-end" to papr_scm Makefile.
Current implementation 'struct nd_cmd_pkg' clearly depends on gcc
extension of having a flexible payload array which is allowed to be not
necessarily placed at the end of a containing struct. So the problem can be
attributed to difference in compiler implementations between GCC and
Clang rather than how 'struct nd_pdsm_cmd_pkg' and 'struct nd_cmd_pkg'
are laid out.
--
Cheers
~ Vaibhav
^ permalink raw reply
* Re: PowerPC KVM-PR issue
From: Christian Zigotzky @ 2020-06-10 11:23 UTC (permalink / raw)
To: linuxppc-dev, Alexander Graf, kvm-ppc@vger.kernel.org
Cc: Darren Stevens, R.T.Dickinson, Christian Zigotzky
In-Reply-To: <cf99a8c0-3bad-d089-de54-e02d3dba7f72@xenosoft.de>
On 10 June 2020 at 11:06 am, Christian Zigotzky wrote:
> On 10 June 2020 at 00:18 am, Christian Zigotzky wrote:
>> Hello,
>>
>> KVM-PR doesn't work anymore on my Nemo board [1]. I figured out that
>> the Git kernels and the kernel 5.7 are affected.
>>
>> Error message: Fienix kernel: kvmppc_exit_pr_progint: emulation at
>> 700 failed (00000000)
>>
>> I can boot virtual QEMU PowerPC machines with KVM-PR with the kernel
>> 5.6 without any problems on my Nemo board.
>>
>> I tested it with QEMU 2.5.0 and QEMU 5.0.0 today.
>>
>> Could you please check KVM-PR on your PowerPC machine?
>>
>> Thanks,
>> Christian
>>
>> [1] https://en.wikipedia.org/wiki/AmigaOne_X1000
>
> I figured out that the PowerPC updates 5.7-1 [1] are responsible for
> the KVM-PR issue. Please test KVM-PR on your PowerPC machines and
> check the PowerPC updates 5.7-1 [1].
>
> Thanks
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d38c07afc356ddebaa3ed8ecb3f553340e05c969
>
>
I tested the latest Git kernel with Mac-on-Linux/KVM-PR today.
Unfortunately I can't use KVM-PR with MoL anymore because of this issue
(see screenshots [1]). Please check the PowerPC updates 5.7-1.
Thanks
[1]
-
https://i.pinimg.com/originals/0c/b3/64/0cb364a40241fa2b7f297d4272bbb8b7.png
-
https://i.pinimg.com/originals/9a/61/d1/9a61d170b1c9f514f7a78a3014ffd18f.png
^ permalink raw reply
* Re: [PATCH] powerpc/pseries/svm: Fixup align argument in alloc_shared_lppaca() function
From: Michael Ellerman @ 2020-06-10 11:21 UTC (permalink / raw)
To: Satheesh Rajendran, linuxppc-dev
Cc: Sukadev Bhattiprolu, Ram Pai, linux-kernel, Satheesh Rajendran,
Laurent Dufour, Thiago Jung Bauermann
In-Reply-To: <20200609113909.17236-1-sathnaga@linux.vnet.ibm.com>
Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> writes:
> Argument "align" in alloc_shared_lppaca() function was unused inside the
> function. Let's fix it and update code comment.
I think it would be better to drop the align argument entirely and keep
that logic, and the comment, internal to alloc_shared_lppaca().
cheers
^ permalink raw reply
* Re: [PATCH] tty: serial: cpm_uart: Fix behaviour for non existing GPIOs
From: Linus Walleij @ 2020-06-10 11:18 UTC (permalink / raw)
To: Christophe Leroy
Cc: Greg Kroah-Hartman, linuxppc-dev@lists.ozlabs.org list,
linux-kernel@vger.kernel.org, open list:SERIAL DRIVERS,
Jiri Slaby
In-Reply-To: <bafd8df9e743c433196c727293c5015620fae2b8.1591428452.git.christophe.leroy@csgroup.eu>
Hi Christophe!
On Sat, Jun 6, 2020 at 9:30 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
> gpiod = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
>
> - if (gpiod) {
> + if (!IS_ERR_OR_NULL(gpiod)) {
> if (i == GPIO_RTS || i == GPIO_DTR)
> ret = gpiod_direction_output(gpiod, 0);
> else
This code, and the way descriptors are used in the driver leads
me to believe that the right solution is to use the optional
call with a hard error check:
gpiod = devm_gpiod_get_index_optional(...);
if (IS_ERR(gpiod))
return PTR_ERR(gpiod);
if (gpiod) {
... followed by the old code ...
This makes sure that the array member is left NULL if there is no
GPIO on this line, and all other errors, such as -EPROBE_DEFER
which currently absolutely does not work, will lead to us properly
exiting with an error.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 0/6] consolidate PowerPC instruction encoding macros
From: Michael Ellerman @ 2020-06-10 10:50 UTC (permalink / raw)
To: Balamuruhan S
Cc: christophe.leroy, ravi.bangoria, jniethe5, Balamuruhan S, paulus,
sandipan, naveen.n.rao, linuxppc-dev
In-Reply-To: <20200526081523.92463-1-bala24@linux.ibm.com>
Balamuruhan S <bala24@linux.ibm.com> writes:
> ppc-opcode.h have base instruction encoding wrapped with stringify_in_c()
> for raw encoding to have compatibility. But there are redundant macros for
> base instruction encodings in bpf, instruction emulation test infrastructure
> and powerpc selftests.
>
> Currently PPC_INST_* macros are used for encoding instruction opcode and PPC_*
> for raw instuction encoding, this rfc patchset introduces PPC_RAW_* macros for
> base instruction encoding and reuse it from elsewhere. With this change we can
> avoid redundant macro definitions in multiple files and start adding new
> instructions in ppc-opcode.h in future.
Sorry this series collided with the prefixed instruction support and I
didn't have time to resolve the conflicts.
Can you please rebase on top of current mainline and resend.
cheers
^ permalink raw reply
* Re: [PATCH 1/6] powerpc/ppc-opcode: introduce PPC_RAW_* macros for base instruction encoding
From: Michael Ellerman @ 2020-06-10 10:50 UTC (permalink / raw)
To: Balamuruhan S
Cc: christophe.leroy, ravi.bangoria, jniethe5, Balamuruhan S, paulus,
sandipan, naveen.n.rao, linuxppc-dev
In-Reply-To: <20200526081523.92463-2-bala24@linux.ibm.com>
Balamuruhan S <bala24@linux.ibm.com> writes:
> Introduce PPC_RAW_* macros to have all the bare encoding of ppc
> instructions. Move `VSX_XX*()` and `TMRN()` macros up to reuse it.
>
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 183 ++++++++++++++++++++++++--
> 1 file changed, 175 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index 2a39c716c343..e3540be1fc17 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -431,6 +431,181 @@
> #define __PPC_EH(eh) 0
> #endif
>
> +/* Base instruction encoding */
> +#define PPC_RAW_CP_ABORT (PPC_INST_CP_ABORT)
> +#define PPC_RAW_COPY(a, b) (PPC_INST_COPY | ___PPC_RA(a) | \
> + ___PPC_RB(b))
> +#define PPC_RAW_DARN(t, l) (PPC_INST_DARN | ___PPC_RT(t) | \
> + (((l) & 0x3) << 16))
I know you're copying the existing formatting by wrapping these lines,
but please don't.
It hurts rather than improves readability.
For a line like the one above, just let it be long, eg:
#define PPC_RAW_DARN(t, l) (PPC_INST_DARN | ___PPC_RT(t) | (((l) & 0x3) << 16))
Yeah it's 91 columns but who cares.
For the really long ones like:
> +#define PPC_RAW_TLBIE_5(rb, rs, ric, prs, r) \
> + (PPC_INST_TLBIE | \
> + ___PPC_RB(rb) | \
> + ___PPC_RS(rs) | \
> + ___PPC_RIC(ric) | \
> + ___PPC_PRS(prs) | \
> + ___PPC_R(r))
I think this is the best option:
#define PPC_RAW_TLBIE_5(rb, rs, ric, prs, r) \
(PPC_INST_TLBIE | ___PPC_RB(rb) | ___PPC_RS(rs) | ___PPC_RIC(ric) | ___PPC_PRS(prs) | ___PPC_R(r))
Which is long, but I don't think wrapping it helps.
If we didn't have those ridiculous ___PPC_RX macros it would be a bit
shorter, but I guess that's a rework for another day.
cheers
^ permalink raw reply
* [RFC PATCH v2 3/3] ASoC: fsl_asrc_dma: Reuse the dma channel if available in Back-End
From: Shengjiu Wang @ 2020-06-10 10:05 UTC (permalink / raw)
To: lars, perex, tiwai, lgirdwood, broonie, timur, nicoleotsuka,
Xiubo.Lee, festevam, alsa-devel, linux-kernel, linuxppc-dev
In-Reply-To: <cover.1591783089.git.shengjiu.wang@nxp.com>
The dma channel has been requested by Back-End cpu dai driver already.
If fsl_asrc_dma requests dma chan with same dma:tx symlink, then
there will be below warning with SDMA.
[ 48.174236] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink
or with EDMA the request operation will fail for EDMA channel
can only be requested once.
So If we can reuse the dma channel of Back-End, then the issue can be
fixed.
In order to get the dma channel which is already requested in Back-End.
we use the exported two functions (snd_soc_lookup_component_nolocked
and soc_component_to_pcm). If we can get the dma channel, then reuse it,
if can't, then request a new one.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
sound/soc/fsl/fsl_asrc_common.h | 2 ++
sound/soc/fsl/fsl_asrc_dma.c | 52 +++++++++++++++++++++++++--------
2 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/sound/soc/fsl/fsl_asrc_common.h b/sound/soc/fsl/fsl_asrc_common.h
index 77665b15c8db..09512bc79b80 100644
--- a/sound/soc/fsl/fsl_asrc_common.h
+++ b/sound/soc/fsl/fsl_asrc_common.h
@@ -32,6 +32,7 @@ enum asrc_pair_index {
* @dma_chan: inputer and output DMA channels
* @dma_data: private dma data
* @pos: hardware pointer position
+ * @req_dma_chan_dev_to_dev: flag for release dev_to_dev chan
* @private: pair private area
*/
struct fsl_asrc_pair {
@@ -45,6 +46,7 @@ struct fsl_asrc_pair {
struct dma_chan *dma_chan[2];
struct imx_dma_data dma_data;
unsigned int pos;
+ bool req_dma_chan_dev_to_dev;
void *private;
};
diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index d6a3fc5f87e5..5ecb77d466d3 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -133,6 +133,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
struct snd_dmaengine_dai_dma_data *dma_params_fe = NULL;
struct snd_dmaengine_dai_dma_data *dma_params_be = NULL;
+ struct dma_chan *tmp_chan = NULL, *tmp_chan_new = NULL;
struct snd_pcm_runtime *runtime = substream->runtime;
struct fsl_asrc_pair *pair = runtime->private_data;
struct fsl_asrc *asrc = pair->asrc;
@@ -142,7 +143,6 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
int stream = substream->stream;
struct imx_dma_data *tmp_data;
struct snd_soc_dpcm *dpcm;
- struct dma_chan *tmp_chan;
struct device *dev_be;
u8 dir = tx ? OUT : IN;
dma_cap_mask_t mask;
@@ -152,6 +152,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
for_each_dpcm_be(rtd, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_pcm_substream *substream_be;
+ struct snd_soc_component *component_be;
struct snd_soc_dai *dai = asoc_rtd_to_cpu(be, 0);
if (dpcm->fe != rtd)
@@ -160,6 +161,9 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
substream_be = snd_soc_dpcm_get_substream(be, stream);
dma_params_be = snd_soc_dai_get_dma_data(dai, substream_be);
dev_be = dai->dev;
+ component_be = snd_soc_lookup_component_nolocked(dev_be, SND_DMAENGINE_PCM_DRV_NAME);
+ if (component_be)
+ tmp_chan = soc_component_to_pcm(component_be)->chan[substream->stream];
break;
}
@@ -205,10 +209,14 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
*/
if (!asrc->use_edma) {
/* Get DMA request of Back-End */
- tmp_chan = dma_request_slave_channel(dev_be, tx ? "tx" : "rx");
+ if (!tmp_chan) {
+ tmp_chan_new = dma_request_slave_channel(dev_be, tx ? "tx" : "rx");
+ tmp_chan = tmp_chan_new;
+ }
tmp_data = tmp_chan->private;
pair->dma_data.dma_request = tmp_data->dma_request;
- dma_release_channel(tmp_chan);
+ if (tmp_chan_new)
+ dma_release_channel(tmp_chan_new);
/* Get DMA request of Front-End */
tmp_chan = asrc->get_dma_channel(pair, dir);
@@ -220,9 +228,26 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
pair->dma_chan[dir] =
dma_request_channel(mask, filter, &pair->dma_data);
+ pair->req_dma_chan_dev_to_dev = true;
} else {
- pair->dma_chan[dir] =
- asrc->get_dma_channel(pair, dir);
+ /*
+ * With EDMA, there is two dma channels can be used for p2p,
+ * one is from ASRC, one is from another peripheral
+ * (ESAI or SAI). Previously we select the dma channel of ASRC,
+ * but find an issue for ideal ratio case, there is no control
+ * for data copy speed, the speed is faster than sample
+ * frequency.
+ *
+ * So we switch to use dma channel of peripheral (ESAI or SAI),
+ * that copy speed of DMA is controlled by data consumption
+ * speed in the peripheral FIFO.
+ */
+ pair->req_dma_chan_dev_to_dev = false;
+ pair->dma_chan[dir] = tmp_chan;
+ if (!pair->dma_chan[dir]) {
+ pair->dma_chan[dir] = dma_request_slave_channel(dev_be, tx ? "tx" : "rx");
+ pair->req_dma_chan_dev_to_dev = true;
+ }
}
if (!pair->dma_chan[dir]) {
@@ -261,7 +286,8 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
ret = dmaengine_slave_config(pair->dma_chan[dir], &config_be);
if (ret) {
dev_err(dev, "failed to config DMA channel for Back-End\n");
- dma_release_channel(pair->dma_chan[dir]);
+ if (pair->req_dma_chan_dev_to_dev)
+ dma_release_channel(pair->dma_chan[dir]);
return ret;
}
@@ -273,19 +299,21 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
static int fsl_asrc_dma_hw_free(struct snd_soc_component *component,
struct snd_pcm_substream *substream)
{
+ bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
struct snd_pcm_runtime *runtime = substream->runtime;
struct fsl_asrc_pair *pair = runtime->private_data;
+ u8 dir = tx ? OUT : IN;
snd_pcm_set_runtime_buffer(substream, NULL);
- if (pair->dma_chan[IN])
- dma_release_channel(pair->dma_chan[IN]);
+ if (pair->dma_chan[!dir])
+ dma_release_channel(pair->dma_chan[!dir]);
- if (pair->dma_chan[OUT])
- dma_release_channel(pair->dma_chan[OUT]);
+ if (pair->dma_chan[dir] && pair->req_dma_chan_dev_to_dev)
+ dma_release_channel(pair->dma_chan[dir]);
- pair->dma_chan[IN] = NULL;
- pair->dma_chan[OUT] = NULL;
+ pair->dma_chan[!dir] = NULL;
+ pair->dma_chan[dir] = NULL;
return 0;
}
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v2 2/3] ASoC: dmaengine_pcm: export soc_component_to_pcm
From: Shengjiu Wang @ 2020-06-10 10:05 UTC (permalink / raw)
To: lars, perex, tiwai, lgirdwood, broonie, timur, nicoleotsuka,
Xiubo.Lee, festevam, alsa-devel, linux-kernel, linuxppc-dev
In-Reply-To: <cover.1591783089.git.shengjiu.wang@nxp.com>
In DPCM case, Front-End needs to get the dma chan which has
been requested by Back-End and reuse it.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
include/sound/dmaengine_pcm.h | 11 +++++++++++
sound/soc/soc-generic-dmaengine-pcm.c | 12 ------------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index b65220685920..8c5e38180fb0 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -161,4 +161,15 @@ int snd_dmaengine_pcm_prepare_slave_config(struct snd_pcm_substream *substream,
#define SND_DMAENGINE_PCM_DRV_NAME "snd_dmaengine_pcm"
+struct dmaengine_pcm {
+ struct dma_chan *chan[SNDRV_PCM_STREAM_LAST + 1];
+ const struct snd_dmaengine_pcm_config *config;
+ struct snd_soc_component component;
+ unsigned int flags;
+};
+
+static inline struct dmaengine_pcm *soc_component_to_pcm(struct snd_soc_component *p)
+{
+ return container_of(p, struct dmaengine_pcm, component);
+}
#endif
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index f728309a0833..80a4e71f2d95 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -21,18 +21,6 @@
*/
#define SND_DMAENGINE_PCM_FLAG_NO_RESIDUE BIT(31)
-struct dmaengine_pcm {
- struct dma_chan *chan[SNDRV_PCM_STREAM_LAST + 1];
- const struct snd_dmaengine_pcm_config *config;
- struct snd_soc_component component;
- unsigned int flags;
-};
-
-static struct dmaengine_pcm *soc_component_to_pcm(struct snd_soc_component *p)
-{
- return container_of(p, struct dmaengine_pcm, component);
-}
-
static struct device *dmaengine_dma_dev(struct dmaengine_pcm *pcm,
struct snd_pcm_substream *substream)
{
--
2.21.0
^ permalink raw reply related
* [RFC PATCH v2 0/3] ASoC: fsl_asrc_dma: Reuse the dma channel if available in Back-End
From: Shengjiu Wang @ 2020-06-10 10:05 UTC (permalink / raw)
To: lars, perex, tiwai, lgirdwood, broonie, timur, nicoleotsuka,
Xiubo.Lee, festevam, alsa-devel, linux-kernel, linuxppc-dev
Reuse the dma channel if available in Back-End
Shengjiu Wang (3):
ASoC: soc-card: export snd_soc_lookup_component_nolocked
ASoC: dmaengine_pcm: export soc_component_to_pcm
ASoC: fsl_asrc_dma: Reuse the dma channel if available in Back-End
changes in v2:
- update according to Mark's comments and split the patch
include/sound/dmaengine_pcm.h | 11 ++++++
include/sound/soc.h | 2 ++
sound/soc/fsl/fsl_asrc_common.h | 2 ++
sound/soc/fsl/fsl_asrc_dma.c | 52 ++++++++++++++++++++-------
sound/soc/soc-core.c | 3 +-
sound/soc/soc-generic-dmaengine-pcm.c | 12 -------
6 files changed, 57 insertions(+), 25 deletions(-)
--
2.21.0
^ permalink raw reply
* [RFC PATCH v2 1/3] ASoC: soc-card: export snd_soc_lookup_component_nolocked
From: Shengjiu Wang @ 2020-06-10 10:05 UTC (permalink / raw)
To: lars, perex, tiwai, lgirdwood, broonie, timur, nicoleotsuka,
Xiubo.Lee, festevam, alsa-devel, linux-kernel, linuxppc-dev
In-Reply-To: <cover.1591783089.git.shengjiu.wang@nxp.com>
snd_soc_lookup_component_nolocked can be used for the DPCM case
that Front-End needs to get the unused platform component but
added by Back-End cpu dai driver.
If the component is gotten, then we can get the dma chan created
by Back-End component and reused it in Front-End.
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
include/sound/soc.h | 2 ++
sound/soc/soc-core.c | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 74868436ac79..565612a8d690 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -444,6 +444,8 @@ int devm_snd_soc_register_component(struct device *dev,
const struct snd_soc_component_driver *component_driver,
struct snd_soc_dai_driver *dai_drv, int num_dai);
void snd_soc_unregister_component(struct device *dev);
+struct snd_soc_component *snd_soc_lookup_component_nolocked(struct device *dev,
+ const char *driver_name);
struct snd_soc_component *snd_soc_lookup_component(struct device *dev,
const char *driver_name);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b07eca2c6ccc..d4c73e86d058 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -310,7 +310,7 @@ struct snd_soc_component *snd_soc_rtdcom_lookup(struct snd_soc_pcm_runtime *rtd,
}
EXPORT_SYMBOL_GPL(snd_soc_rtdcom_lookup);
-static struct snd_soc_component
+struct snd_soc_component
*snd_soc_lookup_component_nolocked(struct device *dev, const char *driver_name)
{
struct snd_soc_component *component;
@@ -329,6 +329,7 @@ static struct snd_soc_component
return found_component;
}
+EXPORT_SYMBOL_GPL(snd_soc_lookup_component_nolocked);
struct snd_soc_component *snd_soc_lookup_component(struct device *dev,
const char *driver_name)
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox