[PATCH 0/3] Implement a software workaround for Falkor erratum 1041

[PATCH 0/3] Implement a software workaround for Falkor erratum 1041

[Shanker Donthineni]

On Falkor CPU, we’ve discovered a hardware issue which might lead to a
kernel crash or the unexpected behavior. The Falkor core may errantly
access memory locations on speculative instruction fetches. This may
happen whenever MMU translation state, SCTLR_ELn[M] bit is being changed
from enabled to disabled for the currently running exception level. To
prevent the errant hardware behavior, software must execute an ISB
immediately prior to executing the MSR that changes SCTLR_ELn[M] from a
value of 1 to 0. To simplify the complexity of a workaround, this patch
series issues an ISB whenever SCTLR_ELn[M] is changed to 0 to fix the
Falkor erratum 1041.

Patch1:
  - CPUTYPE definitions for Falkor CPU.

Patch2:
  - Define two ASM helper macros to read/write SCTLR_ELn register.

Patch3:
  - Actual workaround changes for erratum E1041.

Shanker Donthineni (3):
  arm64: Define cputype macros for Falkor CPU
  arm64: Prepare SCTLR_ELn accesses to handle Falkor erratum 1041
  arm64: Add software workaround for Falkor erratum 1041

 Documentation/arm64/silicon-errata.txt |  1 +
 arch/arm64/Kconfig                     | 10 ++++++++++
 arch/arm64/include/asm/assembler.h     | 35 ++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/cpucaps.h       |  3 ++-
 arch/arm64/include/asm/cputype.h       |  2 ++
 arch/arm64/kernel/cpu-reset.S          |  4 ++--
 arch/arm64/kernel/cpu_errata.c         | 16 ++++++++++++++++
 arch/arm64/kernel/efi-entry.S          |  8 ++++----
 arch/arm64/kernel/head.S               | 18 ++++++++---------
 arch/arm64/kernel/relocate_kernel.S    |  4 ++--
 arch/arm64/kvm/hyp-init.S              |  6 +++---
 arch/arm64/mm/proc.S                   |  6 +++---
 12 files changed, 89 insertions(+), 24 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/3] arm64: Define cputype macros for Falkor CPU

[Shanker Donthineni]

Add cputype definition macros for Qualcomm Datacenter Technologies
Falkor CPU in cputype.h. It's unfortunate that the first revision
of the Falkor CPU used the wrong part number 0x800, got fixed in v2
chip with part number 0xC00, and would be used the same value for
future revisions.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
---
 arch/arm64/include/asm/cputype.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index 235e77d..cbf08d7 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -91,6 +91,7 @@
 #define BRCM_CPU_PART_VULCAN		0x516
 
 #define QCOM_CPU_PART_FALKOR_V1		0x800
+#define QCOM_CPU_PART_FALKOR		0xC00
 
 #define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
 #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
@@ -99,6 +100,7 @@
 #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX)
 #define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX)
 #define MIDR_QCOM_FALKOR_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, QCOM_CPU_PART_FALKOR_V1)
+#define MIDR_QCOM_FALKOR MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, QCOM_CPU_PART_FALKOR)
 
 #ifndef __ASSEMBLY__
 
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/3] arm64: Prepare SCTLR_ELn accesses to handle Falkor erratum 1041

[Shanker Donthineni]

This patch introduces two helper macros read_sctlr and write_sctlr
to access system register SCTLR_ELn. Replace all MSR/MRS references
to sctlr_el1{el2} with macros.

This should cause no behavioral change.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 arch/arm64/include/asm/assembler.h  | 18 ++++++++++++++++++
 arch/arm64/kernel/cpu-reset.S       |  4 ++--
 arch/arm64/kernel/efi-entry.S       |  8 ++++----
 arch/arm64/kernel/head.S            | 18 +++++++++---------
 arch/arm64/kernel/relocate_kernel.S |  4 ++--
 arch/arm64/kvm/hyp-init.S           |  6 +++---
 arch/arm64/mm/proc.S                |  6 +++---
 7 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index d58a625..b6dfb4f 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -499,4 +499,22 @@
 #endif
 	.endm
 
+/**
+ * Read value of the system control register SCTLR_ELn.
+ *   eln: which system control register.
+ *   reg: contents of the SCTLR_ELn.
+ */
+	.macro	read_sctlr, eln, reg
+	mrs	\reg, sctlr_\eln
+	.endm
+
+/**
+ * Write the value to the system control register SCTLR_ELn.
+ *   eln: which system control register.
+ *   reg: the value to be written.
+ */
+	.macro	write_sctlr, eln, reg
+	msr	sctlr_\eln, \reg
+	.endm
+
 #endif	/* __ASM_ASSEMBLER_H */
diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S
index 65f42d2..9224abd 100644
--- a/arch/arm64/kernel/cpu-reset.S
+++ b/arch/arm64/kernel/cpu-reset.S
@@ -34,10 +34,10 @@
  */
 ENTRY(__cpu_soft_restart)
 	/* Clear sctlr_el1 flags. */
-	mrs	x12, sctlr_el1
+	read_sctlr el1, x12
 	ldr	x13, =SCTLR_ELx_FLAGS
 	bic	x12, x12, x13
-	msr	sctlr_el1, x12
+	write_sctlr el1, x12
 	isb
 
 	cbz	x0, 1f				// el2_switch?
diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
index 4e6ad35..acae627 100644
--- a/arch/arm64/kernel/efi-entry.S
+++ b/arch/arm64/kernel/efi-entry.S
@@ -93,17 +93,17 @@ ENTRY(entry)
 	mrs	x0, CurrentEL
 	cmp	x0, #CurrentEL_EL2
 	b.ne	1f
-	mrs	x0, sctlr_el2
+	read_sctlr el2, x0
 	bic	x0, x0, #1 << 0	// clear SCTLR.M
 	bic	x0, x0, #1 << 2	// clear SCTLR.C
-	msr	sctlr_el2, x0
+	write_sctlr el2, x0
 	isb
 	b	2f
 1:
-	mrs	x0, sctlr_el1
+	read_sctlr el1, x0
 	bic	x0, x0, #1 << 0	// clear SCTLR.M
 	bic	x0, x0, #1 << 2	// clear SCTLR.C
-	msr	sctlr_el1, x0
+	write_sctlr el1, x0
 	isb
 2:
 	/* Jump to kernel entry point */
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 0b243ec..b8d5b73 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -388,18 +388,18 @@ ENTRY(el2_setup)
 	mrs	x0, CurrentEL
 	cmp	x0, #CurrentEL_EL2
 	b.eq	1f
