On Wed, 25 May 2022 at 12:48, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
>
>
> On 25/05/2022 08:09, Ard Biesheuvel wrote:
> > On Tue, 24 May 2022 at 19:49, Jon Hunter <jonathanh@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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@xxxxxxxxxxxxx/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@xxxxxxxxxx>
>
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?
