* [PATCH v3 0/2] Remaining VFP fixes @ 2012-02-01 14:03 Will Deacon 2012-02-01 14:03 ` [PATCH v3 1/2] ARM: vfp: move user vfp state save/restore code out of signal.c Will Deacon 2012-02-01 14:03 ` [PATCH v3 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler Will Deacon 0 siblings, 2 replies; 5+ messages in thread From: Will Deacon @ 2012-02-01 14:03 UTC (permalink / raw) To: linux-arm-kernel Hello, This is v3 of the patches originally posted here: v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/082168.html v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/082383.html Patches 1-3 from v2 of the series are now in the patch system, so this posting is a rework of what was previously patch 4/4. The main change is that the VFP save/restore code is now moved out of signal.c and into vfpmodule.c, since that is where the low-level knowledge belongs. I tested the changes with the `risu' tool for 24 hours on my (A9) vexpress without any issues. Cheers, Will Will Deacon (2): ARM: vfp: move user vfp state save/restore code out of signal.c ARM: vfp: clear fpscr length and stride bits on entry to sig handler arch/arm/include/asm/thread_info.h | 7 +++ arch/arm/kernel/signal.c | 57 ++------------------- arch/arm/vfp/vfpmodule.c | 99 ++++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 52 deletions(-) -- 1.7.4.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/2] ARM: vfp: move user vfp state save/restore code out of signal.c 2012-02-01 14:03 [PATCH v3 0/2] Remaining VFP fixes Will Deacon @ 2012-02-01 14:03 ` Will Deacon 2012-02-01 14:03 ` [PATCH v3 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler Will Deacon 1 sibling, 0 replies; 5+ messages in thread From: Will Deacon @ 2012-02-01 14:03 UTC (permalink / raw) To: linux-arm-kernel 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> --- 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..142bffd 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_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..b160fbc 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_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..3e35e35 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_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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler 2012-02-01 14:03 [PATCH v3 0/2] Remaining VFP fixes Will Deacon 2012-02-01 14:03 ` [PATCH v3 1/2] ARM: vfp: move user vfp state save/restore code out of signal.c Will Deacon @ 2012-02-01 14:03 ` Will Deacon 2012-02-03 11:12 ` Dave Martin 1 sibling, 1 reply; 5+ messages in thread From: Will Deacon @ 2012-02-01 14:03 UTC (permalink / raw) To: linux-arm-kernel 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 3e35e35..49cf328 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -562,6 +562,21 @@ int vfp_preserve_user_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 before entry + * to the signal handler. + */ + hwstate->fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK); + + /* + * Disable VFP in the hwstate so that we can detect if it was + * used by the signal handler. + */ + 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler 2012-02-01 14:03 ` [PATCH v3 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler Will Deacon @ 2012-02-03 11:12 ` Dave Martin 2012-02-03 11:58 ` Will Deacon 0 siblings, 1 reply; 5+ messages in thread From: Dave Martin @ 2012-02-03 11:12 UTC (permalink / raw) To: linux-arm-kernel On Wed, Feb 01, 2012 at 02:03:12PM +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 3e35e35..49cf328 100644 > --- a/arch/arm/vfp/vfpmodule.c > +++ b/arch/arm/vfp/vfpmodule.c > @@ -562,6 +562,21 @@ int vfp_preserve_user_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 before entry > + * to the signal handler. > + */ > + hwstate->fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK); > + > + /* > + * Disable VFP in the hwstate so that we can detect if it was > + * used by the signal handler. > + */ > + hwstate->fpexc &= ~FPEXC_EN; > return 0; > } ^ These additions are nothing to do with preserving the VFP state. Instead, they set up a clean signal frame, under the assumption that the interrupted thread's state has already been preserved. Would this be more cleanly factored out as a separate function, something like vfp_setup_sigframe(), so: setup_sigframe() { /* ... */ vfp_preserve_user_hwstate(); vfp_setup_sigframe(); } ...? Alternatively, if this is an abstraction too far, and the "preserve" and "setup sigframe" actions will never make sense independently of each other, you could just rename the function to something clearer, like vfp_user_preserve_setup_sigframe(). Cheers ---Dave > > @@ -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 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler 2012-02-03 11:12 ` Dave Martin @ 2012-02-03 11:58 ` Will Deacon 0 siblings, 0 replies; 5+ messages in thread From: Will Deacon @ 2012-02-03 11:58 UTC (permalink / raw) To: linux-arm-kernel Hi Dave, Thanks for looking at this. On Fri, Feb 03, 2012 at 11:12:09AM +0000, Dave Martin wrote: > On Wed, Feb 01, 2012 at 02:03:12PM +0000, Will Deacon wrote: > > diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c > > index 3e35e35..49cf328 100644 > > --- a/arch/arm/vfp/vfpmodule.c > > +++ b/arch/arm/vfp/vfpmodule.c > > @@ -562,6 +562,21 @@ int vfp_preserve_user_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 before entry > > + * to the signal handler. > > + */ > > + hwstate->fpscr &= ~(FPSCR_LENGTH_MASK | FPSCR_STRIDE_MASK); > > + > > + /* > > + * Disable VFP in the hwstate so that we can detect if it was > > + * used by the signal handler. > > + */ > > + hwstate->fpexc &= ~FPEXC_EN; > > return 0; > > } > > ^ > These additions are nothing to do with preserving the VFP state. > Instead, they set up a clean signal frame, under the assumption that the > interrupted thread's state has already been preserved. > > Would this be more cleanly factored out as a separate function, > something like vfp_setup_sigframe(), so: > > setup_sigframe() { > /* ... */ > > vfp_preserve_user_hwstate(); > vfp_setup_sigframe(); > } > > ...? The whole reason for moving this code out of signal.c was to avoid polluting that file with VFP-related code. I don't want to turn the problem on its head and end up with signal-related code in vfpmodule.c (so yes, I'll fix those comments :). setup_sigframe certainly sounds out of place here. > Alternatively, if this is an abstraction too far, and the "preserve" and > "setup sigframe" actions will never make sense independently of each > other, you could just rename the function to something clearer, like > vfp_user_preserve_setup_sigframe(). Indeed, this comes down to my inability to think of a decent function name. I'd like to avoid the _sigframe bit, so I'll have a think about some better terminology. I could try running a competition. Expect a v4 when inspiration strikes. Will ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-02-03 11:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-01 14:03 [PATCH v3 0/2] Remaining VFP fixes Will Deacon 2012-02-01 14:03 ` [PATCH v3 1/2] ARM: vfp: move user vfp state save/restore code out of signal.c Will Deacon 2012-02-01 14:03 ` [PATCH v3 2/2] ARM: vfp: clear fpscr length and stride bits on entry to sig handler Will Deacon 2012-02-03 11:12 ` Dave Martin 2012-02-03 11:58 ` Will Deacon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).