| Message ID | 1311689651-7372-1-git-send-email-dave.martin@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 26 Jul 2011, Dave Martin wrote: > Currently, it's possible to set the kernel to ignore alignment > faults when changing the alignment fault handling mode at runtime > via /proc/sys/alignment, even though this is undesirable on ARMv6 > and above, where it can result in infinite spins where an un-fixed- > up instruction repeatedly faults. This may be the case only if the U bit in CP15 reg 1 is set, right? > In addition, the kernel clobbers any alignment mode specified on > the command-line if running on ARMv6 or above. > > This patch factors out the necessary safety check into a couple of > new helper functions, and checks and modifies the fault handling > mode as appropriate on boot and on writes to /proc/cpu/alignment. > > Prior to ARMv6, the behaviour is unchanged. > > For ARMv6 and above, the behaviour changes as follows: > > * Attempting to ignore faults on ARMv6 results in the mode being > forced to UM_FIXUP instead. A warning is printed if this > happened as a result of a write to /proc/cpu/alignment. The > user's UM_WARN bit (if present) is still honoured. Why not clearing the U bit as well as the A bit to preserve consistency with the pre ARMv6 behavior? Nicolas
On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote: > Why not clearing the U bit as well as the A bit to preserve consistency > with the pre ARMv6 behavior? Because some CPUs have the U bit forced to 1. IOW, it's not possible to disable it.
On Tue, 26 Jul 2011, Russell King - ARM Linux wrote: > On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote: > > Why not clearing the U bit as well as the A bit to preserve consistency > > with the pre ARMv6 behavior? > > Because some CPUs have the U bit forced to 1. IOW, it's not possible to > disable it. Oh well... Nicolas
On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote: > On Tue, 26 Jul 2011, Dave Martin wrote: > > > Currently, it's possible to set the kernel to ignore alignment > > faults when changing the alignment fault handling mode at runtime > > via /proc/sys/alignment, even though this is undesirable on ARMv6 > > and above, where it can result in infinite spins where an un-fixed- > > up instruction repeatedly faults. > > This may be the case only if the U bit in CP15 reg 1 is set, right? > > > In addition, the kernel clobbers any alignment mode specified on > > the command-line if running on ARMv6 or above. > > > > This patch factors out the necessary safety check into a couple of > > new helper functions, and checks and modifies the fault handling > > mode as appropriate on boot and on writes to /proc/cpu/alignment. > > > > Prior to ARMv6, the behaviour is unchanged. > > > > For ARMv6 and above, the behaviour changes as follows: > > > > * Attempting to ignore faults on ARMv6 results in the mode being > > forced to UM_FIXUP instead. A warning is printed if this > > happened as a result of a write to /proc/cpu/alignment. The > > user's UM_WARN bit (if present) is still honoured. > > Why not clearing the U bit as well as the A bit to preserve consistency > with the pre ARMv6 behavior? The old unaligned access behaviour is deprecated in ARMv6 (I believe) and is not supported at all in ARMv7 -- i.e., you can't turn the U bit off. So this would be an additional special behaviour for ARMv6 only. I'm not sure that introducing yet another behaviour is useful here; anyone with legacy userspace software relying on the old unaligned access model presumably fixed it years ago. Really, the patch was just intended resolve the inconsistency where the policy the kernel enforces for the alignment fixup mode on bootup is not enforced at run-time. Cheers ---Dave
On Wed, 27 Jul 2011, Dave Martin wrote: > On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote: > > Why not clearing the U bit as well as the A bit to preserve consistency > > with the pre ARMv6 behavior? > > The old unaligned access behaviour is deprecated in ARMv6 (I believe) > and is not supported at all in ARMv7 -- i.e., you can't turn the U bit > off. So this would be an additional special behaviour for ARMv6 only. > > I'm not sure that introducing yet another behaviour is useful here; > anyone with legacy userspace software relying on the old unaligned > access model presumably fixed it years ago. > > Really, the patch was just intended resolve the inconsistency where > the policy the kernel enforces for the alignment fixup mode on bootup > is not enforced at run-time. OK, that makes sense then. Nicolas
On Wed, Jul 27, 2011 at 10:10:46AM -0400, Nicolas Pitre wrote: > On Wed, 27 Jul 2011, Dave Martin wrote: > > > On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote: > > > Why not clearing the U bit as well as the A bit to preserve consistency > > > with the pre ARMv6 behavior? > > > > The old unaligned access behaviour is deprecated in ARMv6 (I believe) > > and is not supported at all in ARMv7 -- i.e., you can't turn the U bit > > off. So this would be an additional special behaviour for ARMv6 only. > > > > I'm not sure that introducing yet another behaviour is useful here; > > anyone with legacy userspace software relying on the old unaligned > > access model presumably fixed it years ago. > > > > Really, the patch was just intended resolve the inconsistency where > > the policy the kernel enforces for the alignment fixup mode on bootup > > is not enforced at run-time. > > OK, that makes sense then. Can I take that as an Ack? Cheers ---Dave
On Wed, 27 Jul 2011, Dave Martin wrote: > On Wed, Jul 27, 2011 at 10:10:46AM -0400, Nicolas Pitre wrote: > > On Wed, 27 Jul 2011, Dave Martin wrote: > > > > > On Tue, Jul 26, 2011 at 05:28:57PM -0400, Nicolas Pitre wrote: > > > > Why not clearing the U bit as well as the A bit to preserve consistency > > > > with the pre ARMv6 behavior? > > > > > > The old unaligned access behaviour is deprecated in ARMv6 (I believe) > > > and is not supported at all in ARMv7 -- i.e., you can't turn the U bit > > > off. So this would be an additional special behaviour for ARMv6 only. > > > > > > I'm not sure that introducing yet another behaviour is useful here; > > > anyone with legacy userspace software relying on the old unaligned > > > access model presumably fixed it years ago. > > > > > > Really, the patch was just intended resolve the inconsistency where > > > the policy the kernel enforces for the alignment fixup mode on bootup > > > is not enforced at run-time. > > > > OK, that makes sense then. > > Can I take that as an Ack? /me looks at the patch again. OK, looks fine, ACK. Nicolas > > Cheers > ---Dave >
diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c index 724ba3b..8611f5d 100644 --- a/arch/arm/mm/alignment.c +++ b/arch/arm/mm/alignment.c @@ -95,6 +95,33 @@ static const char *usermode_action[] = { "signal+warn" }; +/* Return true if and only if the ARMv6 unaligned access model is in use. */ +static bool cpu_is_v6_unaligned(void) +{ + return cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U); +} + +static int safe_usermode(int new_usermode, bool warn) +{ + /* + * ARMv6 and later CPUs can perform unaligned accesses for + * most single load and store instructions up to word size. + * LDM, STM, LDRD and STRD still need to be handled. + * + * Ignoring the alignment fault is not an option on these + * CPUs since we spin re-faulting the instruction without + * making any progress. + */ + if (cpu_is_v6_unaligned() && !(new_usermode & (UM_FIXUP | UM_SIGNAL))) { + new_usermode |= UM_FIXUP; + + if (warn) + printk(KERN_WARNING "alignment: ignoring faults is unsafe on this CPU. Defaulting to fixup mode.\n"); + } + + return new_usermode; +} + static int alignment_proc_show(struct seq_file *m, void *v) { seq_printf(m, "User:\t\t%lu\n", ai_user); @@ -125,7 +152,7 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer if (get_user(mode, buffer)) return -EFAULT; if (mode >= '0' && mode <= '5') - ai_usermode = mode - '0'; + ai_usermode = safe_usermode(mode - '0', true); } return count; } @@ -923,20 +950,11 @@ static int __init alignment_init(void) return -ENOMEM; #endif - /* - * ARMv6 and later CPUs can perform unaligned accesses for - * most single load and store instructions up to word size. - * LDM, STM, LDRD and STRD still need to be handled. - * - * Ignoring the alignment fault is not an option on these - * CPUs since we spin re-faulting the instruction without - * making any progress. - */ - if (cpu_architecture() >= CPU_ARCH_ARMv6 && (cr_alignment & CR_U)) { + if (cpu_is_v6_unaligned()) { cr_alignment &= ~CR_A; cr_no_alignment &= ~CR_A; set_cr(cr_alignment); - ai_usermode = UM_FIXUP; + ai_usermode = safe_usermode(ai_usermode, false); } hook_fault_code(1, do_alignment, SIGBUS, BUS_ADRALN,
Currently, it's possible to set the kernel to ignore alignment faults when changing the alignment fault handling mode at runtime via /proc/sys/alignment, even though this is undesirable on ARMv6 and above, where it can result in infinite spins where an un-fixed- up instruction repeatedly faults. In addition, the kernel clobbers any alignment mode specified on the command-line if running on ARMv6 or above. This patch factors out the necessary safety check into a couple of new helper functions, and checks and modifies the fault handling mode as appropriate on boot and on writes to /proc/cpu/alignment. Prior to ARMv6, the behaviour is unchanged. For ARMv6 and above, the behaviour changes as follows: * Attempting to ignore faults on ARMv6 results in the mode being forced to UM_FIXUP instead. A warning is printed if this happened as a result of a write to /proc/cpu/alignment. The user's UM_WARN bit (if present) is still honoured. * An alignment= argument from the kernel command-line is now honoured, except that the kernel will modify the specified mode as described above. This is allows modes such as UM_SIGNAL and UM_WARN to be active immediately from boot, which is useful for debugging purposes. Signed-off-by: Dave Martin <dave.martin@linaro.org> --- v2: Missing semicolon added in safe_usermode(); arch/arm/mm/alignment.c | 42 ++++++++++++++++++++++++++++++------------ 1 files changed, 30 insertions(+), 12 deletions(-)