-	mrs	x0, sctlr_el1
+	read_sctlr el1, x0
 CPU_BE(	orr	x0, x0, #(3 << 24)	)	// Set the EE and E0E bits for EL1
 CPU_LE(	bic	x0, x0, #(3 << 24)	)	// Clear the EE and E0E bits for EL1
-	msr	sctlr_el1, x0
+	write_sctlr el1, x0
 	mov	w0, #BOOT_CPU_MODE_EL1		// This cpu booted in EL1
 	isb
 	ret
 
-1:	mrs	x0, sctlr_el2
+1:	read_sctlr el2, x0
 CPU_BE(	orr	x0, x0, #(1 << 25)	)	// Set the EE bit for EL2
 CPU_LE(	bic	x0, x0, #(1 << 25)	)	// Clear the EE bit for EL2
-	msr	sctlr_el2, x0
+	write_sctlr el2, x0
 
 #ifdef CONFIG_ARM64_VHE
 	/*
@@ -511,7 +511,7 @@ install_el2_stub:
 	mov	x0, #0x0800			// Set/clear RES{1,0} bits
 CPU_BE(	movk	x0, #0x33d0, lsl #16	)	// Set EE and E0E on BE systems
 CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
-	msr	sctlr_el1, x0
+	write_sctlr el1, x0
 
 	/* Coprocessor traps. */
 	mov	x0, #0x33ff
@@ -664,7 +664,7 @@ ENTRY(__enable_mmu)
 	msr	ttbr0_el1, x1			// load TTBR0
 	msr	ttbr1_el1, x2			// load TTBR1
 	isb
-	msr	sctlr_el1, x0
+	write_sctlr el1, x0
 	isb
 	/*
 	 * Invalidate the local I-cache so that any instructions fetched
@@ -716,7 +716,7 @@ ENDPROC(__relocate_kernel)
 __primary_switch:
 #ifdef CONFIG_RANDOMIZE_BASE
 	mov	x19, x0				// preserve new SCTLR_EL1 value
-	mrs	x20, sctlr_el1			// preserve old SCTLR_EL1 value
+	read_sctlr el1, x20			// preserve old SCTLR_EL1 value
 #endif
 
 	bl	__enable_mmu
@@ -732,14 +732,14 @@ __primary_switch:
 	 * to take into account by discarding the current kernel mapping and
 	 * creating a new one.
 	 */
-	msr	sctlr_el1, x20			// disable the MMU
+	write_sctlr el1, x20			// disable the MMU
 	isb
 	bl	__create_page_tables		// recreate kernel mapping
 
 	tlbi	vmalle1				// Remove any stale TLB entries
 	dsb	nsh
 
-	msr	sctlr_el1, x19			// re-enable the MMU
+	write_sctlr el1, x19			// re-enable the MMU
 	isb
 	ic	iallu				// flush instructions fetched
 	dsb	nsh				// via old mapping
diff --git a/arch/arm64/kernel/relocate_kernel.S b/arch/arm64/kernel/relocate_kernel.S
index ce704a4..4381c92 100644
--- a/arch/arm64/kernel/relocate_kernel.S
+++ b/arch/arm64/kernel/relocate_kernel.S
@@ -42,10 +42,10 @@ ENTRY(arm64_relocate_new_kernel)
 	mrs	x0, CurrentEL
 	cmp	x0, #CurrentEL_EL2
 	b.ne	1f
-	mrs	x0, sctlr_el2
+	read_sctlr el2, x0
 	ldr	x1, =SCTLR_ELx_FLAGS
 	bic	x0, x0, x1
-	msr	sctlr_el2, x0
+	write_sctlr el2, x0
 	isb
 1:
 
diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S
index 3f96155..22996a3 100644
--- a/arch/arm64/kvm/hyp-init.S
+++ b/arch/arm64/kvm/hyp-init.S
@@ -113,7 +113,7 @@ __do_hyp_init:
 	 */
 	ldr	x4, =(SCTLR_EL2_RES1 | (SCTLR_ELx_FLAGS & ~SCTLR_ELx_A))
 CPU_BE(	orr	x4, x4, #SCTLR_ELx_EE)
-	msr	sctlr_el2, x4
+	write_sctlr el2, x4
 	isb
 
 	/* Set the stack and new vectors */
@@ -148,10 +148,10 @@ reset:
 	 * Reset kvm back to the hyp stub. Do not clobber x0-x4 in
 	 * case we coming via HVC_SOFT_RESTART.
 	 */
-	mrs	x5, sctlr_el2
+	read_sctlr el2, x5
 	ldr	x6, =SCTLR_ELx_FLAGS
 	bic	x5, x5, x6		// Clear SCTL_M and etc
-	msr	sctlr_el2, x5
+	write_sctlr el2, x5
 	isb
 
 	/* Install stub vectors */
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 877d42f..958c3a1 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -69,7 +69,7 @@ ENTRY(cpu_do_suspend)
 	mrs	x7, vbar_el1
 	mrs	x8, mdscr_el1
 	mrs	x9, oslsr_el1
-	mrs	x10, sctlr_el1
+	read_sctlr el1, x10
 	mrs	x11, tpidr_el1
 	mrs	x12, sp_el0
 	stp	x2, x3, [x0]
@@ -115,7 +115,7 @@ ENTRY(cpu_do_resume)
 	disable_dbg
 	msr	mdscr_el1, x10
 
-	msr	sctlr_el1, x12
+	write_sctlr el1, x12
 	msr	tpidr_el1, x13
 	msr	sp_el0, x14
 	/*
@@ -217,7 +217,7 @@ ENTRY(__cpu_setup)
 	 */
 	adr	x5, crval
 	ldp	w5, w6, [x5]
-	mrs	x0, sctlr_el1
+	read_sctlr el1, x0
 	bic	x0, x0, x5			// clear bits
 	orr	x0, x0, x6			// set bits
 	/*
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

The ARM architecture defines the memory locations that are permitted
to be accessed as the result of a speculative instruction fetch from
an exception level for which all stages of translation are disabled.
Specifically, the core is permitted to speculatively fetch from the
4KB region containing the current program counter and next 4KB.

When translation is changed from enabled to disabled for the running
exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
Falkor core may errantly speculatively access memory locations outside
of the 4KB region permitted by the architecture. The errant memory
access may lead to one of the following unexpected behaviors.

1) A System Error Interrupt (SEI) being raised by the Falkor core due
   to the errant memory access attempting to access a region of memory
   that is protected by a slave-side memory protection unit.
2) Unpredictable device behavior due to a speculative read from device
   memory. This behavior may only occur if the instruction cache is
   disabled prior to or coincident with translation being changed from
   enabled to disabled.

To avoid the errant behavior, software must execute an ISB immediately
prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
 Documentation/arm64/silicon-errata.txt |  1 +
 arch/arm64/Kconfig                     | 10 ++++++++++
 arch/arm64/include/asm/assembler.h     | 17 +++++++++++++++++
 arch/arm64/include/asm/cpucaps.h       |  3 ++-
 arch/arm64/kernel/cpu_errata.c         | 16 ++++++++++++++++
 arch/arm64/kernel/efi-entry.S          |  4 ++--
 arch/arm64/kernel/head.S               |  4 ++--
 7 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 66e8ce1..704770c0 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -74,3 +74,4 @@ stable kernels.
 | Qualcomm Tech. | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003    |
 | Qualcomm Tech. | Falkor v1       | E1009           | QCOM_FALKOR_ERRATUM_1009    |
 | Qualcomm Tech. | QDF2400 ITS     | E0065           | QCOM_QDF2400_ERRATUM_0065   |
+| Qualcomm Tech. | Falkor v{1,2}   | E1041           | QCOM_FALKOR_ERRATUM_1041    |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 0df64a6..7e933fb 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
 
 	  If unsure, say Y.
 
+config QCOM_FALKOR_ERRATUM_1041
+	bool "Falkor E1041: Speculative instruction fetches might cause errant memory access"
+	default y
+	help
+	  Falkor CPU may speculatively fetch instructions from an improper
+	  memory location when MMU translation is changed from SCTLR_ELn[M]=1
+	  to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
+
+	  If unsure, say Y.
+
 endmenu
 
 
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index b6dfb4f..4c91efb 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -30,6 +30,7 @@
 #include <asm/pgtable-hwdef.h>
 #include <asm/ptrace.h>
 #include <asm/thread_info.h>
+#include <asm/alternative.h>
 
 /*
  * Enable and disable interrupts.
@@ -514,6 +515,22 @@
  *   reg: the value to be written.
  */
 	.macro	write_sctlr, eln, reg
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
+	tbnz    \reg, #0, 8000f          // enable MMU?
+	isb
+8000:
+alternative_else_nop_endif
+#endif
+	msr	sctlr_\eln, \reg
+	.endm
+
+	.macro	early_write_sctlr, eln, reg
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
+	tbnz    \reg, #0, 8000f          // enable MMU?
+	isb
+8000:
+#endif
 	msr	sctlr_\eln, \reg
 	.endm
 
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index 8da6216..7f7a59d 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -40,7 +40,8 @@
 #define ARM64_WORKAROUND_858921			19
 #define ARM64_WORKAROUND_CAVIUM_30115		20
 #define ARM64_HAS_DCPOP				21
+#define ARM64_WORKAROUND_QCOM_FALKOR_E1041	22
 
-#define ARM64_NCAPS				22
+#define ARM64_NCAPS				23
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index 0e27f86..27f9a45 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -179,6 +179,22 @@ static int cpu_enable_trap_ctr_access(void *__unused)
 			   MIDR_CPU_VAR_REV(0, 0)),
 	},
 #endif
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
+	{
+		.desc = "Qualcomm Technologies Falkor erratum 1041",
+		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
+		MIDR_RANGE(MIDR_QCOM_FALKOR_V1,
+			MIDR_CPU_VAR_REV(0, 0),
+			MIDR_CPU_VAR_REV(0, 0)),
+	},
+	{
+		.desc = "Qualcomm Technologies Falkor erratum 1041",
+		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
+		MIDR_RANGE(MIDR_QCOM_FALKOR,
+			MIDR_CPU_VAR_REV(0, 1),
+			MIDR_CPU_VAR_REV(0, 2)),
+	},
+#endif
 #ifdef CONFIG_ARM64_ERRATUM_858921
 	{
 	/* Cortex-A73 all versions */
diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
index acae627..c31be1b 100644
--- a/arch/arm64/kernel/efi-entry.S
+++ b/arch/arm64/kernel/efi-entry.S
@@ -96,14 +96,14 @@ ENTRY(entry)
 	read_sctlr el2, x0
 	bic	x0, x0, #1 << 0	// clear SCTLR.M
 	bic	x0, x0, #1 << 2	// clear SCTLR.C
-	write_sctlr el2, x0
+	early_write_sctlr el2, x0
 	isb
 	b	2f
 1:
 	read_sctlr el1, x0
 	bic	x0, x0, #1 << 0	// clear SCTLR.M
 	bic	x0, x0, #1 << 2	// clear SCTLR.C
-	write_sctlr el1, x0
+	early_write_sctlr el1, x0
 	isb
 2:
 	/* Jump to kernel entry point */
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index b8d5b73..9512ce7 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -511,7 +511,7 @@ install_el2_stub:
 	mov	x0, #0x0800			// Set/clear RES{1,0} bits
 CPU_BE(	movk	x0, #0x33d0, lsl #16	)	// Set EE and E0E on BE systems
 CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
-	write_sctlr el1, x0
+	early_write_sctlr el1, x0
 
 	/* Coprocessor traps. */
 	mov	x0, #0x33ff
@@ -732,7 +732,7 @@ __primary_switch:
 	 * to take into account by discarding the current kernel mapping and
 	 * creating a new one.
 	 */
-	write_sctlr el1, x20			// disable the MMU
+	early_write_sctlr el1, x20		// disable the MMU
 	isb
 	bl	__create_page_tables		// recreate kernel mapping
 
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Robin Murphy]

On 03/11/17 03:27, Shanker Donthineni wrote:
> The ARM architecture defines the memory locations that are permitted
> to be accessed as the result of a speculative instruction fetch from
> an exception level for which all stages of translation are disabled.
> Specifically, the core is permitted to speculatively fetch from the
> 4KB region containing the current program counter and next 4KB.
> 
> When translation is changed from enabled to disabled for the running
> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
> Falkor core may errantly speculatively access memory locations outside
> of the 4KB region permitted by the architecture. The errant memory
> access may lead to one of the following unexpected behaviors.
> 
> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>    to the errant memory access attempting to access a region of memory
>    that is protected by a slave-side memory protection unit.
> 2) Unpredictable device behavior due to a speculative read from device
>    memory. This behavior may only occur if the instruction cache is
>    disabled prior to or coincident with translation being changed from
>    enabled to disabled.
> 
> To avoid the errant behavior, software must execute an ISB immediately
> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
>  Documentation/arm64/silicon-errata.txt |  1 +
>  arch/arm64/Kconfig                     | 10 ++++++++++
>  arch/arm64/include/asm/assembler.h     | 17 +++++++++++++++++
>  arch/arm64/include/asm/cpucaps.h       |  3 ++-
>  arch/arm64/kernel/cpu_errata.c         | 16 ++++++++++++++++
>  arch/arm64/kernel/efi-entry.S          |  4 ++--
>  arch/arm64/kernel/head.S               |  4 ++--
>  7 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 66e8ce1..704770c0 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -74,3 +74,4 @@ stable kernels.
>  | Qualcomm Tech. | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003    |
>  | Qualcomm Tech. | Falkor v1       | E1009           | QCOM_FALKOR_ERRATUM_1009    |
>  | Qualcomm Tech. | QDF2400 ITS     | E0065           | QCOM_QDF2400_ERRATUM_0065   |
> +| Qualcomm Tech. | Falkor v{1,2}   | E1041           | QCOM_FALKOR_ERRATUM_1041    |
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0df64a6..7e933fb 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
>  
>  	  If unsure, say Y.
>  
> +config QCOM_FALKOR_ERRATUM_1041
> +	bool "Falkor E1041: Speculative instruction fetches might cause errant memory access"
> +	default y
> +	help
> +	  Falkor CPU may speculatively fetch instructions from an improper
> +	  memory location when MMU translation is changed from SCTLR_ELn[M]=1
> +	  to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
> +
> +	  If unsure, say Y.
> +
>  endmenu
>  
>  
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index b6dfb4f..4c91efb 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -30,6 +30,7 @@
>  #include <asm/pgtable-hwdef.h>
>  #include <asm/ptrace.h>
>  #include <asm/thread_info.h>
> +#include <asm/alternative.h>
>  
>  /*
>   * Enable and disable interrupts.
> @@ -514,6 +515,22 @@
>   *   reg: the value to be written.
>   */
>  	.macro	write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
> +	tbnz    \reg, #0, 8000f          // enable MMU?

Do we really need the branch here? It's not like enabling the MMU is
something we do on the syscall fastpath, and I can't imagine an extra
ISB hurts much (and is probably comparable to a mispredicted branch
anyway). In fact, is there any noticeable hit on other
microarchitectures if we save the alternative bother and just do it
unconditionally always?

Robin.

> +	isb
> +8000:
> +alternative_else_nop_endif
> +#endif
> +	msr	sctlr_\eln, \reg
> +	.endm
> +
> +	.macro	early_write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +	tbnz    \reg, #0, 8000f          // enable MMU?
> +	isb
> +8000:
> +#endif
>  	msr	sctlr_\eln, \reg
>  	.endm
>  
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 8da6216..7f7a59d 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -40,7 +40,8 @@
>  #define ARM64_WORKAROUND_858921			19
>  #define ARM64_WORKAROUND_CAVIUM_30115		20
>  #define ARM64_HAS_DCPOP				21
> +#define ARM64_WORKAROUND_QCOM_FALKOR_E1041	22
>  
> -#define ARM64_NCAPS				22
> +#define ARM64_NCAPS				23
>  
>  #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 0e27f86..27f9a45 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -179,6 +179,22 @@ static int cpu_enable_trap_ctr_access(void *__unused)
>  			   MIDR_CPU_VAR_REV(0, 0)),
>  	},
>  #endif
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +	{
> +		.desc = "Qualcomm Technologies Falkor erratum 1041",
> +		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
> +		MIDR_RANGE(MIDR_QCOM_FALKOR_V1,
> +			MIDR_CPU_VAR_REV(0, 0),
> +			MIDR_CPU_VAR_REV(0, 0)),
> +	},
> +	{
> +		.desc = "Qualcomm Technologies Falkor erratum 1041",
> +		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
> +		MIDR_RANGE(MIDR_QCOM_FALKOR,
> +			MIDR_CPU_VAR_REV(0, 1),
> +			MIDR_CPU_VAR_REV(0, 2)),
> +	},
> +#endif
>  #ifdef CONFIG_ARM64_ERRATUM_858921
>  	{
>  	/* Cortex-A73 all versions */
> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> index acae627..c31be1b 100644
> --- a/arch/arm64/kernel/efi-entry.S
> +++ b/arch/arm64/kernel/efi-entry.S
> @@ -96,14 +96,14 @@ ENTRY(entry)
>  	read_sctlr el2, x0
>  	bic	x0, x0, #1 << 0	// clear SCTLR.M
>  	bic	x0, x0, #1 << 2	// clear SCTLR.C
> -	write_sctlr el2, x0
> +	early_write_sctlr el2, x0
>  	isb
>  	b	2f
>  1:
>  	read_sctlr el1, x0
>  	bic	x0, x0, #1 << 0	// clear SCTLR.M
>  	bic	x0, x0, #1 << 2	// clear SCTLR.C
> -	write_sctlr el1, x0
> +	early_write_sctlr el1, x0
>  	isb
>  2:
>  	/* Jump to kernel entry point */
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index b8d5b73..9512ce7 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -511,7 +511,7 @@ install_el2_stub:
>  	mov	x0, #0x0800			// Set/clear RES{1,0} bits
>  CPU_BE(	movk	x0, #0x33d0, lsl #16	)	// Set EE and E0E on BE systems
>  CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
> -	write_sctlr el1, x0
> +	early_write_sctlr el1, x0
>  
>  	/* Coprocessor traps. */
>  	mov	x0, #0x33ff
> @@ -732,7 +732,7 @@ __primary_switch:
>  	 * to take into account by discarding the current kernel mapping and
>  	 * creating a new one.
>  	 */
> -	write_sctlr el1, x20			// disable the MMU
> +	early_write_sctlr el1, x20		// disable the MMU
>  	isb
>  	bl	__create_page_tables		// recreate kernel mapping
>  
> 

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

Hi Robin, Thanks for your review comments. 

On 11/03/2017 10:11 AM, Robin Murphy wrote:
> On 03/11/17 03:27, Shanker Donthineni wrote:
>> The ARM architecture defines the memory locations that are permitted
>> to be accessed as the result of a speculative instruction fetch from
>> an exception level for which all stages of translation are disabled.
>> Specifically, the core is permitted to speculatively fetch from the
>> 4KB region containing the current program counter and next 4KB.
>>
>> When translation is changed from enabled to disabled for the running
>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>> Falkor core may errantly speculatively access memory locations outside
>> of the 4KB region permitted by the architecture. The errant memory
>> access may lead to one of the following unexpected behaviors.
>>
>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>    to the errant memory access attempting to access a region of memory
>>    that is protected by a slave-side memory protection unit.
>> 2) Unpredictable device behavior due to a speculative read from device
>>    memory. This behavior may only occur if the instruction cache is
>>    disabled prior to or coincident with translation being changed from
>>    enabled to disabled.
>>
>> To avoid the errant behavior, software must execute an ISB immediately
>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>>  Documentation/arm64/silicon-errata.txt |  1 +
>>  arch/arm64/Kconfig                     | 10 ++++++++++
>>  arch/arm64/include/asm/assembler.h     | 17 +++++++++++++++++
>>  arch/arm64/include/asm/cpucaps.h       |  3 ++-
>>  arch/arm64/kernel/cpu_errata.c         | 16 ++++++++++++++++
>>  arch/arm64/kernel/efi-entry.S          |  4 ++--
>>  arch/arm64/kernel/head.S               |  4 ++--
>>  7 files changed, 50 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
>> index 66e8ce1..704770c0 100644
>> --- a/Documentation/arm64/silicon-errata.txt
>> +++ b/Documentation/arm64/silicon-errata.txt
>> @@ -74,3 +74,4 @@ stable kernels.
>>  | Qualcomm Tech. | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003    |
>>  | Qualcomm Tech. | Falkor v1       | E1009           | QCOM_FALKOR_ERRATUM_1009    |
>>  | Qualcomm Tech. | QDF2400 ITS     | E0065           | QCOM_QDF2400_ERRATUM_0065   |
>> +| Qualcomm Tech. | Falkor v{1,2}   | E1041           | QCOM_FALKOR_ERRATUM_1041    |
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 0df64a6..7e933fb 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
>>  
>>  	  If unsure, say Y.
>>  
>> +config QCOM_FALKOR_ERRATUM_1041
>> +	bool "Falkor E1041: Speculative instruction fetches might cause errant memory access"
>> +	default y
>> +	help
>> +	  Falkor CPU may speculatively fetch instructions from an improper
>> +	  memory location when MMU translation is changed from SCTLR_ELn[M]=1
>> +	  to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
>> +
>> +	  If unsure, say Y.
>> +
>>  endmenu
>>  
>>  
>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>> index b6dfb4f..4c91efb 100644
>> --- a/arch/arm64/include/asm/assembler.h
>> +++ b/arch/arm64/include/asm/assembler.h
>> @@ -30,6 +30,7 @@
>>  #include <asm/pgtable-hwdef.h>
>>  #include <asm/ptrace.h>
>>  #include <asm/thread_info.h>
>> +#include <asm/alternative.h>
>>  
>>  /*
>>   * Enable and disable interrupts.
>> @@ -514,6 +515,22 @@
>>   *   reg: the value to be written.
>>   */
>>  	.macro	write_sctlr, eln, reg
>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>> +	tbnz    \reg, #0, 8000f          // enable MMU?
> 
> Do we really need the branch here? It's not like enabling the MMU is
> something we do on the syscall fastpath, and I can't imagine an extra
> ISB hurts much (and is probably comparable to a mispredicted branch

I don't have any strong opinion on whether to use an ISB conditionally
or unconditionally. Yes, the current kernel code is not touching
SCTLR_ELn register on the system call fast path. I would like to keep
it as a conditional ISB in case if the future kernel accesses the
SCTLR_ELn on the fast path. An extra ISB should not hurt a lot but I
believe it has more overhead than the TBZ+branch mis-prediction on Falkor
CPU. This patch has been tested on the real hardware to fix the problem.

I'm open to change to an unconditional ISB if it's the better fix.

> anyway). In fact, is there any noticeable hit on other
> microarchitectures if we save the alternative bother and just do it
> unconditionally always?
> 

I can't comment on the performance impacts of other CPUs since I don't
have access to their development platforms. I'll prefer alternatives
just to avoid the unnecessary overhead on future Qualcomm Datacenter
server CPUs and regression on other CPUs because of inserting an ISB
prior to SCTLR_ELn register update. Let's see what rest of the ARM 
maintainers think about always using an ISB instead of alternatives.

> Robin.
> 
>> +	isb
>> +8000:
>> +alternative_else_nop_endif
>> +#endif
>> +	msr	sctlr_\eln, \reg
>> +	.endm
>> +
>> +	.macro	early_write_sctlr, eln, reg
>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>> +	tbnz    \reg, #0, 8000f          // enable MMU?
>> +	isb
>> +8000:
>> +#endif
>>  	msr	sctlr_\eln, \reg
>>  	.endm
>>  
>> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
>> index 8da6216..7f7a59d 100644
>> --- a/arch/arm64/include/asm/cpucaps.h
>> +++ b/arch/arm64/include/asm/cpucaps.h
>> @@ -40,7 +40,8 @@
>>  #define ARM64_WORKAROUND_858921			19
>>  #define ARM64_WORKAROUND_CAVIUM_30115		20
>>  #define ARM64_HAS_DCPOP				21
>> +#define ARM64_WORKAROUND_QCOM_FALKOR_E1041	22
>>  
>> -#define ARM64_NCAPS				22
>> +#define ARM64_NCAPS				23
>>  
>>  #endif /* __ASM_CPUCAPS_H */
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index 0e27f86..27f9a45 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -179,6 +179,22 @@ static int cpu_enable_trap_ctr_access(void *__unused)
>>  			   MIDR_CPU_VAR_REV(0, 0)),
>>  	},
>>  #endif
>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>> +	{
>> +		.desc = "Qualcomm Technologies Falkor erratum 1041",
>> +		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
>> +		MIDR_RANGE(MIDR_QCOM_FALKOR_V1,
>> +			MIDR_CPU_VAR_REV(0, 0),
>> +			MIDR_CPU_VAR_REV(0, 0)),
>> +	},
>> +	{
>> +		.desc = "Qualcomm Technologies Falkor erratum 1041",
>> +		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
>> +		MIDR_RANGE(MIDR_QCOM_FALKOR,
>> +			MIDR_CPU_VAR_REV(0, 1),
>> +			MIDR_CPU_VAR_REV(0, 2)),
>> +	},
>> +#endif
>>  #ifdef CONFIG_ARM64_ERRATUM_858921
>>  	{
>>  	/* Cortex-A73 all versions */
>> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
>> index acae627..c31be1b 100644
>> --- a/arch/arm64/kernel/efi-entry.S
>> +++ b/arch/arm64/kernel/efi-entry.S
>> @@ -96,14 +96,14 @@ ENTRY(entry)
>>  	read_sctlr el2, x0
>>  	bic	x0, x0, #1 << 0	// clear SCTLR.M
>>  	bic	x0, x0, #1 << 2	// clear SCTLR.C
>> -	write_sctlr el2, x0
>> +	early_write_sctlr el2, x0
>>  	isb
>>  	b	2f
>>  1:
>>  	read_sctlr el1, x0
>>  	bic	x0, x0, #1 << 0	// clear SCTLR.M
>>  	bic	x0, x0, #1 << 2	// clear SCTLR.C
>> -	write_sctlr el1, x0
>> +	early_write_sctlr el1, x0
>>  	isb
>>  2:
>>  	/* Jump to kernel entry point */
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index b8d5b73..9512ce7 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -511,7 +511,7 @@ install_el2_stub:
>>  	mov	x0, #0x0800			// Set/clear RES{1,0} bits
>>  CPU_BE(	movk	x0, #0x33d0, lsl #16	)	// Set EE and E0E on BE systems
>>  CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
>> -	write_sctlr el1, x0
>> +	early_write_sctlr el1, x0
>>  
>>  	/* Coprocessor traps. */
>>  	mov	x0, #0x33ff
>> @@ -732,7 +732,7 @@ __primary_switch:
>>  	 * to take into account by discarding the current kernel mapping and
>>  	 * creating a new one.
>>  	 */
>> -	write_sctlr el1, x20			// disable the MMU
>> +	early_write_sctlr el1, x20		// disable the MMU
>>  	isb
>>  	bl	__create_page_tables		// recreate kernel mapping
>>  
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [3/3] arm64: Add software workaround for Falkor erratum 1041

[Manoj Iyer]

On Thu, 2 Nov 2017, Shanker Donthineni wrote:

> The ARM architecture defines the memory locations that are permitted
> to be accessed as the result of a speculative instruction fetch from
> an exception level for which all stages of translation are disabled.
> Specifically, the core is permitted to speculatively fetch from the
> 4KB region containing the current program counter and next 4KB.
>
> When translation is changed from enabled to disabled for the running
> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
> Falkor core may errantly speculatively access memory locations outside
> of the 4KB region permitted by the architecture. The errant memory
> access may lead to one of the following unexpected behaviors.
>
> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>   to the errant memory access attempting to access a region of memory
>   that is protected by a slave-side memory protection unit.
> 2) Unpredictable device behavior due to a speculative read from device
>   memory. This behavior may only occur if the instruction cache is
>   disabled prior to or coincident with translation being changed from
>   enabled to disabled.
>
> To avoid the errant behavior, software must execute an ISB immediately
> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> Documentation/arm64/silicon-errata.txt |  1 +
> arch/arm64/Kconfig                     | 10 ++++++++++
> arch/arm64/include/asm/assembler.h     | 17 +++++++++++++++++
> arch/arm64/include/asm/cpucaps.h       |  3 ++-
> arch/arm64/kernel/cpu_errata.c         | 16 ++++++++++++++++
> arch/arm64/kernel/efi-entry.S          |  4 ++--
> arch/arm64/kernel/head.S               |  4 ++--
> 7 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 66e8ce1..704770c0 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -74,3 +74,4 @@ stable kernels.
> | Qualcomm Tech. | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003    |
> | Qualcomm Tech. | Falkor v1       | E1009           | QCOM_FALKOR_ERRATUM_1009    |
> | Qualcomm Tech. | QDF2400 ITS     | E0065           | QCOM_QDF2400_ERRATUM_0065   |
> +| Qualcomm Tech. | Falkor v{1,2}   | E1041           | QCOM_FALKOR_ERRATUM_1041    |
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0df64a6..7e933fb 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
>
> 	  If unsure, say Y.
>
> +config QCOM_FALKOR_ERRATUM_1041
> +	bool "Falkor E1041: Speculative instruction fetches might cause errant memory access"
> +	default y
> +	help
> +	  Falkor CPU may speculatively fetch instructions from an improper
> +	  memory location when MMU translation is changed from SCTLR_ELn[M]=1
> +	  to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
> +
> +	  If unsure, say Y.
> +
> endmenu
>
>
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index b6dfb4f..4c91efb 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -30,6 +30,7 @@
> #include <asm/pgtable-hwdef.h>
> #include <asm/ptrace.h>
> #include <asm/thread_info.h>
> +#include <asm/alternative.h>
>
> /*
>  * Enable and disable interrupts.
> @@ -514,6 +515,22 @@
>  *   reg: the value to be written.
>  */
> 	.macro	write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
> +	tbnz    \reg, #0, 8000f          // enable MMU?
> +	isb
> +8000:
> +alternative_else_nop_endif
> +#endif
> +	msr	sctlr_\eln, \reg
> +	.endm
> +
> +	.macro	early_write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +	tbnz    \reg, #0, 8000f          // enable MMU?
> +	isb
> +8000:
> +#endif
> 	msr	sctlr_\eln, \reg
> 	.endm
>
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index 8da6216..7f7a59d 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -40,7 +40,8 @@
> #define ARM64_WORKAROUND_858921			19
> #define ARM64_WORKAROUND_CAVIUM_30115		20
> #define ARM64_HAS_DCPOP				21
> +#define ARM64_WORKAROUND_QCOM_FALKOR_E1041	22
>
> -#define ARM64_NCAPS				22
> +#define ARM64_NCAPS				23
>
> #endif /* __ASM_CPUCAPS_H */
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 0e27f86..27f9a45 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -179,6 +179,22 @@ static int cpu_enable_trap_ctr_access(void *__unused)
> 			   MIDR_CPU_VAR_REV(0, 0)),
> 	},
> #endif
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +	{
> +		.desc = "Qualcomm Technologies Falkor erratum 1041",
> +		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
> +		MIDR_RANGE(MIDR_QCOM_FALKOR_V1,
> +			MIDR_CPU_VAR_REV(0, 0),
> +			MIDR_CPU_VAR_REV(0, 0)),
> +	},
> +	{
> +		.desc = "Qualcomm Technologies Falkor erratum 1041",
> +		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1041,
> +		MIDR_RANGE(MIDR_QCOM_FALKOR,
> +			MIDR_CPU_VAR_REV(0, 1),
> +			MIDR_CPU_VAR_REV(0, 2)),
> +	},
> +#endif
> #ifdef CONFIG_ARM64_ERRATUM_858921
> 	{
> 	/* Cortex-A73 all versions */
> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S
> index acae627..c31be1b 100644
> --- a/arch/arm64/kernel/efi-entry.S
> +++ b/arch/arm64/kernel/efi-entry.S
> @@ -96,14 +96,14 @@ ENTRY(entry)
> 	read_sctlr el2, x0
> 	bic	x0, x0, #1 << 0	// clear SCTLR.M
> 	bic	x0, x0, #1 << 2	// clear SCTLR.C
> -	write_sctlr el2, x0
> +	early_write_sctlr el2, x0
> 	isb
> 	b	2f
> 1:
> 	read_sctlr el1, x0
> 	bic	x0, x0, #1 << 0	// clear SCTLR.M
> 	bic	x0, x0, #1 << 2	// clear SCTLR.C
> -	write_sctlr el1, x0
> +	early_write_sctlr el1, x0
> 	isb
> 2:
> 	/* Jump to kernel entry point */
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index b8d5b73..9512ce7 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -511,7 +511,7 @@ install_el2_stub:
> 	mov	x0, #0x0800			// Set/clear RES{1,0} bits
> CPU_BE(	movk	x0, #0x33d0, lsl #16	)	// Set EE and E0E on BE systems
> CPU_LE(	movk	x0, #0x30d0, lsl #16	)	// Clear EE and E0E on LE systems
> -	write_sctlr el1, x0
> +	early_write_sctlr el1, x0
>
> 	/* Coprocessor traps. */
> 	mov	x0, #0x33ff
> @@ -732,7 +732,7 @@ __primary_switch:
> 	 * to take into account by discarding the current kernel mapping and
> 	 * creating a new one.
> 	 */
> -	write_sctlr el1, x20			// disable the MMU
> +	early_write_sctlr el1, x20		// disable the MMU
> 	isb
> 	bl	__create_page_tables		// recreate kernel mapping
>
>

I applied the 3 patches to Ubuntu 4.13.0-16-generic (Artful) kernel and
ran stress-ng cpu tests on QDF2400 server as follows:

sudo ./stress-ng --pathological -v --cpu 100 --cpu-load 80 --cpu-method
all --cpu-online 500 --matrix  100 --matrix-method  all --matrix-size 8192
--vm 10 --vm-hang 10 --vm-method  all --switch 100 --numa 100

Where stress-ng would spawn N workers and test cpu offline/online, perform
matrix operations, do rapid context switchs, and anonymous mmaps. Although
I was not able to reproduce the erratum on the stock 4.13 kernel using the
same test case, the patched kernel did not seem to introduce any
regressions either. I ran the stress-ng tests for over 8hrs found the
system to be stable.

Tested-by: Manoj Iyer <manoj.iyer@canonical.com>

Regards
--
============================
Manoj Iyer
Ubuntu/Canonical
ARM Servers - Cloud
============================

Re: [3/3] arm64: Add software workaround for Falkor erratum 1041

[James Morse]

Hi Manoj,

On 08/11/17 19:05, Manoj Iyer wrote:
> On Thu, 2 Nov 2017, Shanker Donthineni wrote:
>> The ARM architecture defines the memory locations that are permitted
>> to be accessed as the result of a speculative instruction fetch from
>> an exception level for which all stages of translation are disabled.
>> Specifically, the core is permitted to speculatively fetch from the
>> 4KB region containing the current program counter and next 4KB.
>>
>> When translation is changed from enabled to disabled for the running
>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>> Falkor core may errantly speculatively access memory locations outside
>> of the 4KB region permitted by the architecture. The errant memory
>> access may lead to one of the following unexpected behaviors.

> I applied the 3 patches to Ubuntu 4.13.0-16-generic (Artful) kernel and
> ran stress-ng cpu tests on QDF2400 server

[...]

> Where stress-ng would spawn N workers and test cpu offline/online, perform
> matrix operations, do rapid context switchs, and anonymous mmaps. Although
> I was not able to reproduce the erratum on the stock 4.13 kernel using the
> same test case, the patched kernel did not seem to introduce any
> regressions either. I ran the stress-ng tests for over 8hrs found the
> system to be stable.


Could you throw kexec and KVM into the mix? This issue only shows up when we
disable the MMU, which we almost never do.

For CPU offline/online we make the PSCI 'offline' call with the MMU enabled.
When the CPU comes back firmware has reset the EL2/EL1 SCTLR from a higher
exception level, so it won't hit this issue.

One place we do this is kexec, where we drop into purgatory with the MMU disabled.

The other is KVM unloading itself to return to the hyp stub. You can stress this
by starting and stopping a VM. When the number of VMs reaches 0 KVM should
unload via 'kvm_arch_hardware_disable()'.


Thanks,

James

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[James Morse]

Hi Shanker, Robin,

On 04/11/17 21:43, Shanker Donthineni wrote:
> On 11/03/2017 10:11 AM, Robin Murphy wrote:
>> On 03/11/17 03:27, Shanker Donthineni wrote:
>>> The ARM architecture defines the memory locations that are permitted
>>> to be accessed as the result of a speculative instruction fetch from
>>> an exception level for which all stages of translation are disabled.
>>> Specifically, the core is permitted to speculatively fetch from the
>>> 4KB region containing the current program counter and next 4KB.
>>>
>>> When translation is changed from enabled to disabled for the running
>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>> Falkor core may errantly speculatively access memory locations outside
>>> of the 4KB region permitted by the architecture. The errant memory
>>> access may lead to one of the following unexpected behaviors.
>>>
>>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>>    to the errant memory access attempting to access a region of memory
>>>    that is protected by a slave-side memory protection unit.
>>> 2) Unpredictable device behavior due to a speculative read from device
>>>    memory. This behavior may only occur if the instruction cache is
>>>    disabled prior to or coincident with translation being changed from
>>>    enabled to disabled.
>>>
>>> To avoid the errant behavior, software must execute an ISB immediately
>>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.


>>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>>> index b6dfb4f..4c91efb 100644
>>> --- a/arch/arm64/include/asm/assembler.h
>>> +++ b/arch/arm64/include/asm/assembler.h
>>> @@ -30,6 +30,7 @@
>>>  #include <asm/pgtable-hwdef.h>
>>>  #include <asm/ptrace.h>
>>>  #include <asm/thread_info.h>
>>> +#include <asm/alternative.h>
>>>  
>>>  /*
>>>   * Enable and disable interrupts.
>>> @@ -514,6 +515,22 @@
>>>   *   reg: the value to be written.
>>>   */
>>>  	.macro	write_sctlr, eln, reg
>>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>>> +	tbnz    \reg, #0, 8000f          // enable MMU?

Won't this match any change that leaves the MMU enabled?

I think the macro is making this more confusing. Disabling the MMU is obvious
from the call-site, (and really rare!). Trying to work it out from a macro makes
it more complicated than necessary.


>> Do we really need the branch here? It's not like enabling the MMU is
>> something we do on the syscall fastpath, and I can't imagine an extra
>> ISB hurts much (and is probably comparable to a mispredicted branch

> I don't have any strong opinion on whether to use an ISB conditionally
> or unconditionally. Yes, the current kernel code is not touching
> SCTLR_ELn register on the system call fast path. I would like to keep
> it as a conditional ISB in case if the future kernel accesses the
> SCTLR_ELn on the fast path. An extra ISB should not hurt a lot but I
> believe it has more overhead than the TBZ+branch mis-prediction on Falkor
> CPU. This patch has been tested on the real hardware to fix the problem.

> I'm open to change to an unconditional ISB if it's the better fix.
> 
>> anyway). In fact, is there any noticeable hit on other
>> microarchitectures if we save the alternative bother and just do it
>> unconditionally always?
>>
> 
> I can't comment on the performance impacts of other CPUs since I don't
> have access to their development platforms. I'll prefer alternatives
> just to avoid the unnecessary overhead on future Qualcomm Datacenter
> server CPUs and regression on other CPUs because of inserting an ISB

I think hiding errata on other CPUs is a good argument.

My suggestion would be:
> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> 	isb
> #endif

In head.S and efi-entry.S, as these run before alternatives.
Then use alternatives to add just the isb in the mmu-off path for the other callers.


> prior to SCTLR_ELn register update. Let's see what rest of the ARM 
> maintainers think about always using an ISB instead of alternatives.


Thanks,

James

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

Hi James,

On 11/09/2017 05:08 AM, James Morse wrote:
> Hi Shanker, Robin,
> 
> On 04/11/17 21:43, Shanker Donthineni wrote:
>> On 11/03/2017 10:11 AM, Robin Murphy wrote:
>>> On 03/11/17 03:27, Shanker Donthineni wrote:
>>>> The ARM architecture defines the memory locations that are permitted
>>>> to be accessed as the result of a speculative instruction fetch from
>>>> an exception level for which all stages of translation are disabled.
>>>> Specifically, the core is permitted to speculatively fetch from the
>>>> 4KB region containing the current program counter and next 4KB.
>>>>
>>>> When translation is changed from enabled to disabled for the running
>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>> Falkor core may errantly speculatively access memory locations outside
>>>> of the 4KB region permitted by the architecture. The errant memory
>>>> access may lead to one of the following unexpected behaviors.
>>>>
>>>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>>>    to the errant memory access attempting to access a region of memory
>>>>    that is protected by a slave-side memory protection unit.
>>>> 2) Unpredictable device behavior due to a speculative read from device
>>>>    memory. This behavior may only occur if the instruction cache is
>>>>    disabled prior to or coincident with translation being changed from
>>>>    enabled to disabled.
>>>>
>>>> To avoid the errant behavior, software must execute an ISB immediately
>>>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
> 
>>>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>>>> index b6dfb4f..4c91efb 100644
>>>> --- a/arch/arm64/include/asm/assembler.h
>>>> +++ b/arch/arm64/include/asm/assembler.h
>>>> @@ -30,6 +30,7 @@
>>>>  #include <asm/pgtable-hwdef.h>
>>>>  #include <asm/ptrace.h>
>>>>  #include <asm/thread_info.h>
>>>> +#include <asm/alternative.h>
>>>>  
>>>>  /*
>>>>   * Enable and disable interrupts.
>>>> @@ -514,6 +515,22 @@
>>>>   *   reg: the value to be written.
>>>>   */
>>>>  	.macro	write_sctlr, eln, reg
>>>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>>>> +	tbnz    \reg, #0, 8000f          // enable MMU?
> 
> Won't this match any change that leaves the MMU enabled?
> 

Yes. No need to apply workaround if the MMU is going to be enabled.

> I think the macro is making this more confusing. Disabling the MMU is obvious
> from the call-site, (and really rare!). Trying to work it out from a macro makes
> it more complicated than necessary.
>

Not clear, are you suggesting not to use read{write}_sctlr() macros instead apply 
the workaround from the call-site based on the MMU-on status? If yes, It simplifies
the code logic but CONFIG_QCOM_FALKOR_ERRATUM_1041 references are scatter everywhere. 
 
> 
>>> Do we really need the branch here? It's not like enabling the MMU is
>>> something we do on the syscall fastpath, and I can't imagine an extra
>>> ISB hurts much (and is probably comparable to a mispredicted branch
> 
>> I don't have any strong opinion on whether to use an ISB conditionally
>> or unconditionally. Yes, the current kernel code is not touching
>> SCTLR_ELn register on the system call fast path. I would like to keep
>> it as a conditional ISB in case if the future kernel accesses the
>> SCTLR_ELn on the fast path. An extra ISB should not hurt a lot but I
>> believe it has more overhead than the TBZ+branch mis-prediction on Falkor
>> CPU. This patch has been tested on the real hardware to fix the problem.
> 
>> I'm open to change to an unconditional ISB if it's the better fix.
>>
>>> anyway). In fact, is there any noticeable hit on other
>>> microarchitectures if we save the alternative bother and just do it
>>> unconditionally always?
>>>
>>
>> I can't comment on the performance impacts of other CPUs since I don't
>> have access to their development platforms. I'll prefer alternatives
>> just to avoid the unnecessary overhead on future Qualcomm Datacenter
>> server CPUs and regression on other CPUs because of inserting an ISB
> 
> I think hiding errata on other CPUs is a good argument.
> 
> My suggestion would be:
>> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>> 	isb
>> #endif
> 
> In head.S and efi-entry.S, as these run before alternatives.
> Then use alternatives to add just the isb in the mmu-off path for the other callers.
> 
> 

Thanks for your opinion on this one, I'll change to an unconditional ISB in v2 patch.
After this change the enable_mmu() issues two ISBs before writing to SCTLR_EL1. Are
you okay with this behavior?

 ENTRY(__enable_mmu)
        mrs     x1, ID_AA64MMFR0_EL1
        ubfx    x2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
        cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
        b.ne    __no_granule_support
        update_early_cpu_boot_status 0, x1, x2
        adrp    x1, idmap_pg_dir
        adrp    x2, swapper_pg_dir
        msr     ttbr0_el1, x1                   // load TTBR0
        msr     ttbr1_el1, x2                   // load TTBR1
        isb
        write_sctlr el1, x0
        isb


>> prior to SCTLR_ELn register update. Let's see what rest of the ARM 
>> maintainers think about always using an ISB instead of alternatives.
> 
> 
> Thanks,
> 
> James
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [3/3] arm64: Add software workaround for Falkor erratum 1041

[Manoj Iyer]

James,

(sorry for top-posting)

Applied patch 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )

