[RFC PATCH v2 0/2] ARM: VFP: Save / restore VFP state on the signal handler path

[RFC PATCH v2 0/2] ARM: VFP: Save / restore VFP state on the signal handler path

[Imre Deak]

This fixes a VFP corruption problem in a process's main thread, when
both the main thread and a signal handler triggered for the process
uses floating point ops.

Changes since v1:
- save / restore VFP exception registers as well, to support user
  space exception handling/VFP emulation.
- extend the existing ptrace VFP struct with the exception regs,
  so that the VFP frame is equivalent for the ptrace and signal
  handler case.

--Imre

[RFC PATCH v2 1/2] ARM: VFP: add support to sync the VFP state of the current thread

[Imre Deak]

So far vfp_sync_state worked only for threads other than the current
one. This worked for tracing other threads, but not for PTRACE_TRACEME.
Syncing for the current thread will also be needed by an upcoming patch
adding support for VFP context save / restore around signal handlers.

For SMP we need get_cpu now, since we have to protect the FPEXC
register, other than this things remained the same for threads other
than the current.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
---
 arch/arm/vfp/vfpmodule.c |   46 +++++++++++++++++++++++++++++++---------------
 1 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index f60a540..d0b6e98 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -426,12 +426,19 @@ static inline void vfp_pm_init(void) { }
 #endif /* CONFIG_PM */
 
 /*
- * Synchronise the hardware VFP state of a thread other than current with the
- * saved one. This function is used by the ptrace mechanism.
+ * Synchronise the hardware VFP state of a thread with the saved one.
+ * This function is used by the ptrace mechanism and the signal handler path.
  */
-#ifdef CONFIG_SMP
 void vfp_sync_state(struct thread_info *thread)
 {
+	unsigned int cpu = get_cpu();
+	u32 fpexc = fmrx(FPEXC);
+	int vfp_enabled;
+	int self;
+
+	vfp_enabled = fpexc & FPEXC_EN;
+	self = thread == current_thread_info();
+#ifdef CONFIG_SMP
 	/*
 	 * On SMP systems, the VFP state is automatically saved at every
 	 * context switch. We mark the thread VFP state as belonging to a
@@ -439,18 +446,22 @@ void vfp_sync_state(struct thread_info *thread)
 	 * needed.
 	 */
 	thread->vfpstate.hard.cpu = NR_CPUS;
-}
-#else
-void vfp_sync_state(struct thread_info *thread)
-{
-	unsigned int cpu = get_cpu();
-	u32 fpexc = fmrx(FPEXC);
-
 	/*
-	 * If VFP is enabled, the previous state was already saved and
-	 * last_VFP_context updated.
+	 * Only the current thread's saved VFP context can be out-of-date.
+	 * For others there is nothing else to do, since we already ensured
+	 * force loading above.
 	 */
-	if (fpexc & FPEXC_EN)
+	if (!self)
+		goto out;
+#endif
+	/*
+	 * If the VFP is enabled only the current thread's saved VFP
+	 * context can get out-of-date. For other threads the context
+	 * was updated when the current thread started to use the VFP.
+	 * This also means that the context will be reloaded next time
+	 * the thread uses the VFP, so no need to enforce it.
+	 */
+	if (vfp_enabled && !self)
 		goto out;
 
 	if (!last_VFP_context[cpu])
@@ -459,8 +470,14 @@ void vfp_sync_state(struct thread_info *thread)
 	/*
 	 * Save the last VFP state on this CPU.
 	 */
-	fmxr(FPEXC, fpexc | FPEXC_EN);
+	if (!vfp_enabled)
+		fmxr(FPEXC, fpexc | FPEXC_EN);
 	vfp_save_state(last_VFP_context[cpu], fpexc);
+	/*
+	 * Disable VFP in case it was enabled so that the force reload
+	 * can happen.
+	 */
+	fpexc &= ~FPEXC_EN;
 	fmxr(FPEXC, fpexc);
 
 	/*
@@ -472,7 +489,6 @@ void vfp_sync_state(struct thread_info *thread)
 out:
 	put_cpu();
 }
-#endif
 
 #include <linux/smp.h>
 
-- 
1.6.6

[RFC PATCH v2 1/2] ARM: VFP: add support to sync the VFP state of the current thread

[Russell King - ARM Linux]

On Thu, Feb 04, 2010 at 11:38:02PM +0200, Imre Deak wrote:
> So far vfp_sync_state worked only for threads other than the current
> one. This worked for tracing other threads, but not for PTRACE_TRACEME.
> Syncing for the current thread will also be needed by an upcoming patch
> adding support for VFP context save / restore around signal handlers.
> 
> For SMP we need get_cpu now, since we have to protect the FPEXC
> register, other than this things remained the same for threads other
> than the current.

I don't think we should make this function any more complicated than it
already is.

For entering signal handlers, the situation is a lot more simple than
that found in vfp_sync_state() - since we know that we're always going
to be running the thread we're concerned with:

	- if VFP is enabled
		- use vfp_save_state() to save a copy of VFP state
		- disable VFP
	- if VFP is disabled
		- save a copy of ti->vfp_state

On signal return:

	- restore copy of ti->vfp_state
	- atomically (wrt preemption)
		- if last_VFP_context[cpu] is &ti->vfp_state
			- NULL out last_VFP_context[cpu]
		- disable VFP

There's no need to fiddle with ti->vfp_state.hard.cpu.

Note that saving the vfp_hard_struct to userspace is not on since it
then becomes a userspace API; it has to be translated into something
that we can keep better control over for userspace.  Moreover,
vfp_hard_struct contains state private to the kernel; allowing userspace
to have control over things like 'cpu' is a very bad idea.

[RFC PATCH v2 1/2] ARM: VFP: add support to sync the VFP state of the current thread

[Russell King - ARM Linux]

On Sat, Feb 06, 2010 at 10:32:30AM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 04, 2010 at 11:38:02PM +0200, Imre Deak wrote:
> > So far vfp_sync_state worked only for threads other than the current
> > one. This worked for tracing other threads, but not for PTRACE_TRACEME.
> > Syncing for the current thread will also be needed by an upcoming patch
> > adding support for VFP context save / restore around signal handlers.
> > 
> > For SMP we need get_cpu now, since we have to protect the FPEXC
> > register, other than this things remained the same for threads other
> > than the current.
> 
> I don't think we should make this function any more complicated than it
> already is.

... and the more I look at vfp_sync_state(), the more I believe it's
trying to do its job in a really obscure way.

Essentially, last_VFP_context[] tracks who owns the state in the VFP
hardware.  If last_VFP_context[] is the context for the thread which
we're interested in, then the VFP hardware has context which is not
saved in the software state - so we need to bring the software state
up to date.

If last_VFP_context[] is for some other thread, we really don't care
what state the VFP hardware is in; it doesn't contain any information
pertinent to the thread we're trying to deal with - so leave the
hardware well alone.

As a side effect, this makes vfp_sync_state() callable for the current
thread, and allows it to do the right thing there as well.

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index f60a540..6447d78 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -444,32 +444,28 @@ void vfp_sync_state(struct thread_info *thread)
 void vfp_sync_state(struct thread_info *thread)
 {
 	unsigned int cpu = get_cpu();
-	u32 fpexc = fmrx(FPEXC);
 
 	/*
-	 * If VFP is enabled, the previous state was already saved and
-	 * last_VFP_context updated.
+	 * If the thread we're interested in is the current owner of the
+	 * hardware VFP state, then we need to save its state.
 	 */
-	if (fpexc & FPEXC_EN)
-		goto out;
-
-	if (!last_VFP_context[cpu])
-		goto out;
+	if (last_VFP_context[cpu] == &thread->vfpstate) {
+		u32 fpexc = fmrx(FPEXC);
 
-	/*
-	 * Save the last VFP state on this CPU.
-	 */
-	fmxr(FPEXC, fpexc | FPEXC_EN);
-	vfp_save_state(last_VFP_context[cpu], fpexc);
-	fmxr(FPEXC, fpexc);
+		/*
+		 * Save the last VFP state on this CPU.
+		 */
+		fmxr(FPEXC, fpexc | FPEXC_EN);
+		vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);
+		fmxr(FPEXC, fpexc);
 
-	/*
-	 * Set the context to NULL to force a reload the next time the thread
-	 * uses the VFP.
-	 */
-	last_VFP_context[cpu] = NULL;
+		/*
+		 * Set the context to NULL to force a reload the next time
+		 * the thread uses the VFP.
+		 */
+		last_VFP_context[cpu] = NULL;
+	}
 
