| Message ID | 1452834254-22078-9-git-send-email-cyrilbur@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 2016-01-15 at 16:04 +1100, Cyril Bur wrote: > This patch adds the ability to be able to save the VSX registers to > the > thread struct without giving up (disabling the facility) next time > the > process returns to userspace. > > This patch builds on a previous optimisation for the FPU and VEC > registers > in the thread copy path to avoid a possibly pointless reload of VSX > state. > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > --- > arch/powerpc/include/asm/switch_to.h | 1 - > arch/powerpc/kernel/ppc_ksyms.c | 4 ---- > arch/powerpc/kernel/process.c | 23 ++++++++++++++++++----- > arch/powerpc/kernel/vector.S | 17 ----------------- > 4 files changed, 18 insertions(+), 27 deletions(-) > > diff --git a/arch/powerpc/include/asm/switch_to.h > b/arch/powerpc/include/asm/switch_to.h > index 29dda9d..4dfcd3e 100644 > --- a/arch/powerpc/include/asm/switch_to.h > +++ b/arch/powerpc/include/asm/switch_to.h > @@ -52,7 +52,6 @@ static inline void disable_kernel_altivec(void) > extern void enable_kernel_vsx(void); > extern void flush_vsx_to_thread(struct task_struct *); > extern void giveup_vsx(struct task_struct *); > -extern void __giveup_vsx(struct task_struct *); > static inline void disable_kernel_vsx(void) > { > msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX); > diff --git a/arch/powerpc/kernel/ppc_ksyms.c > b/arch/powerpc/kernel/ppc_ksyms.c > index 41e1607..ef7024da 100644 > --- a/arch/powerpc/kernel/ppc_ksyms.c > +++ b/arch/powerpc/kernel/ppc_ksyms.c > @@ -28,10 +28,6 @@ EXPORT_SYMBOL(load_vr_state); > EXPORT_SYMBOL(store_vr_state); > #endif > > -#ifdef CONFIG_VSX > -EXPORT_SYMBOL_GPL(__giveup_vsx); > -#endif > - > #ifdef CONFIG_EPAPR_PARAVIRT > EXPORT_SYMBOL(epapr_hypercall_start); > #endif > diff --git a/arch/powerpc/kernel/process.c > b/arch/powerpc/kernel/process.c > index 5566c32..3d907b8 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -252,20 +252,33 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); > #endif /* CONFIG_ALTIVEC */ > > #ifdef CONFIG_VSX > -void giveup_vsx(struct task_struct *tsk) > +void __giveup_vsx(struct task_struct *tsk) > { > - check_if_tm_restore_required(tsk); > - > - msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX); > if (tsk->thread.regs->msr & MSR_FP) > __giveup_fpu(tsk); > if (tsk->thread.regs->msr & MSR_VEC) > __giveup_altivec(tsk); > + tsk->thread.regs->msr &= ~MSR_VSX; > +} > + > +void giveup_vsx(struct task_struct *tsk) > +{ > + check_if_tm_restore_required(tsk); > + > + msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX); > __giveup_vsx(tsk); > msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX); > } > EXPORT_SYMBOL(giveup_vsx); > > +void save_vsx(struct task_struct *tsk) > +{ > + if (tsk->thread.regs->msr & MSR_FP) > + save_fpu(tsk); > + if (tsk->thread.regs->msr & MSR_VEC) > + save_altivec(tsk); > +} > + > void enable_kernel_vsx(void) > { > WARN_ON(preemptible()); > @@ -465,7 +478,7 @@ void save_all(struct task_struct *tsk) > #endif > #ifdef CONFIG_VSX > if (usermsr & MSR_VSX) > - __giveup_vsx(tsk); > + save_vsx(tsk); This seems suboptimal. save_vsx() will call save_fpu() and save_altivec() again, which you just called earlier in save_all(). save_vsx() is only used here, so could be static. Also, put the #ifdef junk as part of the function so that the caller doesn't have to deal with it. Mikey > #endif > #ifdef CONFIG_SPE > if (usermsr & MSR_SPE) > diff --git a/arch/powerpc/kernel/vector.S > b/arch/powerpc/kernel/vector.S > index 51b0c17..1c2e7a3 100644 > --- a/arch/powerpc/kernel/vector.S > +++ b/arch/powerpc/kernel/vector.S > @@ -151,23 +151,6 @@ _GLOBAL(load_up_vsx) > std r12,_MSR(r1) > b fast_exception_return > > -/* > - * __giveup_vsx(tsk) > - * Disable VSX for the task given as the argument. > - * Does NOT save vsx registers. > - */ > -_GLOBAL(__giveup_vsx) > - addi r3,r3,THREAD /* want THREAD of > task */ > - ld r5,PT_REGS(r3) > - cmpdi 0,r5,0 > - beq 1f > - ld r4,_MSR-STACK_FRAME_OVERHEAD(r5) > - lis r3,MSR_VSX@h > - andc r4,r4,r3 /* disable VSX for > previous task */ > - std r4,_MSR-STACK_FRAME_OVERHEAD(r5) > -1: > - blr > - > #endif /* CONFIG_VSX */ > >
On Fri, 15 Jan 2016 17:25:26 +1100 Michael Neuling <mikey@neuling.org> wrote: > On Fri, 2016-01-15 at 16:04 +1100, Cyril Bur wrote: > > This patch adds the ability to be able to save the VSX registers to > > the > > thread struct without giving up (disabling the facility) next time > > the > > process returns to userspace. > > > > This patch builds on a previous optimisation for the FPU and VEC > > registers > > in the thread copy path to avoid a possibly pointless reload of VSX > > state. > > > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > > --- > > arch/powerpc/include/asm/switch_to.h | 1 - > > arch/powerpc/kernel/ppc_ksyms.c | 4 ---- > > arch/powerpc/kernel/process.c | 23 ++++++++++++++++++----- > > arch/powerpc/kernel/vector.S | 17 ----------------- > > 4 files changed, 18 insertions(+), 27 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/switch_to.h > > b/arch/powerpc/include/asm/switch_to.h > > index 29dda9d..4dfcd3e 100644 > > --- a/arch/powerpc/include/asm/switch_to.h > > +++ b/arch/powerpc/include/asm/switch_to.h > > @@ -52,7 +52,6 @@ static inline void disable_kernel_altivec(void) > > extern void enable_kernel_vsx(void); > > extern void flush_vsx_to_thread(struct task_struct *); > > extern void giveup_vsx(struct task_struct *); > > -extern void __giveup_vsx(struct task_struct *); > > static inline void disable_kernel_vsx(void) > > { > > msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX); > > diff --git a/arch/powerpc/kernel/ppc_ksyms.c > > b/arch/powerpc/kernel/ppc_ksyms.c > > index 41e1607..ef7024da 100644 > > --- a/arch/powerpc/kernel/ppc_ksyms.c > > +++ b/arch/powerpc/kernel/ppc_ksyms.c > > @@ -28,10 +28,6 @@ EXPORT_SYMBOL(load_vr_state); > > EXPORT_SYMBOL(store_vr_state); > > #endif > > > > -#ifdef CONFIG_VSX > > -EXPORT_SYMBOL_GPL(__giveup_vsx); > > -#endif > > - > > #ifdef CONFIG_EPAPR_PARAVIRT > > EXPORT_SYMBOL(epapr_hypercall_start); > > #endif > > diff --git a/arch/powerpc/kernel/process.c > > b/arch/powerpc/kernel/process.c > > index 5566c32..3d907b8 100644 > > --- a/arch/powerpc/kernel/process.c > > +++ b/arch/powerpc/kernel/process.c > > @@ -252,20 +252,33 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); > > #endif /* CONFIG_ALTIVEC */ > > > > #ifdef CONFIG_VSX > > -void giveup_vsx(struct task_struct *tsk) > > +void __giveup_vsx(struct task_struct *tsk) > > { > > - check_if_tm_restore_required(tsk); > > - > > - msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX); > > if (tsk->thread.regs->msr & MSR_FP) > > __giveup_fpu(tsk); > > if (tsk->thread.regs->msr & MSR_VEC) > > __giveup_altivec(tsk); > > + tsk->thread.regs->msr &= ~MSR_VSX; > > +} > > + > > +void giveup_vsx(struct task_struct *tsk) > > +{ > > + check_if_tm_restore_required(tsk); > > + > > + msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX); > > __giveup_vsx(tsk); > > msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX); > > } > > EXPORT_SYMBOL(giveup_vsx); > > > > +void save_vsx(struct task_struct *tsk) > > +{ > > + if (tsk->thread.regs->msr & MSR_FP) > > + save_fpu(tsk); > > + if (tsk->thread.regs->msr & MSR_VEC) > > + save_altivec(tsk); > > +} > > + > > void enable_kernel_vsx(void) > > { > > WARN_ON(preemptible()); > > @@ -465,7 +478,7 @@ void save_all(struct task_struct *tsk) > > #endif > > #ifdef CONFIG_VSX > > if (usermsr & MSR_VSX) > > - __giveup_vsx(tsk); > > + save_vsx(tsk); > > This seems suboptimal. save_vsx() will call save_fpu() and > save_altivec() again, which you just called earlier in save_all(). > Ah yes, will fix > save_vsx() is only used here, so could be static. > Thanks. > Also, put the #ifdef junk as part of the function so that the caller > doesn't have to deal with it. > Can do absolutely, however this means that in save_all I can't check if the function needs to be called or not. For example, without CONFIG_VSX, MSR_VSX won't exist which means we might end up calling save_vsx THEN checking MSR_VSX and returning early. I'm happy to defer to you and mpe on what's nicer, I would side with avoiding the function call at the cost of ugly #ifdefs but I can always see the merits of clean code. Thanks for the review, Cyril > Mikey > > > #endif > > #ifdef CONFIG_SPE > > if (usermsr & MSR_SPE) > > diff --git a/arch/powerpc/kernel/vector.S > > b/arch/powerpc/kernel/vector.S > > index 51b0c17..1c2e7a3 100644 > > --- a/arch/powerpc/kernel/vector.S > > +++ b/arch/powerpc/kernel/vector.S > > @@ -151,23 +151,6 @@ _GLOBAL(load_up_vsx) > > std r12,_MSR(r1) > > b fast_exception_return > > > > -/* > > - * __giveup_vsx(tsk) > > - * Disable VSX for the task given as the argument. > > - * Does NOT save vsx registers. > > - */ > > -_GLOBAL(__giveup_vsx) > > - addi r3,r3,THREAD /* want THREAD of > > task */ > > - ld r5,PT_REGS(r3) > > - cmpdi 0,r5,0 > > - beq 1f > > - ld r4,_MSR-STACK_FRAME_OVERHEAD(r5) > > - lis r3,MSR_VSX@h > > - andc r4,r4,r3 /* disable VSX for > > previous task */ > > - std r4,_MSR-STACK_FRAME_OVERHEAD(r5) > > -1: > > - blr > > - > > #endif /* CONFIG_VSX */ > > > >
> > Also, put the #ifdef junk as part of the function so that the caller > > doesn't have to deal with it. > > > > Can do absolutely, however this means that in save_all I can't check if the > function needs to be called or not. For example, without CONFIG_VSX, MSR_VSX > won't exist which means we might end up calling save_vsx THEN checking MSR_VSX > and returning early. MSR_VSX should exist even without CONFIG_VSX, so you should be able to do it. > I'm happy to defer to you and mpe on what's nicer, I would side with avoiding > the function call at the cost of ugly #ifdefs but I can always see the merits > of clean code. I'd prefer the cleanup. This code has been pretty bad in the past for being a bit of an #ifdef mess, so it'd be nice to clean it up a bit if possible. Mikey
diff --git a/arch/powerpc/include/asm/switch_to.h b/arch/powerpc/include/asm/switch_to.h index 29dda9d..4dfcd3e 100644 --- a/arch/powerpc/include/asm/switch_to.h +++ b/arch/powerpc/include/asm/switch_to.h @@ -52,7 +52,6 @@ static inline void disable_kernel_altivec(void) extern void enable_kernel_vsx(void); extern void flush_vsx_to_thread(struct task_struct *); extern void giveup_vsx(struct task_struct *); -extern void __giveup_vsx(struct task_struct *); static inline void disable_kernel_vsx(void) { msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX); diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c index 41e1607..ef7024da 100644 --- a/arch/powerpc/kernel/ppc_ksyms.c +++ b/arch/powerpc/kernel/ppc_ksyms.c @@ -28,10 +28,6 @@ EXPORT_SYMBOL(load_vr_state); EXPORT_SYMBOL(store_vr_state); #endif -#ifdef CONFIG_VSX -EXPORT_SYMBOL_GPL(__giveup_vsx); -#endif - #ifdef CONFIG_EPAPR_PARAVIRT EXPORT_SYMBOL(epapr_hypercall_start); #endif diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 5566c32..3d907b8 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -252,20 +252,33 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); #endif /* CONFIG_ALTIVEC */ #ifdef CONFIG_VSX -void giveup_vsx(struct task_struct *tsk) +void __giveup_vsx(struct task_struct *tsk) { - check_if_tm_restore_required(tsk); - - msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX); if (tsk->thread.regs->msr & MSR_FP) __giveup_fpu(tsk); if (tsk->thread.regs->msr & MSR_VEC) __giveup_altivec(tsk); + tsk->thread.regs->msr &= ~MSR_VSX; +} + +void giveup_vsx(struct task_struct *tsk) +{ + check_if_tm_restore_required(tsk); + + msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX); __giveup_vsx(tsk); msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX); } EXPORT_SYMBOL(giveup_vsx); +void save_vsx(struct task_struct *tsk) +{ + if (tsk->thread.regs->msr & MSR_FP) + save_fpu(tsk); + if (tsk->thread.regs->msr & MSR_VEC) + save_altivec(tsk); +} + void enable_kernel_vsx(void) { WARN_ON(preemptible()); @@ -465,7 +478,7 @@ void save_all(struct task_struct *tsk) #endif #ifdef CONFIG_VSX if (usermsr & MSR_VSX) - __giveup_vsx(tsk); + save_vsx(tsk); #endif #ifdef CONFIG_SPE if (usermsr & MSR_SPE) diff --git a/arch/powerpc/kernel/vector.S b/arch/powerpc/kernel/vector.S index 51b0c17..1c2e7a3 100644 --- a/arch/powerpc/kernel/vector.S +++ b/arch/powerpc/kernel/vector.S @@ -151,23 +151,6 @@ _GLOBAL(load_up_vsx) std r12,_MSR(r1) b fast_exception_return -/* - * __giveup_vsx(tsk) - * Disable VSX for the task given as the argument. - * Does NOT save vsx registers. - */ -_GLOBAL(__giveup_vsx) - addi r3,r3,THREAD /* want THREAD of task */ - ld r5,PT_REGS(r3) - cmpdi 0,r5,0 - beq 1f - ld r4,_MSR-STACK_FRAME_OVERHEAD(r5) - lis r3,MSR_VSX@h - andc r4,r4,r3 /* disable VSX for previous task */ - std r4,_MSR-STACK_FRAME_OVERHEAD(r5) -1: - blr - #endif /* CONFIG_VSX */
This patch adds the ability to be able to save the VSX registers to the thread struct without giving up (disabling the facility) next time the process returns to userspace. This patch builds on a previous optimisation for the FPU and VEC registers in the thread copy path to avoid a possibly pointless reload of VSX state. Signed-off-by: Cyril Bur <cyrilbur@gmail.com> --- arch/powerpc/include/asm/switch_to.h | 1 - arch/powerpc/kernel/ppc_ksyms.c | 4 ---- arch/powerpc/kernel/process.c | 23 ++++++++++++++++++----- arch/powerpc/kernel/vector.S | 17 ----------------- 4 files changed, 18 insertions(+), 27 deletions(-)