- Start 20 VMs one at a time

In a loop:
- Stop (virsh destroy) 20 VMs one at a time
- Start (virsh start) 20 VMs one at a time.

The system reset's itself after starting the last VM on the 1st loop 
displaying the following:

awrep6 login: [ 603.349141] ACPI CPPC: PCC check channel failed. Status=0
[ 603.765101] ACPI CPPC: PCC check channel failed. Status=0
[ 603.937389] ACPI CPPC: PCC check channel failed. Status=0
[ 608.285495] ACPI CPPC: PCC check channel failed. Status=0
[ 608.289481] ACPI CPPC: PCC check channel failed. Status=0

SYS_DBG: Running SDI image (immediate mode)
SYS_DBG: Ram Dump Init
SYS_DBG: Failed to init SD card
SYS_DBG: Resetting system!

Followed by the following messages on system reboot:
[ 6.616891] BERT: Error records from previous boot:
[ 6.621655] [Hardware Error]: event severity: fatal
[ 6.626516] [Hardware Error]: imprecise tstamp: 0000-00-00 00:00:00
[ 6.632851] [Hardware Error]: Error 0, type: fatal
[ 6.637713] [Hardware Error]: section type: unknown, 
d2e2621c-f936-468d-0d84-15a4ed015c8b
[ 6.646045] [Hardware Error]: section length: 0x238
[ 6.651082] [Hardware Error]: 00000000: 72724502 5220726f 6f736165 
6e55206e .Error Reason Un
[ 6.659761] [Hardware Error]: 00000010: 776f6e6b 0000006e 00000000 
00000000 known...........
[ 6.668442] [Hardware Error]: 00000020: 00000000 00000000 00000000 
00000000 ................
[ 6.677122] [Hardware Error]: 00000030: 00000000 00000000 00000000 
00000000 ................