-out:
 	put_cpu();
 }
 #endif

[RFC PATCH v2 1/2] ARM: VFP: add support to sync the VFP state of the current thread

[Russell King - ARM Linux]

On Sat, Feb 06, 2010 at 11:20:48AM +0000, Russell King - ARM Linux wrote:
> +		/*
> +		 * Save the last VFP state on this CPU.
> +		 */
> +		fmxr(FPEXC, fpexc | FPEXC_EN);
> +		vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);
> +		fmxr(FPEXC, fpexc);

Erm, slight correction here:

+		fmxr(FPEXC, fpexc & ~FPEXC_EN);

[RFC PATCH v2 1/2] ARM: VFP: add support to sync the VFP state of the current thread

[Russell King - ARM Linux]

On Sat, Feb 06, 2010 at 11:32:49AM +0000, Russell King - ARM Linux wrote:
> On Sat, Feb 06, 2010 at 11:20:48AM +0000, Russell King - ARM Linux wrote:
> > +		/*
> > +		 * Save the last VFP state on this CPU.
> > +		 */
> > +		fmxr(FPEXC, fpexc | FPEXC_EN);
> > +		vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);
> > +		fmxr(FPEXC, fpexc);
> 
> Erm, slight correction here:
> 
> +		fmxr(FPEXC, fpexc & ~FPEXC_EN);

And finally... let's improve the ptrace code to allow more efficient
monitoring of VFP state.  There's no point invalidating the hardware
context just because we want to inspect the VFP state via ptrace.

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 2dfb7d7..b74970e 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -115,7 +115,8 @@ extern void iwmmxt_task_restore(struct thread_info *, void *);
 extern void iwmmxt_task_release(struct thread_info *);
 extern void iwmmxt_task_switch(struct thread_info *);
 
-extern void vfp_sync_state(struct thread_info *thread);
+extern void vfp_sync_hwstate(struct thread_info *);
+extern void vfp_flush_hwstate(struct thread_info *);
 
 #endif
 
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index bdf002b..08f899f 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -700,7 +700,7 @@ static int ptrace_getvfpregs(struct task_struct *tsk, void __user *data)
 	union vfp_state *vfp = &thread->vfpstate;
 	struct user_vfp __user *ufp = data;
 
