| Message ID | 1633673269-15048-1-git-send-email-huangzhaoyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] arch: ARM64: add isb before enable pan | expand |
Hi, 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? > 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
On Fri, Oct 8, 2021 at 4:01 PM Will Deacon <will@kernel.org> wrote: > > Hi, > > 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 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 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 . 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
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 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. Can you please change the ARM64_HAS_PAN type to ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE? I wonder whether system_uses_ttbr0_pan() changes its output when all CPUs had been brought up and system_uses_hw_pan() returns true.
On Fri, Oct 8, 2021 at 4:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > 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 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. > > Can you please change the ARM64_HAS_PAN type to > ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE? I wonder whether > system_uses_ttbr0_pan() changes its output when all CPUs had been > brought up and system_uses_hw_pan() returns true. ok, thanks. We will try. Is it a workaround for known defect? > > -- > Catalin
On Fri, Oct 08, 2021 at 04:55:05PM +0800, Zhaoyang Huang wrote: > On Fri, Oct 8, 2021 at 4:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > 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 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. > > > > Can you please change the ARM64_HAS_PAN type to > > ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE? I wonder whether > > system_uses_ttbr0_pan() changes its output when all CPUs had been > > brought up and system_uses_hw_pan() returns true. > > ok, thanks. We will try. Is it a workaround for known defect? No, other than the potential kernel bug you reported.
On Fri, Oct 8, 2021 at 5:07 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Fri, Oct 08, 2021 at 04:55:05PM +0800, Zhaoyang Huang wrote: > > On Fri, Oct 8, 2021 at 4:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > 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 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. > > > > > > Can you please change the ARM64_HAS_PAN type to > > > ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE? I wonder whether > > > system_uses_ttbr0_pan() changes its output when all CPUs had been > > > brought up and system_uses_hw_pan() returns true. > > > > ok, thanks. We will try. Is it a workaround for known defect? > > No, other than the potential kernel bug you reported. Changing the type to ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE doesn't work for this issue. > > -- > Catalin
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
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
On Mon, Oct 11, 2021 at 07:08:00PM +0800, Zhaoyang Huang wrote: > On Mon, Oct 11, 2021 at 5:38 PM Mark Rutland <mark.rutland@arm.com> wrote: > > 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 Ah, thanks for this. So this is because in arch/arm64/mm/cache.S we do (abbreviated): | ENTRY(flush_icache_range) | /* FALLTHROUGH */ | ENTRY(__flush_cache_user_range) | uaccess_ttbr0_enable x2, x3, x4 | | ... | | uaccess_ttbr0_disable x1, x2 | ret | ENDPROC(flush_icache_range) ... and even if I-caches are coherent and we don't execute junk for something we've patched but not invalidated, since we patch each alternative site in turn we can end up with unbalanced calls to enable/disable across a number of patches. It looks like 429388682dc266e7a693f9c27e3aabd341d55343 isn't in the current linux-4.14.y, so we should post a backported version. From local testing, that applies trivially. Who wants to post that to stable? I can do if people want. Thanks, Mark.
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); } #endif /* CONFIG_ARM64_PAN */