On Thu, 9 Nov 2017, James Morse wrote:

> Hi Manoj,
>
> On 08/11/17 19:05, Manoj Iyer wrote:
>> On Thu, 2 Nov 2017, Shanker Donthineni wrote:
>>> The ARM architecture defines the memory locations that are permitted
>>> to be accessed as the result of a speculative instruction fetch from
>>> an exception level for which all stages of translation are disabled.
>>> Specifically, the core is permitted to speculatively fetch from the
>>> 4KB region containing the current program counter and next 4KB.
>>>
>>> When translation is changed from enabled to disabled for the running
>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>> Falkor core may errantly speculatively access memory locations outside
>>> of the 4KB region permitted by the architecture. The errant memory
>>> access may lead to one of the following unexpected behaviors.
>
>> I applied the 3 patches to Ubuntu 4.13.0-16-generic (Artful) kernel and
>> ran stress-ng cpu tests on QDF2400 server
>
> [...]
>
>> Where stress-ng would spawn N workers and test cpu offline/online, perform
>> matrix operations, do rapid context switchs, and anonymous mmaps. Although
>> I was not able to reproduce the erratum on the stock 4.13 kernel using the
>> same test case, the patched kernel did not seem to introduce any
>> regressions either. I ran the stress-ng tests for over 8hrs found the
>> system to be stable.
>
>
> Could you throw kexec and KVM into the mix? This issue only shows up when we
> disable the MMU, which we almost never do.
>
> For CPU offline/online we make the PSCI 'offline' call with the MMU enabled.
> When the CPU comes back firmware has reset the EL2/EL1 SCTLR from a higher
> exception level, so it won't hit this issue.
>
> One place we do this is kexec, where we drop into purgatory with the MMU disabled.
>
> The other is KVM unloading itself to return to the hyp stub. You can stress this
> by starting and stopping a VM. When the number of VMs reaches 0 KVM should
> unload via 'kvm_arch_hardware_disable()'.
>
>
> Thanks,
>
> James
>
>

