Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Jon Hunter]

Hi Ard,

On 28/03/2022 14:47, Ard Biesheuvel wrote:
> The Spectre-BHB mitigations were inadvertently left disabled for
> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
> that case. So fix that.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>   arch/arm/mm/proc-v7-bugs.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> index 06dbfb968182..fb9f3eb6bf48 100644
> --- a/arch/arm/mm/proc-v7-bugs.c
> +++ b/arch/arm/mm/proc-v7-bugs.c
> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
>   {
>   	if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
>   		cpu_v7_spectre_v2_init();
> +	cpu_v7_spectre_bhb_init();
>   }
>   
>   void cpu_v7_bugs_init(void)


Since this patch has been merged, I am seeing a ton of messages when 
booting Linux on tegra124-jetson-tk1 ...

[ 1233.327547] CPU0: Spectre BHB: using loop workaround
[ 1233.327795] CPU1: Spectre BHB: using loop workaround
[ 1233.328270] CPU1: Spectre BHB: using loop workaround
[ 1233.328700] CPU1: Spectre BHB: using loop workaround
[ 1233.355477] CPU2: Spectre BHB: using loop workaround
** 7 printk messages dropped **
[ 1233.366271] CPU0: Spectre BHB: using loop workaround
[ 1233.366580] CPU0: Spectre BHB: using loop workaround
[ 1233.366815] CPU1: Spectre BHB: using loop workaround
[ 1233.405475] CPU1: Spectre BHB: using loop workaround
[ 1233.405874] CPU0: Spectre BHB: using loop workaround
[ 1233.406041] CPU1: Spectre BHB: using loop workaround
** 1 printk messages dropped **

This is significantly increasing the boot time and causing failures if 
the boot takes too long.

I see that Dmitry has posted a fix for one of these cases [0], but the 
above also needs to be fixed.

Thanks
Jon


[0] 
https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
-- 
nvpublic

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Ard Biesheuvel]

On Tue, 24 May 2022 at 16:50, Jon Hunter <jonathanh@nvidia.com> wrote:
>
> Hi Ard,
>
> On 28/03/2022 14:47, Ard Biesheuvel wrote:
> > The Spectre-BHB mitigations were inadvertently left disabled for
> > Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
> > that case. So fix that.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   arch/arm/mm/proc-v7-bugs.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> > index 06dbfb968182..fb9f3eb6bf48 100644
> > --- a/arch/arm/mm/proc-v7-bugs.c
> > +++ b/arch/arm/mm/proc-v7-bugs.c
> > @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
> >   {
> >       if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
> >               cpu_v7_spectre_v2_init();
> > +     cpu_v7_spectre_bhb_init();
> >   }
> >
> >   void cpu_v7_bugs_init(void)
>
>
> Since this patch has been merged, I am seeing a ton of messages when
> booting Linux on tegra124-jetson-tk1 ...
>

Thanks for the report. The good news is that the Spectre BHB
protection is finally taking effect on your system.

> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
> [ 1233.328700] CPU1: Spectre BHB: using loop workaround
> [ 1233.355477] CPU2: Spectre BHB: using loop workaround
> ** 7 printk messages dropped **
> [ 1233.366271] CPU0: Spectre BHB: using loop workaround
> [ 1233.366580] CPU0: Spectre BHB: using loop workaround
> [ 1233.366815] CPU1: Spectre BHB: using loop workaround
> [ 1233.405475] CPU1: Spectre BHB: using loop workaround
> [ 1233.405874] CPU0: Spectre BHB: using loop workaround
> [ 1233.406041] CPU1: Spectre BHB: using loop workaround
> ** 1 printk messages dropped **
>
> This is significantly increasing the boot time and causing failures if
> the boot takes too long.
>
> I see that Dmitry has posted a fix for one of these cases [0], but the
> above also needs to be fixed.
>

Agreed.

Do we have any idea why this init hook is being called so often? Is
this expected on systems that do agressive suspend/resume for power
management, or is there another existing issue here that is just
coming to light due to the newly added printk() ?

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Jon Hunter]