-	vfp_sync_state(thread);
+	vfp_sync_hwstate(thread);
 
 	/* copy the floating point registers */
 	if (copy_to_user(&ufp->fpregs, &vfp->hard.fpregs,
@@ -723,7 +723,7 @@ static int ptrace_setvfpregs(struct task_struct *tsk, void __user *data)
 	union vfp_state *vfp = &thread->vfpstate;
 	struct user_vfp __user *ufp = data;
 
-	vfp_sync_state(thread);
+	vfp_sync_hwstate(thread);
 
 	/* copy the floating point registers */
 	if (copy_from_user(&vfp->hard.fpregs, &ufp->fpregs,
@@ -734,6 +734,8 @@ static int ptrace_setvfpregs(struct task_struct *tsk, void __user *data)
 	if (get_user(vfp->hard.fpscr, &ufp->fpscr))
 		return -EFAULT;
 
+	vfp_flush_hwstate(thread);
+
 	return 0;
 }
 #endif
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 86a57ae..f61c622 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -430,7 +430,11 @@ static inline void vfp_pm_init(void) { }
  * saved one. This function is used by the ptrace mechanism.
  */
 #ifdef CONFIG_SMP
-void vfp_sync_state(struct thread_info *thread)
+void vfp_sync_hwstate(struct thread_info *thread)
+{
+}
+
+void vfp_flush_hwstate(struct thread_info *thread)
 {
 	/*
 	 * On SMP systems, the VFP state is automatically saved at every
@@ -441,7 +445,7 @@ void vfp_sync_state(struct thread_info *thread)
 	thread->vfpstate.hard.cpu = NR_CPUS;
 }
 #else
-void vfp_sync_state(struct thread_info *thread)
+void vfp_sync_hwstate(struct thread_info *thread)
 {
 	unsigned int cpu = get_cpu();
 
@@ -457,6 +461,23 @@ void vfp_sync_state(struct thread_info *thread)
 		 */
 		fmxr(FPEXC, fpexc | FPEXC_EN);
 		vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);
+		fmxr(FPEXC, fpexc);
+	}
+
+	put_cpu();
+}
+
+void vfp_flush_hwstate(struct thread_info *thread)
+{
+	unsigned int cpu = get_cpu();
+
+	/*
+	 * If the thread we're interested in is the current owner of the
+	 * hardware VFP state, then we need to save its state.
+	 */
+	if (last_VFP_context[cpu] == &thread->vfpstate) {
+		u32 fpexc = fmrx(FPEXC);
+
 		fmxr(FPEXC, fpexc & ~FPEXC_EN);
 
 		/*

[RFC PATCH v2 1/2] ARM: VFP: add support to sync the VFP state of the current thread

[Imre Deak]

On Sat, Feb 06, 2010 at 12:20:48PM +0100, ext Russell King - ARM Linux wrote:
> On Sat, Feb 06, 2010 at 10:32:30AM +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 04, 2010 at 11:38:02PM +0200, Imre Deak wrote:
> > > So far vfp_sync_state worked only for threads other than the current
> > > one. This worked for tracing other threads, but not for PTRACE_TRACEME.
> > > Syncing for the current thread will also be needed by an upcoming patch
> > > adding support for VFP context save / restore around signal handlers.
> > > 
> > > For SMP we need get_cpu now, since we have to protect the FPEXC
> > > register, other than this things remained the same for threads other
> > > than the current.
> > 
> > I don't think we should make this function any more complicated than it
> > already is.
> 
> ... and the more I look at vfp_sync_state(), the more I believe it's
> trying to do its job in a really obscure way.
> 
> Essentially, last_VFP_context[] tracks who owns the state in the VFP
> hardware.  If last_VFP_context[] is the context for the thread which
> we're interested in, then the VFP hardware has context which is not
> saved in the software state - so we need to bring the software state
> up to date.
> 
> If last_VFP_context[] is for some other thread, we really don't care
> what state the VFP hardware is in; it doesn't contain any information
> pertinent to the thread we're trying to deal with - so leave the
> hardware well alone.
> 
> As a side effect, this makes vfp_sync_state() callable for the current
> thread, and allows it to do the right thing there as well.

Agreed, vfp_sync_state is cleaner and more efficient this way. One note
is that the SMP version will still not update the software state in case
it's called for the current thread and the VFP is enabled.

--Imre

> 
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index f60a540..6447d78 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -444,32 +444,28 @@ void vfp_sync_state(struct thread_info *thread)
>  void vfp_sync_state(struct thread_info *thread)
>  {
>  	unsigned int cpu = get_cpu();
> -	u32 fpexc = fmrx(FPEXC);
>  
>  	/*
> -	 * If VFP is enabled, the previous state was already saved and
> -	 * last_VFP_context updated.
> +	 * If the thread we're interested in is the current owner of the
> +	 * hardware VFP state, then we need to save its state.
>  	 */
> -	if (fpexc & FPEXC_EN)
> -		goto out;
> -
> -	if (!last_VFP_context[cpu])
> -		goto out;
> +	if (last_VFP_context[cpu] == &thread->vfpstate) {
> +		u32 fpexc = fmrx(FPEXC);
>  
> -	/*
> -	 * Save the last VFP state on this CPU.
> -	 */
> -	fmxr(FPEXC, fpexc | FPEXC_EN);
> -	vfp_save_state(last_VFP_context[cpu], fpexc);
> -	fmxr(FPEXC, fpexc);
> +		/*
> +		 * Save the last VFP state on this CPU.
> +		 */
> +		fmxr(FPEXC, fpexc | FPEXC_EN);
> +		vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);
> +		fmxr(FPEXC, fpexc);
>  
> -	/*
> -	 * Set the context to NULL to force a reload the next time the thread
> -	 * uses the VFP.
> -	 */
> -	last_VFP_context[cpu] = NULL;
> +		/*
> +		 * Set the context to NULL to force a reload the next time
> +		 * the thread uses the VFP.
> +		 */
> +		last_VFP_context[cpu] = NULL;
> +	}
>  
> -out:
>  	put_cpu();
>  }
>  #endif

[RFC PATCH v2 2/2] ARM: VFP: preserve the HW context when calling signal handlers

[Imre Deak]

Signal handlers can use floating point, so prevent them to corrupt
the main thread's VFP context. So far there were two signal stack
frame formats defined based on the VFP implementation, but the user
struct used for ptrace covers all posibilities, so use it for the
signal stack too. This patch extends the user struct and leaves
its magic number the same, in the hope that user space code does
not depend on its size and can parse the original regs w/o
problems.

Support to save / restore the exception registers was added by
Will Deacon.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>

---
 arch/arm/include/asm/ucontext.h |   19 +++-----
 arch/arm/include/asm/user.h     |    3 +
 arch/arm/kernel/signal.c        |   91 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 97 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/ucontext.h b/arch/arm/include/asm/ucontext.h
index bf65e9f..1c3236b 100644
--- a/arch/arm/include/asm/ucontext.h
+++ b/arch/arm/include/asm/ucontext.h
@@ -59,23 +59,18 @@ struct iwmmxt_sigframe {
 #endif /* CONFIG_IWMMXT */
 
 #ifdef CONFIG_VFP
-#if __LINUX_ARM_ARCH__ < 6
-/* For ARM pre-v6, we use fstmiax and fldmiax.  This adds one extra
- * word after the registers, and a word of padding@the end for
- * alignment.  */
 #define VFP_MAGIC		0x56465001
-#define VFP_STORAGE_SIZE	152
-#else
-#define VFP_MAGIC		0x56465002
-#define VFP_STORAGE_SIZE	144
-#endif
 
 struct vfp_sigframe
 {
 	unsigned long		magic;
 	unsigned long		size;
-	union vfp_state		storage;
-};
+	struct user_vfp		ufp;
+} __attribute__((__aligned__(8)));
+
+/* 8 byte for magic and size, 272 byte for ufp */
+#define VFP_STORAGE_SIZE	sizeof(struct vfp_sigframe)
+
 #endif /* CONFIG_VFP */
 
 /*
@@ -91,7 +86,7 @@ struct aux_sigframe {
 #ifdef CONFIG_IWMMXT
 	struct iwmmxt_sigframe	iwmmxt;
 #endif
-#if 0 && defined CONFIG_VFP /* Not yet saved.  */
+#ifdef CONFIG_VFP
 	struct vfp_sigframe	vfp;
 #endif
 	/* Something that isn't a valid magic number for any coprocessor.  */
diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
index df95e05..ea7e44d 100644
--- a/arch/arm/include/asm/user.h
+++ b/arch/arm/include/asm/user.h
@@ -88,6 +88,9 @@ struct user{
 struct user_vfp {
 	unsigned long long fpregs[32];
 	unsigned long fpscr;
+	unsigned long fpexc;
+	unsigned long fpinst;
+	unsigned long fpinst2;
 };
 
 #endif /* _ARM_USER_H */
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index e7714f3..6a36851 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -18,6 +18,7 @@
 #include <asm/cacheflush.h>
 #include <asm/ucontext.h>
 #include <asm/unistd.h>
+#include <asm/vfp.h>
 
 #include "ptrace.h"
 #include "signal.h"
@@ -175,6 +176,88 @@ static int restore_iwmmxt_context(struct iwmmxt_sigframe *frame)
 
 #endif
 
+#ifdef CONFIG_VFP
+
+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_state(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.fpexc, err);
+	__put_user_error(h->fpinst, &frame->ufp.fpinst, err);
+	__put_user_error(h->fpinst2, &frame->ufp.fpinst2, err);
+
+	return err ? -EFAULT : 0;
+}
+
+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;
+
+	vfp_sync_state(thread);
+	__get_user_error(magic, &frame->magic, err);
+	__get_user_error(size, &frame->size, err);
+
+	if (err)
+		return -EFAULT;
+	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
+		return -EINVAL;
+
+	/*
+	 * 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.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.fpinst, err);
+	__get_user_error(h->fpinst2, &frame->ufp.fpinst2, err);
+
+	return err ? -EFAULT : 0;
+}
+
+#endif
+
 /*
  * Do a signal return; undo the signal stack.  These are aligned to 64-bit.
  */
@@ -233,8 +316,8 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
 		err |= restore_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_restore_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= restore_vfp_context(&aux->vfp);
 #endif
 
 	return err;
@@ -348,8 +431,8 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
 		err |= preserve_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_save_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= preserve_vfp_context(&aux->vfp);
 #endif
 	__put_user_error(0, &aux->end_magic, err);
 
-- 
1.6.6

[RFC PATCH v2 2/2] ARM: VFP: preserve the HW context when calling signal handlers

[Russell King - ARM Linux]

On Thu, Feb 04, 2010 at 11:38:30PM +0200, Imre Deak wrote:
> diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
> index df95e05..ea7e44d 100644
> --- a/arch/arm/include/asm/user.h
> +++ b/arch/arm/include/asm/user.h
> @@ -88,6 +88,9 @@ struct user{
>  struct user_vfp {
>  	unsigned long long fpregs[32];
>  	unsigned long fpscr;
> +	unsigned long fpexc;
> +	unsigned long fpinst;
> +	unsigned long fpinst2;

Absolutely no way is this anywhere near suitable.  This is a _userspace_
_visible_ API change, breaking anyone using the ptrace API by overwriting
additional memory which they will not be expecting.

[RFC PATCH v2 2/2] ARM: VFP: preserve the HW context when calling signal handlers

[Imre Deak]

On Sat, Feb 06, 2010 at 10:25:44AM +0100, ext Russell King - ARM Linux wrote:
> On Thu, Feb 04, 2010 at 11:38:30PM +0200, Imre Deak wrote:
> > diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
> > index df95e05..ea7e44d 100644
> > --- a/arch/arm/include/asm/user.h
> > +++ b/arch/arm/include/asm/user.h
> > @@ -88,6 +88,9 @@ struct user{
> >  struct user_vfp {
> >  	unsigned long long fpregs[32];
> >  	unsigned long fpscr;
> > +	unsigned long fpexc;
> > +	unsigned long fpinst;
> > +	unsigned long fpinst2;
> 
> Absolutely no way is this anywhere near suitable.  This is a _userspace_
> _visible_ API change, breaking anyone using the ptrace API by overwriting
> additional memory which they will not be expecting.

Right, don't know what made me think that this will work out. Perhaps
someone mentioning that the corresponding IOCTL is not in use yet. But that
was about half a year ago :)

I'll resend adding the new regs only to the signal frame, leaving the above
as is.

--Imre

[RFC PATCH v2 2/2] ARM: VFP: preserve the HW context when calling signal handlers

[Russell King - ARM Linux]

On Sat, Feb 06, 2010 at 12:02:21PM +0200, Imre Deak wrote:
> Right, don't know what made me think that this will work out. Perhaps
> someone mentioning that the corresponding IOCTL is not in use yet. But that
> was about half a year ago :)
> 
> I'll resend adding the new regs only to the signal frame, leaving the above
> as is.

Second point on this.  Currently, the VFP context which we thought
about saving onto the sigframe looks like this:

#if __LINUX_ARM_ARCH__ < 6
/* For ARM pre-v6, we use fstmiax and fldmiax.  This adds one extra
 * word after the registers, and a word of padding at the end for
 * alignment.  */
#define VFP_MAGIC               0x56465001
#define VFP_STORAGE_SIZE        152
#else
#define VFP_MAGIC               0x56465002
#define VFP_STORAGE_SIZE        144
#endif

struct vfp_sigframe
{
        unsigned long           magic;
        unsigned long           size;
        union vfp_state         storage;
};

This is horribly outdated.  We save:

- 16 or 32 64-bit registers depending on whether VFPv3
- one 32-bit word of fpmx state if < ARMv6
- one 32-bit word of fpexc
- one 32-bit word of fpscr
- one 32-bit word of fpinst
- one 32-bit word of fpinst2
- cpu if SMP

This gives potentially the following options:

VFPv3	ARMv6	SMP
n	n	n	16*8+5*4 = 148
y	n	n	32*8+5*4 = 276
n	y	n	16*8+4*4 = 144
y	y	n	32*8+4*4 = 272
n	n	y	16*8+6*4 = 152	*
y	n	y	32*8+6*4 = 280	*
n	y	y	16*8+5*4 = 148
y	y	y	32*8+5*4 = 276

The two marked with '*' are very unlikely to occur.

I think this technically comes under the heading of 'a disaster
waiting to happen'.

We currently have no way to convey these possibilities to anything
dealing with stack frames; certainly userspace applications which may
decide to inspect the sigframe aren't going to deal with all these
possibilities correctly - if we're lucky, they'll get one case right.

The stack frame should not care about whether we're running on SMP or
not - and that rules out using vfp_hard_struct or vfp_state in the
sigframe.  So we're into having a different structure.

Since sigframes are tagged, let's make use of that facility.  Let's
save the 64-bit VFP registers - that way, the size of this structure
defines how many registers there are.  num_regs = struct size / 8.

Save fpmx_state as a separate tagged entity if it's present.  (I doubt
anyone has need to use this - it's just required to preserve VFP state.)

Then, save the remainder of the state information (fpexc, fpscr, fpinst,
fpinst2 but _not_ cpu) as another separate tagged entity.

This means anyone who wants to inspect the VFP state has two or three
tags to look for, but they're all well-defined, and are hopefully
protected against the complexities of having to work out how to decode
the current variable sized structure which we have@present.

[RFC PATCH v2 2/2] ARM: VFP: preserve the HW context when calling signal handlers

[Imre Deak]

On Sat, Feb 06, 2010 at 01:12:03PM +0100, ext Russell King - ARM Linux wrote:
> On Sat, Feb 06, 2010 at 12:02:21PM +0200, Imre Deak wrote:
> > Right, don't know what made me think that this will work out. Perhaps
> > someone mentioning that the corresponding IOCTL is not in use yet. But that
> > was about half a year ago :)
> > 
> > I'll resend adding the new regs only to the signal frame, leaving the above
> > as is.
> 
> Second point on this.  Currently, the VFP context which we thought
> about saving onto the sigframe looks like this:
> 
> #if __LINUX_ARM_ARCH__ < 6
> /* For ARM pre-v6, we use fstmiax and fldmiax.  This adds one extra
>  * word after the registers, and a word of padding at the end for
>  * alignment.  */
> #define VFP_MAGIC               0x56465001
> #define VFP_STORAGE_SIZE        152
> #else
> #define VFP_MAGIC               0x56465002
> #define VFP_STORAGE_SIZE        144
> #endif
> 
> struct vfp_sigframe
> {
>         unsigned long           magic;
>         unsigned long           size;
>         union vfp_state         storage;
> };
> 
> This is horribly outdated.  We save:
> 
> - 16 or 32 64-bit registers depending on whether VFPv3
> - one 32-bit word of fpmx state if < ARMv6
> - one 32-bit word of fpexc
> - one 32-bit word of fpscr
> - one 32-bit word of fpinst
> - one 32-bit word of fpinst2
> - cpu if SMP
>
> This gives potentially the following options:
> 
> VFPv3	ARMv6	SMP
> n	n	n	16*8+5*4 = 148
> y	n	n	32*8+5*4 = 276
> n	y	n	16*8+4*4 = 144
> y	y	n	32*8+4*4 = 272
> n	n	y	16*8+6*4 = 152	*
> y	n	y	32*8+6*4 = 280	*
> n	y	y	16*8+5*4 = 148
> y	y	y	32*8+5*4 = 276

But the proposed patch didn't use union vfp_state, but a fixed size
struct for all 8 possibilities. Then the only drawback would be
undefined regs in certain cases, but the register positions would be
fixed. Also cpu is not part of the that struct.

> 
> The two marked with '*' are very unlikely to occur.
> 
> I think this technically comes under the heading of 'a disaster
> waiting to happen'.
> 
> We currently have no way to convey these possibilities to anything
> dealing with stack frames; certainly userspace applications which may
> decide to inspect the sigframe aren't going to deal with all these
> possibilities correctly - if we're lucky, they'll get one case right.
> 
> The stack frame should not care about whether we're running on SMP or
> not - and that rules out using vfp_hard_struct or vfp_state in the
> sigframe.  So we're into having a different structure.
> 
> Since sigframes are tagged, let's make use of that facility.  Let's
> save the 64-bit VFP registers - that way, the size of this structure
> defines how many registers there are.  num_regs = struct size / 8.
> 
> Save fpmx_state as a separate tagged entity if it's present.  (I doubt
> anyone has need to use this - it's just required to preserve VFP state.)
> 
> Then, save the remainder of the state information (fpexc, fpscr, fpinst,
> fpinst2 but _not_ cpu) as another separate tagged entity.
> 
> This means anyone who wants to inspect the VFP state has two or three
> tags to look for, but they're all well-defined, and are hopefully
> protected against the complexities of having to work out how to decode
> the current variable sized structure which we have at present.

Would this still give a benefit over the one fixed struct solution?

--Imre

[RFC PATCH v3 0/3] ARM: VFP: Save / restore VFP state on the signal handler path

[imre.deak at nokia.com]

This fixes a VFP corruption problem in a process's main thread, when
both the main thread and a signal handler triggered for the process
uses floating point ops.

Changes since v2 according to Russell's comments and patches:
- refactor vfp_sync_state to make it clearer
- break vfp_sync_state into vfp_{save/restore}_hwstate for efficiency
- Keep the additional VFP exception registers in a separate struct. This
  way we'll keep the existing ABI intact.

--Imre

[RFC PATCH v4 0/3] ARM: VFP: Save / restore VFP state on the signal handler path

[imre.deak at nokia.com]

Changes since v3:
- Rebased against mainline v2.6.34-rc3, since some parts have already
  been applied there.

Note that this problem might affect more people than previously thought.
The CodeSourcery 2009q1-203 glibc_ports has some NEON optimized functions;
for example using memset or memcpy in a signal handler and floating point
in the main thread can corrupt the main thread's context. You can test this
by replacing the floating point ops in the signal handler below with some
dummy memset/memcpy.

I could only test these patches on a Cortex A8 UP system; it would be great
if someone with an SMP board could give it a go. The test app to reproduce
the problem (I've built it with -mfloat-abi=softfp -mfpu=neon):

#include <stdio.h>
#include <signal.h>
#include <stdlib.h>
#include <string.h>

#include <sys/time.h>

void handler(int signum)
{
	volatile float f;

	f = 50;
	f /= 2;
	f *= 2;
	if (f != 50) {
		printf("signal %3.3f\n", f);
		exit(-1);
	}
}

int main(int argc, char **argv)
{
	struct itimerval ival;

	if (signal(SIGALRM, handler) == SIG_ERR) {
		perror("signal");
		exit(-1);
	}

	memset(&ival, 0, sizeof(ival));
	ival.it_value.tv_usec = 1000;
	ival.it_interval.tv_usec = 10000;

	if (setitimer(ITIMER_REAL, &ival, NULL) < 0) {
		perror("setitimer");
		exit(-1);
	}

	while (1) {
		volatile float f = 30;

		f /= 3;
		f *= 3;
		if (f != 30) {
			printf("main %3.3f\n", f);
			exit(-1);
		}
	}
}

[RFC PATCH v5 0/2] ARM: VFP: Save / restore VFP state on the signal handler path

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

Changes since v4:
- Do not add the packed attribute to user_vfp or user_vfp_exc.
  I added packed to these structs to prevent the extra padding in
  vfp_sigframe. But actually the padding is in accordance with the
  ARM ABI. Also removing the padding from user_vfp would break
  the ptrace interface.

--Imre

[RFC PATCH v5 1/2] ARM: VFP: fix the SMP versions of vfp_{sync, flush}_hwstate

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

Recently the UP versions of these functions were refactored and as
a side effect it became possible to call them for the current thread.
This isn't true for the SMP versions however, so fix this up.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
---
 arch/arm/vfp/vfpmodule.c |   31 ++++++++++---------------------
 1 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 7f3f59f..477f240 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -428,26 +428,6 @@ static void vfp_pm_init(void)
 static inline void vfp_pm_init(void) { }
 #endif /* CONFIG_PM */
 
-/*
- * Synchronise the hardware VFP state of a thread other than current with the
- * saved one. This function is used by the ptrace mechanism.
- */
-#ifdef CONFIG_SMP
-void vfp_sync_hwstate(struct thread_info *thread)
-{
-}
-
-void vfp_flush_hwstate(struct thread_info *thread)
-{
-	/*
-	 * On SMP systems, the VFP state is automatically saved at every
-	 * context switch. We mark the thread VFP state as belonging to a
-	 * non-existent CPU so that the saved one will be reloaded when
-	 * needed.
-	 */
-	thread->vfpstate.hard.cpu = NR_CPUS;
-}
-#else
 void vfp_sync_hwstate(struct thread_info *thread)
 {
 	unsigned int cpu = get_cpu();
@@ -490,9 +470,18 @@ void vfp_flush_hwstate(struct thread_info *thread)
 		last_VFP_context[cpu] = NULL;
 	}
 
+#ifdef CONFIG_SMP
+	/*
+	 * For SMP we still have to take care of the case where the thread
+	 * migrates to another CPU and then back to the original CPU on which
+	 * the last VFP user is still the same thread. Mark the thread VFP
+	 * state as belonging to a non-existent CPU so that the saved one will
+	 * be reloaded in the above case.
+	 */
+	thread->vfpstate.hard.cpu = NR_CPUS;
+#endif
 	put_cpu();
 }
-#endif
 
 #include <linux/smp.h>
 
-- 
1.7.0.2

[RFC PATCH v5 1/2] ARM: VFP: fix the SMP versions of vfp_{sync,flush}_hwstate

[Russell King - ARM Linux]

On Fri, Apr 02, 2010 at 05:36:02PM +0300, imre.deak at nokia.com wrote:
> From: Imre Deak <imre.deak@nokia.com>
> 
> Recently the UP versions of these functions were refactored and as
> a side effect it became possible to call them for the current thread.
> This isn't true for the SMP versions however, so fix this up.

This look safe to me; thanks.

[RFC PATCH v5 2/2] ARM: VFP: preserve the HW context when calling signal handlers

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

Signal handlers can use floating point, so prevent them to corrupt
the main thread's VFP context. So far there were two signal stack
frame formats defined based on the VFP implementation, but the user
struct used for ptrace covers all posibilities, so use it for the
signal stack too.

Introduce also a new user struct for VFP exception registers. In
this too fields not relevant to the current VFP architecture are
ignored.

Support to save / restore the exception registers was added by
Will Deacon.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/ucontext.h |   23 +++++-----
 arch/arm/include/asm/user.h     |   12 +++++-
 arch/arm/kernel/signal.c        |   93 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 111 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/ucontext.h b/arch/arm/include/asm/ucontext.h
index bf65e9f..47f023a 100644
--- a/arch/arm/include/asm/ucontext.h
+++ b/arch/arm/include/asm/ucontext.h
@@ -59,23 +59,22 @@ struct iwmmxt_sigframe {
 #endif /* CONFIG_IWMMXT */
 
 #ifdef CONFIG_VFP
-#if __LINUX_ARM_ARCH__ < 6
-/* For ARM pre-v6, we use fstmiax and fldmiax.  This adds one extra
- * word after the registers, and a word of padding@the end for
- * alignment.  */
 #define VFP_MAGIC		0x56465001
-#define VFP_STORAGE_SIZE	152
-#else
-#define VFP_MAGIC		0x56465002
-#define VFP_STORAGE_SIZE	144
-#endif
 
 struct vfp_sigframe
 {
 	unsigned long		magic;
 	unsigned long		size;
-	union vfp_state		storage;
-};
+	struct user_vfp		ufp;
+	struct user_vfp_exc	ufp_exc;
+} __attribute__((__aligned__(8)));
+
+/*
+ *  8 byte for magic and size, 264 byte for ufp, 12 bytes for ufp_exc,
+ *  4 bytes padding.
+ */
+#define VFP_STORAGE_SIZE	sizeof(struct vfp_sigframe)
+
 #endif /* CONFIG_VFP */
 
 /*
@@ -91,7 +90,7 @@ struct aux_sigframe {
 #ifdef CONFIG_IWMMXT
 	struct iwmmxt_sigframe	iwmmxt;
 #endif
-#if 0 && defined CONFIG_VFP /* Not yet saved.  */
+#ifdef CONFIG_VFP
 	struct vfp_sigframe	vfp;
 #endif
 	/* Something that isn't a valid magic number for any coprocessor.  */
diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
index df95e05..05ac4b0 100644
--- a/arch/arm/include/asm/user.h
+++ b/arch/arm/include/asm/user.h
@@ -83,11 +83,21 @@ struct user{
 
 /*
  * User specific VFP registers. If only VFPv2 is present, registers 16 to 31
- * are ignored by the ptrace system call.
+ * are ignored by the ptrace system call and the signal handler.
  */
 struct user_vfp {
 	unsigned long long fpregs[32];
 	unsigned long fpscr;
 };
 
+/*
+ * VFP exception registers exposed to user space during signal delivery.
+ * Fields not relavant to the current VFP architecture are ignored.
+ */
+struct user_vfp_exc {
+	unsigned long	fpexc;
+	unsigned long	fpinst;
+	unsigned long	fpinst2;
+};
+
 #endif /* _ARM_USER_H */
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index e7714f3..907d5a6 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -18,6 +18,7 @@
 #include <asm/cacheflush.h>
 #include <asm/ucontext.h>
 #include <asm/unistd.h>
+#include <asm/vfp.h>
 
 #include "ptrace.h"
 #include "signal.h"
@@ -175,6 +176,90 @@ static int restore_iwmmxt_context(struct iwmmxt_sigframe *frame)
 
 #endif
 
+#ifdef CONFIG_VFP
+
+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);
+
+	return err ? -EFAULT : 0;
+}
+
+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);
+	__get_user_error(size, &frame->size, err);
+
+	if (err)
+		return -EFAULT;
+	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
+		return -EINVAL;
+
+	/*
+	 * 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);
+
+	if (!err)
+		vfp_flush_hwstate(thread);
+
+	return err ? -EFAULT : 0;
+}
+
+#endif
+
 /*
  * Do a signal return; undo the signal stack.  These are aligned to 64-bit.
  */