--
============================
Manoj Iyer
Ubuntu/Canonical
ARM Servers - Cloud
============================

Re: [3/3] arm64: Add software workaround for Falkor erratum 1041

[Manoj Iyer]

On Thu, 9 Nov 2017, Manoj Iyer wrote:

>
> James,
>
> (sorry for top-posting)
>
> Applied patch 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )
>
> - Start 20 VMs one at a time
>
> In a loop:
> - Stop (virsh destroy) 20 VMs one at a time
> - Start (virsh start) 20 VMs one at a time.

Fixing some confusion I might have introduced in my prev email.

- Applied all 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )

- Created 20 VMs one at a time

In a loop:
- Stop (virsh destroy) 20 VMs one at a time
- Start (virsh start) 20 VMs one at a time.

>
> The system reset's itself after starting the last VM on the 1st loop 
> displaying the following:
>
> awrep6 login: [ 603.349141] ACPI CPPC: PCC check channel failed. Status=0
> [ 603.765101] ACPI CPPC: PCC check channel failed. Status=0
> [ 603.937389] ACPI CPPC: PCC check channel failed. Status=0
> [ 608.285495] ACPI CPPC: PCC check channel failed. Status=0
> [ 608.289481] ACPI CPPC: PCC check channel failed. Status=0
>
> SYS_DBG: Running SDI image (immediate mode)
> SYS_DBG: Ram Dump Init
> SYS_DBG: Failed to init SD card
> SYS_DBG: Resetting system!
>
> Followed by the following messages on system reboot:
> [ 6.616891] BERT: Error records from previous boot:
> [ 6.621655] [Hardware Error]: event severity: fatal
> [ 6.626516] [Hardware Error]: imprecise tstamp: 0000-00-00 00:00:00
> [ 6.632851] [Hardware Error]: Error 0, type: fatal
> [ 6.637713] [Hardware Error]: section type: unknown, 
> d2e2621c-f936-468d-0d84-15a4ed015c8b
> [ 6.646045] [Hardware Error]: section length: 0x238
> [ 6.651082] [Hardware Error]: 00000000: 72724502 5220726f 6f736165 6e55206e 
> .Error Reason Un
> [ 6.659761] [Hardware Error]: 00000010: 776f6e6b 0000006e 00000000 00000000 
> known...........
> [ 6.668442] [Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 
> ................
> [ 6.677122] [Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 
> ................
>
>
> On Thu, 9 Nov 2017, James Morse wrote:
>
>> Hi Manoj,
>> 
>> On 08/11/17 19:05, Manoj Iyer wrote:
>>> On Thu, 2 Nov 2017, Shanker Donthineni wrote:
>>>> The ARM architecture defines the memory locations that are permitted
>>>> to be accessed as the result of a speculative instruction fetch from
>>>> an exception level for which all stages of translation are disabled.
>>>> Specifically, the core is permitted to speculatively fetch from the
>>>> 4KB region containing the current program counter and next 4KB.
>>>> 
>>>> When translation is changed from enabled to disabled for the running
>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>> Falkor core may errantly speculatively access memory locations outside
>>>> of the 4KB region permitted by the architecture. The errant memory
>>>> access may lead to one of the following unexpected behaviors.
>> 
>>> I applied the 3 patches to Ubuntu 4.13.0-16-generic (Artful) kernel and
>>> ran stress-ng cpu tests on QDF2400 server
>> 
>> [...]
>> 
>>> Where stress-ng would spawn N workers and test cpu offline/online, perform
>>> matrix operations, do rapid context switchs, and anonymous mmaps. Although
>>> I was not able to reproduce the erratum on the stock 4.13 kernel using the
>>> same test case, the patched kernel did not seem to introduce any
>>> regressions either. I ran the stress-ng tests for over 8hrs found the
>>> system to be stable.
>> 
>> 
>> Could you throw kexec and KVM into the mix? This issue only shows up when 
>> we
>> disable the MMU, which we almost never do.
>> 
>> For CPU offline/online we make the PSCI 'offline' call with the MMU 
>> enabled.
>> When the CPU comes back firmware has reset the EL2/EL1 SCTLR from a higher
>> exception level, so it won't hit this issue.
>> 
>> One place we do this is kexec, where we drop into purgatory with the MMU 
>> disabled.
>> 
>> The other is KVM unloading itself to return to the hyp stub. You can stress 
>> this
>> by starting and stopping a VM. When the number of VMs reaches 0 KVM should
>> unload via 'kvm_arch_hardware_disable()'.
>> 
>> 
>> Thanks,
>> 
>> James
>> 
>> 
>
> --
> ============================
> Manoj Iyer
> Ubuntu/Canonical
> ARM Servers - Cloud
> ============================
>
>

--
============================
Manoj Iyer
Ubuntu/Canonical
ARM Servers - Cloud
============================

Re: [3/3] arm64: Add software workaround for Falkor erratum 1041

[Manoj Iyer]

James,

Looks like my VM test raised a false alarm. I retested stock Artful 4.13 
kernel (No erratum 1041 patches applied).

Host: Ubuntu Artful 4.13 kernel with *no* erratum 1041 patches applied.
Guest: Ubuntu Zesty (4.10) kernel.

- Created 20 VMs one at a time

In a loop:
- Stop (virsh destroy) 20 VMs one at a time
- Start (virsh start) 20 VMs one at a time.

And, I am able to reproduce the system reset issue I previously reported. 
I think the problem I reported with VMs might have nothing to do with the 
erratum 1041 patches, and probably needs to be root caused seperately.

With stock 4.13 kernel (no erratum 1041 patches applied):

awrep6 login: [  461.881379] ACPI CPPC: PCC check channel failed. Status=0
[  462.051194] ACPI CPPC: PCC check channel failed. Status=0
[  462.223137] ACPI CPPC: PCC check channel failed. Status=0
[  462.633790] ACPI CPPC: PCC check channel failed. Status=0
[  463.231971] ACPI CPPC: PCC check channel failed. Status=0
[  463.403163] ACPI CPPC: PCC check channel failed. Status=0
[  463.822936] ACPI CPPC: PCC check channel failed. Status=0
[  463.995222] ACPI CPPC: PCC check channel failed. Status=0
[  464.130962] ACPI CPPC: PCC check channel failed. Status=0
[  464.258973] ACPI CPPC: PCC check channel failed. Status=0
[  465.283028] ACPI CPPC: PCC check channel failed. Status=0


SYS_DBG: Running SDI image (immediate mode)
SYS_DBG: Ram Dump Init
SYS_DBG: Failed to init SD card
SYS_DBG: Resetting system!


On Thu, 9 Nov 2017, Manoj Iyer wrote:

>
>
>
> On Thu, 9 Nov 2017, Manoj Iyer wrote:
>
>> 
>> James,
>> 
>> (sorry for top-posting)
>> 
>> Applied patch 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )
>> 
>> - Start 20 VMs one at a time
>> 
>> In a loop:
>> - Stop (virsh destroy) 20 VMs one at a time
>> - Start (virsh start) 20 VMs one at a time.
>
> Fixing some confusion I might have introduced in my prev email.
>
> - Applied all 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )
>
> - Created 20 VMs one at a time
>
> In a loop:
> - Stop (virsh destroy) 20 VMs one at a time
> - Start (virsh start) 20 VMs one at a time.
>
>> 
>> The system reset's itself after starting the last VM on the 1st loop 
>> displaying the following:
>> 
>> awrep6 login: [ 603.349141] ACPI CPPC: PCC check channel failed. Status=0
>> [ 603.765101] ACPI CPPC: PCC check channel failed. Status=0
>> [ 603.937389] ACPI CPPC: PCC check channel failed. Status=0
>> [ 608.285495] ACPI CPPC: PCC check channel failed. Status=0
>> [ 608.289481] ACPI CPPC: PCC check channel failed. Status=0
>> 
>> SYS_DBG: Running SDI image (immediate mode)
>> SYS_DBG: Ram Dump Init
>> SYS_DBG: Failed to init SD card
>> SYS_DBG: Resetting system!
>> 
>> Followed by the following messages on system reboot:
>> [ 6.616891] BERT: Error records from previous boot:
>> [ 6.621655] [Hardware Error]: event severity: fatal
>> [ 6.626516] [Hardware Error]: imprecise tstamp: 0000-00-00 00:00:00
>> [ 6.632851] [Hardware Error]: Error 0, type: fatal
>> [ 6.637713] [Hardware Error]: section type: unknown, 
>> d2e2621c-f936-468d-0d84-15a4ed015c8b
>> [ 6.646045] [Hardware Error]: section length: 0x238
>> [ 6.651082] [Hardware Error]: 00000000: 72724502 5220726f 6f736165 6e55206e 
>> .Error Reason Un
>> [ 6.659761] [Hardware Error]: 00000010: 776f6e6b 0000006e 00000000 00000000 
>> known...........
>> [ 6.668442] [Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 
>> ................
>> [ 6.677122] [Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 
>> ................
>> 
>> 
>> On Thu, 9 Nov 2017, James Morse wrote:
>> 
>>> Hi Manoj,
>>> 
>>> On 08/11/17 19:05, Manoj Iyer wrote:
>>>> On Thu, 2 Nov 2017, Shanker Donthineni wrote:
>>>>> The ARM architecture defines the memory locations that are permitted
>>>>> to be accessed as the result of a speculative instruction fetch from
>>>>> an exception level for which all stages of translation are disabled.
>>>>> Specifically, the core is permitted to speculatively fetch from the
>>>>> 4KB region containing the current program counter and next 4KB.
>>>>> 
>>>>> When translation is changed from enabled to disabled for the running
>>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>>> Falkor core may errantly speculatively access memory locations outside
>>>>> of the 4KB region permitted by the architecture. The errant memory
>>>>> access may lead to one of the following unexpected behaviors.
>>> 
>>>> I applied the 3 patches to Ubuntu 4.13.0-16-generic (Artful) kernel and
>>>> ran stress-ng cpu tests on QDF2400 server
>>> 
>>> [...]
>>> 
>>>> Where stress-ng would spawn N workers and test cpu offline/online, 
>>>> perform
>>>> matrix operations, do rapid context switchs, and anonymous mmaps. 
>>>> Although
>>>> I was not able to reproduce the erratum on the stock 4.13 kernel using 
>>>> the
>>>> same test case, the patched kernel did not seem to introduce any
>>>> regressions either. I ran the stress-ng tests for over 8hrs found the
>>>> system to be stable.
>>> 
>>> 
>>> Could you throw kexec and KVM into the mix? This issue only shows up when 
>>> we
>>> disable the MMU, which we almost never do.
>>> 
>>> For CPU offline/online we make the PSCI 'offline' call with the MMU 
>>> enabled.
>>> When the CPU comes back firmware has reset the EL2/EL1 SCTLR from a higher
>>> exception level, so it won't hit this issue.
>>> 
>>> One place we do this is kexec, where we drop into purgatory with the MMU 
>>> disabled.
>>> 
>>> The other is KVM unloading itself to return to the hyp stub. You can 
>>> stress this
>>> by starting and stopping a VM. When the number of VMs reaches 0 KVM should
>>> unload via 'kvm_arch_hardware_disable()'.
>>> 
>>> 
>>> Thanks,
>>> 
>>> James
>>> 
>>> 
>> 
>> --
>> ============================
>> Manoj Iyer
>> Ubuntu/Canonical
>> ARM Servers - Cloud
>> ============================
>> 
>> 
>
> --
> ============================
> Manoj Iyer
> Ubuntu/Canonical
> ARM Servers - Cloud
> ============================
>
>

--
============================
Manoj Iyer
Ubuntu/Canonical
ARM Servers - Cloud
============================

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[James Morse]

Hi Shanker,

On 09/11/17 15:22, Shanker Donthineni wrote:
> On 11/09/2017 05:08 AM, James Morse wrote:
>> On 04/11/17 21:43, Shanker Donthineni wrote:
>>> On 11/03/2017 10:11 AM, Robin Murphy wrote:
>>>> On 03/11/17 03:27, Shanker Donthineni wrote:
>>>>> The ARM architecture defines the memory locations that are permitted
>>>>> to be accessed as the result of a speculative instruction fetch from
>>>>> an exception level for which all stages of translation are disabled.
>>>>> Specifically, the core is permitted to speculatively fetch from the
>>>>> 4KB region containing the current program counter and next 4KB.
>>>>>
>>>>> When translation is changed from enabled to disabled for the running
>>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>>> Falkor core may errantly speculatively access memory locations outside
>>>>> of the 4KB region permitted by the architecture. The errant memory
>>>>> access may lead to one of the following unexpected behaviors.
>>>>>
>>>>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>>>>    to the errant memory access attempting to access a region of memory
>>>>>    that is protected by a slave-side memory protection unit.
>>>>> 2) Unpredictable device behavior due to a speculative read from device
>>>>>    memory. This behavior may only occur if the instruction cache is
>>>>>    disabled prior to or coincident with translation being changed from
>>>>>    enabled to disabled.
>>>>>
>>>>> To avoid the errant behavior, software must execute an ISB immediately
>>>>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.

>>>>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>>>>> index b6dfb4f..4c91efb 100644
>>>>> --- a/arch/arm64/include/asm/assembler.h
>>>>> +++ b/arch/arm64/include/asm/assembler.h
>>>>> @@ -514,6 +515,22 @@
>>>>>   *   reg: the value to be written.
>>>>>   */
>>>>>  	.macro	write_sctlr, eln, reg
>>>>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>>>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>>>>> +	tbnz    \reg, #0, 8000f          // enable MMU?
>>
>> Won't this match any change that leaves the MMU enabled?
> 
> Yes. No need to apply workaround if the MMU is going to be enabled.

(Sorry, looks like I had this upside down)

My badly-made-point is you can't know if the MMU is being disabled unless you
have both the old and new values.

As an example, in el2_setup, (where the MMU is disabled), we set the EE/E0E bits
to match the kernel's endianness. Won't your macro will insert an unnecessary
isb? Is this needed for the errata workaround?


>> I think the macro is making this more confusing. Disabling the MMU is obvious
>> from the call-site, (and really rare!). Trying to work it out from a macro makes
>> it more complicated than necessary.

> Not clear, are you suggesting not to use read{write}_sctlr() macros instead apply 
> the workaround from the call-site based on the MMU-on status?

Yes. This is the only way to patch only the locations that turn the MMU off.


> If yes, It simplifies
> the code logic but CONFIG_QCOM_FALKOR_ERRATUM_1041 references are scatter everywhere. 

Wouldn't they only appear in the places that are affected by the errata?
This is exactly what we want, anyone touching that code now knows they need to
double check this behaviour, (and ask you to test it!).

Otherwise we have a macro second guessing what is happening, if its not quite
right (because some information has been lost), we're now not sure what we need
to do if we ever refactor any of this code.

[...]

>>> I'll prefer alternatives
>>> just to avoid the unnecessary overhead on future Qualcomm Datacenter
>>> server CPUs and regression on other CPUs because of inserting an ISB
>>
>> I think hiding errata on other CPUs is a good argument.
>>
>> My suggestion would be:
>>> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>> 	isb
>>> #endif
>>
>> In head.S and efi-entry.S, as these run before alternatives.
>> Then use alternatives to add just the isb in the mmu-off path for the other callers.

> Thanks for your opinion on this one, I'll change to an unconditional ISB in v2 patch.
> After this change the enable_mmu() issues two ISBs before writing to SCTLR_EL1.

Another great reason not to wrap this in a macro, there may already be a
suitable isb, in which case a comment will suffice.


> Are you okay with this behavior?

Back-to-back isb doesn't sound like a good idea.


>  ENTRY(__enable_mmu)
>         mrs     x1, ID_AA64MMFR0_EL1
>         ubfx    x2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
>         cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
>         b.ne    __no_granule_support
>         update_early_cpu_boot_status 0, x1, x2
>         adrp    x1, idmap_pg_dir
>         adrp    x2, swapper_pg_dir
>         msr     ttbr0_el1, x1                   // load TTBR0
>         msr     ttbr1_el1, x2                   // load TTBR1
>         isb
>         write_sctlr el1, x0
>         isb

Now I'm thoroughly confused. Isn't this one of the sequences that doesn't hit
the issue? Here we're switching SCTLR.M from 0 to 1.


Thanks,

James

Re: [3/3] arm64: Add software workaround for Falkor erratum 1041

[Manoj Iyer]

On Thu, 9 Nov 2017, Manoj Iyer wrote:

>
> James,
>
> Looks like my VM test raised a false alarm. I retested stock Artful 4.13 
> kernel (No erratum 1041 patches applied).
>

James, an update on the crash (false alarm). We suspect this is a firmware 
crash due to a possible fw bug. Once this is addressed I will be able to 
send you the test results you requested on VM start/stop with the erratum 
1041 patches applied.


> Host: Ubuntu Artful 4.13 kernel with *no* erratum 1041 patches applied.
> Guest: Ubuntu Zesty (4.10) kernel.
>
> - Created 20 VMs one at a time
>
> In a loop:
> - Stop (virsh destroy) 20 VMs one at a time
> - Start (virsh start) 20 VMs one at a time.
>
> And, I am able to reproduce the system reset issue I previously reported. I 
> think the problem I reported with VMs might have nothing to do with the 
> erratum 1041 patches, and probably needs to be root caused seperately.
>
> With stock 4.13 kernel (no erratum 1041 patches applied):
>
> awrep6 login: [  461.881379] ACPI CPPC: PCC check channel failed. Status=0
> [  462.051194] ACPI CPPC: PCC check channel failed. Status=0
> [  462.223137] ACPI CPPC: PCC check channel failed. Status=0
> [  462.633790] ACPI CPPC: PCC check channel failed. Status=0
> [  463.231971] ACPI CPPC: PCC check channel failed. Status=0
> [  463.403163] ACPI CPPC: PCC check channel failed. Status=0
> [  463.822936] ACPI CPPC: PCC check channel failed. Status=0
> [  463.995222] ACPI CPPC: PCC check channel failed. Status=0
> [  464.130962] ACPI CPPC: PCC check channel failed. Status=0
> [  464.258973] ACPI CPPC: PCC check channel failed. Status=0
> [  465.283028] ACPI CPPC: PCC check channel failed. Status=0
>
>
> SYS_DBG: Running SDI image (immediate mode)
> SYS_DBG: Ram Dump Init
> SYS_DBG: Failed to init SD card
> SYS_DBG: Resetting system!
>
>
> On Thu, 9 Nov 2017, Manoj Iyer wrote:
>
>> 
>> 
>> 
>> On Thu, 9 Nov 2017, Manoj Iyer wrote:
>> 
>>> 
>>> James,
>>> 
>>> (sorry for top-posting)
>>> 
>>> Applied patch 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )
>>> 
>>> - Start 20 VMs one at a time
>>> 
>>> In a loop:
>>> - Stop (virsh destroy) 20 VMs one at a time
>>> - Start (virsh start) 20 VMs one at a time.
>> 
>> Fixing some confusion I might have introduced in my prev email.
>> 
>> - Applied all 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )
>> 
>> - Created 20 VMs one at a time
>> 
>> In a loop:
>> - Stop (virsh destroy) 20 VMs one at a time
>> - Start (virsh start) 20 VMs one at a time.
>> 
>>> 
>>> The system reset's itself after starting the last VM on the 1st loop 
>>> displaying the following:
>>> 
>>> awrep6 login: [ 603.349141] ACPI CPPC: PCC check channel failed. Status=0
>>> [ 603.765101] ACPI CPPC: PCC check channel failed. Status=0
>>> [ 603.937389] ACPI CPPC: PCC check channel failed. Status=0
>>> [ 608.285495] ACPI CPPC: PCC check channel failed. Status=0
>>> [ 608.289481] ACPI CPPC: PCC check channel failed. Status=0
>>> 
>>> SYS_DBG: Running SDI image (immediate mode)
>>> SYS_DBG: Ram Dump Init
>>> SYS_DBG: Failed to init SD card
>>> SYS_DBG: Resetting system!
>>> 
>>> Followed by the following messages on system reboot:
>>> [ 6.616891] BERT: Error records from previous boot:
>>> [ 6.621655] [Hardware Error]: event severity: fatal
>>> [ 6.626516] [Hardware Error]: imprecise tstamp: 0000-00-00 00:00:00
>>> [ 6.632851] [Hardware Error]: Error 0, type: fatal
>>> [ 6.637713] [Hardware Error]: section type: unknown, 
>>> d2e2621c-f936-468d-0d84-15a4ed015c8b
>>> [ 6.646045] [Hardware Error]: section length: 0x238
>>> [ 6.651082] [Hardware Error]: 00000000: 72724502 5220726f 6f736165 
>>> 6e55206e .Error Reason Un
>>> [ 6.659761] [Hardware Error]: 00000010: 776f6e6b 0000006e 00000000 
>>> 00000000 known...........
>>> [ 6.668442] [Hardware Error]: 00000020: 00000000 00000000 00000000 
>>> 00000000 ................
>>> [ 6.677122] [Hardware Error]: 00000030: 00000000 00000000 00000000 
>>> 00000000 ................
>>> 
>>> 
>>> On Thu, 9 Nov 2017, James Morse wrote:
>>> 
>>>> Hi Manoj,
>>>> 
>>>> On 08/11/17 19:05, Manoj Iyer wrote:
>>>>> On Thu, 2 Nov 2017, Shanker Donthineni wrote:
>>>>>> The ARM architecture defines the memory locations that are permitted
>>>>>> to be accessed as the result of a speculative instruction fetch from
>>>>>> an exception level for which all stages of translation are disabled.
>>>>>> Specifically, the core is permitted to speculatively fetch from the
>>>>>> 4KB region containing the current program counter and next 4KB.
>>>>>> 
>>>>>> When translation is changed from enabled to disabled for the running
>>>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>>>> Falkor core may errantly speculatively access memory locations outside
>>>>>> of the 4KB region permitted by the architecture. The errant memory
>>>>>> access may lead to one of the following unexpected behaviors.
>>>> 
>>>>> I applied the 3 patches to Ubuntu 4.13.0-16-generic (Artful) kernel and
>>>>> ran stress-ng cpu tests on QDF2400 server
>>>> 
>>>> [...]
>>>> 
>>>>> Where stress-ng would spawn N workers and test cpu offline/online, 
>>>>> perform
>>>>> matrix operations, do rapid context switchs, and anonymous mmaps. 
>>>>> Although
>>>>> I was not able to reproduce the erratum on the stock 4.13 kernel using 
>>>>> the
>>>>> same test case, the patched kernel did not seem to introduce any
>>>>> regressions either. I ran the stress-ng tests for over 8hrs found the
>>>>> system to be stable.
>>>> 
>>>> 
>>>> Could you throw kexec and KVM into the mix? This issue only shows up when 
>>>> we
>>>> disable the MMU, which we almost never do.
>>>> 
>>>> For CPU offline/online we make the PSCI 'offline' call with the MMU 
>>>> enabled.
>>>> When the CPU comes back firmware has reset the EL2/EL1 SCTLR from a 
>>>> higher
>>>> exception level, so it won't hit this issue.
>>>> 
>>>> One place we do this is kexec, where we drop into purgatory with the MMU 
>>>> disabled.
>>>> 
>>>> The other is KVM unloading itself to return to the hyp stub. You can 
>>>> stress this
>>>> by starting and stopping a VM. When the number of VMs reaches 0 KVM 
>>>> should
>>>> unload via 'kvm_arch_hardware_disable()'.
>>>> 
>>>> 
>>>> Thanks,
>>>> 
>>>> James
>>>> 
>>>> 
>>> 
>>> --
>>> ============================
>>> Manoj Iyer
>>> Ubuntu/Canonical
>>> ARM Servers - Cloud
>>> ============================
>>> 
>>> 
>> 
>> --
>> ============================
>> Manoj Iyer
>> Ubuntu/Canonical
>> ARM Servers - Cloud
>> ============================
>> 
>> 
>
> --
> ============================
> Manoj Iyer
> Ubuntu/Canonical
> ARM Servers - Cloud
> ============================
>
>