On 24/05/2022 16:21, Ard Biesheuvel wrote:
> On Tue, 24 May 2022 at 16:50, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> Hi Ard,
>>
>> On 28/03/2022 14:47, Ard Biesheuvel wrote:
>>> The Spectre-BHB mitigations were inadvertently left disabled for
>>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
>>> that case. So fix that.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    arch/arm/mm/proc-v7-bugs.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
>>> index 06dbfb968182..fb9f3eb6bf48 100644
>>> --- a/arch/arm/mm/proc-v7-bugs.c
>>> +++ b/arch/arm/mm/proc-v7-bugs.c
>>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
>>>    {
>>>        if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
>>>                cpu_v7_spectre_v2_init();
>>> +     cpu_v7_spectre_bhb_init();
>>>    }
>>>
>>>    void cpu_v7_bugs_init(void)
>>
>>
>> Since this patch has been merged, I am seeing a ton of messages when
>> booting Linux on tegra124-jetson-tk1 ...
>>
> 
> Thanks for the report. The good news is that the Spectre BHB
> protection is finally taking effect on your system.
> 
>> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
>> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
>> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
>> [ 1233.328700] CPU1: Spectre BHB: using loop workaround
>> [ 1233.355477] CPU2: Spectre BHB: using loop workaround
>> ** 7 printk messages dropped **
>> [ 1233.366271] CPU0: Spectre BHB: using loop workaround
>> [ 1233.366580] CPU0: Spectre BHB: using loop workaround
>> [ 1233.366815] CPU1: Spectre BHB: using loop workaround
>> [ 1233.405475] CPU1: Spectre BHB: using loop workaround
>> [ 1233.405874] CPU0: Spectre BHB: using loop workaround
>> [ 1233.406041] CPU1: Spectre BHB: using loop workaround
>> ** 1 printk messages dropped **
>>
>> This is significantly increasing the boot time and causing failures if
>> the boot takes too long.
>>
>> I see that Dmitry has posted a fix for one of these cases [0], but the
>> above also needs to be fixed.
>>
> 
> Agreed.
> 
> Do we have any idea why this init hook is being called so often? Is
> this expected on systems that do agressive suspend/resume for power
> management, or is there another existing issue here that is just
> coming to light due to the newly added printk() ?


So far I have not dug any further, but I do know it is not doing any 
aggressive suspend/resume. From what Dmitry indicated he saw this during 
CPU idle transitions and this platform would definitely by exercising 
CPU idle.

Jon

-- 
nvpublic

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Russell King (Oracle)]

On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
> Hi Ard,
> 
> On 28/03/2022 14:47, Ard Biesheuvel wrote:
> > The Spectre-BHB mitigations were inadvertently left disabled for
> > Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
> > that case. So fix that.
> > 
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >   arch/arm/mm/proc-v7-bugs.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> > index 06dbfb968182..fb9f3eb6bf48 100644
> > --- a/arch/arm/mm/proc-v7-bugs.c
> > +++ b/arch/arm/mm/proc-v7-bugs.c
> > @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
> >   {
> >   	if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
> >   		cpu_v7_spectre_v2_init();
> > +	cpu_v7_spectre_bhb_init();
> >   }
> >   void cpu_v7_bugs_init(void)
> 
> 
> Since this patch has been merged, I am seeing a ton of messages when booting
> Linux on tegra124-jetson-tk1 ...
> 
> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
> [ 1233.328270] CPU1: Spectre BHB: using loop workaround

Now that you mention this, I vaguely remember some email on the list a
while ago about this being caused by something like cpuidle - but I'm
unable to find it now.

> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/

That was probably it.

We can't really do this for the other print, because the system status
can change as a result of CPUs being brought online. :(

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Jon Hunter]