@@ -233,8 +318,8 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
 		err |= restore_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_restore_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= restore_vfp_context(&aux->vfp);
 #endif
 
 	return err;
@@ -348,8 +433,8 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
 		err |= preserve_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_save_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= preserve_vfp_context(&aux->vfp);
 #endif
 	__put_user_error(0, &aux->end_magic, err);
 
-- 
1.7.0.2

[RFC PATCH v5 2/2] ARM: VFP: preserve the HW context when calling signal handlers

[Russell King - ARM Linux]

On Fri, Apr 02, 2010 at 05:36:03PM +0300, imre.deak at nokia.com wrote:
> From: Imre Deak <imre.deak@nokia.com>
> 
> Signal handlers can use floating point, so prevent them to corrupt
> the main thread's VFP context. So far there were two signal stack
> frame formats defined based on the VFP implementation, but the user
> struct used for ptrace covers all posibilities, so use it for the
> signal stack too.
> 
> Introduce also a new user struct for VFP exception registers. In
> this too fields not relevant to the current VFP architecture are
> ignored.
> 
> Support to save / restore the exception registers was added by
> Will Deacon.

Look fine to me, but I'd like an ack from Daniel since he partly looks
after the toolchain side of things and has dealt with this area before.

[RFC PATCH v5 0/2] ARM: VFP: Save / restore VFP state on the signal handler path

