[PATCH 1/5] powerpc: Test MSR_FP and MSR_VEC when enabling/flushing VSX

[PATCH 1/5] powerpc: Test MSR_FP and MSR_VEC when enabling/flushing VSX

[Benjamin Herrenschmidt]

VSX uses a combination of the old vector registers, the old FP registers
and new "second halves" of the FP registers.

Thus when we need to see the VSX state in the thread struct
(flush_vsx_to_thread) or when we'll use the VSX in the kernel
(enable_kernel_vsx) we need to ensure they are all flushed into
the thread struct if either of them is individually enabled.

Unfortunately we only tested if the whole VSX was enabled, not
if they were individually enabled.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/process.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 9f3e2c932dcc..883216b4296a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -362,7 +362,8 @@ void enable_kernel_vsx(void)
 
 	cpumsr = msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX);
 
-	if (current->thread.regs && (current->thread.regs->msr & MSR_VSX)) {
+	if (current->thread.regs &&
+	    (current->thread.regs->msr & (MSR_VSX|MSR_VEC|MSR_FP))) {
 		check_if_tm_restore_required(current);
 		/*
 		 * If a thread has already been reclaimed then the
@@ -386,7 +387,7 @@ void flush_vsx_to_thread(struct task_struct *tsk)
 {
 	if (tsk->thread.regs) {
 		preempt_disable();
-		if (tsk->thread.regs->msr & MSR_VSX) {
+		if (tsk->thread.regs->msr & (MSR_VSX|MSR_VEC|MSR_FP)) {
 			BUG_ON(tsk != current);
 			giveup_vsx(tsk);
 		}
-- 
2.13.4

[PATCH 2/5] powerpc: Fix missing CR before {

[Benjamin Herrenschmidt]

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/process.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 883216b4296a..14b9a3c46c5d 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -230,7 +230,8 @@ void enable_kernel_fp(void)
 }
 EXPORT_SYMBOL(enable_kernel_fp);
 
-static int restore_fp(struct task_struct *tsk) {
+static int restore_fp(struct task_struct *tsk)
+{
 	if (tsk->thread.load_fp || msr_tm_active(tsk->thread.regs->msr)) {
 		load_fp_state(&current->thread.fp_state);
 		current->thread.load_fp++;
-- 
2.13.4

[PATCH 3/5] powerpc: Remove redundant fp/altivec giveup code

[Benjamin Herrenschmidt]

__giveup_vsx already calls those two functions

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/process.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 14b9a3c46c5d..bfbd6083f841 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -375,10 +375,6 @@ void enable_kernel_vsx(void)
 		 */
 		if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr))
 			return;
-		if (current->thread.regs->msr & MSR_FP)
-			__giveup_fpu(current);
-		if (current->thread.regs->msr & MSR_VEC)
-			__giveup_altivec(current);
 		__giveup_vsx(current);
 	}
 }
-- 
2.13.4

[PATCH 4/5] powerpc: Remove redundant clear of MSR_VSX in __giveup_vsx()

[Benjamin Herrenschmidt]

__giveup_fpu() already does it and we cannot have MSR_VSX set
without having MSR_FP also set.

This also adds a warning to check we indeed do

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/process.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index bfbd6083f841..fc285fab7118 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -331,11 +331,19 @@ static inline int restore_altivec(struct task_struct *tsk) { return 0; }
 #ifdef CONFIG_VSX
 static void __giveup_vsx(struct task_struct *tsk)
 {
-	if (tsk->thread.regs->msr & MSR_FP)
+	unsigned long msr = tsk->thread.regs->msr;
+
+	/*
+	 * We should never be ssetting MSR_VSX without also setting
+	 * MSR_FP and MSR_VEC
+	 */
+	WARN_ON((msr & MSR_VSX) && !((msr & MSR_FP) && (msr & MSR_VEC)));
+
+	/* __giveup_fpu will clear MSR_VSX */
+	if (msr & MSR_FP)
 		__giveup_fpu(tsk);
-	if (tsk->thread.regs->msr & MSR_VEC)
+	if (msr & MSR_VEC)
 		__giveup_altivec(tsk);
-	tsk->thread.regs->msr &= ~MSR_VSX;
 }
 
 static void giveup_vsx(struct task_struct *tsk)
-- 
2.13.4

[PATCH 5/5] powerpc: Remove more redundant VSX save/tests

[Benjamin Herrenschmidt]

__giveup_vsx/save_vsx are completely equivalent to testing MSR_FP
and MSR_VEC and calling the corresponding giveup/save function so
just remove the spurious VSX cases. Also add WARN_ONs checking that
we never have VSX enabled without the two other.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/process.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index fc285fab7118..7093b46b3603 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -355,14 +355,6 @@ static void giveup_vsx(struct task_struct *tsk)
 	msr_check_and_clear(MSR_FP|MSR_VEC|MSR_VSX);
 }
 
