ARM: vfp: move user vfp state save/restore code out of signal.c

The ARM PCS mandates that the length and stride bits of the fpscr are cleared on entry to and return from a public interface. Although signal handlers run asynchronously with respect to the interrupted function, the handler itself expects to run as though it has been called like a normal function.

This patch updates the state mirroring the VFP hardware before entry to a signal handler so that it adheres to the PCS. Furthermore, we disable VFP to ensure that we trap on any floating point operation performed by the signal handler and synchronise the hardware appropriately. A check is inserted after the signal handler to avoid redundant flushing if VFP was not used.

Reported-by Peter Maydell <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/vfp/vfpmodule.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

arch/arm/vfp/vfpmodule.c +15 -0

index 1dfe7d8..269f40d 100644
@@ -562,6 +562,21 @@ int vfp_preserve_user_clear_hwstate(struct user_vfp __user *ufp,
 
 	if (err)
 		return -EFAULT;
+
+	/* Ensure that VFP is disabled. */
+	vfp_flush_hwstate(thread);
+
+	/*
+	 * As per the PCS, clear the length and stride bits for function
+	 * entry.
+	 */
+	hwstate->fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK);
+
+	/*
+	 * Disable VFP in the hwstate so that we can detect if it gets
+	 * used.
+	 */
+	hwstate->fpexc &= ~FPEXC_EN;
 	return 0;
 }
 

@@ -574,7 +589,12 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
 	unsigned long fpexc;
 	int err = 0;
 

Hi Will

I've bisected a screen corruption problem on vexpress down to this commit, I've commented at the end of the patch at to what I see the problem being...

Quoted 12 lines

(quoted diff) +6 -1

@@ -574,7 +589,12 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
 	unsigned long fpexc;
 	int err = 0;
 
-	vfp_flush_hwstate(thread);
+	/*
+	 * If VFP has been used, then disable it to avoid corrupting
+	 * the new thread state.
+	 */
+	if (hwstate->fpexc & FPEXC_EN)
+		vfp_flush_hwstate(thread);
 