On 24/05/2022 18:03, Russell King (Oracle) wrote:
> On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
>> Hi Ard,
>>
>> On 28/03/2022 14:47, Ard Biesheuvel wrote:
>>> The Spectre-BHB mitigations were inadvertently left disabled for
>>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
>>> that case. So fix that.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    arch/arm/mm/proc-v7-bugs.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
>>> index 06dbfb968182..fb9f3eb6bf48 100644
>>> --- a/arch/arm/mm/proc-v7-bugs.c
>>> +++ b/arch/arm/mm/proc-v7-bugs.c
>>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
>>>    {
>>>    	if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
>>>    		cpu_v7_spectre_v2_init();
>>> +	cpu_v7_spectre_bhb_init();
>>>    }
>>>    void cpu_v7_bugs_init(void)
>>
>>
>> Since this patch has been merged, I am seeing a ton of messages when booting
>> Linux on tegra124-jetson-tk1 ...
>>
>> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
>> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
>> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
> 
> Now that you mention this, I vaguely remember some email on the list a
> while ago about this being caused by something like cpuidle - but I'm
> unable to find it now.
> 
>> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
> 
> That was probably it.


I am seeing ...

[    4.415167] CPU0: Spectre BHB: using loop workaround
[    4.417621] [<c01109a0>] (unwind_backtrace) from [<c010b7ac>] (show_stack+0x10/0x14)
[    4.430291] [<c010b7ac>] (show_stack) from [<c09c2b38>] (dump_stack+0xc0/0xd4)
[    4.437512] [<c09c2b38>] (dump_stack) from [<c011a6c8>] (cpu_v7_spectre_bhb_init+0xd8/0x190)
[    4.445943] [<c011a6c8>] (cpu_v7_spectre_bhb_init) from [<c010dee8>] (cpu_suspend+0xac/0xc8)
[    4.454377] [<c010dee8>] (cpu_suspend) from [<c011e7e4>] (tegra114_idle_power_down+0x74/0x78)
[    4.462898] [<c011e7e4>] (tegra114_idle_power_down) from [<c06d3b44>] (cpuidle_enter_state+0x130/0x524)
[    4.472286] [<c06d3b44>] (cpuidle_enter_state) from [<c0164a30>] (do_idle+0x1b0/0x200)
[    4.480199] [<c0164a30>] (do_idle) from [<c0164d28>] (cpu_startup_entry+0x18/0x1c)
[    4.487762] [<c0164d28>] (cpu_startup_entry) from [<801018cc>] (0x801018cc)


So definitely CPU idle.
  
> We can't really do this for the other print, because the system status
> can change as a result of CPUs being brought online. :(


How about making this a pr_debug as opposed to pr_info?

Jon

-- 
nvpublic

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Ard Biesheuvel]

