Hi Alex,
Below two patches are important for CPU's suspend/resume flow; These two patches have been merged into mainline kernel already.
If without Sudeep's patch, cpu will get wrong context when it returns back and finally introduce system hang issue. So patch 1 is fatal.
For Will Deacon's patch, I have not seen obvious issue if without it on Hikey. But for more safe, also ported this patch to fetch correct instructions from PoU but discard potential stale instructions which fetched from PoC.
Sudeep Holla (1): arm64: restore cpu suspend/resume functionality
Will Deacon (1): arm64: mm: ensure patched kernel text is fetched from PoU
arch/arm64/kernel/head.S | 8 ++++++++ arch/arm64/kernel/sleep.S | 9 ++++++++- arch/arm64/mm/proc.S | 1 - 3 files changed, 16 insertions(+), 2 deletions(-)
From: Sudeep Holla sudeep.holla@arm.com
Commit 4b3dc9679cf7 ("arm64: force CONFIG_SMP=y and remove redundant #ifdefs") accidentally retained code for !CONFIG_SMP in cpu_resume function. This resulted in the hash index being zeroed in x7 after proper computation, which is then used to get the cpu context pointer while resuming.
This patch removes the remanant code and restores back the cpu suspend/ resume functionality.
Fixes: 4b3dc9679cf7 ("arm64: force CONFIG_SMP=y and remove redundant #ifdefs") Signed-off-by: Sudeep Holla sudeep.holla@arm.com Cc: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Cc: Catalin Marinas catalin.marinas@arm.com Cc: Will Deacon will.deacon@arm.com Signed-off-by: Will Deacon will.deacon@arm.com --- arch/arm64/kernel/sleep.S | 1 - 1 file changed, 1 deletion(-)
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S index 5686a3a..fb3128e 100644 --- a/arch/arm64/kernel/sleep.S +++ b/arch/arm64/kernel/sleep.S @@ -158,7 +158,6 @@ ENTRY(cpu_resume) ldp w5, w6, [x8, #(MPIDR_HASH_SHIFTS + 8)] compute_mpidr_hash x7, x3, x4, x5, x6, x1, x2 /* x7 contains hash index, let's use it to grab context pointer */ - mov x7, xzr ldr_l x0, sleep_save_sp + SLEEP_SAVE_SP_PHYS ldr x0, [x0, x7, lsl #3] /* load sp from context */
From: Will Deacon will.deacon@arm.com
The arm64 booting document requires that the bootloader has cleaned the kernel image to the PoC. However, when a CPU re-enters the kernel due to either a CPU hotplug "on" event or resuming from a low-power state (e.g. cpuidle), the kernel text may in-fact be dirty at the PoU due to things like alternative patching or even module loading.
Thanks to I-cache speculation with the MMU off, stale instructions could be fetched prior to enabling the MMU, potentially leading to crashes when executing regions of code that have been modified at runtime.
This patch addresses the issue by ensuring that the local I-cache is invalidated immediately after a CPU has enabled its MMU but before jumping out of the identity mapping. Any stale instructions fetched from the PoC will then be discarded and refetched correctly from the PoU. Patching kernel text executed prior to the MMU being enabled is prohibited, so the early entry code will always be clean.
Reviewed-by: Mark Rutland mark.rutland@arm.com Tested-by: Mark Rutland mark.rutland@arm.com Signed-off-by: Will Deacon will.deacon@arm.com --- arch/arm64/kernel/head.S | 8 ++++++++ arch/arm64/kernel/sleep.S | 8 ++++++++ arch/arm64/mm/proc.S | 1 - 3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 699444e..7158822 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -681,5 +681,13 @@ __enable_mmu: isb msr sctlr_el1, x0 isb + /* + * Invalidate the local I-cache so that any instructions fetched + * speculatively from the PoC are discarded, since they may have + * been dynamically patched at the PoU. + */ + ic iallu + dsb nsh + isb br x27 ENDPROC(__enable_mmu) diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S index fb3128e..f586f7c 100644 --- a/arch/arm64/kernel/sleep.S +++ b/arch/arm64/kernel/sleep.S @@ -133,6 +133,14 @@ ENTRY(cpu_resume_mmu) ldr x3, =cpu_resume_after_mmu msr sctlr_el1, x0 // restore sctlr_el1 isb + /* + * Invalidate the local I-cache so that any instructions fetched + * speculatively from the PoC are discarded, since they may have + * been dynamically patched at the PoU. + */ + ic iallu + dsb nsh + isb br x3 // global jump to virtual address ENDPROC(cpu_resume_mmu) .popsection diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S index e1d0e0a..82e145e 100644 --- a/arch/arm64/mm/proc.S +++ b/arch/arm64/mm/proc.S @@ -193,7 +193,6 @@ ENDPROC(cpu_do_switch_mm) * value of the SCTLR_EL1 register. */ ENTRY(__cpu_setup) - ic iallu // I+BTB cache invalidate tlbi vmalle1is // invalidate I + D TLBs dsb ish
Thanks a lot Leo! A good finding! Both of a them looks good.
The bug was introduced by PSCI. So it's clear in LTS.
Thanks Alex
On 03/25/2016 11:22 PM, Leo Yan wrote:
Hi Alex,
Below two patches are important for CPU's suspend/resume flow; These two patches have been merged into mainline kernel already.
If without Sudeep's patch, cpu will get wrong context when it returns back and finally introduce system hang issue. So patch 1 is fatal.
For Will Deacon's patch, I have not seen obvious issue if without it on Hikey. But for more safe, also ported this patch to fetch correct instructions from PoU but discard potential stale instructions which fetched from PoC.
Sudeep Holla (1): arm64: restore cpu suspend/resume functionality
Will Deacon (1): arm64: mm: ensure patched kernel text is fetched from PoU
arch/arm64/kernel/head.S | 8 ++++++++ arch/arm64/kernel/sleep.S | 9 ++++++++- arch/arm64/mm/proc.S | 1 - 3 files changed, 16 insertions(+), 2 deletions(-)
linaro-kernel@lists.linaro.org
-
Alex Shi -
Leo Yan