Review int vfp_restore_user_hwstate(struct user_vfp __user *ufp, · line 600

Quoted 2 lines

(quoted diff) +0 -0

 	/*
 	 * Copy the floating point registers. There can be unused

If the signal handler uses VFP, will it actually cause hwstate->fpexc & FPEXC_EN to be set? Won't it instead just enable the VFP in the hardware registers? (It looks to me that hwstate only gets updated by vfp_flush_hwstate().)

This certainly seems to be the case in my screen corruption situation where on entry to vfp_restore_user_hwstate() "fmrx(FPEXC) & FPEXC_EN" is true and "hwstate->fpexc & FPEXC_EN" is false.

With the code as it stands this means that on return from a signal handler the vfp hardware registers will be in whatever state the signal handler left them in, not the thread's state at the point the signal happened.

Quoted 3 lines · Jon Medhurst (Tixy)

I've bisected a screen corruption problem on vexpress down to this commit, I've commented at the end of the patch at to what I see the problem being...

Interesting and thanks for the heads up. I've been running with this patch applied for ages and haven't seen any problems myself but I've also been running headless. What's the best way to reproduce the problem on vexpress? I can connect a display if required.

Quoted 1 line · Jon Medhurst (Tixy)

Hi Will

Hi Jon,

Quoted 3 lines

I've bisected a screen corruption problem on vexpress down to this commit, I've commented at the end of the patch at to what I see the problem being...

Interesting and thanks for the heads up. I've been running with this patch applied for ages and haven't seen any problems myself but I've also been running headless. What's the best way to reproduce the problem on vexpress? I can connect a display if required.

Quoted 13 lines · Will Deacon

Quoted 12 lines

(quoted diff) +6 -1

@@ -574,7 +589,12 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
 	unsigned long fpexc;
 	int err = 0;
 
-	vfp_flush_hwstate(thread);
+	/*
+	 * If VFP has been used, then disable it to avoid corrupting
+	 * the new thread state.
+	 */
+	if (hwstate->fpexc & FPEXC_EN)
+		vfp_flush_hwstate(thread);
 

Quoted 7 lines

Review int vfp_restore_user_hwstate(struct user_vfp __user *ufp, · line 600

Quoted 2 lines

(quoted diff) +0 -0

 	/*
 	 * Copy the floating point registers. There can be unused

If the signal handler uses VFP, will it actually cause hwstate->fpexc & FPEXC_EN to be set? Won't it instead just enable the VFP in the hardware registers? (It looks to me that hwstate only gets updated by vfp_flush_hwstate().)

The idea is that we disable VFP in hardware (via vfp_flush_hwstate) prior to entering the signal handler. Then we clear hwstate->fpexc.FPEXC_EN so that we can detect if VFP gets used during the handler, since the exception handling code will set that back up if we trapon a VFP operation (see vfp_support_entry in vfphw.S).

Quoted 3 lines

This certainly seems to be the case in my screen corruption situation where on entry to vfp_restore_user_hwstate() "fmrx(FPEXC) & FPEXC_EN" is true and "hwstate->fpexc & FPEXC_EN" is false.

That sounds wrong to me. Out of interest, can you try this patch please?:

http://git.kernel.org/?p=linux/kernel/git/will/linux.git;a=commit;h=468c963e0210bf8108b17cf75066f25f39cabb56

Of course, if you're not running with CONFIG_PREEMPT, that shouldn't make a difference.

Quoted 5 lines · Will Deacon

The idea is that we disable VFP in hardware (via vfp_flush_hwstate) prior to entering the signal handler. Then we clear hwstate->fpexc.FPEXC_EN so that we can detect if VFP gets used during the handler, since the exception handling code will set that back up if we trapon a VFP operation (see vfp_support_entry in vfphw.S).

I had a look at vfp_support_entry. I can see it loading the hardware registers from the saved vfp state, but can't see where it updates the fpexc value in the saved vfp state. (I also can't think of why it would need to, but this is the first time I've looked Linux vfp code so there could be lots I've missed.)

You're right, in the lazy case (i.e. SMP) we update the hwstate on context-switch rather than on the fault. Your quick fix looks good to me (that is, unconditionally flushing the state after handling a signal). I was initially worried that the added flushing would cause a performance hit on the usual path, where VFP is not used in the handler, however since we flush before the signal handler, it really won't make a lot of difference.

I took the liberty of writing a commit message, so can I add your S-o-B to this please (I wasn't sure which email address to use)?

Thanks,

Will

Subject: [PATCH] ARM: vfp: fix VFP flushing regression on sigreturn path

Commit ff9a184c ("ARM: 7400/1: vfp: clear fpscr length and stride bits on entry to sig handler") flushes the VFP state prior to entering a signal handler so that a VFP operation inside the handler will trap and force a restore of ABI-compliant registers. Restoring the registers on the sigreturn path is predicated on the saved thread state indicating that VFP was used by the handler -- however for SMP platforms this is only set on context-switch, making the check unreliable and causing VFP register corruption in userspace.

This patch unconditionally flushes the VFP state after a signal handler. Since we already perform the flush before the handler and the flushing itself happens lazily, the effect on performance is negligible.

Reported-by Jon Medhurst <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/vfp/vfpmodule.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

arch/arm/vfp/vfpmodule.c +2 -12

index c5767b5..b0197b2 100644
@@ -577,12 +577,6 @@ int vfp_preserve_user_clear_hwstate(struct user_vfp __user *ufp,
 	 * entry.
 	 */
 	hwstate->fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK);
-
-	/*
-	 * Disable VFP in the hwstate so that we can detect if it gets
-	 * used.
-	 */
-	hwstate->fpexc &= ~FPEXC_EN;
 	return 0;
 }
 

@@ -595,12 +589,8 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
 	unsigned long fpexc;
 	int err = 0;
 
-	/*
-	 * If VFP has been used, then disable it to avoid corrupting
-	 * the new thread state.
-	 */
-	if (hwstate->fpexc & FPEXC_EN)
-		vfp_flush_hwstate(thread);
+	/* Disable VFP to avoid corrupting the new thread state. */
+	vfp_flush_hwstate(thread);
 
 	/*
 	 * Copy the floating point registers. There can be unused

Quoted 7 lines · Will Deacon

You're right, in the lazy case (i.e. SMP) we update the hwstate on context-switch rather than on the fault. Your quick fix looks good to me (that is, unconditionally flushing the state after handling a signal). I was initially worried that the added flushing would cause a performance hit on the usual path, where VFP is not used in the handler, however since we flush before the signal handler, it really won't make a lot of difference.

Quoted 2 lines

I took the liberty of writing a commit message, so can I add your S-o-B to this please (I wasn't sure which email address to use)?

Thanks, it's the day job, so...

Signed-off-by Jon Medhurst <[email protected]>