[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Yuanyuan Zhong]

The current thread trying to clear the held vfp state may not be
the owner of hw state. For example,
	Core0			Core1
				Thread1 uses VFP.
				  Thread1 vfpstate.hard.cpu = 1.
				  vfp_current_hw_state[1] points to Thread1
				    vfpstate.
	Going to suspend.
	Freeze Thread1.
				Thread1 is switched out.
				VFP HW registers saved to Thread1 vfpstate.
	Core0 disables Core1.
				Stopper thread calls vfp_force_reload().
				Stopper thread vfpstate.hard.cpu = NR_CPUS.
				...
				(No PM notifier for non-idle path. So
				  vfp_pm_suspend() is NOT called on Core1.)
				...
				Core1 is off and VFP HW registers are lost.
	...
	Core0 enables Core1.
	Core0 thaw Thread1.
	Thread1 migrate to Core1
	  before using VFP.
				Thread1 starts using VFP.
				Now we have vfp_current_hw_state[1] points
				  to Thread1 vfpstate. And Thread1 has
				  vfpstate.hard.cpu = 1.
				Thread1 does not need to reload saved vfpstate
				  to VFP HW.
				Thread1 continues running using corrupted VFP
				HW register.
This change fixes above gap by always clearing vfp_current_hw_state when
vfp_force_reload() is called.

Signed-off-by: Yuanyuan Zhong <zyy@motorola.com>
---
 arch/arm/vfp/vfpmodule.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 52b8f40..5f132c0 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -84,6 +84,7 @@ static void vfp_force_reload(unsigned int cpu, struct thread_info *thread)
 	}
 #ifdef CONFIG_SMP
 	thread->vfpstate.hard.cpu = NR_CPUS;
+	vfp_current_hw_state[cpu] = NULL;
 #endif
 }
 
-- 
1.7.10.4

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Russell King - ARM Linux]

On Wed, Oct 02, 2013 at 04:59:47PM -0500, Yuanyuan Zhong wrote:
> The current thread trying to clear the held vfp state may not be
> the owner of hw state. For example,
> 	Core0			Core1
> 				Thread1 uses VFP.
> 				  Thread1 vfpstate.hard.cpu = 1.
> 				  vfp_current_hw_state[1] points to Thread1
> 				    vfpstate.
> 	Going to suspend.
> 	Freeze Thread1.
> 				Thread1 is switched out.
> 				VFP HW registers saved to Thread1 vfpstate.

Correct so far.  At this point:

				vfp_current_hw_state[1] = &thread1->vfpstate;
				thread1->vfpstate.hard.cpu = 1;

> 	Core0 disables Core1.
> 				Stopper thread calls vfp_force_reload().
> 				Stopper thread vfpstate.hard.cpu = NR_CPUS.

Correct, except there's another part to this.  vfp_state_in_hw() returns
true here, since thread1->vfpstate.hard.cpu is the dying CPU (CPU 1), and
vfp_current_hw_state[1] is &thread1->vfpstate.  So we also do this:

				clear FPEXC_EN
				vfp_current_hw_state[1] = NULL;

> 				...
> 				(No PM notifier for non-idle path. So
> 				  vfp_pm_suspend() is NOT called on Core1.)
> 				...
> 				Core1 is off and VFP HW registers are lost.
> 	...
> 	Core0 enables Core1.
> 	Core0 thaw Thread1.
> 	Thread1 migrate to Core1
> 	  before using VFP.
> 				Thread1 starts using VFP.
> 				Now we have vfp_current_hw_state[1] points
> 				  to Thread1 vfpstate. And Thread1 has
> 				  vfpstate.hard.cpu = 1.

No.  With my above correction:
				vfp_current_hw_state[1] = NULL

and that forces a reload of the saved context.

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Yuanyuan ZHONG]

