Re: [PATCH v2 2/2] arm64: Add software workaround for Falkor erratum 1041
Hi,
Sorry, I've posted a wrong patch which causes the compilation errors.
Please disregard this patch, I posted v3 patch to fix the build
issue.
https://patchwork.kernel.org/patch/10055077/
On 11/12/2017 07:16 PM, 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 4K and next 4K.
>
> 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.
>
> The conditions leading to this erratum will not occur when either of the
> following occur:
> 1) A higher exception level disables translation of a lower exception level
>(e.g. EL2 changing SCTLR_EL1[M] from a value of 1 to 0).
> 2) An exception level disabling its stage-1 translation if its stage-2
> translation is enabled (e.g. EL1 changing SCTLR_EL1[M] from a value of 1
> to 0 when HCR_EL2[VM] has a value of 1).
>
> 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
> ---
> Documentation/arm64/silicon-errata.txt | 1 +
> arch/arm64/Kconfig | 10 ++
> arch/arm64/include/asm/assembler.h | 18 ++
> arch/arm64/include/asm/cpucaps.h | 3 ++-
> arch/arm64/kernel/cpu-reset.S | 1 +
> arch/arm64/kernel/cpu_errata.c | 16
> arch/arm64/kernel/efi-entry.S | 2 ++
> arch/arm64/kernel/head.S | 1 +
> arch/arm64/kernel/relocate_kernel.S| 1 +
> arch/arm64/kvm/hyp-init.S | 1 +
> 10 files changed, 53 insertions(+), 1 deletion(-)
>
> 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..8f73eac 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_E1041
> + 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 d58a625..eb11cdf 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -499,4 +499,22 @@
> #endif
> .endm
>
> +/**
> + * Errata workaround prior to disable MMU. Insert an ISB immediately prior
> + * to executing the MSR that will change SCTLR_ELn[M] from a value of 1 to 0.
> + */
> + .macro pre_disable_mmu_workaround
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_E1041
> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
> + isb
> +alternative_else_nop_endif
> +#endif
> + .end
> +
> + .macro pre_disable_mmu_early_workaround
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_E1041
> + isb
> +#endif
> + .end
> +
> #endif /* __ASM_ASSEMBLER_H */
> 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
[PATCH v3 1/2] arm64: Define cputype macros for Falkor CPU
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 --- 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_V10x800 +#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. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v3 2/2] arm64: Add software workaround for Falkor erratum 1041
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 4K and next 4K.
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.
The conditions leading to this erratum will not occur when either of the
following occur:
1) A higher exception level disables translation of a lower exception level
(e.g. EL2 changing SCTLR_EL1[M] from a value of 1 to 0).
2) An exception level disabling its stage-1 translation if its stage-2
translation is enabled (e.g. EL1 changing SCTLR_EL1[M] from a value of 1
to 0 when HCR_EL2[VM] has a value of 1).
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
---
Changes since v1:
Apply the workaround where it's required.
Changes since v2:
Repost the corrected patches.
Documentation/arm64/silicon-errata.txt | 1 +
arch/arm64/Kconfig | 10 ++
arch/arm64/include/asm/assembler.h | 19 +++
arch/arm64/include/asm/cpucaps.h | 3 ++-
arch/arm64/kernel/cpu-reset.S | 1 +
arch/arm64/kernel/cpu_errata.c | 16
arch/arm64/kernel/efi-entry.S | 2 ++
arch/arm64/kernel/head.S | 1 +
arch/arm64/kernel/relocate_kernel.S| 1 +
arch/arm64/kvm/hyp-init.S | 1 +
10 files changed, 54 insertions(+), 1 deletion(-)
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..8f73eac 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_E1041
+ 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 d58a625..dd9cec5 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
/*
* Enable and disable interrupts.
@@ -499,4 +500,22 @@
#endif
.endm
+/**
+ * Errata workaround prior to disable MMU. Insert an ISB immediately prior
+ * to executing the MSR that will change SCTLR_ELn[M] from a value of 1 to 0.
+ */
+ .macro pre_disable_mmu_workaround
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_E1041
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
+ isb
+alternative_else_nop_endif
+#endif
+ .endm
+
+ .macro pre_disable_mmu_early_workaround
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_E1041
+ isb
+#endif
+ .endm
+
#endif /* __ASM_ASSEMBLER_H */
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_85892119
#define ARM64_WORKAROUND_CAVIUM_30115 20
#define ARM64_HAS_DCPOP21
+#define ARM64_WORKAROUND_QCOM_FALKOR_E1041 22
-#define ARM64_NCAPS22
+#def
[PATCH v3 0/2] Implement a software workaround for Falkor erratum 1041
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. Patch2 from V1 series got dropped to accommodate review comments. Apply the workaround where it's required. Posted wrong the patches in v2. Patch1: - CPUTYPE definitions for Falkor CPU. Patch2: - Actual workaround changes for erratum E1041. Shanker Donthineni (2): arm64: Define cputype macros for Falkor CPU arm64: Add software workaround for Falkor erratum 1041 Documentation/arm64/silicon-errata.txt | 1 + arch/arm64/Kconfig | 10 ++ arch/arm64/include/asm/assembler.h | 18 ++ arch/arm64/include/asm/cpucaps.h | 3 ++- arch/arm64/include/asm/cputype.h | 2 ++ arch/arm64/kernel/cpu-reset.S | 1 + arch/arm64/kernel/cpu_errata.c | 16 arch/arm64/kernel/efi-entry.S | 2 ++ arch/arm64/kernel/head.S | 1 + arch/arm64/kernel/relocate_kernel.S| 1 + arch/arm64/kvm/hyp-init.S | 1 + 11 files changed, 55 insertions(+), 1 deletion(-) -- 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. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v2 0/2] Implement a software workaround for Falkor erratum 1041
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. Patch2 from V1 series got dropped to accommodate review comments. Apply the workaround where it's required. Patch1: - CPUTYPE definitions for Falkor CPU. Patch2: - Actual workaround changes for erratum E1041. Shanker Donthineni (2): arm64: Define cputype macros for Falkor CPU arm64: Add software workaround for Falkor erratum 1041 Documentation/arm64/silicon-errata.txt | 1 + arch/arm64/Kconfig | 10 ++ arch/arm64/include/asm/assembler.h | 18 ++ arch/arm64/include/asm/cpucaps.h | 3 ++- arch/arm64/include/asm/cputype.h | 2 ++ arch/arm64/kernel/cpu-reset.S | 1 + arch/arm64/kernel/cpu_errata.c | 16 arch/arm64/kernel/efi-entry.S | 2 ++ arch/arm64/kernel/head.S | 1 + arch/arm64/kernel/relocate_kernel.S| 1 + arch/arm64/kvm/hyp-init.S | 1 + 11 files changed, 55 insertions(+), 1 deletion(-) -- 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. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v2 2/2] arm64: Add software workaround for Falkor erratum 1041
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 4K and next 4K.
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.
The conditions leading to this erratum will not occur when either of the
following occur:
1) A higher exception level disables translation of a lower exception level
(e.g. EL2 changing SCTLR_EL1[M] from a value of 1 to 0).
2) An exception level disabling its stage-1 translation if its stage-2
translation is enabled (e.g. EL1 changing SCTLR_EL1[M] from a value of 1
to 0 when HCR_EL2[VM] has a value of 1).
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
---
Documentation/arm64/silicon-errata.txt | 1 +
arch/arm64/Kconfig | 10 ++
arch/arm64/include/asm/assembler.h | 18 ++
arch/arm64/include/asm/cpucaps.h | 3 ++-
arch/arm64/kernel/cpu-reset.S | 1 +
arch/arm64/kernel/cpu_errata.c | 16
arch/arm64/kernel/efi-entry.S | 2 ++
arch/arm64/kernel/head.S | 1 +
arch/arm64/kernel/relocate_kernel.S| 1 +
arch/arm64/kvm/hyp-init.S | 1 +
10 files changed, 53 insertions(+), 1 deletion(-)
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..8f73eac 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_E1041
+ 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 d58a625..eb11cdf 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -499,4 +499,22 @@
#endif
.endm
+/**
+ * Errata workaround prior to disable MMU. Insert an ISB immediately prior
+ * to executing the MSR that will change SCTLR_ELn[M] from a value of 1 to 0.
+ */
+ .macro pre_disable_mmu_workaround
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_E1041
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
+ isb
+alternative_else_nop_endif
+#endif
+ .end
+
+ .macro pre_disable_mmu_early_workaround
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_E1041
+ isb
+#endif
+ .end
+
#endif /* __ASM_ASSEMBLER_H */
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_85892119
#define ARM64_WORKAROUND_CAVIUM_30115 20
#define ARM64_HAS_DCPOP21
+#define ARM64_WORKAROUND_QCOM_FALKOR_E1041 22
-#define ARM64_NCAPS22
+#define ARM64_NCAPS23
#endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S
index 65f42d2..2a752cb 100644
--- a/arch/arm64/kernel/cpu-reset.S
[PATCH v2 1/2] arm64: Define cputype macros for Falkor CPU
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 --- 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_V10x800 +#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. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041
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
