[PATCH] Prevent process migration during vfp_init()

[PATCH] Prevent process migration during vfp_init()

[Hyungwoo Yang]

Hello,

I think I've found a bug and I think it can happen to anyone if there
us heavy load on cores during init stage.

I really need your opinion.

In vfp_init() in "arch/arm/vfp/vfpmodule.c", if there is process
migration between vfp_enable() and smp_call_function() then kernel
crashes.

=== kernel log when system crashes ===
[ 4.319730] VFP support v0.3: implementor 41 architecture 3 part 30
variant 9 rev 1
[ 4.329163] Unable to handle kernel paging request at virtual address 7c512ed8
[ 4.336511] pgd = c0004000
[ 4.339323] [7c512ed8] *pgd=00000000
[ 4.343127] Internal error: Oops: 5 [#1] PREEMPT SMP
[ 4.348166] last sysfs file:
[ 4.351282] Modules linked in:
[ 4.354503] CPU: 0 Tainted: G W (2.6.39.4 #1)
[ 4.360078] PC is at task_rq_lock+0x2c/0x74
[ 4.364412] LR is at try_to_wake_up+0x40/0x440
[ 4.368929] pc : [<c0073178>] lr : [<c007fd98>] psr: 20000193

=== Why it happens ===
I've found the error happens only when there's process migration just
after vfp_init().
Due to the migration, a VFP which is not enabled is accessed and
kernel crashes => smp_call_function() doesn't work as it is expected.

===== original code =====

      if (cpu_arch >= CPU_ARCH_ARMv6)
              vfp_enable(NULL); <== if migration happens just after
vfp_enable(NULL), kernel crashes.
                :
                :
      vfpsid = fmrx(FPSID); <== if migration happens, read tries to
access disbled VFP unit.
                :
                :
     if (VFP_arch)
              printk("not present\n");
      else if (vfpsid & FPSID_NODOUBLE) {
              printk("no double precision support\n");
      } else {
              hotcpu_notifier(vfp_hotplug, 0);

              smp_call_function(vfp_enable, NULL, 1); <== if migration
happens, smp_call_function will not work as it is expected.
=======================

Do you have any opinion?


There're a few ways of preventing migration (like set affinity or
disable premption) but the following is one of the way.
======== Here is my fix ============

>From f96fc79d508235706462336239eb30d66e2e6c0b Mon Sep 17 00:00:00 2001
From: Hyungwoo Yang <hyungwooy@nvidia.com>
Date: Fri, 4 May 2012 11:22:59 -0700
Subject: [PATCH] System crashes if there is process migration during
vfp_init() call.

During vfp_init(), if a process which called vfp_enable() is migrated just
after the call, then the process executing the rest of code will access
a VFP unit which is not ENABLED and also smp_call_function() will not work
as it is expected.

This patch prevents accessing VFP unit disabled by preventing migration
and also replaces smp_call_function() with on_each_cpu() to make sure that
no VFP remains disabled.

Signed-off-by: Hyungwoo Yang <hyungwooy@nvidia.com>

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index bc683b8..6f33e4d 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -655,7 +655,9 @@ static int __init vfp_init(void)
 {
 	unsigned int vfpsid;
 	unsigned int cpu_arch = cpu_architecture();
-
+#ifdef CONFIG_SMP
+	preempt_disable();
+#endif
 	if (cpu_arch >= CPU_ARCH_ARMv6)
 		vfp_enable(NULL);

@@ -667,7 +669,9 @@ static int __init vfp_init(void)
 	vfp_vector = vfp_testing_entry;
 	barrier();
 	vfpsid = fmrx(FPSID);
-	barrier();
+#ifdef CONFIG_SMP
+	preempt_enable();
+#endif
 	vfp_vector = vfp_null_entry;

 	printk(KERN_INFO "VFP support v0.3: ");
@@ -678,7 +682,7 @@ static int __init vfp_init(void)
 	} else {
 		hotcpu_notifier(vfp_hotplug, 0);

-		smp_call_function(vfp_enable, NULL, 1);
+		on_each_cpu(vfp_enable, NULL, 1);

 		VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT;  /*
Extract the architecture version */
 		printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
-- 
1.7.0.4

[PATCH] Prevent process migration during vfp_init()

[Will Deacon]

On Fri, May 04, 2012 at 09:28:15PM +0100, Hyungwoo Yang wrote:
> Hello,

Hi,

Some comments inline and a proposed v2.

> From f96fc79d508235706462336239eb30d66e2e6c0b Mon Sep 17 00:00:00 2001
> From: Hyungwoo Yang <hyungwooy@nvidia.com>
> Date: Fri, 4 May 2012 11:22:59 -0700
> Subject: [PATCH] System crashes if there is process migration during
> vfp_init() call.
> 
> During vfp_init(), if a process which called vfp_enable() is migrated just
> after the call, then the process executing the rest of code will access
> a VFP unit which is not ENABLED and also smp_call_function() will not work
> as it is expected.
> 
> This patch prevents accessing VFP unit disabled by preventing migration
> and also replaces smp_call_function() with on_each_cpu() to make sure that
> no VFP remains disabled.
> 
> Signed-off-by: Hyungwoo Yang <hyungwooy@nvidia.com>
> 
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index bc683b8..6f33e4d 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -655,7 +655,9 @@ static int __init vfp_init(void)
>  {
>  	unsigned int vfpsid;
>  	unsigned int cpu_arch = cpu_architecture();
> -
> +#ifdef CONFIG_SMP
> +	preempt_disable();
> +#endif

I don't think it's worth having the SMP guards for this.

>  	if (cpu_arch >= CPU_ARCH_ARMv6)
>  		vfp_enable(NULL);

I think we can avoid this redundant enable if we move some code around (see
below).

> @@ -667,7 +669,9 @@ static int __init vfp_init(void)
>  	vfp_vector = vfp_testing_entry;
>  	barrier();
>  	vfpsid = fmrx(FPSID);
> -	barrier();
> +#ifdef CONFIG_SMP
> +	preempt_enable();
> +#endif

If we're not running a PREEMPT kernel, you just removed a compiler barrier.

How about something like the following instead? I've also added a BUG_ON to
vfp_enable to stop this catching us out in future:

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index bc683b8..8e08983 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
+#include <linux/hardirq.h>
 #include <linux/kernel.h>
 #include <linux/notifier.h>
 #include <linux/signal.h>
@@ -432,7 +433,10 @@ void VFP_bounce(u32 trigger, u32 fpexc, struct pt_regs *regs)
 
 static void vfp_enable(void *unused)
 {
-       u32 access = get_copro_access();
+       u32 access;
+
+       BUG_ON(preemptible());
+       access = get_copro_access();
 
        /*
         * Enable full access to VFP (cp10 and cp11)
@@ -657,18 +661,20 @@ static int __init vfp_init(void)
        unsigned int cpu_arch = cpu_architecture();
 
        if (cpu_arch >= CPU_ARCH_ARMv6)
-               vfp_enable(NULL);
+               on_each_cpu(vfp_enable, NULL, 1);
 
        /*
         * First check that there is a VFP that we can use.
         * The handler is already setup to just log calls, so
         * we just need to read the VFPSID register.
         */
+       preempt_disable();
        vfp_vector = vfp_testing_entry;
        barrier();
        vfpsid = fmrx(FPSID);
        barrier();
        vfp_vector = vfp_null_entry;
+       preempt_enable();
 
        printk(KERN_INFO "VFP support v0.3: ");
        if (VFP_arch)
@@ -678,8 +684,6 @@ static int __init vfp_init(void)
        } else {
                hotcpu_notifier(vfp_hotplug, 0);
 
-               smp_call_function(vfp_enable, NULL, 1);
-
                VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT;  /* Extract the architecture version */
                printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
                        (vfpsid & FPSID_IMPLEMENTER_MASK) >> FPSID_IMPLEMENTER_BIT,

Will

[PATCH] Prevent process migration during vfp_init()

[Hyungwoo Yang]

Hello Will,

Thank you for your comments. I didn't check the implementation of
preempt_disable() on non-preemptible kernel case.

I think we don't need preempt_disable()/preempt_enable() call in your proposal.
I mean, since we have enabled on every VFPs in cores online(
on_each_cpu() ), we don't need to worry about accessing a VFP which is
disabled. So we don't need to worry about migration after
"on_each_cpu()", right?

- Hyungwoo Yang

On Tue, May 8, 2012 at 4:13 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, May 04, 2012 at 09:28:15PM +0100, Hyungwoo Yang wrote:
>> Hello,
>
> Hi,
>
> Some comments inline and a proposed v2.
>
>> From f96fc79d508235706462336239eb30d66e2e6c0b Mon Sep 17 00:00:00 2001
>> From: Hyungwoo Yang <hyungwooy@nvidia.com>
>> Date: Fri, 4 May 2012 11:22:59 -0700
>> Subject: [PATCH] System crashes if there is process migration during
>> vfp_init() call.
>>
>> During vfp_init(), if a process which called vfp_enable() is migrated just
>> after the call, then the process executing the rest of code will access
>> a VFP unit which is not ENABLED and also smp_call_function() will not work
>> as it is expected.
>>
>> This patch prevents accessing VFP unit disabled by preventing migration
>> and also replaces smp_call_function() with on_each_cpu() to make sure that
>> no VFP remains disabled.
>>
>> Signed-off-by: Hyungwoo Yang <hyungwooy@nvidia.com>
>>
>> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
>> index bc683b8..6f33e4d 100644
>> --- a/arch/arm/vfp/vfpmodule.c
>> +++ b/arch/arm/vfp/vfpmodule.c
>> @@ -655,7 +655,9 @@ static int __init vfp_init(void)
>> ?{
>> ? ? ? unsigned int vfpsid;
>> ? ? ? unsigned int cpu_arch = cpu_architecture();
>> -
>> +#ifdef CONFIG_SMP
>> + ? ? preempt_disable();
>> +#endif
>
> I don't think it's worth having the SMP guards for this.
>
>> ? ? ? if (cpu_arch >= CPU_ARCH_ARMv6)
>> ? ? ? ? ? ? ? vfp_enable(NULL);
>
> I think we can avoid this redundant enable if we move some code around (see
> below).
>
>> @@ -667,7 +669,9 @@ static int __init vfp_init(void)
>> ? ? ? vfp_vector = vfp_testing_entry;
>> ? ? ? barrier();
>> ? ? ? vfpsid = fmrx(FPSID);
>> - ? ? barrier();
>> +#ifdef CONFIG_SMP
>> + ? ? preempt_enable();
>> +#endif
>
> If we're not running a PREEMPT kernel, you just removed a compiler barrier.
>
> How about something like the following instead? I've also added a BUG_ON to
> vfp_enable to stop this catching us out in future:
>
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index bc683b8..8e08983 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -11,6 +11,7 @@
> ?#include <linux/types.h>
> ?#include <linux/cpu.h>
> ?#include <linux/cpu_pm.h>
> +#include <linux/hardirq.h>
> ?#include <linux/kernel.h>
> ?#include <linux/notifier.h>
> ?#include <linux/signal.h>
> @@ -432,7 +433,10 @@ void VFP_bounce(u32 trigger, u32 fpexc, struct pt_regs *regs)
>
> ?static void vfp_enable(void *unused)
> ?{
> - ? ? ? u32 access = get_copro_access();
> + ? ? ? u32 access;
> +
> + ? ? ? BUG_ON(preemptible());
> + ? ? ? access = get_copro_access();
>
> ? ? ? ?/*
> ? ? ? ? * Enable full access to VFP (cp10 and cp11)
> @@ -657,18 +661,20 @@ static int __init vfp_init(void)
> ? ? ? ?unsigned int cpu_arch = cpu_architecture();
>
> ? ? ? ?if (cpu_arch >= CPU_ARCH_ARMv6)
> - ? ? ? ? ? ? ? vfp_enable(NULL);
> + ? ? ? ? ? ? ? on_each_cpu(vfp_enable, NULL, 1);
>
> ? ? ? ?/*
> ? ? ? ? * First check that there is a VFP that we can use.
> ? ? ? ? * The handler is already setup to just log calls, so
> ? ? ? ? * we just need to read the VFPSID register.
> ? ? ? ? */
> + ? ? ? preempt_disable();
> ? ? ? ?vfp_vector = vfp_testing_entry;
> ? ? ? ?barrier();
> ? ? ? ?vfpsid = fmrx(FPSID);
> ? ? ? ?barrier();
> ? ? ? ?vfp_vector = vfp_null_entry;
> + ? ? ? preempt_enable();
>
> ? ? ? ?printk(KERN_INFO "VFP support v0.3: ");
> ? ? ? ?if (VFP_arch)
> @@ -678,8 +684,6 @@ static int __init vfp_init(void)
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?hotcpu_notifier(vfp_hotplug, 0);
>
> - ? ? ? ? ? ? ? smp_call_function(vfp_enable, NULL, 1);
> -
> ? ? ? ? ? ? ? ?VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT; ?/* Extract the architecture version */
> ? ? ? ? ? ? ? ?printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
> ? ? ? ? ? ? ? ? ? ? ? ?(vfpsid & FPSID_IMPLEMENTER_MASK) >> FPSID_IMPLEMENTER_BIT,
>
> Will

[PATCH] Prevent process migration during vfp_init()

[Will Deacon]

On Tue, May 08, 2012 at 06:24:27PM +0100, Hyungwoo Yang wrote:
> I think we don't need preempt_disable()/preempt_enable() call in your proposal.
> I mean, since we have enabled on every VFPs in cores online(
> on_each_cpu() ), we don't need to worry about accessing a VFP which is
> disabled. So we don't need to worry about migration after
> "on_each_cpu()", right?

Yes, that sounds reasonable to me since any thread migration will imply the
barriers we need for the VFP exception vector to be correct. In which case the
patch can be further reduced to what I've got below.

It seems happy enough on my quad A9 running a bunch of paranoia FP tests --
do you have a particular testcase which was exhibiting this failure when you
reported the issue?

Cheers,

Will


diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index bc683b8..c5767b5 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
+#include <linux/hardirq.h>
 #include <linux/kernel.h>
 #include <linux/notifier.h>
 #include <linux/signal.h>
@@ -432,7 +433,10 @@ void VFP_bounce(u32 trigger, u32 fpexc, struct pt_regs *regs)
 
 static void vfp_enable(void *unused)
 {
-       u32 access = get_copro_access();
+       u32 access;
+
+       BUG_ON(preemptible());
+       access = get_copro_access();
 
        /*
         * Enable full access to VFP (cp10 and cp11)
@@ -657,7 +661,7 @@ static int __init vfp_init(void)
        unsigned int cpu_arch = cpu_architecture();
 
        if (cpu_arch >= CPU_ARCH_ARMv6)
-               vfp_enable(NULL);
+               on_each_cpu(vfp_enable, NULL, 1);
 
        /*
         * First check that there is a VFP that we can use.
@@ -678,8 +682,6 @@ static int __init vfp_init(void)
        } else {
                hotcpu_notifier(vfp_hotplug, 0);
 
-               smp_call_function(vfp_enable, NULL, 1);
-
                VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT;  /* Extract the architecture version */
                printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
                        (vfpsid & FPSID_IMPLEMENTER_MASK) >> FPSID_IMPLEMENTER_BIT,

[PATCH] Prevent process migration during vfp_init()

[Hyungwoo Yang]

Yes, I've already tested without preemption, it seems it is working fine.

for the failure, it is simple. on my A9 quad, due to heavy loads
during "late initcall", sometimes process migration happens just after
vfp_enable() but before smp_call_function(). it means there can be a
VFP disabled but accessed since smp_call_function() is for the rest of
cores not the core running it.

For example, if enable_vfp() is executed on core0 but
smp_call_function() is excuted on core1 due to migration, then VFP on
core1 is not enabled. it will be enabled next time when the core1
online again.

- Hyungwoo Yang

On Tue, May 8, 2012 at 11:04 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, May 08, 2012 at 06:24:27PM +0100, Hyungwoo Yang wrote:
>> I think we don't need preempt_disable()/preempt_enable() call in your proposal.
>> I mean, since we have enabled on every VFPs in cores online(
>> on_each_cpu() ), we don't need to worry about accessing a VFP which is
>> disabled. So we don't need to worry about migration after
>> "on_each_cpu()", right?
>
> Yes, that sounds reasonable to me since any thread migration will imply the
> barriers we need for the VFP exception vector to be correct. In which case the
> patch can be further reduced to what I've got below.
>
> It seems happy enough on my quad A9 running a bunch of paranoia FP tests --
> do you have a particular testcase which was exhibiting this failure when you
> reported the issue?
>
> Cheers,
>
> Will
>
>
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index bc683b8..c5767b5 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -11,6 +11,7 @@
> ?#include <linux/types.h>
> ?#include <linux/cpu.h>
> ?#include <linux/cpu_pm.h>
> +#include <linux/hardirq.h>
> ?#include <linux/kernel.h>
> ?#include <linux/notifier.h>
> ?#include <linux/signal.h>
> @@ -432,7 +433,10 @@ void VFP_bounce(u32 trigger, u32 fpexc, struct pt_regs *regs)
>
> ?static void vfp_enable(void *unused)
> ?{
> - ? ? ? u32 access = get_copro_access();
> + ? ? ? u32 access;
> +
> + ? ? ? BUG_ON(preemptible());
> + ? ? ? access = get_copro_access();
>
> ? ? ? ?/*
> ? ? ? ? * Enable full access to VFP (cp10 and cp11)
> @@ -657,7 +661,7 @@ static int __init vfp_init(void)
> ? ? ? ?unsigned int cpu_arch = cpu_architecture();
>
> ? ? ? ?if (cpu_arch >= CPU_ARCH_ARMv6)
> - ? ? ? ? ? ? ? vfp_enable(NULL);
> + ? ? ? ? ? ? ? on_each_cpu(vfp_enable, NULL, 1);
>
> ? ? ? ?/*
> ? ? ? ? * First check that there is a VFP that we can use.
> @@ -678,8 +682,6 @@ static int __init vfp_init(void)
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?hotcpu_notifier(vfp_hotplug, 0);
>
> - ? ? ? ? ? ? ? smp_call_function(vfp_enable, NULL, 1);
> -
> ? ? ? ? ? ? ? ?VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT; ?/* Extract the architecture version */
> ? ? ? ? ? ? ? ?printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
> ? ? ? ? ? ? ? ? ? ? ? ?(vfpsid & FPSID_IMPLEMENTER_MASK) >> FPSID_IMPLEMENTER_BIT,

[PATCH] Prevent process migration during vfp_init()

[Will Deacon]

On Tue, May 08, 2012 at 07:28:08PM +0100, Hyungwoo Yang wrote:
> Yes, I've already tested without preemption, it seems it is working fine.

Cracking, thanks for confirming that.

> for the failure, it is simple. on my A9 quad, due to heavy loads
> during "late initcall", sometimes process migration happens just after
> vfp_enable() but before smp_call_function(). it means there can be a
> VFP disabled but accessed since smp_call_function() is for the rest of
> cores not the core running it.
> 
> For example, if enable_vfp() is executed on core0 but
> smp_call_function() is excuted on core1 due to migration, then VFP on
> core1 is not enabled. it will be enabled next time when the core1
> online again.

Yup, your initial explanation was clear enough, I just wanted to check that
this did indeed solve your testcase given that I've been lucky enough not to
hit the issue on my boards.

Can I add your signed-off-by to the following please?

Will


Subject: [PATCH] ARM: vfp: ensure preemption is disabled when enabling VFP access

The vfp_enable function enables access to the VFP co-processor register
space (cp10 and cp11) on the current CPU and must be called with
preemption disabled. Unfortunately, the vfp_init late initcall does not
disable preemption and can lead to an oops during boot if thread
migration occurs at the wrong time and we end up attempting to access
the FPSID on a CPU with VFP access disabled.

This patch fixes the initcall to call vfp_enable from a non-preemptible
context on each CPU and adds a BUG_ON(preemptible) to ensure that any
similar problems are easily spotted in the future.

Reported-by: Hyungwoo Yang <hwoo.yang@gmail.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/vfp/vfpmodule.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index bc683b8..c5767b5 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
+#include <linux/hardirq.h>
 #include <linux/kernel.h>
 #include <linux/notifier.h>
 #include <linux/signal.h>
@@ -432,7 +433,10 @@ void VFP_bounce(u32 trigger, u32 fpexc, struct pt_regs *regs)
 
 static void vfp_enable(void *unused)
 {
-	u32 access = get_copro_access();
+	u32 access;
+
+	BUG_ON(preemptible());
+	access = get_copro_access();
 
 	/*
 	 * Enable full access to VFP (cp10 and cp11)
@@ -657,7 +661,7 @@ static int __init vfp_init(void)
 	unsigned int cpu_arch = cpu_architecture();
 
 	if (cpu_arch >= CPU_ARCH_ARMv6)
-		vfp_enable(NULL);
+		on_each_cpu(vfp_enable, NULL, 1);
 
 	/*
 	 * First check that there is a VFP that we can use.
@@ -678,8 +682,6 @@ static int __init vfp_init(void)
 	} else {
 		hotcpu_notifier(vfp_hotplug, 0);
 
-		smp_call_function(vfp_enable, NULL, 1);
-
 		VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT;  /* Extract the architecture version */
 		printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
 			(vfpsid & FPSID_IMPLEMENTER_MASK) >> FPSID_IMPLEMENTER_BIT,
-- 
1.7.4.1

[PATCH] Prevent process migration during vfp_init()

[Hyungwoo Yang]

Sure, please go ahead.

On Tue, May 8, 2012 at 11:45 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, May 08, 2012 at 07:28:08PM +0100, Hyungwoo Yang wrote:
>> Yes, I've already tested without preemption, it seems it is working fine.
>
> Cracking, thanks for confirming that.
>
>> for the failure, it is simple. on my A9 quad, due to heavy loads
>> during "late initcall", sometimes process migration happens just after
>> vfp_enable() but before smp_call_function(). it means there can be a
>> VFP disabled but accessed since smp_call_function() is for the rest of
>> cores not the core running it.
>>
>> For example, if enable_vfp() is executed on core0 but
>> smp_call_function() is excuted on core1 due to migration, then VFP on
>> core1 is not enabled. it will be enabled next time when the core1
>> online again.
>
> Yup, your initial explanation was clear enough, I just wanted to check that
> this did indeed solve your testcase given that I've been lucky enough not to
> hit the issue on my boards.
>
> Can I add your signed-off-by to the following please?
>
> Will
>
>
> Subject: [PATCH] ARM: vfp: ensure preemption is disabled when enabling VFP access
>
> The vfp_enable function enables access to the VFP co-processor register
> space (cp10 and cp11) on the current CPU and must be called with
> preemption disabled. Unfortunately, the vfp_init late initcall does not
> disable preemption and can lead to an oops during boot if thread
> migration occurs at the wrong time and we end up attempting to access
> the FPSID on a CPU with VFP access disabled.
>
> This patch fixes the initcall to call vfp_enable from a non-preemptible
> context on each CPU and adds a BUG_ON(preemptible) to ensure that any
> similar problems are easily spotted in the future.
>
> Reported-by: Hyungwoo Yang <hwoo.yang@gmail.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> ?arch/arm/vfp/vfpmodule.c | ? 10 ++++++----
> ?1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index bc683b8..c5767b5 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -11,6 +11,7 @@
> ?#include <linux/types.h>
> ?#include <linux/cpu.h>
> ?#include <linux/cpu_pm.h>
> +#include <linux/hardirq.h>
> ?#include <linux/kernel.h>
> ?#include <linux/notifier.h>
> ?#include <linux/signal.h>
> @@ -432,7 +433,10 @@ void VFP_bounce(u32 trigger, u32 fpexc, struct pt_regs *regs)
>
> ?static void vfp_enable(void *unused)
> ?{
> - ? ? ? u32 access = get_copro_access();
> + ? ? ? u32 access;
> +
> + ? ? ? BUG_ON(preemptible());
> + ? ? ? access = get_copro_access();
>
> ? ? ? ?/*
> ? ? ? ? * Enable full access to VFP (cp10 and cp11)
> @@ -657,7 +661,7 @@ static int __init vfp_init(void)
> ? ? ? ?unsigned int cpu_arch = cpu_architecture();
>
> ? ? ? ?if (cpu_arch >= CPU_ARCH_ARMv6)
> - ? ? ? ? ? ? ? vfp_enable(NULL);
> + ? ? ? ? ? ? ? on_each_cpu(vfp_enable, NULL, 1);
>
> ? ? ? ?/*
> ? ? ? ? * First check that there is a VFP that we can use.
> @@ -678,8 +682,6 @@ static int __init vfp_init(void)
> ? ? ? ?} else {
> ? ? ? ? ? ? ? ?hotcpu_notifier(vfp_hotplug, 0);
>
> - ? ? ? ? ? ? ? smp_call_function(vfp_enable, NULL, 1);
> -
> ? ? ? ? ? ? ? ?VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT; ?/* Extract the architecture version */
> ? ? ? ? ? ? ? ?printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
> ? ? ? ? ? ? ? ? ? ? ? ?(vfpsid & FPSID_IMPLEMENTER_MASK) >> FPSID_IMPLEMENTER_BIT,
> --
> 1.7.4.1

[PATCH] Prevent process migration during vfp_init()

[Russell King - ARM Linux]

On Tue, May 08, 2012 at 07:04:30PM +0100, Will Deacon wrote:
> On Tue, May 08, 2012 at 06:24:27PM +0100, Hyungwoo Yang wrote:
> > I think we don't need preempt_disable()/preempt_enable() call in your proposal.
> > I mean, since we have enabled on every VFPs in cores online(
> > on_each_cpu() ), we don't need to worry about accessing a VFP which is
> > disabled. So we don't need to worry about migration after
> > "on_each_cpu()", right?
> 
> Yes, that sounds reasonable to me since any thread migration will imply the
> barriers we need for the VFP exception vector to be correct. In which case the
> patch can be further reduced to what I've got below.
> 
> It seems happy enough on my quad A9 running a bunch of paranoia FP tests --
> do you have a particular testcase which was exhibiting this failure when you
> reported the issue?

It's pointless doing FP tests for this level of change - the code you're
modifying is only run at startup to enable access to VFP.  If you can
execute a single VFP instruction in userspace on each CPU, then your test
has passed.  Further VFP instructions do not gain you any additional test
coverage.

The only comment I have is whether that BUG_ON(preemptible()) - preferably
WARN_ON() - should be inside get_copro_access() itself, in a similar way
to smp_processor_id().

[PATCH] Prevent process migration during vfp_init()

[Will Deacon]

On Wed, May 09, 2012 at 10:26:50AM +0100, Russell King - ARM Linux wrote:
> On Tue, May 08, 2012 at 07:04:30PM +0100, Will Deacon wrote:
> > It seems happy enough on my quad A9 running a bunch of paranoia FP tests --
> > do you have a particular testcase which was exhibiting this failure when you
> > reported the issue?
> 
> It's pointless doing FP tests for this level of change - the code you're
> modifying is only run at startup to enable access to VFP.  If you can
> execute a single VFP instruction in userspace on each CPU, then your test
> has passed.  Further VFP instructions do not gain you any additional test
> coverage.

Agreed -- I ran the tests to ensure that we did execute user VFP instructions
on each CPU since I'm not sure how VFPified my basic userspace is.

> The only comment I have is whether that BUG_ON(preemptible()) - preferably
> WARN_ON() - should be inside get_copro_access() itself, in a similar way
> to smp_processor_id().

I guess the BUG vs WARN debate comes down to what the consequences are of
continuing if we are preemptible. In the latter case, don't we leave
ourselves open to kernel exceptions if we try to migrate a VFP-using thread
onto a CPU without the co-processor permissions set correctly? From a
debugging point-of-view, the BUG_ON might then be more informative. Why do
you prefer WARN for this?

We could move the BUG/WARN into get_copro_access, although it will likely be
used as a pair with set_copro_access so really we want the preempt check to
be across the two calls.

Cheers,

Will

[PATCH] Prevent process migration during vfp_init()

[Hyungwoo Yang]

Hello,

I think I've found a bug but actually I'm not sure whether it only
happens to me due to our changes in kernel.

I really need your opinion.

In vfp_init() in "arch/arm/vfp/vfpmodule.c", if there is process
migration between vfp_enable() and smp_call_function() then kernel
crashes.

===== original code =====

       if (cpu_arch >= CPU_ARCH_ARMv6)
               vfp_enable(NULL); <== if migration happens just after
vfp_enable(NULL), kernel crashes.
                 :
                 :
       vfpsid = fmrx(FPSID); <== if migration happens, read tries to
access disbled VFP unit.
                 :
                 :
      if (VFP_arch)
               printk("not present\n");
       else if (vfpsid & FPSID_NODOUBLE) {
               printk("no double precision support\n");
       } else {
               hotcpu_notifier(vfp_hotplug, 0);

               smp_call_function(vfp_enable, NULL, 1); <== if migration happens,
smp_call_function will not work as it is expected.
=======================

Do you have any opinion?


There're a few ways of preventing migration (like set affinity or
disable premption) but the following is one of the way.
=======================

>From 6d48d0aaac03e845646b445ad02ef3c228dcfdb9 Mon Sep 17 00:00:00 2001
From: Hyungwoo Yang <hyungwooy@nvidia.com>
Date: Thu, 3 May 2012 16:49:13 -0700
Subject: [PATCH] ARM: vfp: Prevent process migration during vfp_init()

System crashes if there is process migration during vfp_init() call.

During vfp_init(), if a process which called vfp_enable() is migrated just
after the call, then the process executing the rest of code will access
a VFP unit which is not ENABLED and also smp_call_function() will not work
as it is expected.

This patch prevents accessing VFP unit disabled by preventing migration
and also replaces smp_call_function() with on_each_cpu() to make sure that
no VFP remains disabled.

Signed-off-by: Hyungwoo Yang <hyungwooy@nvidia.com>
---
 arch/arm/vfp/vfpmodule.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index bc683b8..fefa4cb 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -655,7 +655,9 @@ static int __init vfp_init(void)
 {
 	unsigned int vfpsid;
 	unsigned int cpu_arch = cpu_architecture();
-
+#ifdef CONFIG_SMP
+	preempt_disable();
+#endif
 	if (cpu_arch >= CPU_ARCH_ARMv6)
 		vfp_enable(NULL);

@@ -669,6 +671,9 @@ static int __init vfp_init(void)
 	vfpsid = fmrx(FPSID);
 	barrier();
 	vfp_vector = vfp_null_entry;
+#ifdef CONFIG_SMP
+	preempt_enable();
+#endif

 	printk(KERN_INFO "VFP support v0.3: ");
 	if (VFP_arch)
@@ -678,7 +683,7 @@ static int __init vfp_init(void)
 	} else {
 		hotcpu_notifier(vfp_hotplug, 0);

-		smp_call_function(vfp_enable, NULL, 1);
+		on_each_cpu(vfp_enable, NULL, 1);

 		VFP_arch = (vfpsid & FPSID_ARCH_MASK) >> FPSID_ARCH_BIT;  /*
Extract the architecture version */
 		printk("implementor %02x architecture %d part %02x variant %x rev %x\n",
-- 
1.7.0.4