--
============================
Manoj Iyer
Ubuntu/Canonical
ARM Servers - Cloud
============================

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

[Shanker Donthineni]

Hi James,

On 11/10/2017 04:24 AM, James Morse wrote:
> Hi Shanker,
> 
> On 09/11/17 15:22, Shanker Donthineni wrote:
>> On 11/09/2017 05:08 AM, James Morse wrote:
>>> On 04/11/17 21:43, Shanker Donthineni wrote:
>>>> On 11/03/2017 10:11 AM, Robin Murphy wrote:
>>>>> On 03/11/17 03:27, Shanker Donthineni wrote:
>>>>>> The ARM architecture defines the memory locations that are permitted
>>>>>> to be accessed as the result of a speculative instruction fetch from
>>>>>> an exception level for which all stages of translation are disabled.
>>>>>> Specifically, the core is permitted to speculatively fetch from the
>>>>>> 4KB region containing the current program counter and next 4KB.
>>>>>>
>>>>>> When translation is changed from enabled to disabled for the running
>>>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>>>> Falkor core may errantly speculatively access memory locations outside
>>>>>> of the 4KB region permitted by the architecture. The errant memory
>>>>>> access may lead to one of the following unexpected behaviors.
>>>>>>
>>>>>> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>>>>>>    to the errant memory access attempting to access a region of memory
>>>>>>    that is protected by a slave-side memory protection unit.
>>>>>> 2) Unpredictable device behavior due to a speculative read from device
>>>>>>    memory. This behavior may only occur if the instruction cache is
>>>>>>    disabled prior to or coincident with translation being changed from
>>>>>>    enabled to disabled.
>>>>>>
>>>>>> To avoid the errant behavior, software must execute an ISB immediately
>>>>>> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
>>>>>> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
>>>>>> index b6dfb4f..4c91efb 100644
>>>>>> --- a/arch/arm64/include/asm/assembler.h
>>>>>> +++ b/arch/arm64/include/asm/assembler.h
>>>>>> @@ -514,6 +515,22 @@
>>>>>>   *   reg: the value to be written.
>>>>>>   */
>>>>>>  	.macro	write_sctlr, eln, reg
>>>>>> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>>>>> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
>>>>>> +	tbnz    \reg, #0, 8000f          // enable MMU?
>>>
>>> Won't this match any change that leaves the MMU enabled?
>>
>> Yes. No need to apply workaround if the MMU is going to be enabled.
> 
> (Sorry, looks like I had this upside down)
> 
> My badly-made-point is you can't know if the MMU is being disabled unless you
> have both the old and new values.
> 
> As an example, in el2_setup, (where the MMU is disabled), we set the EE/E0E bits
> to match the kernel's endianness. Won't your macro will insert an unnecessary
> isb? Is this needed for the errata workaround?
> 

