On Thu, 16 May 2013, Ard Biesheuvel wrote:

In order to safely support the use of NEON or VFP instructions in kernel mode, some precautions need to be taken:

• the userland context that may be present in the registers (even if the NEON/VFP is currently disabled) must be stored under the correct task (which may not be 'current' in the UP case),
• to avoid having to keep track of additional vfpstates for the kernel side, disallow the use of NEON/VFP in interrupt context and run with preemption disabled,
• after use, re-enable preemption and re-enable the lazy restore machinery by disabling the NEON/VFP unit.

This patch adds the functions kernel_vfp_begin() and kernel_vfp_end() which take care of the above.

Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org

[...]

diff --git a/arch/arm/include/asm/vfp.h b/arch/arm/include/asm/vfp.h index f4ab34f..421506b 100644 --- a/arch/arm/include/asm/vfp.h +++ b/arch/arm/include/asm/vfp.h @@ -5,6 +5,11 @@

• First, the standard VFP set.

*/ +#ifndef __ASSEMBLY__ +void kernel_vfp_begin(void); +void kernel_vfp_end(void); +#endif

#define FPSID cr0 #define FPSCR cr1 #define MVFR1 cr6

Isn't this hunk badly located, given the comment just above it? I'd move it at the end of the file instead.

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index 5dfbb0b..e30a6335 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -20,6 +20,7 @@ #include <linux/init.h> #include <linux/uaccess.h> #include <linux/user.h> +#include <linux/export.h> #include <asm/cp15.h> #include <asm/cputype.h> @@ -649,6 +650,45 @@ static int vfp_hotplug(struct notifier_block *b, unsigned long action, } /*

• • Kernel-side NEON/VFP support functions
• */

+void kernel_vfp_begin(void) +{

• struct thread_info *thread = current_thread_info();
• unsigned int cpu = get_cpu();
• u32 fpexc;
• /* Avoid using the NEON/VFP in interrupt context */
• might_sleep();

This is wrong as the get_cpu() above already disabled preemption. You therefore cannot sleep at this point. Better move the get_cpu() after this.

• preempt_disable();

This is also redundant. You could simply move put_cpu() to kernel_vfp_end() instead.

• fpexc = fmrx(FPEXC) | FPEXC_EN;
• fmxr(FPEXC, fpexc);
• /*

• * Save the userland NEON/VFP state. Under UP, the owner could be a task
  

• * other than 'current'
  

• */
  

• if (vfp_state_in_hw(cpu, thread))

• vfp_save_state(&thread->vfpstate, fpexc);
  

+#ifndef CONFIG_SMP

• else if (vfp_current_hw_state[cpu] != NULL)

• vfp_save_state(vfp_current_hw_state[cpu], fpexc);
  

+#endif

• vfp_current_hw_state[cpu] = NULL;
• put_cpu();

+} +EXPORT_SYMBOL(kernel_vfp_begin);

+void kernel_vfp_end(void) +{

• /* Disable the NEON/VFP unit. */
• fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
• preempt_enable();

+} +EXPORT_SYMBOL(kernel_vfp_end);

+/*

• VFP support code initialisation.

*/ static int __init vfp_init(void) -- 1.8.1.2