On Tue, 24 May 2022 at 19:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 24/05/2022 18:03, Russell King (Oracle) wrote:
> > On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
> >> Hi Ard,
> >>
> >> On 28/03/2022 14:47, Ard Biesheuvel wrote:
> >>> The Spectre-BHB mitigations were inadvertently left disabled for
> >>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
> >>> that case. So fix that.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>    arch/arm/mm/proc-v7-bugs.c | 1 +
> >>>    1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> >>> index 06dbfb968182..fb9f3eb6bf48 100644
> >>> --- a/arch/arm/mm/proc-v7-bugs.c
> >>> +++ b/arch/arm/mm/proc-v7-bugs.c
> >>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
> >>>    {
> >>>     if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
> >>>             cpu_v7_spectre_v2_init();
> >>> +   cpu_v7_spectre_bhb_init();
> >>>    }
> >>>    void cpu_v7_bugs_init(void)
> >>
> >>
> >> Since this patch has been merged, I am seeing a ton of messages when booting
> >> Linux on tegra124-jetson-tk1 ...
> >>
> >> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
> >> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
> >> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
> >
> > Now that you mention this, I vaguely remember some email on the list a
> > while ago about this being caused by something like cpuidle - but I'm
> > unable to find it now.
> >
> >> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
> >
> > That was probably it.
>
>
> I am seeing ...
>
> [    4.415167] CPU0: Spectre BHB: using loop workaround
> [    4.417621] [<c01109a0>] (unwind_backtrace) from [<c010b7ac>] (show_stack+0x10/0x14)
> [    4.430291] [<c010b7ac>] (show_stack) from [<c09c2b38>] (dump_stack+0xc0/0xd4)
> [    4.437512] [<c09c2b38>] (dump_stack) from [<c011a6c8>] (cpu_v7_spectre_bhb_init+0xd8/0x190)
> [    4.445943] [<c011a6c8>] (cpu_v7_spectre_bhb_init) from [<c010dee8>] (cpu_suspend+0xac/0xc8)
> [    4.454377] [<c010dee8>] (cpu_suspend) from [<c011e7e4>] (tegra114_idle_power_down+0x74/0x78)
> [    4.462898] [<c011e7e4>] (tegra114_idle_power_down) from [<c06d3b44>] (cpuidle_enter_state+0x130/0x524)
> [    4.472286] [<c06d3b44>] (cpuidle_enter_state) from [<c0164a30>] (do_idle+0x1b0/0x200)
> [    4.480199] [<c0164a30>] (do_idle) from [<c0164d28>] (cpu_startup_entry+0x18/0x1c)
> [    4.487762] [<c0164d28>] (cpu_startup_entry) from [<801018cc>] (0x801018cc)
>
>
> So definitely CPU idle.
>
> > We can't really do this for the other print, because the system status
> > can change as a result of CPUs being brought online. :(
>
>
> How about making this a pr_debug as opposed to pr_info?
>

We should pull the pr_info() into the conditional so that it only
executes the first time around:

--- a/arch/arm/mm/proc-v7-bugs.c
+++ b/arch/arm/mm/proc-v7-bugs.c
@@ -209,10 +209,10 @@ static int spectre_bhb_install_workaround(int method)
                        return SPECTRE_VULNERABLE;

                spectre_bhb_method = method;
-       }

-       pr_info("CPU%u: Spectre BHB: using %s workaround\n",
-               smp_processor_id(), spectre_bhb_method_name(method));
+               pr_info("CPU%u: Spectre BHB: using %s workaround\n",
+                       smp_processor_id(), spectre_bhb_method_name(method));
+       }

        return SPECTRE_MITIGATED;
 }

If you can confirm that this fixes the issue, I can send it out as a
proper patch.

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Jon Hunter]

