arm: Is VFP hotplug notifiers wrong?

arm: Is VFP hotplug notifiers wrong?

[Kohji Okuno]

Dear Thomas and all,

Could you please confirm about the following commit, again?

http://git.armlinux.org.uk/cgit/linux-arm.git/commit/arch/arm/vfp/vfpmodule.c?id=e5b61bafe70477e05e1dce0d6ca4ec181e23cb2a


The avobe commit eliminated the following fix, I think.

http://git.armlinux.org.uk/cgit/linux-arm.git/commit/arch/arm/vfp/vfpmodule.c?id=384b38b66947b06999b3e39a596d4f2fb94f77e4


vfp_force_reload() called from vfp_dying_cpu() does not clear
vfp_current_hw_state[cpu], because cpu stopper task does not own the
context held in the VFP hardware.

Best regards,
 Kohji Okuno

arm: Is VFP hotplug notifiers wrong?

[Russell King - ARM Linux]

On Tue, Jan 09, 2018 at 08:12:21PM +0900, Kohji Okuno wrote:
> Dear Thomas and all,
> 
> Could you please confirm about the following commit, again?
> 
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/arch/arm/vfp/vfpmodule.c?id=e5b61bafe70477e05e1dce0d6ca4ec181e23cb2a
> 
> 
> The avobe commit eliminated the following fix, I think.
> 
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/arch/arm/vfp/vfpmodule.c?id=384b38b66947b06999b3e39a596d4f2fb94f77e4
> 
> 
> vfp_force_reload() called from vfp_dying_cpu() does not clear
> vfp_current_hw_state[cpu], because cpu stopper task does not own the
> context held in the VFP hardware.

You are correct, tglx's patch was wrong, since the state in the CPU may
not be the current thread's state, so vfp_force_reload() may not do
anything.

vfp_force_reload() forces the reload of the specified state for the
specified CPU.  What the original hotplug code did was to ensure that
the CPU's state would be reloaded when it came back up.

I do wish that people wouldn't combine functional changes and cleanups
into one patch - it makes this kind of thing harder to spot in review
and also means when we encounter crap like this, it means we can't
simply revert the cleanup.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

arm: Is VFP hotplug notifiers wrong?

[okuno.kohji at jp.panasonic.com]

Dear Russel and Thomas,

Thank you for your quick response.
Thomas, do you create the patch?

Best regards,
 Kohji Okuno

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx at linutronix.de]
> Sent: Tuesday, January 9, 2018 10:58 PM
> To: Russell King - ARM Linux
> Cc: Okuno Kohji (?? ??); linux-arm-kernel at lists.infradead.org
> Subject: Re: arm: Is VFP hotplug notifiers wrong?
> 
> On Tue, 9 Jan 2018, Russell King - ARM Linux wrote:
> 
> > On Tue, Jan 09, 2018 at 08:12:21PM +0900, Kohji Okuno wrote:
> > > Dear Thomas and all,
> > >
> > > Could you please confirm about the following commit, again?
> > >
> > > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/arch/arm/vfp/vf
> > > pmodule.c?id=e5b61bafe70477e05e1dce0d6ca4ec181e23cb2a
> > >
> > >
> > > The avobe commit eliminated the following fix, I think.
> > >
> > > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/arch/arm/vfp/vf
> > > pmodule.c?id=384b38b66947b06999b3e39a596d4f2fb94f77e4
> > >
> > >
> > > vfp_force_reload() called from vfp_dying_cpu() does not clear
> > > vfp_current_hw_state[cpu], because cpu stopper task does not own the
> > > context held in the VFP hardware.
> >
> > You are correct, tglx's patch was wrong, since the state in the CPU
> > may not be the current thread's state, so vfp_force_reload() may not
> > do anything.
> >
> > vfp_force_reload() forces the reload of the specified state for the
> > specified CPU.  What the original hotplug code did was to ensure that
> > the CPU's state would be reloaded when it came back up.
> >
> > I do wish that people wouldn't combine functional changes and cleanups
> > into one patch - it makes this kind of thing harder to spot in review
> > and also means when we encounter crap like this, it means we can't
> > simply revert the cleanup.
> 
> sorry about that....

arm: Is VFP hotplug notifiers wrong?

[Russell King - ARM Linux]

On Tue, Jan 09, 2018 at 02:46:30PM +0000, okuno.kohji at jp.panasonic.com wrote:
> Dear Russel and Thomas,
> 
> Thank you for your quick response.
> Thomas, do you create the patch?

I'm afraid to say that I think everyone is super busy with spectre
and meltdown at the moment, sorry.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

arm: Is VFP hotplug notifiers wrong?

[Fabio Estevam]

Hi Okuno,

On Tue, Jan 9, 2018 at 12:46 PM,  <okuno.kohji@jp.panasonic.com> wrote:
> Dear Russel and Thomas,
>
> Thank you for your quick response.
> Thomas, do you create the patch?

Looks like the fix should be like this:

--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -648,7 +648,7 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
  */
 static int vfp_dying_cpu(unsigned int cpu)
 {
-       vfp_force_reload(cpu, current_thread_info());
+       vfp_current_hw_state[cpu] = NULL;
        return 0;
 }

Could you please test it?

Thanks

arm: Is VFP hotplug notifiers wrong?

[okuno.kohji at jp.panasonic.com]

Dear Fabio and Russel,

Fabio, thank you for providing your patch. It should be work good.
Unfortunately, I do not have an environment to try out the latest kernel right now.
Indeed, I encountered the issue(*) for VFP in using kernel 3.10.x. Then, I noticed the problem at the master.
kernel 3.10.x does not introduce this fix.

(*) Under certain conditions, VFP registers becomes undefined values after resume.
   - a last thread using VFP before suspend was executed by CPU other than CPU#0
   - the first thread using VFP after resume was the same thread as above,
     and, the thread was executed on the same CPU as before suspend.

Best regards,
 Kohji Okuno

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: Wednesday, January 10, 2018 12:02 AM
> To: Okuno Kohji (?? ??)
> Cc: Thomas Gleixner; Russell King - ARM Linux; moderated list:ARM/FREESCALE
> IMX / MXC ARM ARCHITECTURE
> Subject: Re: arm: Is VFP hotplug notifiers wrong?
> 
> Hi Okuno,
> 
> On Tue, Jan 9, 2018 at 12:46 PM,  <okuno.kohji@jp.panasonic.com> wrote:
> > Dear Russel and Thomas,
> >
> > Thank you for your quick response.
> > Thomas, do you create the patch?
> 
> Looks like the fix should be like this:
> 
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -648,7 +648,7 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
>   */
>  static int vfp_dying_cpu(unsigned int cpu)  {
> -       vfp_force_reload(cpu, current_thread_info());
> +       vfp_current_hw_state[cpu] = NULL;
>         return 0;
>  }
> 
> Could you please test it?
> 
> Thanks