[Jamie Lokier]

imre.deak at nokia.com wrote:
> From: Imre Deak <imre.deak@nokia.com>
> 
> Changes since v4:
> - Do not add the packed attribute to user_vfp or user_vfp_exc.
>   I added packed to these structs to prevent the extra padding in
>   vfp_sigframe. But actually the padding is in accordance with the
>   ARM ABI.

EABI, OABI or both?

In general it's a really good idea, if you know there will be padding,
to insert dummy fields where the padding goes so that it doesn't make
any difference what compiler, ABI and settings are used.

-- Jamie

[RFC PATCH v5 0/2] ARM: VFP: Save / restore VFP state on the signal handler path

[Imre Deak]

On Tue, Apr 13, 2010 at 12:04:40AM +0200, ext Jamie Lokier wrote:
> imre.deak at nokia.com wrote:
> > From: Imre Deak <imre.deak@nokia.com>
> > 
> > Changes since v4:
> > - Do not add the packed attribute to user_vfp or user_vfp_exc.
> >   I added packed to these structs to prevent the extra padding in
> >   vfp_sigframe. But actually the padding is in accordance with the
> >   ARM ABI.
> 
> EABI, OABI or both?

I checked only EABI, which specifies alignment to the struct's alignment,
which is 8 bytes for struct user_vfp.

I haven't found an official OABI spec, but some googling turned up
that there the requirement is only to align to word size, so we'd
have a difference in the struct vfp_sigframe's size and the alignment
of the user_vfp_exc within.

> In general it's a really good idea, if you know there will be padding,
> to insert dummy fields where the padding goes so that it doesn't make
> any difference what compiler, ABI and settings are used.

Ok. If I understand correctly this is needed for kernels with support
for both OABI and EABI.

There is implicit alignment in struct user_vfp and struct user_vfp_exc.
I can fix up user_vfp_exc since it's new and not used by anyone, but
user_vfp is used by ptrace. I don't see a problem adding the padding to
it either since the kernel ever accesses it on a field-by-field basis,
but for kernels built with an OABI compiler this would mean an increase
in the struct's size. If this is not a problem I'd follow up with a 2/2
patch according to the following.