Yes, It's not required in this case. I'll post a v2 patch and apply the workaround
where it's absolutely required. Seems handling a workaround inside helper macros
causing confusion.

> 
>>> I think the macro is making this more confusing. Disabling the MMU is obvious
>>> from the call-site, (and really rare!). Trying to work it out from a macro makes
>>> it more complicated than necessary.
> 
>> Not clear, are you suggesting not to use read{write}_sctlr() macros instead apply 
>> the workaround from the call-site based on the MMU-on status?
> 
> Yes. This is the only way to patch only the locations that turn the MMU off.
> 
> 
>> If yes, It simplifies
>> the code logic but CONFIG_QCOM_FALKOR_ERRATUM_1041 references are scatter everywhere. 
> 
> Wouldn't they only appear in the places that are affected by the errata?
> This is exactly what we want, anyone touching that code now knows they need to
> double check this behaviour, (and ask you to test it!).
> 
> Otherwise we have a macro second guessing what is happening, if its not quite
> right (because some information has been lost), we're now not sure what we need
> to do if we ever refactor any of this code.
> 
> [...]
> 
>>>> I'll prefer alternatives
>>>> just to avoid the unnecessary overhead on future Qualcomm Datacenter
>>>> server CPUs and regression on other CPUs because of inserting an ISB
>>>
>>> I think hiding errata on other CPUs is a good argument.
>>>
>>> My suggestion would be:
>>>> #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
>>>> 	isb
>>>> #endif
>>>
>>> In head.S and efi-entry.S, as these run before alternatives.
>>> Then use alternatives to add just the isb in the mmu-off path for the other callers.
> 
>> Thanks for your opinion on this one, I'll change to an unconditional ISB in v2 patch.
>> After this change the enable_mmu() issues two ISBs before writing to SCTLR_EL1.
> 
> Another great reason not to wrap this in a macro, there may already be a
> suitable isb, in which case a comment will suffice.
> 
> 
>> Are you okay with this behavior?
> 
> Back-to-back isb doesn't sound like a good idea.
> 
> 
>>  ENTRY(__enable_mmu)
>>         mrs     x1, ID_AA64MMFR0_EL1
>>         ubfx    x2, x1, #ID_AA64MMFR0_TGRAN_SHIFT, 4
>>         cmp     x2, #ID_AA64MMFR0_TGRAN_SUPPORTED
>>         b.ne    __no_granule_support
>>         update_early_cpu_boot_status 0, x1, x2
>>         adrp    x1, idmap_pg_dir
>>         adrp    x2, swapper_pg_dir
>>         msr     ttbr0_el1, x1                   // load TTBR0
>>         msr     ttbr1_el1, x2                   // load TTBR1
>>         isb
>>         write_sctlr el1, x0
>>         isb
> 
> Now I'm thoroughly confused. Isn't this one of the sequences that doesn't hit
> the issue? Here we're switching SCTLR.M from 0 to 1.
> 
> 
> Thanks,
> 
> James
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [3/3] arm64: Add software workaround for Falkor erratum 1041

