[PATCH v4 1/2] ARM: vfp: move user vfp state save/restore code out of signal.c

[PATCH v4 1/2] ARM: vfp: move user vfp state save/restore code out of signal.c

[Will Deacon]

The user VFP state must be preserved (subject to ucontext modifications)
across invocation of a signal handler and this is currently handled by
vfp_{preserve,restore}_context in signal.c

Since this code requires intimate low-level knowledge of the VFP state,
this patch moves it into vfpmodule.c.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---

v4: minor naming changes to appease Dave's dislike of the old ones.

 arch/arm/include/asm/thread_info.h |    7 +++
 arch/arm/kernel/signal.c           |   57 ++-----------------------
 arch/arm/vfp/vfpmodule.c           |   79 ++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 52 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index d4c24d4..0f04d84 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -118,6 +118,13 @@ extern void iwmmxt_task_switch(struct thread_info *);
 extern void vfp_sync_hwstate(struct thread_info *);
 extern void vfp_flush_hwstate(struct thread_info *);
 
+struct user_vfp;
+struct user_vfp_exc;
+
+extern int vfp_preserve_user_clear_hwstate(struct user_vfp __user *,
+					   struct user_vfp_exc __user *);
+extern int vfp_restore_user_hwstate(struct user_vfp __user *,
+				    struct user_vfp_exc __user *);
 #endif
 
 /*
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 9e617bd..b02ce1d 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -179,44 +179,23 @@ static int restore_iwmmxt_context(struct iwmmxt_sigframe *frame)
 
 static int preserve_vfp_context(struct vfp_sigframe __user *frame)
 {
-	struct thread_info *thread = current_thread_info();
-	struct vfp_hard_struct *h = &thread->vfpstate.hard;
 	const unsigned long magic = VFP_MAGIC;
 	const unsigned long size = VFP_STORAGE_SIZE;
 	int err = 0;
 
-	vfp_sync_hwstate(thread);
 	__put_user_error(magic, &frame->magic, err);
 	__put_user_error(size, &frame->size, err);
 
-	/*
-	 * Copy the floating point registers. There can be unused
-	 * registers see asm/hwcap.h for details.
-	 */
-	err |= __copy_to_user(&frame->ufp.fpregs, &h->fpregs,
-			      sizeof(h->fpregs));
-	/*
-	 * Copy the status and control register.
-	 */
-	__put_user_error(h->fpscr, &frame->ufp.fpscr, err);
-
-	/*
-	 * Copy the exception registers.
-	 */
-	__put_user_error(h->fpexc, &frame->ufp_exc.fpexc, err);
-	__put_user_error(h->fpinst, &frame->ufp_exc.fpinst, err);
-	__put_user_error(h->fpinst2, &frame->ufp_exc.fpinst2, err);
+	if (err)
+		return -EFAULT;
 
-	return err ? -EFAULT : 0;
+	return vfp_preserve_user_clear_hwstate(&frame->ufp, &frame->ufp_exc);
 }
 
 static int restore_vfp_context(struct vfp_sigframe __user *frame)
 {
-	struct thread_info *thread = current_thread_info();
-	struct vfp_hard_struct *h = &thread->vfpstate.hard;
 	unsigned long magic;
 	unsigned long size;
-	unsigned long fpexc;
 	int err = 0;
 
 	__get_user_error(magic, &frame->magic, err);
@@ -227,33 +206,7 @@ static int restore_vfp_context(struct vfp_sigframe __user *frame)
 	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
 		return -EINVAL;
 
-	vfp_flush_hwstate(thread);
-
-	/*
-	 * Copy the floating point registers. There can be unused
-	 * registers see asm/hwcap.h for details.
-	 */
-	err |= __copy_from_user(&h->fpregs, &frame->ufp.fpregs,
-				sizeof(h->fpregs));
-	/*
-	 * Copy the status and control register.
-	 */
-	__get_user_error(h->fpscr, &frame->ufp.fpscr, err);
-
-	/*
-	 * Sanitise and restore the exception registers.
-	 */
-	__get_user_error(fpexc, &frame->ufp_exc.fpexc, err);
-	/* Ensure the VFP is enabled. */
-	fpexc |= FPEXC_EN;
-	/* Ensure FPINST2 is invalid and the exception flag is cleared. */
-	fpexc &= ~(FPEXC_EX | FPEXC_FP2V);
-	h->fpexc = fpexc;
-
-	__get_user_error(h->fpinst, &frame->ufp_exc.fpinst, err);
-	__get_user_error(h->fpinst2, &frame->ufp_exc.fpinst2, err);
-
-	return err ? -EFAULT : 0;
+	return vfp_restore_user_hwstate(&frame->ufp, &frame->ufp_exc);
 }
 
 #endif
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 8f3ccdd..1dfe7d8 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -17,6 +17,8 @@
 #include <linux/sched.h>
 #include <linux/smp.h>
 #include <linux/init.h>
