Remaining VFP fixes
Subsystems (2)
v3 cover Remaining VFP fixes
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(-)
Signature · 1 line
1.7.4.1
PATCH v3 1/2 ARM: vfp: move user vfp state save/restore code out of signal.c
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.
--- 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(-)
arch/arm/include/asm/thread_info.h +7 -0
@@ -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 /*
arch/arm/kernel/signal.c +4 -51
81 lines not on this page · raw message
arch/arm/vfp/vfpmodule.c +79 -0
91 lines not on this page · raw message
Signature · 1 line
1.7.4.1
PATCH v3 2/2 ARM: vfp: clear fpscr length and stride bits on entry to sig handler
In reply to Will DeaconThe 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.
--- arch/arm/vfp/vfpmodule.c | 22 +++++++++++++++++++++- 1 files changed, 21 insertions(+), 1 deletions(-)
arch/arm/vfp/vfpmodule.c +21 -1
@@ -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
Signature · 1 line
1.7.4.1
PATCH v3 2/2 ARM: vfp: clear fpscr length and stride bits on entry to sig handler
Quoted 34 lines · 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 <[email protected]>Signed-off-by Will Deacon <[email protected]>--- arch/arm/vfp/vfpmodule.c | 22 +++++++++++++++++++++- 1 files changed, 21 insertions(+), 1 deletions(-)arch/arm/vfp/vfpmodule.c +7
-0@@ -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.
arch/arm/vfp/vfpmodule.c int vfp_preserve_user_hwstate(struct user_vfp __user *ufp, · line 581
Quoted 10 lines
arch/arm/vfp/vfpmodule.c +8
-0+*/ + 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
Quoted 23 lines
arch/arm/vfp/vfpmodule.c +6
-1@@ -574,7 +589,12 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,unsigned long fpexc;int err =0 ; -vfp_flush_hwstate (thread); +/* +* If VFP has been used, then disable it to avoid corrupting +* the new thread state. +*/ +if (hwstate->fpexc &FPEXC_EN ) +vfp_flush_hwstate (thread);/* * Copy the floating point registers. There can be unused Signature · 7 lines
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
PATCH v3 2/2 ARM: vfp: clear fpscr length and stride bits on entry to sig handler
Hi Dave,
Thanks for looking at this.
Quoted 35 lines · Dave Martin
Quoted 15 lines · Will Deacon
arch/arm/vfp/vfpmodule.c +7
-0@@ -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. arch/arm/vfp/vfpmodule.c int vfp_preserve_user_hwstate(struct user_vfp __user *ufp, · line 581
Quoted 10 lines
arch/arm/vfp/vfpmodule.c +8
-0+*/ + 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:
Quoted 8 lines
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.
Quoted 4 lines
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