On Wed, Oct 2, 2013 at 5:11 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Oct 02, 2013 at 04:59:47PM -0500, Yuanyuan Zhong wrote:
>> The current thread trying to clear the held vfp state may not be
>> the owner of hw state. For example,
>>       Core0                   Core1
>>                               Thread1 uses VFP.
>>                                 Thread1 vfpstate.hard.cpu = 1.
>>                                 vfp_current_hw_state[1] points to Thread1
>>                                   vfpstate.
>>       Going to suspend.
>>       Freeze Thread1.
>>                               Thread1 is switched out.
>>                               VFP HW registers saved to Thread1 vfpstate.
>
> Correct so far.  At this point:
>
>                                 vfp_current_hw_state[1] = &thread1->vfpstate;
>                                 thread1->vfpstate.hard.cpu = 1;
>
>>       Core0 disables Core1.
>>                               Stopper thread calls vfp_force_reload().
>>                               Stopper thread vfpstate.hard.cpu = NR_CPUS.
>
> Correct, except there's another part to this.  vfp_state_in_hw() returns
> true here, since thread1->vfpstate.hard.cpu is the dying CPU (CPU 1), and
> vfp_current_hw_state[1] is &thread1->vfpstate.  So we also do this:
>
>                                 clear FPEXC_EN
>                                 vfp_current_hw_state[1] = NULL;
>
vfp_state_in_hw() returns false. It's checking current_thread_info()
which is the stopper thread migration/1, not thread1.
>>                               ...
>>                               (No PM notifier for non-idle path. So
>>                                 vfp_pm_suspend() is NOT called on Core1.)
>>                               ...
>>                               Core1 is off and VFP HW registers are lost.
>>       ...
>>       Core0 enables Core1.
>>       Core0 thaw Thread1.
>>       Thread1 migrate to Core1
>>         before using VFP.
>>                               Thread1 starts using VFP.
>>                               Now we have vfp_current_hw_state[1] points
>>                                 to Thread1 vfpstate. And Thread1 has
>>                                 vfpstate.hard.cpu = 1.
>
> No.  With my above correction:
>                                 vfp_current_hw_state[1] = NULL
>
> and that forces a reload of the saved context.

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Yuanyuan ZHONG]

Hi Russell, et al

If there is no further comments, I'll submit it to patch system.
Thanks.

On Wed, Oct 2, 2013 at 5:21 PM, Yuanyuan ZHONG <zyy@motorola.com> wrote:
> On Wed, Oct 2, 2013 at 5:11 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Oct 02, 2013 at 04:59:47PM -0500, Yuanyuan Zhong wrote:
>>> The current thread trying to clear the held vfp state may not be
>>> the owner of hw state. For example,
>>>       Core0                   Core1
>>>                               Thread1 uses VFP.
>>>                                 Thread1 vfpstate.hard.cpu = 1.
>>>                                 vfp_current_hw_state[1] points to Thread1
>>>                                   vfpstate.
>>>       Going to suspend.
>>>       Freeze Thread1.
>>>                               Thread1 is switched out.
>>>                               VFP HW registers saved to Thread1 vfpstate.
>>
>> Correct so far.  At this point:
>>
>>                                 vfp_current_hw_state[1] = &thread1->vfpstate;
>>                                 thread1->vfpstate.hard.cpu = 1;
>>
>>>       Core0 disables Core1.
>>>                               Stopper thread calls vfp_force_reload().
>>>                               Stopper thread vfpstate.hard.cpu = NR_CPUS.
>>
>> Correct, except there's another part to this.  vfp_state_in_hw() returns
>> true here, since thread1->vfpstate.hard.cpu is the dying CPU (CPU 1), and
>> vfp_current_hw_state[1] is &thread1->vfpstate.  So we also do this:
>>
>>                                 clear FPEXC_EN
>>                                 vfp_current_hw_state[1] = NULL;
>>
> vfp_state_in_hw() returns false. It's checking current_thread_info()
> which is the stopper thread migration/1, not thread1.
>>>                               ...
>>>                               (No PM notifier for non-idle path. So
>>>                                 vfp_pm_suspend() is NOT called on Core1.)
>>>                               ...
>>>                               Core1 is off and VFP HW registers are lost.
>>>       ...
>>>       Core0 enables Core1.
>>>       Core0 thaw Thread1.
>>>       Thread1 migrate to Core1
>>>         before using VFP.
>>>                               Thread1 starts using VFP.
>>>                               Now we have vfp_current_hw_state[1] points
>>>                                 to Thread1 vfpstate. And Thread1 has
>>>                                 vfpstate.hard.cpu = 1.
>>
>> No.  With my above correction:
>>                                 vfp_current_hw_state[1] = NULL
>>
>> and that forces a reload of the saved context.

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Russell King - ARM Linux]