+#include <linux/uaccess.h>
+#include <linux/user.h>
 
 #include <asm/cputype.h>
 #include <asm/thread_notify.h>
@@ -527,6 +529,83 @@ void vfp_flush_hwstate(struct thread_info *thread)
 }
 
 /*
+ * Save the current VFP state into the provided structures and prepare
+ * for entry into a new function (signal handler).
+ */
+int vfp_preserve_user_clear_hwstate(struct user_vfp __user *ufp,
+				    struct user_vfp_exc __user *ufp_exc)
+{
+	struct thread_info *thread = current_thread_info();
+	struct vfp_hard_struct *hwstate = &thread->vfpstate.hard;
+	int err = 0;
+
+	/* Ensure that the saved hwstate is up-to-date. */
+	vfp_sync_hwstate(thread);
+
+	/*
+	 * Copy the floating point registers. There can be unused
+	 * registers see asm/hwcap.h for details.
+	 */
+	err |= __copy_to_user(&ufp->fpregs, &hwstate->fpregs,
+			      sizeof(hwstate->fpregs));
+	/*
+	 * Copy the status and control register.
+	 */
+	__put_user_error(hwstate->fpscr, &ufp->fpscr, err);
+
+	/*
+	 * Copy the exception registers.
+	 */
+	__put_user_error(hwstate->fpexc, &ufp_exc->fpexc, err);
+	__put_user_error(hwstate->fpinst, &ufp_exc->fpinst, err);
+	__put_user_error(hwstate->fpinst2, &ufp_exc->fpinst2, err);
+
+	if (err)
+		return -EFAULT;
+	return 0;
+}
+
+/* Sanitise and restore the current VFP state from the provided structures. */
+int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
+			     struct user_vfp_exc __user *ufp_exc)
+{
+	struct thread_info *thread = current_thread_info();
+	struct vfp_hard_struct *hwstate = &thread->vfpstate.hard;
+	unsigned long fpexc;
+	int err = 0;
+
+	vfp_flush_hwstate(thread);
+
+	/*
+	 * Copy the floating point registers. There can be unused
+	 * registers see asm/hwcap.h for details.
+	 */
+	err |= __copy_from_user(&hwstate->fpregs, &ufp->fpregs,
+				sizeof(hwstate->fpregs));
+	/*
+	 * Copy the status and control register.
+	 */
+	__get_user_error(hwstate->fpscr, &ufp->fpscr, err);
+
+	/*
+	 * Sanitise and restore the exception registers.
+	 */
+	__get_user_error(fpexc, &ufp_exc->fpexc, err);
+
+	/* Ensure the VFP is enabled. */
+	fpexc |= FPEXC_EN;
+
+	/* Ensure FPINST2 is invalid and the exception flag is cleared. */
+	fpexc &= ~(FPEXC_EX | FPEXC_FP2V);
+	hwstate->fpexc = fpexc;
+
+	__get_user_error(hwstate->fpinst, &ufp_exc->fpinst, err);
+	__get_user_error(hwstate->fpinst2, &ufp_exc->fpinst2, err);
+
+	return err ? -EFAULT : 0;
+}
+
+/*
  * VFP hardware can lose all context when a CPU goes offline.
  * As we will be running in SMP mode with CPU hotplug, we will save the
  * hardware state at every thread switch.  We clear our held state when
-- 
1.7.4.1

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Will Deacon]

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 <peter.maydell@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/vfp/vfpmodule.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 1dfe7d8..269f40d 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -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;
 
-	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);
 
 	/*
 	 * Copy the floating point registers. There can be unused
-- 
1.7.4.1

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Jon Medhurst (Tixy)]

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...

On Thu, 2012-02-23 at 15:07 +0000, Will Deacon wrote:
> 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 <peter.maydell@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/vfp/vfpmodule.c |   22 +++++++++++++++++++++-
>  1 files changed, 21 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index 1dfe7d8..269f40d 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -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;
>  
> -	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);
>  
>  	/*
>  	 * 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.

Assuming that I have understood things correctly, then I plan on posting
a patch that would make code changes like...

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index bc683b8..386a81a 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -574,11 +574,6 @@ int vfp_preserve_user_clear_hwstate(struct user_vfp
__user *ufp,
         */
        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;
 }
 
