From: Mark Rutland <mark.rutland@arm.com>
To: Zhaoyang Huang <huangzhaoyang@gmail.com>
Cc: Will Deacon <will@kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Ionela Voinescu <ionela.voinescu@arm.com>,
Quentin Perret <qperret@google.com>,
Vladimir Murzin <vladimir.murzin@arm.com>,
linux-arm-kernel@lists.infradead.org,
Zhaoyang Huang <zhaoyang.huang@unisoc.com>,
LKML <linux-kernel@vger.kernel.org>, Ke Wang <ke.wang@unisoc.com>,
ping.zhou1@unisoc.com
Subject: Re: [RFC PATCH] arch: ARM64: add isb before enable pan
Date: Mon, 11 Oct 2021 10:38:18 +0100 [thread overview]
Message-ID: <20211011093803.GA1421@C02TD0UTHF1T.local> (raw)
In-Reply-To: <CAGWkznEh6RuEgxTH-vHB1kMjb0CERigqpL4+f0Lg1X1_VBQuMQ@mail.gmail.com>
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.
> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-10-11 9:41 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-08 6:07 [RFC PATCH] arch: ARM64: add isb before enable pan Huangzhaoyang
2021-10-08 8:01 ` Will Deacon
2021-10-08 8:34 ` Zhaoyang Huang
2021-10-08 8:45 ` Catalin Marinas
2021-10-08 8:55 ` Zhaoyang Huang
2021-10-08 9:07 ` Catalin Marinas
2021-10-11 2:49 ` Zhaoyang Huang
2021-10-11 9:38 ` Mark Rutland [this message]
2021-10-11 11:08 ` Zhaoyang Huang
2021-10-11 12:15 ` Mark Rutland
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211011093803.GA1421@C02TD0UTHF1T.local \
--to=mark.rutland@arm.com \
--cc=catalin.marinas@arm.com \
--cc=huangzhaoyang@gmail.com \
--cc=ionela.voinescu@arm.com \
--cc=ke.wang@unisoc.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ping.zhou1@unisoc.com \
--cc=qperret@google.com \
--cc=suzuki.poulose@arm.com \
--cc=vladimir.murzin@arm.com \
--cc=will@kernel.org \
--cc=zhaoyang.huang@unisoc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).