On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
> Hi Russell, et al
> 
> If there is no further comments, I'll submit it to patch system.
> Thanks.

No, I still don't agree with your patch as being the correct fix.  Look
at the code that you're creating:

        if (vfp_state_in_hw(cpu, thread)) {
                fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
                vfp_current_hw_state[cpu] = NULL;
        }
#ifdef CONFIG_SMP
        thread->vfpstate.hard.cpu = NR_CPUS;
        vfp_current_hw_state[cpu] = NULL;
#endif

So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
pointer for this CPU?

What actually needs fixing is the call to this function in the hotplug
thread:

                vfp_force_reload((long)hcpu, current_thread_info());

I'd suggest this becomes:

		unsigned cpu = (long)hcpu;
		if (vfp_current_hw_state[cpu])
			vfp_force_reload(cpu, vfp_current_hw_state[cpu]);

That will force the right hw_state to be forced to reload, rather than
passing in the wrong thread state.

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Yuanyuan ZHONG]

On Fri, Oct 11, 2013 at 7:07 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
>> Hi Russell, et al
>>
>> If there is no further comments, I'll submit it to patch system.
>> Thanks.
>
> No, I still don't agree with your patch as being the correct fix.  Look
> at the code that you're creating:
>
>         if (vfp_state_in_hw(cpu, thread)) {
>                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>                 vfp_current_hw_state[cpu] = NULL;
>         }
> #ifdef CONFIG_SMP
>         thread->vfpstate.hard.cpu = NR_CPUS;
>         vfp_current_hw_state[cpu] = NULL;
> #endif
>
> So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
> pointer for this CPU?

I think ensuring local vfp_current_hw_state is invalidated is what
vfp_force_reload() need to do when SMP is enabled.

>
> What actually needs fixing is the call to this function in the hotplug
> thread:
>
>                 vfp_force_reload((long)hcpu, current_thread_info());
>
> I'd suggest this becomes:
>
>                 unsigned cpu = (long)hcpu;
>                 if (vfp_current_hw_state[cpu])
>                         vfp_force_reload(cpu, vfp_current_hw_state[cpu]);
>
> That will force the right hw_state to be forced to reload, rather than
> passing in the wrong thread state.

No. This is not right. Look at the other places vfp_force_reload() are
called, they are all using current_thread_info(). I think only
current_thread_info() is safe for vfp_force_reload().
With your suggestion:
(1) vfp_current_hw_state[cpu] is &thread->vfpstate, not thread.
(2) If the thread exited on another CPU and its stack has been
reclaimed and reallocated, the "thread->vfpstate.hard.cpu = NR_CPUS;"
in vfp_force_reload() will cause memory corruption.

Thanks.

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Russell King - ARM Linux]

On Fri, Oct 11, 2013 at 11:12:19AM -0500, Yuanyuan ZHONG wrote:
> On Fri, Oct 11, 2013 at 7:07 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
> >> Hi Russell, et al
> >>
> >> If there is no further comments, I'll submit it to patch system.
> >> Thanks.
> >
> > No, I still don't agree with your patch as being the correct fix.  Look
> > at the code that you're creating:
> >
> >         if (vfp_state_in_hw(cpu, thread)) {
> >                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
> >                 vfp_current_hw_state[cpu] = NULL;
> >         }
> > #ifdef CONFIG_SMP
> >         thread->vfpstate.hard.cpu = NR_CPUS;
> >         vfp_current_hw_state[cpu] = NULL;
> > #endif
> >
> > So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
> > pointer for this CPU?
> 
> I think ensuring local vfp_current_hw_state is invalidated is what
> vfp_force_reload() need to do when SMP is enabled.