Please let me know if it's ok with you.

--Imre

--- a/arch/arm/include/asm/ucontext.h
+++ b/arch/arm/include/asm/ucontext.h
@@ -59,23 +59,19 @@ struct iwmmxt_sigframe {
 #endif /* CONFIG_IWMMXT */

 #ifdef CONFIG_VFP
-#if __LINUX_ARM_ARCH__ < 6
-/* For ARM pre-v6, we use fstmiax and fldmiax.  This adds one extra
- * word after the registers, and a word of padding@the end for
- * alignment.  */
 #define VFP_MAGIC              0x56465001
-#define VFP_STORAGE_SIZE       152
-#else
-#define VFP_MAGIC              0x56465002
-#define VFP_STORAGE_SIZE       144
-#endif

 struct vfp_sigframe
 {
        unsigned long           magic;
        unsigned long           size;
-       union vfp_state         storage;
-};
+       struct user_vfp         ufp;
+       struct user_vfp_exc     ufp_exc;
+} __attribute__((__aligned__(8)));
+
+/* 8 bytes for magic and size, 264 bytes for ufp, 16 bytes for ufp_exc. */
+#define VFP_STORAGE_SIZE       sizeof(struct vfp_sigframe)
+
 #endif /* CONFIG_VFP */

 /*
@@ -91,7 +87,7 @@ struct aux_sigframe {
 #ifdef CONFIG_IWMMXT
        struct iwmmxt_sigframe  iwmmxt;
 #endif
-#if 0 && defined CONFIG_VFP /* Not yet saved.  */
+#ifdef CONFIG_VFP
        struct vfp_sigframe     vfp;
 #endif
        /* Something that isn't a valid magic number for any coprocessor.  */
diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
index df95e05..07e66fa 100644
--- a/arch/arm/include/asm/user.h
+++ b/arch/arm/include/asm/user.h
@@ -83,11 +83,23 @@ struct user{

 /*
  * User specific VFP registers. If only VFPv2 is present, registers 16 to 31
- * are ignored by the ptrace system call.
+ * are ignored by the ptrace system call and the signal handler.
  */
 struct user_vfp {
        unsigned long long fpregs[32];
        unsigned long fpscr;
+       unsigned long pad;
+};
+
+/*
+ * VFP exception registers exposed to user space during signal delivery.
+ * Fields not relavant to the current VFP architecture are ignored.
+ */
+struct user_vfp_exc {
+       unsigned long   fpexc;
+       unsigned long   fpinst;
+       unsigned long   fpinst2;
+       unsigned long   pad;
 };

 #endif /* _ARM_USER_H */

[RFC PATCH v5 0/2] ARM: VFP: Save / restore VFP state on the signal handler path

[Nicolas Pitre]

On Tue, 13 Apr 2010, Imre Deak wrote:

> On Tue, Apr 13, 2010 at 12:04:40AM +0200, ext Jamie Lokier wrote:
> > imre.deak at nokia.com wrote:
> > > From: Imre Deak <imre.deak@nokia.com>
> > > 
> > > Changes since v4:
> > > - Do not add the packed attribute to user_vfp or user_vfp_exc.
> > >   I added packed to these structs to prevent the extra padding in
> > >   vfp_sigframe. But actually the padding is in accordance with the
> > >   ARM ABI.
> > 
> > EABI, OABI or both?
> 
> I checked only EABI, which specifies alignment to the struct's alignment,
> which is 8 bytes for struct user_vfp.
> 
> I haven't found an official OABI spec, but some googling turned up
> that there the requirement is only to align to word size, so we'd
> have a difference in the struct vfp_sigframe's size and the alignment
> of the user_vfp_exc within.

I think that VFP is defined for EABI only anyway.  I doubt people are 
using VFP with OABI in practice either.


Nicolas

[RFC PATCH v4 0/3] ARM: VFP: Save / restore VFP state on the signal handler path

[Dirk Behme]

On 31.03.2010 23:23, imre.deak at nokia.com wrote:
> Changes since v3:
> - Rebased against mainline v2.6.34-rc3, since some parts have already
>    been applied there.
>
> Note that this problem might affect more people than previously thought.
> The CodeSourcery 2009q1-203 glibc_ports has some NEON optimized functions;
> for example using memset or memcpy in a signal handler and floating point
> in the main thread can corrupt the main thread's context. You can test this
> by replacing the floating point ops in the signal handler below with some
> dummy memset/memcpy.
>
> I could only test these patches on a Cortex A8 UP system; it would be great
> if someone with an SMP board could give it a go. The test app to reproduce
> the problem (I've built it with -mfloat-abi=softfp -mfpu=neon):

Do you think this is relevant for ARM11 SMP (ARM11 MPCore), too? Or do 
you think this is necessary only for NEON systems, i.e. Cortex Ax?

Thanks and best regards

Dirk

[RFC PATCH v4 0/3] ARM: VFP: Save / restore VFP state on the signal handler path

[Imre Deak]

On Wed, Apr 07, 2010 at 06:24:39PM +0200, ext Dirk Behme wrote:
> On 31.03.2010 23:23, imre.deak at nokia.com wrote:
> > Changes since v3:
> > - Rebased against mainline v2.6.34-rc3, since some parts have already
> >    been applied there.
> >
> > Note that this problem might affect more people than previously thought.
> > The CodeSourcery 2009q1-203 glibc_ports has some NEON optimized functions;
> > for example using memset or memcpy in a signal handler and floating point
> > in the main thread can corrupt the main thread's context. You can test this
> > by replacing the floating point ops in the signal handler below with some
> > dummy memset/memcpy.
> >
> > I could only test these patches on a Cortex A8 UP system; it would be great
> > if someone with an SMP board could give it a go. The test app to reproduce
> > the problem (I've built it with -mfloat-abi=softfp -mfpu=neon):
> 
> Do you think this is relevant for ARM11 SMP (ARM11 MPCore), too? Or do 
> you think this is necessary only for NEON systems, i.e. Cortex Ax?

I think ARM11 has VFP, so it is relevant for it. This actually means that it's
enough to provide -mfpu=vfp above..

--Imre

[RFC PATCH v4 0/3] ARM: VFP: Save / restore VFP state on the signal handler path

[Jason McMullan]

On Wed, Apr 7, 2010 at 12:39 PM, Imre Deak <imre.deak@nokia.com> wrote:
> On Wed, Apr 07, 2010 at 06:24:39PM +0200, ext Dirk Behme wrote:
>> Do you think this is relevant for ARM11 SMP (ARM11 MPCore), too? Or do
>> you think this is necessary only for NEON systems, i.e. Cortex Ax?
>
> I think ARM11 has VFP, so it is relevant for it. This actually means that it's
> enough to provide -mfpu=vfp above..

I know of at least one MPCore (ARM11) SoC where the VFP design option was
not enabled. Keep that in mind.

[RFC PATCH v4 1/3] ARM: VFP: fix the SMP versions of vfp_{sync, flush}_hwstate

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

Recently the UP versions of these functions were refactored and as
a side effect it became possible to call them for the current thread.
This isn't true for the SMP versions however, so fix this up.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
---
 arch/arm/vfp/vfpmodule.c |   31 ++++++++++---------------------
 1 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 7f3f59f..477f240 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -428,26 +428,6 @@ static void vfp_pm_init(void)
 static inline void vfp_pm_init(void) { }
 #endif /* CONFIG_PM */
 
-/*
- * Synchronise the hardware VFP state of a thread other than current with the
- * saved one. This function is used by the ptrace mechanism.
- */
-#ifdef CONFIG_SMP
-void vfp_sync_hwstate(struct thread_info *thread)
-{
-}
-
-void vfp_flush_hwstate(struct thread_info *thread)
-{
-	/*
-	 * On SMP systems, the VFP state is automatically saved at every
-	 * context switch. We mark the thread VFP state as belonging to a
-	 * non-existent CPU so that the saved one will be reloaded when
-	 * needed.
-	 */
-	thread->vfpstate.hard.cpu = NR_CPUS;
-}
-#else
 void vfp_sync_hwstate(struct thread_info *thread)
 {
 	unsigned int cpu = get_cpu();
@@ -490,9 +470,18 @@ void vfp_flush_hwstate(struct thread_info *thread)
 		last_VFP_context[cpu] = NULL;
 	}
 
+#ifdef CONFIG_SMP
+	/*
+	 * For SMP we still have to take care of the case where the thread
+	 * migrates to another CPU and then back to the original CPU on which
+	 * the last VFP user is still the same thread. Mark the thread VFP
+	 * state as belonging to a non-existent CPU so that the saved one will
+	 * be reloaded in the above case.
+	 */
+	thread->vfpstate.hard.cpu = NR_CPUS;
+#endif
 	put_cpu();
 }
-#endif
 
 #include <linux/smp.h>
 
-- 
1.7.0.2

[RFC PATCH v4 2/3] ARM: VFP: make user_vfp struct packed

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

An upcoming patch will include user_vfp as a member in another struct and
this will cause the next member to be 8 byte aligned as well. Since the
struct is part of an ABI it's better not leave such alignment decisions
to the compiler.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
---
 arch/arm/include/asm/user.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
index df95e05..5660777 100644
--- a/arch/arm/include/asm/user.h
+++ b/arch/arm/include/asm/user.h
@@ -88,6 +88,6 @@ struct user{
 struct user_vfp {
 	unsigned long long fpregs[32];
 	unsigned long fpscr;
-};
+} __attribute__((packed));
 
 #endif /* _ARM_USER_H */
-- 
1.7.0.2

[RFC PATCH v4 3/3] ARM: VFP: preserve the HW context when calling signal handlers

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

Signal handlers can use floating point, so prevent them to corrupt
the main thread's VFP context. So far there were two signal stack
frame formats defined based on the VFP implementation, but the user
struct used for ptrace covers all posibilities, so use it for the
signal stack too.

Introduce also a new user struct for VFP exception registers. In
this too fields not relevant to the current VFP architecture are
ignored.

Support to save / restore the exception registers was added by
Will Deacon.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/ucontext.h |   20 +++-----
 arch/arm/include/asm/user.h     |   12 +++++-
 arch/arm/kernel/signal.c        |   93 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 108 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/ucontext.h b/arch/arm/include/asm/ucontext.h
index bf65e9f..70b8b03 100644
--- a/arch/arm/include/asm/ucontext.h
+++ b/arch/arm/include/asm/ucontext.h
@@ -59,23 +59,19 @@ struct iwmmxt_sigframe {
 #endif /* CONFIG_IWMMXT */
 
 #ifdef CONFIG_VFP
-#if __LINUX_ARM_ARCH__ < 6
-/* For ARM pre-v6, we use fstmiax and fldmiax.  This adds one extra
- * word after the registers, and a word of padding@the end for
- * alignment.  */
 #define VFP_MAGIC		0x56465001
-#define VFP_STORAGE_SIZE	152
-#else
-#define VFP_MAGIC		0x56465002
-#define VFP_STORAGE_SIZE	144
-#endif
 
 struct vfp_sigframe
 {
 	unsigned long		magic;
 	unsigned long		size;
-	union vfp_state		storage;
-};
+	struct user_vfp		ufp;
+	struct user_vfp_exc	ufp_exc;
+} __attribute__((__aligned__(8)));
+
+/* 8 byte for magic and size, 272 byte for ufp */
+#define VFP_STORAGE_SIZE	sizeof(struct vfp_sigframe)
+
 #endif /* CONFIG_VFP */
 
 /*
@@ -91,7 +87,7 @@ struct aux_sigframe {
 #ifdef CONFIG_IWMMXT
 	struct iwmmxt_sigframe	iwmmxt;
 #endif
-#if 0 && defined CONFIG_VFP /* Not yet saved.  */
+#ifdef CONFIG_VFP
 	struct vfp_sigframe	vfp;
 #endif
 	/* Something that isn't a valid magic number for any coprocessor.  */
diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
index 5660777..f4eb067 100644
--- a/arch/arm/include/asm/user.h
+++ b/arch/arm/include/asm/user.h
@@ -83,11 +83,21 @@ struct user{
 
 /*
  * User specific VFP registers. If only VFPv2 is present, registers 16 to 31
- * are ignored by the ptrace system call.
+ * are ignored by the ptrace system call and the signal handler.
  */
 struct user_vfp {
 	unsigned long long fpregs[32];
 	unsigned long fpscr;
 } __attribute__((packed));
 
+/*
+ * VFP exception registers exposed to user space during signal delivery.
+ * Fields not relavant to the current VFP architecture are ignored.
+ */
+struct user_vfp_exc {
+	unsigned long	fpexc;
+	unsigned long	fpinst;
+	unsigned long	fpinst2;
+} __attribute__((packed));
+
 #endif /* _ARM_USER_H */
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index e7714f3..907d5a6 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -18,6 +18,7 @@
 #include <asm/cacheflush.h>
 #include <asm/ucontext.h>
 #include <asm/unistd.h>
+#include <asm/vfp.h>
 
 #include "ptrace.h"
 #include "signal.h"
@@ -175,6 +176,90 @@ static int restore_iwmmxt_context(struct iwmmxt_sigframe *frame)
 
 #endif
 
+#ifdef CONFIG_VFP
+
+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);
+
+	return err ? -EFAULT : 0;
+}
+
+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);
+	__get_user_error(size, &frame->size, err);
+
+	if (err)
+		return -EFAULT;
+	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
+		return -EINVAL;
+
+	/*
+	 * 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);
+
+	if (!err)
+		vfp_flush_hwstate(thread);
+
+	return err ? -EFAULT : 0;
+}
+
+#endif
+
 /*
  * Do a signal return; undo the signal stack.  These are aligned to 64-bit.
  */
