| Message ID | 20171030053913.20455-1-cyrilbur@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] powerpc: Don't enable FP/Altivec if not checkpointed | expand |
Hi Cyril,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.14-rc7 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Cyril-Bur/powerpc-Don-t-enable-FP-Altivec-if-not-checkpointed/20171102-073816
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-asp8347_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
arch/powerpc/kernel/process.c: In function 'is_transactionally_fp':
>> arch/powerpc/kernel/process.c:243:15: error: 'struct thread_struct' has no member named 'ckpt_regs'
(tsk->thread.ckpt_regs.msr & MSR_FP);
^
arch/powerpc/kernel/process.c:244:1: error: control reaches end of non-void function [-Werror=return-type]
}
^
cc1: all warnings being treated as errors
vim +243 arch/powerpc/kernel/process.c
239
240 static int is_transactionally_fp(struct task_struct *tsk)
241 {
242 return msr_tm_active(tsk->thread.regs->msr) &&
> 243 (tsk->thread.ckpt_regs.msr & MSR_FP);
244 }
245
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, 2017-11-02 at 10:19 +0800, kbuild test robot wrote: > Hi Cyril, > > Thank you for the patch! Yet something to improve: > Once again robot, you have done brilliantly! You're 100% correct and the last thing I want to do is break the build with CONFIG_PPC_TRANSACTIONAL_MEM turned off. Life saver, Thanks so much kbuild. Cyril > [auto build test ERROR on powerpc/next] > [also build test ERROR on v4.14-rc7 next-20171018] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Cyril-Bur/powerpc-Don-t-enable-FP-Altivec-if-not-checkpointed/20171102-073816 > base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next > config: powerpc-asp8347_defconfig (attached as .config) > compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=powerpc > > All errors (new ones prefixed by >>): > > arch/powerpc/kernel/process.c: In function 'is_transactionally_fp': > > > arch/powerpc/kernel/process.c:243:15: error: 'struct thread_struct' has no member named 'ckpt_regs' > > (tsk->thread.ckpt_regs.msr & MSR_FP); > ^ > arch/powerpc/kernel/process.c:244:1: error: control reaches end of non-void function [-Werror=return-type] > } > ^ > cc1: all warnings being treated as errors > > vim +243 arch/powerpc/kernel/process.c > > 239 > 240 static int is_transactionally_fp(struct task_struct *tsk) > 241 { > 242 return msr_tm_active(tsk->thread.regs->msr) && > > 243 (tsk->thread.ckpt_regs.msr & MSR_FP); > 244 } > 245 > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Nov 02, 2017 at 01:44:07PM +1100, Cyril Bur wrote: > On Thu, 2017-11-02 at 10:19 +0800, kbuild test robot wrote: > > Hi Cyril, > > > > Thank you for the patch! Yet something to improve: > > > > Once again robot, you have done brilliantly! You're 100% correct and > the last thing I want to do is break the build with > CONFIG_PPC_TRANSACTIONAL_MEM turned off. > > Life saver, > Thanks so much kbuild. Thanks Cyril, we are glad to do some help. > > Cyril > > > [auto build test ERROR on powerpc/next] > > [also build test ERROR on v4.14-rc7 next-20171018] > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > > > url: https://github.com/0day-ci/linux/commits/Cyril-Bur/powerpc-Don-t-enable-FP-Altivec-if-not-checkpointed/20171102-073816 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next > > config: powerpc-asp8347_defconfig (attached as .config) > > compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 > > reproduce: > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # save the attached .config to linux build tree > > make.cross ARCH=powerpc > > > > All errors (new ones prefixed by >>): > > > > arch/powerpc/kernel/process.c: In function 'is_transactionally_fp': > > > > arch/powerpc/kernel/process.c:243:15: error: 'struct thread_struct' has no member named 'ckpt_regs' > > > > (tsk->thread.ckpt_regs.msr & MSR_FP); > > ^ > > arch/powerpc/kernel/process.c:244:1: error: control reaches end of non-void function [-Werror=return-type] > > } > > ^ > > cc1: all warnings being treated as errors > > > > vim +243 arch/powerpc/kernel/process.c > > > > 239 > > 240 static int is_transactionally_fp(struct task_struct *tsk) > > 241 { > > 242 return msr_tm_active(tsk->thread.regs->msr) && > > > 243 (tsk->thread.ckpt_regs.msr & MSR_FP); > > 244 } > > 245 > > > > --- > > 0-DAY kernel test infrastructure Open Source Technology Center > > https://lists.01.org/pipermail/kbuild-all Intel Corporation >
You are welcome! :) On Thu, Nov 02, 2017 at 01:44:07PM +1100, Cyril Bur wrote: >On Thu, 2017-11-02 at 10:19 +0800, kbuild test robot wrote: >> Hi Cyril, >> >> Thank you for the patch! Yet something to improve: >> > >Once again robot, you have done brilliantly! You're 100% correct and >the last thing I want to do is break the build with >CONFIG_PPC_TRANSACTIONAL_MEM turned off. > >Life saver, >Thanks so much kbuild. > >Cyril > >> [auto build test ERROR on powerpc/next] >> [also build test ERROR on v4.14-rc7 next-20171018] >> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] >> >> url: https://github.com/0day-ci/linux/commits/Cyril-Bur/powerpc-Don-t-enable-FP-Altivec-if-not-checkpointed/20171102-073816 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next >> config: powerpc-asp8347_defconfig (attached as .config) >> compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 >> reproduce: >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # save the attached .config to linux build tree >> make.cross ARCH=powerpc >> >> All errors (new ones prefixed by >>): >> >> arch/powerpc/kernel/process.c: In function 'is_transactionally_fp': >> > > arch/powerpc/kernel/process.c:243:15: error: 'struct thread_struct' has no member named 'ckpt_regs' >> >> (tsk->thread.ckpt_regs.msr & MSR_FP); >> ^ >> arch/powerpc/kernel/process.c:244:1: error: control reaches end of non-void function [-Werror=return-type] >> } >> ^ >> cc1: all warnings being treated as errors >> >> vim +243 arch/powerpc/kernel/process.c >> >> 239 >> 240 static int is_transactionally_fp(struct task_struct *tsk) >> 241 { >> 242 return msr_tm_active(tsk->thread.regs->msr) && >> > 243 (tsk->thread.ckpt_regs.msr & MSR_FP); >> 244 } >> 245 >> >> --- >> 0-DAY kernel test infrastructure Open Source Technology Center >> https://lists.01.org/pipermail/kbuild-all Intel Corporation >
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index a0c74bbf3454..da900cd86324 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -230,9 +230,15 @@ void enable_kernel_fp(void) } EXPORT_SYMBOL(enable_kernel_fp); +static int is_transactionally_fp(struct task_struct *tsk) +{ + return msr_tm_active(tsk->thread.regs->msr) && + (tsk->thread.ckpt_regs.msr & MSR_FP); +} + static int restore_fp(struct task_struct *tsk) { - if (tsk->thread.load_fp || msr_tm_active(tsk->thread.regs->msr)) { + if (tsk->thread.load_fp || is_transactionally_fp(tsk)) { load_fp_state(¤t->thread.fp_state); current->thread.load_fp++; return 1; @@ -311,10 +317,17 @@ void flush_altivec_to_thread(struct task_struct *tsk) } EXPORT_SYMBOL_GPL(flush_altivec_to_thread); +static int is_transactionally_altivec(struct task_struct *tsk) +{ + return msr_tm_active(tsk->thread.regs->msr) && + (tsk->thread.ckpt_regs.msr & MSR_VEC); +} + + static int restore_altivec(struct task_struct *tsk) { if (cpu_has_feature(CPU_FTR_ALTIVEC) && - (tsk->thread.load_vec || msr_tm_active(tsk->thread.regs->msr))) { + (tsk->thread.load_vec || is_transactionally_altivec(tsk))) { load_vr_state(&tsk->thread.vr_state); tsk->thread.used_vr = 1; tsk->thread.load_vec++;
Lazy save and restore of FP/Altivec means that a userspace process can be sent to userspace with FP or Altivec disabled and loaded only as required (by way of an FP/Altivec unavailable exception). Transactional Memory complicates this situation as a transaction could be started without FP/Altivec being loaded up. This causes the hardware to checkpoint incorrect registers. Handling FP/Altivec unavailable exceptions while a thread is transactional requires a reclaim and recheckpoint to ensure the CPU has correct state for both sets of registers. Lazy save and restore of FP/Altivec cannot be done if a process is transactional. If a facility was enabled it must remain enabled whenever a thread is transactional. Commit dc16b553c949 ("powerpc: Always restore FPU/VEC/VSX if hardware transactional memory in use") ensures that the facilities are always enabled if a thread is transactional. A bug in the introduced code may cause it to inadvertently enable a facility that was (and should remain) disabled. The problem with this extraneous enablement is that the registers for the erroneously enabled facility have not been correctly recheckpointed - the recheckpointing code assumed the facility would remain disabled. This causes transactional threads which return to their failure handler to observe incorrect checkpointed registers. Perhaps an example will help illustrate the problem: A userspace process is running and uses both FP and Altivec registers. This process then continues to run for some time without touching either sets of registers. The kernel subsequently disables the facilities as part of lazy save and restore. The userspace process then performs a tbegin and the CPU checkpoints 'junk' FP and Altivec registers. The process then performs a floating point instruction triggering a fp unavailable exception in the kernel. The kernel then loads the FP registers - and only the FP registers. Since the thread is transactional it must perform a reclaim and recheckpoint to ensure both the checkpointed registers and the transactional registers are correct. It then (correctly) enables MSR[FP] for the process. Later (on exception exist) the kernel also (inadvertently) enables MSR[VEC]. The process is then returned to userspace. Since the act of loading the FP registers doomed the transaction we know CPU will fail the transaction, restore its checkpointed registers, and return the process to its failure handler. The problem is that we're now running with Altivec enabled and the 'junk' checkpointed registers are restored. The kernel had only recheckpointed FP. This patch solves this by only activating FP/Altivec if userspace was using them when it entered the kernel and not simply if the process is transactional. Fixes: dc16b553c949 ("powerpc: Always restore FPU/VEC/VSX if hardware transactional memory in use") Signed-off-by: Cyril Bur <cyrilbur@gmail.com> --- arch/powerpc/kernel/process.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)