On 25/05/2022 08:09, Ard Biesheuvel wrote:
> On Tue, 24 May 2022 at 19:49, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 24/05/2022 18:03, Russell King (Oracle) wrote:
>>> On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
>>>> Hi Ard,
>>>>
>>>> On 28/03/2022 14:47, Ard Biesheuvel wrote:
>>>>> The Spectre-BHB mitigations were inadvertently left disabled for
>>>>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
>>>>> that case. So fix that.
>>>>>
>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>> ---
>>>>>     arch/arm/mm/proc-v7-bugs.c | 1 +
>>>>>     1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
>>>>> index 06dbfb968182..fb9f3eb6bf48 100644
>>>>> --- a/arch/arm/mm/proc-v7-bugs.c
>>>>> +++ b/arch/arm/mm/proc-v7-bugs.c
>>>>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
>>>>>     {
>>>>>      if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
>>>>>              cpu_v7_spectre_v2_init();
>>>>> +   cpu_v7_spectre_bhb_init();
>>>>>     }
>>>>>     void cpu_v7_bugs_init(void)
>>>>
>>>>
>>>> Since this patch has been merged, I am seeing a ton of messages when booting
>>>> Linux on tegra124-jetson-tk1 ...
>>>>
>>>> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
>>>> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
>>>> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
>>>
>>> Now that you mention this, I vaguely remember some email on the list a
>>> while ago about this being caused by something like cpuidle - but I'm
>>> unable to find it now.
>>>
>>>> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
>>>
>>> That was probably it.
>>
>>
>> I am seeing ...
>>
>> [    4.415167] CPU0: Spectre BHB: using loop workaround
>> [    4.417621] [<c01109a0>] (unwind_backtrace) from [<c010b7ac>] (show_stack+0x10/0x14)
>> [    4.430291] [<c010b7ac>] (show_stack) from [<c09c2b38>] (dump_stack+0xc0/0xd4)
>> [    4.437512] [<c09c2b38>] (dump_stack) from [<c011a6c8>] (cpu_v7_spectre_bhb_init+0xd8/0x190)
>> [    4.445943] [<c011a6c8>] (cpu_v7_spectre_bhb_init) from [<c010dee8>] (cpu_suspend+0xac/0xc8)
>> [    4.454377] [<c010dee8>] (cpu_suspend) from [<c011e7e4>] (tegra114_idle_power_down+0x74/0x78)
>> [    4.462898] [<c011e7e4>] (tegra114_idle_power_down) from [<c06d3b44>] (cpuidle_enter_state+0x130/0x524)
>> [    4.472286] [<c06d3b44>] (cpuidle_enter_state) from [<c0164a30>] (do_idle+0x1b0/0x200)
>> [    4.480199] [<c0164a30>] (do_idle) from [<c0164d28>] (cpu_startup_entry+0x18/0x1c)
>> [    4.487762] [<c0164d28>] (cpu_startup_entry) from [<801018cc>] (0x801018cc)
>>
>>
>> So definitely CPU idle.
>>
>>> We can't really do this for the other print, because the system status
>>> can change as a result of CPUs being brought online. :(
>>
>>
>> How about making this a pr_debug as opposed to pr_info?
>>
> 
> We should pull the pr_info() into the conditional so that it only
> executes the first time around:
> 
> --- a/arch/arm/mm/proc-v7-bugs.c
> +++ b/arch/arm/mm/proc-v7-bugs.c
> @@ -209,10 +209,10 @@ static int spectre_bhb_install_workaround(int method)
>                          return SPECTRE_VULNERABLE;
> 
>                  spectre_bhb_method = method;
> -       }
> 
> -       pr_info("CPU%u: Spectre BHB: using %s workaround\n",
> -               smp_processor_id(), spectre_bhb_method_name(method));
> +               pr_info("CPU%u: Spectre BHB: using %s workaround\n",
> +                       smp_processor_id(), spectre_bhb_method_name(method));
> +       }
> 
>          return SPECTRE_MITIGATED;
>   }
> 
> If you can confirm that this fixes the issue, I can send it out as a
> proper patch.

That works for me.

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks
Jon

-- 
nvpublic

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Ard Biesheuvel]