@@ -233,8 +318,8 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
 		err |= restore_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_restore_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= restore_vfp_context(&aux->vfp);
 #endif
 
 	return err;
@@ -348,8 +433,8 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
 		err |= preserve_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_save_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= preserve_vfp_context(&aux->vfp);
 #endif
 	__put_user_error(0, &aux->end_magic, err);
 
-- 
1.7.0.2

[RFC PATCH v3 1/3] ARM: VFP: add support to sync the VFP state of the current thread

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

So far vfp_sync_state worked only for threads other than the current
one. This worked for tracing other threads, but not for PTRACE_TRACEME.
Syncing for the current thread will also be needed by an upcoming patch
adding support for VFP context save / restore around signal handlers.

Also break the current vfp_sync_state into vfp_{save/restore}_hwstate
to make things more efficient.

Based on a patch from Russell King.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
---
 arch/arm/include/asm/thread_info.h |    3 +-
 arch/arm/kernel/ptrace.c           |    6 ++-
 arch/arm/vfp/vfpmodule.c           |   71 +++++++++++++++++++-----------------
 3 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 2dfb7d7..1d7f4d5 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -115,7 +115,8 @@ extern void iwmmxt_task_restore(struct thread_info *, void *);
 extern void iwmmxt_task_release(struct thread_info *);
 extern void iwmmxt_task_switch(struct thread_info *);
 
-extern void vfp_sync_state(struct thread_info *thread);
+extern void vfp_save_hwstate(struct thread_info *);
+extern void vfp_restore_hwstate(struct thread_info *);
 
 #endif
 
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index a2ea385..1b1096e 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -669,7 +669,7 @@ static int ptrace_getvfpregs(struct task_struct *tsk, void __user *data)
 	union vfp_state *vfp = &thread->vfpstate;
 	struct user_vfp __user *ufp = data;
 