-static 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)
 {
 	unsigned long cpumsr;
@@ -412,7 +404,6 @@ static int restore_vsx(struct task_struct *tsk)
 }
 #else
 static inline int restore_vsx(struct task_struct *tsk) { return 0; }
-static inline void save_vsx(struct task_struct *tsk) { }
 #endif /* CONFIG_VSX */
 
 #ifdef CONFIG_SPE
@@ -492,6 +483,8 @@ void giveup_all(struct task_struct *tsk)
 	msr_check_and_set(msr_all_available);
 	check_if_tm_restore_required(tsk);
 
+	WARN_ON((usermsr & MSR_VSX) && !((usermsr & MSR_FP) && (usermsr & MSR_VEC)));
+
 #ifdef CONFIG_PPC_FPU
 	if (usermsr & MSR_FP)
 		__giveup_fpu(tsk);
@@ -500,10 +493,6 @@ void giveup_all(struct task_struct *tsk)
 	if (usermsr & MSR_VEC)
 		__giveup_altivec(tsk);
 #endif
-#ifdef CONFIG_VSX
-	if (usermsr & MSR_VSX)
-		__giveup_vsx(tsk);
-#endif
 #ifdef CONFIG_SPE
 	if (usermsr & MSR_SPE)
 		__giveup_spe(tsk);
@@ -562,19 +551,13 @@ void save_all(struct task_struct *tsk)
 
 	msr_check_and_set(msr_all_available);
 
-	/*
-	 * Saving the way the register space is in hardware, save_vsx boils
-	 * down to a save_fpu() and save_altivec()
-	 */
-	if (usermsr & MSR_VSX) {
-		save_vsx(tsk);
-	} else {
-		if (usermsr & MSR_FP)
-			save_fpu(tsk);
+	WARN_ON((usermsr & MSR_VSX) && !((usermsr & MSR_FP) && (usermsr & MSR_VEC)));
 
-		if (usermsr & MSR_VEC)
-			save_altivec(tsk);
-	}
+	if (usermsr & MSR_FP)
+		save_fpu(tsk);
+
+	if (usermsr & MSR_VEC)
+		save_altivec(tsk);
 
 	if (usermsr & MSR_SPE)
 		__giveup_spe(tsk);
-- 
2.13.4

Re: [PATCH 1/5] powerpc: Test MSR_FP and MSR_VEC when enabling/flushing VSX

[Benjamin Herrenschmidt]

On Wed, 2017-08-16 at 16:01 +1000, Benjamin Herrenschmidt wrote:
> VSX uses a combination of the old vector registers, the old FP registers
> and new "second halves" of the FP registers.
> 
> Thus when we need to see the VSX state in the thread struct
> (flush_vsx_to_thread) or when we'll use the VSX in the kernel
> (enable_kernel_vsx) we need to ensure they are all flushed into
> the thread struct if either of them is individually enabled.
> 
> Unfortunately we only tested if the whole VSX was enabled, not
> if they were individually enabled.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

And CC stable.

> ---
>  arch/powerpc/kernel/process.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 9f3e2c932dcc..883216b4296a 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -362,7 +362,8 @@ void enable_kernel_vsx(void)
>  
>  	cpumsr = msr_check_and_set(MSR_FP|MSR_VEC|MSR_VSX);
>  
> -	if (current->thread.regs && (current->thread.regs->msr & MSR_VSX)) {
> +	if (current->thread.regs &&
> +	    (current->thread.regs->msr & (MSR_VSX|MSR_VEC|MSR_FP))) {
>  		check_if_tm_restore_required(current);
>  		/*
>  		 * If a thread has already been reclaimed then the
> @@ -386,7 +387,7 @@ void flush_vsx_to_thread(struct task_struct *tsk)
>  {
>  	if (tsk->thread.regs) {
>  		preempt_disable();
> -		if (tsk->thread.regs->msr & MSR_VSX) {
> +		if (tsk->thread.regs->msr & (MSR_VSX|MSR_VEC|MSR_FP)) {
>  			BUG_ON(tsk != current);
>  			giveup_vsx(tsk);
>  		}

Re: [1/5] powerpc: Test MSR_FP and MSR_VEC when enabling/flushing VSX

[Michael Ellerman]

On Wed, 2017-08-16 at 06:01:14 UTC, Benjamin Herrenschmidt wrote:
> VSX uses a combination of the old vector registers, the old FP registers
> and new "second halves" of the FP registers.
> 
> Thus when we need to see the VSX state in the thread struct
> (flush_vsx_to_thread) or when we'll use the VSX in the kernel
> (enable_kernel_vsx) we need to ensure they are all flushed into
> the thread struct if either of them is individually enabled.
> 
> Unfortunately we only tested if the whole VSX was enabled, not
> if they were individually enabled.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/5a69aec945d27e78abac9fd032533d

cheers

Re: [2/5] powerpc: Fix missing CR before {

[Michael Ellerman]

On Wed, 2017-08-16 at 06:01:15 UTC, Benjamin Herrenschmidt wrote:
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Patches 2-5 applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/6a303833b5e3acbb4c97cc11cc6886

cheers