@@ -591,12 +586,7 @@ 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);
+       vfp_flush_hwstate(thread);
 
        /*
         * Copy the floating point registers. There can be unused


-- 
Tixy 

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Will Deacon]

On Mon, May 14, 2012 at 03:33:55PM +0100, Jon Medhurst (Tixy) wrote:
> Hi Will

Hi Jon,

> 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.

> On Thu, 2012-02-23 at 15:07 +0000, Will Deacon wrote:
> > @@ -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);
> >  
> >  	/*
> >  	 * 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).

> 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.

> 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.

That shouldn't happen -- what we need to figure out is why your previous
paragraph is occurring.

Cheers,

Will

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Will Deacon]

On Mon, May 14, 2012 at 03:59:31PM +0100, Will Deacon wrote:
> On Mon, May 14, 2012 at 03:33:55PM +0100, Jon Medhurst (Tixy) wrote:
> > 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.

Right, I've managed to trigger it on my board with lxdm, so I'll dig further
and let you know. The patch I posted doesn't help, so don't waste time
applying it.

Cheers,

Will

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Jon Medhurst (Tixy)]

On Mon, 2012-05-14 at 15:59 +0100, Will Deacon wrote:
> On Mon, May 14, 2012 at 03:33:55PM +0100, Jon Medhurst (Tixy) wrote:
> > Hi Will
> 
> Hi Jon,
> 
> > 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.

I'm running the Ubuntu Desktop image from Linaro's last release [1] but
with the kernel uImage replaced by one I built based on rc6 + one MMC
hack [2]. Once booted as far as the yellow wallpaper then moving the
mouse around the screen causes black streaks of pixels to be left of the
screen.

> > On Thu, 2012-02-23 at 15:07 +0000, Will Deacon wrote:
> > > @@ -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);
> > >  
> > >  	/*
> > >  	 * 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).

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.)

> > 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.

I'm not, and it didn't. :-)

-- 
Tixy

[1] http://releases.linaro.org/12.04/ubuntu/vexpress/
[2] http://git.linaro.org/gitweb?p=people/tixy/kernel.git;a=shortlog;h=refs/heads/vfp-regs-bug

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Will Deacon]

On Mon, May 14, 2012 at 05:02:59PM +0100, Jon Medhurst (Tixy) wrote:
> On Mon, 2012-05-14 at 15:59 +0100, Will Deacon wrote:
> > 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 <tixy@yxit.co.uk>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/vfp/vfpmodule.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index c5767b5..b0197b2 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -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
-- 
1.7.4.1

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Jon Medhurst (Tixy)]

On Mon, 2012-05-14 at 18:37 +0100, Will Deacon wrote:
> 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, it's the day job, so...

Signed-off-by: Jon Medhurst <tixy@linaro.org>

> 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 <tixy@yxit.co.uk>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/vfp/vfpmodule.c |   14 ++------------
>  1 files changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index c5767b5..b0197b2 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -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

[PATCH v4 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler

[Will Deacon]

On Mon, May 14, 2012 at 06:50:44PM +0100, Jon Medhurst (Tixy) wrote:
> On Mon, 2012-05-14 at 18:37 +0100, Will Deacon wrote:
> > 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, it's the day job, so...
> 
> Signed-off-by: Jon Medhurst <tixy@linaro.org>

Cheers Jon, I'll stick this in the patch system tomorrow. If it doesn't make
the final cut, I'll retrospectively send it to stable when they move to
3.4.

Will