-	vfp_sync_state(thread);
+	vfp_save_hwstate(thread);
 
 	/* copy the floating point registers */
 	if (copy_to_user(&ufp->fpregs, &vfp->hard.fpregs,
@@ -692,7 +692,7 @@ static int ptrace_setvfpregs(struct task_struct *tsk, void __user *data)
 	union vfp_state *vfp = &thread->vfpstate;
 	struct user_vfp __user *ufp = data;
 
-	vfp_sync_state(thread);
+	vfp_save_hwstate(thread);
 
 	/* copy the floating point registers */
 	if (copy_from_user(&vfp->hard.fpregs, &ufp->fpregs,
@@ -703,6 +703,8 @@ static int ptrace_setvfpregs(struct task_struct *tsk, void __user *data)
 	if (get_user(vfp->hard.fpscr, &ufp->fpscr))
 		return -EFAULT;
 
+	vfp_restore_hwstate(thread);
+
 	return 0;
 }
 #endif
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 96abd0f..760cbb2 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -393,54 +393,59 @@ static void vfp_pm_init(void)
 static inline void vfp_pm_init(void) { }
 #endif /* CONFIG_PM */
 
-/*
- * Synchronise the hardware VFP state of a thread other than current with the
- * saved one. This function is used by the ptrace mechanism.
- */
-#ifdef CONFIG_SMP
-void vfp_sync_state(struct thread_info *thread)
+void vfp_save_hwstate(struct thread_info *thread)
 {
+	unsigned int cpu = get_cpu();
+
 	/*
-	 * On SMP systems, the VFP state is automatically saved at every
-	 * context switch. We mark the thread VFP state as belonging to a
-	 * non-existent CPU so that the saved one will be reloaded when
-	 * needed.
+	 * If the thread we're interested in is the current owner of the
+	 * hardware VFP state, then we need to save its state.
 	 */
-	thread->vfpstate.hard.cpu = NR_CPUS;
+	if (last_VFP_context[cpu] == &thread->vfpstate) {
+		u32 fpexc = fmrx(FPEXC);
+
+		/*
+		 * Save the last VFP state on this CPU.
+		 */
+		fmxr(FPEXC, fpexc | FPEXC_EN);
+		vfp_save_state(&thread->vfpstate, fpexc | FPEXC_EN);
+		fmxr(FPEXC, fpexc);
+	}
+
+	put_cpu();
 }
-#else
-void vfp_sync_state(struct thread_info *thread)
+
+void vfp_restore_hwstate(struct thread_info *thread)
 {
 	unsigned int cpu = get_cpu();
-	u32 fpexc = fmrx(FPEXC);
 
 	/*
-	 * If VFP is enabled, the previous state was already saved and
-	 * last_VFP_context updated.
+	 * If the thread we're interested in is the current owner of the
+	 * hardware VFP state, then we need to restore its state.
 	 */
-	if (fpexc & FPEXC_EN)
-		goto out;
-
-	if (!last_VFP_context[cpu])
-		goto out;
+	if (last_VFP_context[cpu] == &thread->vfpstate) {
+		u32 fpexc = fmrx(FPEXC);
 
-	/*
-	 * Save the last VFP state on this CPU.
-	 */
-	fmxr(FPEXC, fpexc | FPEXC_EN);
-	vfp_save_state(last_VFP_context[cpu], fpexc);
-	fmxr(FPEXC, fpexc);
+		fmxr(FPEXC, fpexc & ~FPEXC_EN);
 
+		/*
+		 * Set the context to NULL to force a reload the next time the
+		 * thread uses the VFP.
+		 */
+		last_VFP_context[cpu] = NULL;
+	}
+#ifdef CONFIG_SMP
 	/*
-	 * Set the context to NULL to force a reload the next time the thread
-	 * uses the VFP.
+	 * For SMP we still have to take care of the case where the thread
+	 * migrates to another CPU and then back to the original CPU on which
+	 * the last VFP user is still the same thread. Mark the thread VFP
+	 * state as belonging to a non-existent CPU so that the saved one will
+	 * be reloaded in the above case.
 	 */
-	last_VFP_context[cpu] = NULL;
-
-out:
+	thread->vfpstate.hard.cpu = NR_CPUS;
+#endif
 	put_cpu();
 }
-#endif
 
 #include <linux/smp.h>
 
-- 
1.7.0.2

[RFC PATCH v3 2/3] ARM: VFP: make user_vfp struct packed

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

An upcoming patch will include user_vfp as a member in another struct and
this will cause the next member to be 8 byte aligned as well. Since the
struct is part of an ABI it's better not leave such alignment decisions
to the compiler.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
---
 arch/arm/include/asm/user.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
index df95e05..5660777 100644
--- a/arch/arm/include/asm/user.h
+++ b/arch/arm/include/asm/user.h
@@ -88,6 +88,6 @@ struct user{
 struct user_vfp {
 	unsigned long long fpregs[32];
 	unsigned long fpscr;
-};
+} __attribute__((packed));
 
 #endif /* _ARM_USER_H */
-- 
1.7.0.2

[RFC PATCH v3 3/3] ARM: VFP: preserve the HW context when calling signal handlers

[imre.deak at nokia.com]

From: Imre Deak <imre.deak@nokia.com>

Signal handlers can use floating point, so prevent them to corrupt
the main thread's VFP context. So far there were two signal stack
frame formats defined based on the VFP implementation, but the user
struct used for ptrace covers all posibilities, so use it for the
signal stack too.

Introduce also a new user struct for VFP exception registers. In
this too fields not relevant to the current VFP architecture are
ignored.

Support to save / restore the exception registers was added by
Will Deacon.

Signed-off-by: Imre Deak <imre.deak@nokia.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/ucontext.h |   20 +++-----
 arch/arm/include/asm/user.h     |   12 +++++-
 arch/arm/kernel/signal.c        |   93 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 108 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/ucontext.h b/arch/arm/include/asm/ucontext.h
index bf65e9f..70b8b03 100644
--- a/arch/arm/include/asm/ucontext.h
+++ b/arch/arm/include/asm/ucontext.h
@@ -59,23 +59,19 @@ struct iwmmxt_sigframe {
 #endif /* CONFIG_IWMMXT */
 
 #ifdef CONFIG_VFP
-#if __LINUX_ARM_ARCH__ < 6
-/* For ARM pre-v6, we use fstmiax and fldmiax.  This adds one extra
- * word after the registers, and a word of padding@the end for
- * alignment.  */
 #define VFP_MAGIC		0x56465001
-#define VFP_STORAGE_SIZE	152
-#else
-#define VFP_MAGIC		0x56465002
-#define VFP_STORAGE_SIZE	144
-#endif
 
 struct vfp_sigframe
 {
 	unsigned long		magic;
 	unsigned long		size;
-	union vfp_state		storage;
-};
+	struct user_vfp		ufp;
+	struct user_vfp_exc	ufp_exc;
+} __attribute__((__aligned__(8)));
+
+/* 8 byte for magic and size, 272 byte for ufp */
+#define VFP_STORAGE_SIZE	sizeof(struct vfp_sigframe)
+
 #endif /* CONFIG_VFP */
 
 /*
@@ -91,7 +87,7 @@ struct aux_sigframe {
 #ifdef CONFIG_IWMMXT
 	struct iwmmxt_sigframe	iwmmxt;
 #endif
-#if 0 && defined CONFIG_VFP /* Not yet saved.  */
+#ifdef CONFIG_VFP
 	struct vfp_sigframe	vfp;
 #endif
 	/* Something that isn't a valid magic number for any coprocessor.  */
diff --git a/arch/arm/include/asm/user.h b/arch/arm/include/asm/user.h
index 5660777..f4eb067 100644
--- a/arch/arm/include/asm/user.h
+++ b/arch/arm/include/asm/user.h
@@ -83,11 +83,21 @@ struct user{
 
 /*
  * User specific VFP registers. If only VFPv2 is present, registers 16 to 31
- * are ignored by the ptrace system call.
+ * are ignored by the ptrace system call and the signal handler.
  */
 struct user_vfp {
 	unsigned long long fpregs[32];
 	unsigned long fpscr;
 } __attribute__((packed));
 
+/*
+ * VFP exception registers exposed to user space during signal delivery.
+ * Fields not relavant to the current VFP architecture are ignored.
+ */
+struct user_vfp_exc {
+	unsigned long	fpexc;
+	unsigned long	fpinst;
+	unsigned long	fpinst2;
+} __attribute__((packed));
+
 #endif /* _ARM_USER_H */
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index e7714f3..7e68053 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -18,6 +18,7 @@
 #include <asm/cacheflush.h>
 #include <asm/ucontext.h>
 #include <asm/unistd.h>
+#include <asm/vfp.h>
 
 #include "ptrace.h"
 #include "signal.h"
@@ -175,6 +176,90 @@ static int restore_iwmmxt_context(struct iwmmxt_sigframe *frame)
 
 #endif
 
+#ifdef CONFIG_VFP
+
+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_save_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);
+
+	return err ? -EFAULT : 0;
+}
+
+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);
+	__get_user_error(size, &frame->size, err);
+
+	if (err)
+		return -EFAULT;
+	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
+		return -EINVAL;
+
+	/*
+	 * 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);
+
+	if (!err)
+		vfp_restore_hwstate(thread);
+
+	return err ? -EFAULT : 0;
+}
+
+#endif
+
 /*
  * Do a signal return; undo the signal stack.  These are aligned to 64-bit.
  */
@@ -233,8 +318,8 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
 		err |= restore_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_restore_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= restore_vfp_context(&aux->vfp);
 #endif
 
 	return err;
@@ -348,8 +433,8 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
 		err |= preserve_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP
-//	if (err == 0)
-//		err |= vfp_save_state(&sf->aux.vfp);
+	if (err == 0)
+		err |= preserve_vfp_context(&aux->vfp);
 #endif
 	__put_user_error(0, &aux->end_magic, err);
 
-- 
1.7.0.2