From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ozlabs.org (ozlabs.org [103.22.144.67]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4F1C21A003F for ; Mon, 18 Jan 2016 13:17:57 +1100 (AEDT) Received: from mail-pf0-x244.google.com (mail-pf0-x244.google.com [IPv6:2607:f8b0:400e:c00::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 9C31914031B for ; Mon, 18 Jan 2016 13:11:00 +1100 (AEDT) Received: by mail-pf0-x244.google.com with SMTP id 65so11081441pff.2 for ; Sun, 17 Jan 2016 18:11:00 -0800 (PST) Date: Mon, 18 Jan 2016 13:10:52 +1100 From: Cyril Bur To: Michael Neuling Cc: linuxppc-dev@ozlabs.org Subject: Re: [PATCH V2 8/8] powerpc: Add the ability to save VSX without giving it up Message-ID: <20160118131052.7e3e02d9@camb691> In-Reply-To: <1452839126.25634.43.camel@neuling.org> References: <1452834254-22078-1-git-send-email-cyrilbur@gmail.com> <1452834254-22078-9-git-send-email-cyrilbur@gmail.com> <1452839126.25634.43.camel@neuling.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Fri, 15 Jan 2016 17:25:26 +1100 Michael Neuling 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 > > --- > > 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 */ > > > >