On Wed, 25 May 2022 at 12:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 25/05/2022 08:09, Ard Biesheuvel wrote:
> > On Tue, 24 May 2022 at 19:49, Jon Hunter <jonathanh@nvidia.com> wrote:
> >>
> >>
> >> On 24/05/2022 18:03, Russell King (Oracle) wrote:
> >>> On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
> >>>> Hi Ard,
> >>>>
> >>>> On 28/03/2022 14:47, Ard Biesheuvel wrote:
> >>>>> The Spectre-BHB mitigations were inadvertently left disabled for
> >>>>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
> >>>>> that case. So fix that.
> >>>>>
> >>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>>>> ---
> >>>>>     arch/arm/mm/proc-v7-bugs.c | 1 +
> >>>>>     1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> >>>>> index 06dbfb968182..fb9f3eb6bf48 100644
> >>>>> --- a/arch/arm/mm/proc-v7-bugs.c
> >>>>> +++ b/arch/arm/mm/proc-v7-bugs.c
> >>>>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
> >>>>>     {
> >>>>>      if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
> >>>>>              cpu_v7_spectre_v2_init();
> >>>>> +   cpu_v7_spectre_bhb_init();
> >>>>>     }
> >>>>>     void cpu_v7_bugs_init(void)
> >>>>
> >>>>
> >>>> Since this patch has been merged, I am seeing a ton of messages when booting
> >>>> Linux on tegra124-jetson-tk1 ...
> >>>>
> >>>> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
> >>>> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
> >>>> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
> >>>
> >>> Now that you mention this, I vaguely remember some email on the list a
> >>> while ago about this being caused by something like cpuidle - but I'm
> >>> unable to find it now.
> >>>
> >>>> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
> >>>
> >>> That was probably it.
> >>
> >>
> >> I am seeing ...
> >>
> >> [    4.415167] CPU0: Spectre BHB: using loop workaround
> >> [    4.417621] [<c01109a0>] (unwind_backtrace) from [<c010b7ac>] (show_stack+0x10/0x14)
> >> [    4.430291] [<c010b7ac>] (show_stack) from [<c09c2b38>] (dump_stack+0xc0/0xd4)
> >> [    4.437512] [<c09c2b38>] (dump_stack) from [<c011a6c8>] (cpu_v7_spectre_bhb_init+0xd8/0x190)
> >> [    4.445943] [<c011a6c8>] (cpu_v7_spectre_bhb_init) from [<c010dee8>] (cpu_suspend+0xac/0xc8)
> >> [    4.454377] [<c010dee8>] (cpu_suspend) from [<c011e7e4>] (tegra114_idle_power_down+0x74/0x78)
> >> [    4.462898] [<c011e7e4>] (tegra114_idle_power_down) from [<c06d3b44>] (cpuidle_enter_state+0x130/0x524)
> >> [    4.472286] [<c06d3b44>] (cpuidle_enter_state) from [<c0164a30>] (do_idle+0x1b0/0x200)
> >> [    4.480199] [<c0164a30>] (do_idle) from [<c0164d28>] (cpu_startup_entry+0x18/0x1c)
> >> [    4.487762] [<c0164d28>] (cpu_startup_entry) from [<801018cc>] (0x801018cc)
> >>
> >>
> >> So definitely CPU idle.
> >>
> >>> We can't really do this for the other print, because the system status
> >>> can change as a result of CPUs being brought online. :(
> >>
> >>
> >> How about making this a pr_debug as opposed to pr_info?
> >>
> >
> > We should pull the pr_info() into the conditional so that it only
> > executes the first time around:
> >
> > --- a/arch/arm/mm/proc-v7-bugs.c
> > +++ b/arch/arm/mm/proc-v7-bugs.c
> > @@ -209,10 +209,10 @@ static int spectre_bhb_install_workaround(int method)
> >                          return SPECTRE_VULNERABLE;
> >
> >                  spectre_bhb_method = method;
> > -       }
> >
> > -       pr_info("CPU%u: Spectre BHB: using %s workaround\n",
> > -               smp_processor_id(), spectre_bhb_method_name(method));
> > +               pr_info("CPU%u: Spectre BHB: using %s workaround\n",
> > +                       smp_processor_id(), spectre_bhb_method_name(method));
> > +       }
> >
> >          return SPECTRE_MITIGATED;
> >   }
> >
> > If you can confirm that this fixes the issue, I can send it out as a
> > proper patch.
>
> That works for me.
>
> Tested-by: Jon Hunter <jonathanh@nvidia.com>
>

Thanks Jon.

The only downside here is that the message is only printed once for
all CPUs, unless we are enabling two different methods on ab
big.little system, in which case we print an error.

Personally, I think that is fine, but I'll need to tweak the message
to clarify this.

Russell, any thoughts?

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Jon Hunter]