[Manoj Iyer]

On Fri, 10 Nov 2017, Manoj Iyer wrote:

> On Thu, 9 Nov 2017, Manoj Iyer wrote:
>
>> 
>> James,
>> 
>> Looks like my VM test raised a false alarm. I retested stock Artful 4.13 
>> kernel (No erratum 1041 patches applied).
>> 
>
> James, an update on the crash (false alarm). We suspect this is a firmware 
> crash due to a possible fw bug. Once this is addressed I will be able to send 
> you the test results you requested on VM start/stop with the erratum 1041 
> patches applied.
>

James/Shanker,

I can report that VM start/stop/restart tests worked with the patches 
applied to Ubuntu 4.13 (Artful) kernel on the qdf2400 hardware.

Host: Ubuntu 4.13 with Erratum 1041 patches applied
Guest: Stock Ubuntu 4.13 kernel

- create 20 vms one at a time

10 iteration of:
- Stop (virsh destroy) 20 VMs one at a time
- Start (virsh start) 20 VMs one at a time.

Tested-by: Manoj Iyer <manoj.iyer@canonical.com>

>
>> Host: Ubuntu Artful 4.13 kernel with *no* erratum 1041 patches applied.
>> Guest: Ubuntu Zesty (4.10) kernel.
>> 
>> - Created 20 VMs one at a time
>> 
>> In a loop:
>> - Stop (virsh destroy) 20 VMs one at a time
>> - Start (virsh start) 20 VMs one at a time.
>> 
>> And, I am able to reproduce the system reset issue I previously reported. I 
>> think the problem I reported with VMs might have nothing to do with the 
>> erratum 1041 patches, and probably needs to be root caused seperately.
>> 
>> With stock 4.13 kernel (no erratum 1041 patches applied):
>> 
>> awrep6 login: [  461.881379] ACPI CPPC: PCC check channel failed. Status=0
>> [  462.051194] ACPI CPPC: PCC check channel failed. Status=0
>> [  462.223137] ACPI CPPC: PCC check channel failed. Status=0
>> [  462.633790] ACPI CPPC: PCC check channel failed. Status=0
>> [  463.231971] ACPI CPPC: PCC check channel failed. Status=0
>> [  463.403163] ACPI CPPC: PCC check channel failed. Status=0
>> [  463.822936] ACPI CPPC: PCC check channel failed. Status=0
>> [  463.995222] ACPI CPPC: PCC check channel failed. Status=0
>> [  464.130962] ACPI CPPC: PCC check channel failed. Status=0
>> [  464.258973] ACPI CPPC: PCC check channel failed. Status=0
>> [  465.283028] ACPI CPPC: PCC check channel failed. Status=0
>> 
>> 
>> SYS_DBG: Running SDI image (immediate mode)
>> SYS_DBG: Ram Dump Init
>> SYS_DBG: Failed to init SD card
>> SYS_DBG: Resetting system!
>> 
>> 
>> On Thu, 9 Nov 2017, Manoj Iyer wrote:
>> 
>>> 
>>> 
>>> 
>>> On Thu, 9 Nov 2017, Manoj Iyer wrote:
>>> 
>>>> 
>>>> James,
>>>> 
>>>> (sorry for top-posting)
>>>> 
>>>> Applied patch 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )
>>>> 
>>>> - Start 20 VMs one at a time
>>>> 
>>>> In a loop:
>>>> - Stop (virsh destroy) 20 VMs one at a time
>>>> - Start (virsh start) 20 VMs one at a time.
>>> 
>>> Fixing some confusion I might have introduced in my prev email.
>>> 
>>> - Applied all 3 patches to Ubuntu Artful Kernel ( 4.13.0-16-generic )
>>> 
>>> - Created 20 VMs one at a time
>>> 
>>> In a loop:
>>> - Stop (virsh destroy) 20 VMs one at a time
>>> - Start (virsh start) 20 VMs one at a time.
>>> 
>>>> 
>>>> The system reset's itself after starting the last VM on the 1st loop 
>>>> displaying the following:
>>>> 
>>>> awrep6 login: [ 603.349141] ACPI CPPC: PCC check channel failed. Status=0
>>>> [ 603.765101] ACPI CPPC: PCC check channel failed. Status=0
>>>> [ 603.937389] ACPI CPPC: PCC check channel failed. Status=0
>>>> [ 608.285495] ACPI CPPC: PCC check channel failed. Status=0
>>>> [ 608.289481] ACPI CPPC: PCC check channel failed. Status=0
>>>> 
>>>> SYS_DBG: Running SDI image (immediate mode)
>>>> SYS_DBG: Ram Dump Init
>>>> SYS_DBG: Failed to init SD card
>>>> SYS_DBG: Resetting system!
>>>> 
>>>> Followed by the following messages on system reboot:
>>>> [ 6.616891] BERT: Error records from previous boot:
>>>> [ 6.621655] [Hardware Error]: event severity: fatal
>>>> [ 6.626516] [Hardware Error]: imprecise tstamp: 0000-00-00 00:00:00
>>>> [ 6.632851] [Hardware Error]: Error 0, type: fatal
>>>> [ 6.637713] [Hardware Error]: section type: unknown, 
>>>> d2e2621c-f936-468d-0d84-15a4ed015c8b
>>>> [ 6.646045] [Hardware Error]: section length: 0x238
>>>> [ 6.651082] [Hardware Error]: 00000000: 72724502 5220726f 6f736165 
>>>> 6e55206e .Error Reason Un
>>>> [ 6.659761] [Hardware Error]: 00000010: 776f6e6b 0000006e 00000000 
>>>> 00000000 known...........
>>>> [ 6.668442] [Hardware Error]: 00000020: 00000000 00000000 00000000 
>>>> 00000000 ................
>>>> [ 6.677122] [Hardware Error]: 00000030: 00000000 00000000 00000000 
>>>> 00000000 ................
>>>> 
>>>> 
>>>> On Thu, 9 Nov 2017, James Morse wrote:
>>>> 
>>>>> Hi Manoj,
>>>>> 
>>>>> On 08/11/17 19:05, Manoj Iyer wrote:
>>>>>> On Thu, 2 Nov 2017, Shanker Donthineni wrote:
>>>>>>> The ARM architecture defines the memory locations that are permitted
>>>>>>> to be accessed as the result of a speculative instruction fetch from
>>>>>>> an exception level for which all stages of translation are disabled.
>>>>>>> Specifically, the core is permitted to speculatively fetch from the
>>>>>>> 4KB region containing the current program counter and next 4KB.
>>>>>>> 
>>>>>>> When translation is changed from enabled to disabled for the running
>>>>>>> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
>>>>>>> Falkor core may errantly speculatively access memory locations outside
>>>>>>> of the 4KB region permitted by the architecture. The errant memory
>>>>>>> access may lead to one of the following unexpected behaviors.
>>>>> 
>>>>>> I applied the 3 patches to Ubuntu 4.13.0-16-generic (Artful) kernel and
>>>>>> ran stress-ng cpu tests on QDF2400 server
>>>>> 
>>>>> [...]
>>>>> 
>>>>>> Where stress-ng would spawn N workers and test cpu offline/online, 
>>>>>> perform
>>>>>> matrix operations, do rapid context switchs, and anonymous mmaps. 
>>>>>> Although
>>>>>> I was not able to reproduce the erratum on the stock 4.13 kernel using 
>>>>>> the
>>>>>> same test case, the patched kernel did not seem to introduce any
>>>>>> regressions either. I ran the stress-ng tests for over 8hrs found the
>>>>>> system to be stable.
>>>>> 
>>>>> 
>>>>> Could you throw kexec and KVM into the mix? This issue only shows up 
>>>>> when we
>>>>> disable the MMU, which we almost never do.
>>>>> 
>>>>> For CPU offline/online we make the PSCI 'offline' call with the MMU 
>>>>> enabled.
>>>>> When the CPU comes back firmware has reset the EL2/EL1 SCTLR from a 
>>>>> higher
>>>>> exception level, so it won't hit this issue.
>>>>> 
>>>>> One place we do this is kexec, where we drop into purgatory with the MMU 
>>>>> disabled.
>>>>> 
>>>>> The other is KVM unloading itself to return to the hyp stub. You can 
>>>>> stress this
>>>>> by starting and stopping a VM. When the number of VMs reaches 0 KVM 
>>>>> should
>>>>> unload via 'kvm_arch_hardware_disable()'.
>>>>> 
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> James
>>>>> 
>>>>> 
>>>> 
>>>> --
>>>> ============================
>>>> Manoj Iyer
>>>> Ubuntu/Canonical
>>>> ARM Servers - Cloud
>>>> ============================
>>>> 
>>>> 
>>> 
>>> --
>>> ============================
>>> Manoj Iyer
>>> Ubuntu/Canonical
>>> ARM Servers - Cloud
>>> ============================
>>> 
>>> 
>> 
>> --
>> ============================
>> Manoj Iyer
>> Ubuntu/Canonical
>> ARM Servers - Cloud
>> ============================
>> 
>> 
>
> --
> ============================
> Manoj Iyer
> Ubuntu/Canonical
> ARM Servers - Cloud
> ============================
>
>