vfp_force_reload() is about asking for the state S on CPU N to be
reloaded.  It is not about merely asking for the state on CPU N to
be reloaded.

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Yuanyuan ZHONG]

On Fri, Oct 11, 2013 at 11:26 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Oct 11, 2013 at 11:12:19AM -0500, Yuanyuan ZHONG wrote:
>> On Fri, Oct 11, 2013 at 7:07 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
>> >> Hi Russell, et al
>> >>
>> >> If there is no further comments, I'll submit it to patch system.
>> >> Thanks.
>> >
>> > No, I still don't agree with your patch as being the correct fix.  Look
>> > at the code that you're creating:
>> >
>> >         if (vfp_state_in_hw(cpu, thread)) {
>> >                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>> >                 vfp_current_hw_state[cpu] = NULL;
>> >         }
>> > #ifdef CONFIG_SMP
>> >         thread->vfpstate.hard.cpu = NR_CPUS;
>> >         vfp_current_hw_state[cpu] = NULL;
>> > #endif
>> >
>> > So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
>> > pointer for this CPU?
>>
>> I think ensuring local vfp_current_hw_state is invalidated is what
>> vfp_force_reload() need to do when SMP is enabled.
>
> vfp_force_reload() is about asking for the state S on CPU N to be
> reloaded.  It is not about merely asking for the state on CPU N to
> be reloaded.

Then this function cannot do what we need for cpu hotplug. At that
time we don't know if the state S on CPU N is valid, we can only ask
for the state on CPU N to be reloaded. How about this:

diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 52b8f40..efdd9e0 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -644,6 +644,7 @@ static int vfp_hotplug(struct notifier_block *b,
unsigned long action,
 {
        if (action == CPU_DYING || action == CPU_DYING_FROZEN) {
                vfp_force_reload((long)hcpu, current_thread_info());
+               vfp_current_hw_state[(long)hcpu] = NULL;
        } else if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
                vfp_enable(NULL);
        return NOTIFY_OK;

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Yuanyuan ZHONG]

On Fri, Oct 11, 2013 at 1:08 PM, Yuanyuan ZHONG <zyy@motorola.com> wrote:
> On Fri, Oct 11, 2013 at 11:26 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Fri, Oct 11, 2013 at 11:12:19AM -0500, Yuanyuan ZHONG wrote:
>>> On Fri, Oct 11, 2013 at 7:07 AM, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> wrote:
>>> > On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
>>> >> Hi Russell, et al
>>> >>
>>> >> If there is no further comments, I'll submit it to patch system.
>>> >> Thanks.
>>> >
>>> > No, I still don't agree with your patch as being the correct fix.  Look
>>> > at the code that you're creating:
>>> >
>>> >         if (vfp_state_in_hw(cpu, thread)) {
>>> >                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>>> >                 vfp_current_hw_state[cpu] = NULL;
>>> >         }
>>> > #ifdef CONFIG_SMP
>>> >         thread->vfpstate.hard.cpu = NR_CPUS;
>>> >         vfp_current_hw_state[cpu] = NULL;
>>> > #endif
>>> >
>>> > So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
>>> > pointer for this CPU?
>>>
>>> I think ensuring local vfp_current_hw_state is invalidated is what
>>> vfp_force_reload() need to do when SMP is enabled.
>>
>> vfp_force_reload() is about asking for the state S on CPU N to be
>> reloaded.  It is not about merely asking for the state on CPU N to
>> be reloaded.
>
> Then this function cannot do what we need for cpu hotplug. At that
> time we don't know if the state S on CPU N is valid, we can only ask
> for the state on CPU N to be reloaded. How about this:
>
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index 52b8f40..efdd9e0 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -644,6 +644,7 @@ static int vfp_hotplug(struct notifier_block *b,
> unsigned long action,
>  {
>         if (action == CPU_DYING || action == CPU_DYING_FROZEN) {
>                 vfp_force_reload((long)hcpu, current_thread_info());
> +               vfp_current_hw_state[(long)hcpu] = NULL;
>         } else if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
>                 vfp_enable(NULL);
>         return NOTIFY_OK;

The vfp_force_reload() here does nothing. We can remove it and this this becomes

static int vfp_hotplug(struct notifier_block *b, unsigned long action,
        void *hcpu)
{
        if (action == CPU_DYING || action == CPU_DYING_FROZEN)
                vfp_current_hw_state[(long)hcpu] = NULL;
        else if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
                vfp_enable(NULL);
        return NOTIFY_OK;
}

[PATCH] arm: vfp: always clear vfp_current_hw_state when forcing reload

[Murali N]

On Sun, Oct 13, 2013 at 11:02 AM, Yuanyuan ZHONG <zyy@motorola.com> wrote:
> On Fri, Oct 11, 2013 at 1:08 PM, Yuanyuan ZHONG <zyy@motorola.com> wrote:
>> On Fri, Oct 11, 2013 at 11:26 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Fri, Oct 11, 2013 at 11:12:19AM -0500, Yuanyuan ZHONG wrote:
>>>> On Fri, Oct 11, 2013 at 7:07 AM, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>> > On Thu, Oct 10, 2013 at 11:00:18AM -0500, Yuanyuan ZHONG wrote:
>>>> >> Hi Russell, et al
>>>> >>
>>>> >> If there is no further comments, I'll submit it to patch system.
>>>> >> Thanks.
>>>> >
>>>> > No, I still don't agree with your patch as being the correct fix.  Look
>>>> > at the code that you're creating:
>>>> >
>>>> >         if (vfp_state_in_hw(cpu, thread)) {
>>>> >                 fmxr(FPEXC, fmrx(FPEXC) & ~FPEXC_EN);
>>>> >                 vfp_current_hw_state[cpu] = NULL;
>>>> >         }
>>>> > #ifdef CONFIG_SMP
>>>> >         thread->vfpstate.hard.cpu = NR_CPUS;
>>>> >         vfp_current_hw_state[cpu] = NULL;
>>>> > #endif
>>>> >
>>>> > So when SMP is enabled we unconditionally blat out the vfp_current_hw_state
>>>> > pointer for this CPU?
>>>>
>>>> I think ensuring local vfp_current_hw_state is invalidated is what
>>>> vfp_force_reload() need to do when SMP is enabled.
>>>
>>> vfp_force_reload() is about asking for the state S on CPU N to be
>>> reloaded.  It is not about merely asking for the state on CPU N to
>>> be reloaded.
>>
>> Then this function cannot do what we need for cpu hotplug. At that
>> time we don't know if the state S on CPU N is valid, we can only ask
>> for the state on CPU N to be reloaded. How about this:
>>
>> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
>> index 52b8f40..efdd9e0 100644
>> --- a/arch/arm/vfp/vfpmodule.c
>> +++ b/arch/arm/vfp/vfpmodule.c
>> @@ -644,6 +644,7 @@ static int vfp_hotplug(struct notifier_block *b,
>> unsigned long action,
>>  {
>>         if (action == CPU_DYING || action == CPU_DYING_FROZEN) {
>>                 vfp_force_reload((long)hcpu, current_thread_info());
>> +               vfp_current_hw_state[(long)hcpu] = NULL;
>>         } else if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
>>                 vfp_enable(NULL);
>>         return NOTIFY_OK;
>
> The vfp_force_reload() here does nothing. We can remove it and this this becomes
>
> static int vfp_hotplug(struct notifier_block *b, unsigned long action,
>         void *hcpu)
> {
>         if (action == CPU_DYING || action == CPU_DYING_FROZEN)
>                 vfp_current_hw_state[(long)hcpu] = NULL;
>         else if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
>                 vfp_enable(NULL);
>         return NOTIFY_OK;
> }
>

What is the final path going to be?
As per Russell it seems setting the "vfp_current_hw_state[cpu] =
NULL;" unconditionaly in "vfp_force_reload()" is wrong.

-- 
Regards,
Murali N