On 24/05/2022 18:03, Russell King (Oracle) wrote:
> On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
>> Hi Ard,
>>
>> On 28/03/2022 14:47, Ard Biesheuvel wrote:
>>> The Spectre-BHB mitigations were inadvertently left disabled for
>>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
>>> that case. So fix that.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>    arch/arm/mm/proc-v7-bugs.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
>>> index 06dbfb968182..fb9f3eb6bf48 100644
>>> --- a/arch/arm/mm/proc-v7-bugs.c
>>> +++ b/arch/arm/mm/proc-v7-bugs.c
>>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
>>>    {
>>>    	if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
>>>    		cpu_v7_spectre_v2_init();
>>> +	cpu_v7_spectre_bhb_init();
>>>    }
>>>    void cpu_v7_bugs_init(void)
>>
>>
>> Since this patch has been merged, I am seeing a ton of messages when booting
>> Linux on tegra124-jetson-tk1 ...
>>
>> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
>> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
>> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
> 
> Now that you mention this, I vaguely remember some email on the list a
> while ago about this being caused by something like cpuidle - but I'm
> unable to find it now.
> 
>> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
> 
> That was probably it.
> 
> We can't really do this for the other print, because the system status
> can change as a result of CPUs being brought online. :(
> 

Does it make sense to only print the message if/when the method changes 
as opposed to every time the CPUs are brought online? That way, there 
would still be at least one print showing the current method. I believe 
that is what Ard had proposed.

Jon

-- 
nvpublic

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 16:30, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 24/05/2022 18:03, Russell King (Oracle) wrote:
> > On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
> >> Hi Ard,
> >>
> >> On 28/03/2022 14:47, Ard Biesheuvel wrote:
> >>> The Spectre-BHB mitigations were inadvertently left disabled for
> >>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
> >>> that case. So fix that.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>> ---
> >>>    arch/arm/mm/proc-v7-bugs.c | 1 +
> >>>    1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> >>> index 06dbfb968182..fb9f3eb6bf48 100644
> >>> --- a/arch/arm/mm/proc-v7-bugs.c
> >>> +++ b/arch/arm/mm/proc-v7-bugs.c
> >>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
> >>>    {
> >>>     if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
> >>>             cpu_v7_spectre_v2_init();
> >>> +   cpu_v7_spectre_bhb_init();
> >>>    }
> >>>    void cpu_v7_bugs_init(void)
> >>
> >>
> >> Since this patch has been merged, I am seeing a ton of messages when booting
> >> Linux on tegra124-jetson-tk1 ...
> >>
> >> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
> >> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
> >> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
> >
> > Now that you mention this, I vaguely remember some email on the list a
> > while ago about this being caused by something like cpuidle - but I'm
> > unable to find it now.
> >
> >> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
> >
> > That was probably it.
> >
> > We can't really do this for the other print, because the system status
> > can change as a result of CPUs being brought online. :(
> >
>
> Does it make sense to only print the message if/when the method changes
> as opposed to every time the CPUs are brought online? That way, there
> would still be at least one print showing the current method. I believe
> that is what Ard had proposed.
>

A fix for this issue is already in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=bafa10435c4f34f4b9bda8fc7ee6e4330ebca3ea

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Jon Hunter]

On 07/06/2022 15:32, Ard Biesheuvel wrote:
> On Tue, 7 Jun 2022 at 16:30, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 24/05/2022 18:03, Russell King (Oracle) wrote:
>>> On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
>>>> Hi Ard,
>>>>
>>>> On 28/03/2022 14:47, Ard Biesheuvel wrote:
>>>>> The Spectre-BHB mitigations were inadvertently left disabled for
>>>>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
>>>>> that case. So fix that.
>>>>>
>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>> ---
>>>>>     arch/arm/mm/proc-v7-bugs.c | 1 +
>>>>>     1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
>>>>> index 06dbfb968182..fb9f3eb6bf48 100644
>>>>> --- a/arch/arm/mm/proc-v7-bugs.c
>>>>> +++ b/arch/arm/mm/proc-v7-bugs.c
>>>>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
>>>>>     {
>>>>>      if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
>>>>>              cpu_v7_spectre_v2_init();
>>>>> +   cpu_v7_spectre_bhb_init();
>>>>>     }
>>>>>     void cpu_v7_bugs_init(void)
>>>>
>>>>
>>>> Since this patch has been merged, I am seeing a ton of messages when booting
>>>> Linux on tegra124-jetson-tk1 ...
>>>>
>>>> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
>>>> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
>>>> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
>>>
>>> Now that you mention this, I vaguely remember some email on the list a
>>> while ago about this being caused by something like cpuidle - but I'm
>>> unable to find it now.
>>>
>>>> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
>>>
>>> That was probably it.
>>>
>>> We can't really do this for the other print, because the system status
>>> can change as a result of CPUs being brought online. :(
>>>
>>
>> Does it make sense to only print the message if/when the method changes
>> as opposed to every time the CPUs are brought online? That way, there
>> would still be at least one print showing the current method. I believe
>> that is what Ard had proposed.
>>
> 
> A fix for this issue is already in linux-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=bafa10435c4f34f4b9bda8fc7ee6e4330ebca3ea


Ah wonderful! Sorry I had missed that. Once merged can we pull into 
stable as well?

Jon

-- 
nvpublic

Re: [PATCH 1/6] ARM: spectre-bhb: enable for Cortex-A15

[Ard Biesheuvel]

On Tue, 7 Jun 2022 at 16:35, Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 07/06/2022 15:32, Ard Biesheuvel wrote:
> > On Tue, 7 Jun 2022 at 16:30, Jon Hunter <jonathanh@nvidia.com> wrote:
> >>
> >>
> >> On 24/05/2022 18:03, Russell King (Oracle) wrote:
> >>> On Tue, May 24, 2022 at 03:50:17PM +0100, Jon Hunter wrote:
> >>>> Hi Ard,
> >>>>
> >>>> On 28/03/2022 14:47, Ard Biesheuvel wrote:
> >>>>> The Spectre-BHB mitigations were inadvertently left disabled for
> >>>>> Cortex-A15, due to the fact that cpu_v7_bugs_init() is not called in
> >>>>> that case. So fix that.
> >>>>>
> >>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>>>> ---
> >>>>>     arch/arm/mm/proc-v7-bugs.c | 1 +
> >>>>>     1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c
> >>>>> index 06dbfb968182..fb9f3eb6bf48 100644
> >>>>> --- a/arch/arm/mm/proc-v7-bugs.c
> >>>>> +++ b/arch/arm/mm/proc-v7-bugs.c
> >>>>> @@ -288,6 +288,7 @@ void cpu_v7_ca15_ibe(void)
> >>>>>     {
> >>>>>      if (check_spectre_auxcr(this_cpu_ptr(&spectre_warned), BIT(0)))
> >>>>>              cpu_v7_spectre_v2_init();
> >>>>> +   cpu_v7_spectre_bhb_init();
> >>>>>     }
> >>>>>     void cpu_v7_bugs_init(void)
> >>>>
> >>>>
> >>>> Since this patch has been merged, I am seeing a ton of messages when booting
> >>>> Linux on tegra124-jetson-tk1 ...
> >>>>
> >>>> [ 1233.327547] CPU0: Spectre BHB: using loop workaround
> >>>> [ 1233.327795] CPU1: Spectre BHB: using loop workaround
> >>>> [ 1233.328270] CPU1: Spectre BHB: using loop workaround
> >>>
> >>> Now that you mention this, I vaguely remember some email on the list a
> >>> while ago about this being caused by something like cpuidle - but I'm
> >>> unable to find it now.
> >>>
> >>>> [0] https://lore.kernel.org/linux-arm-kernel/20220519161310.1489625-1-dmitry.osipenko@collabora.com/T/
> >>>
> >>> That was probably it.
> >>>
> >>> We can't really do this for the other print, because the system status
> >>> can change as a result of CPUs being brought online. :(
> >>>
> >>
> >> Does it make sense to only print the message if/when the method changes
> >> as opposed to every time the CPUs are brought online? That way, there
> >> would still be at least one print showing the current method. I believe
> >> that is what Ard had proposed.
> >>
> >
> > A fix for this issue is already in linux-next:
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=bafa10435c4f34f4b9bda8fc7ee6e4330ebca3ea
>
>
> Ah wonderful! Sorry I had missed that. Once merged can we pull into
> stable as well?
>

Yes. It has a fixes: tag so it will most likely get picked up
automatically, but feel free to remind the -stable maintainers once
this patch is merged by Linus.