Re: [RFC PATCH] arch: ARM64: add isb before enable pan

[Zhaoyang Huang <huangzhaoyang@gmail.com>]

On Mon, Oct 11, 2021 at 5:38 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi,
>
> On Fri, Oct 08, 2021 at 04:34:12PM +0800, Zhaoyang Huang wrote:
> > On Fri, Oct 8, 2021 at 4:01 PM Will Deacon <will@kernel.org> wrote:
> > > On Fri, Oct 08, 2021 at 02:07:49PM +0800, Huangzhaoyang wrote:
> > > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > > >
> > > > set_pstate_pan failure is observed in an ARM64 system occasionaly on a reboot
> > > > test, which can be work around by a msleep on the sw context. We assume
> > > > suspicious on disorder of previous instr of disabling SW_PAN and add an isb here.
> > > >
> > > > PS:
> > > > The bootup test failed with a invalid TTBR1_EL1 that equals 0x34000000, which is
> > > > alike racing between on chip PAN and SW_PAN.
> > >
> > > Sorry, but I'm struggling to understand the problem here. Please could you
> > > explain it in more detail?
> > >
> > >   - Why does a TTBR1_EL1 value of `0x34000000` indicate a race?
> > >   - Can you explain the race that you think might be occurring?
> > >   - Why does an ISB prevent the race?
> > Please find panic logs[1], related codes[2], sample of debug patch[3]
> > below. TTBR1_EL1 equals 0x34000000 when panic
>
> Just to check, how do you know the value of TTBR1_EL1 was 0x34000000?
> That isn't in the log sample below -- was that from the output of
> show_pte(), an external debugger, or something else?
>
> I'm assuming from the "(ptrval)" bits below that can't have been from
> show_pte().
>
> > and can NOT be captured
> > by the debug patch during retest (all entrances that msr ttbr1_el1 are
> > under watch) which should work. Adding ISB here to prevent race on
> > TTBR1 from previous access of sysregs which can affect the msr
> > result(the test is still ongoing). Could the race be
> > ARM64_HAS_PAN(automated by core) and SW_PAN.
> >
> > [1]
> > [    0.348000]  [0:    migration/0:   11] Synchronous External Abort:
> > level 1 (translation table walk) (0x96000055) at 0xffffffc000e06004
> > [    0.352000]  [0:    migration/0:   11] Internal error: : 96000055
> > [#1] PREEMPT SMP
> > [    0.352000]  [0:    migration/0:   11] Modules linked in:
> > [    0.352000]  [0:    migration/0:   11] Process migration/0 (pid:
> > 11, stack limit = 0x        (ptrval))
> > [    0.352000]  [0:    migration/0:   11] CPU: 0 PID: 11 Comm:
> > migration/0 Tainted: G S
>
> Assuming I've read the `taint_flags` table correctly, that 'S' is
> `TAINT_CPU_OUT_OF_SPEC`, for which we should dump warnings for at boot
> time. The 'G' indicates the absence of proprietary modules.
>
> Can you provide a full dmesg for a failed boot, please?
>
> Have you made any changes to arch/arm64/kernel/cpufeature.c?
>
> Are you able to test with a mainline kernel?
>
> > 4.14.199-22631304-abA035FXXU0AUJ4_T4 #2
> >
> > [    0.352000]  [0:    migration/0:   11] Hardware name: Spreadtrum
> > UMS9230 1H10 SoC (DT)
> > [    0.352000]  [0:    migration/0:   11] task:         (ptrval)
> > task.stack:         (ptrval)
> > [    0.352000]  [0:    migration/0:   11] pc : patch_alternative+0x68/0x27c
> > [    0.352000]  [0:    migration/0:   11] lr :
> > __apply_alternatives.llvm.7450387295891320208+0x60/0x160
> >
> > [2]
> > __apply_alternatives
> >    for()
> >        patch_alternative    <----panic here in the 2nd round of loop
> > after invoking flush_icache_range
> >        flush_icache_range
> >
> > [3]
> > sub \tmp1, \tmp1, #SWAPPER_DIR_SIZE
> > + tst     \tmp1, #0xffff80000000 // check ttbr1_el1 valid
> > +    b.le    .
>
> What are you trying to detect for here? This is testing both the ASID
> and BADDR[47] bits, so I don;t understand the rationale.
>
> Thanks,
> Mark.
this issue is fixed by the patch 'arm64: Avoid flush_icache_range() in
alternatives patching code(429388682dc266e7a693f9c27e3aabd341d55343)'.
thanks
>
> > msr ttbr1_el1, \tmp1 // set reserved ASID
> >
> > >
> > > > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com>
> > > > ---
> > > >  arch/arm64/kernel/cpufeature.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index efed283..3c0de0d 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -1663,6 +1663,7 @@ static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
> > > >       WARN_ON_ONCE(in_interrupt());
> > > >
> > > >       sysreg_clear_set(sctlr_el1, SCTLR_EL1_SPAN, 0);
> > > > +     isb();
> > > >       set_pstate_pan(1);
> > >
> > > SCTLR_EL1.SPAN only affects the PAN behaviour on taking an exception, which
> > > is itself a context-synchronizing event, so I can't see why the ISB makes
> > > any difference here (at least, for the purposes of PAN).
> > >
> > > Thanks,
> > >
> > > Will