Skip to contentSkip to conversation

arm64: Define Falkor v1 CPU

PATCH Superseded Christopher Covington 6 participants 26 messages Jan 11, 2017 – Jan 24, 2017

Subsystems (4)
Documentation KVM arm64 KVM arm64

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Christopher Covington

author
#0 lore raw

PATCH v3 1/5 arm64: Define Falkor v1 CPU

From: Shanker Donthineni <[email protected]>

Define the MIDR implementer and part number field values for the Qualcomm Datacenter Technologies Falkor processor version 1 in the usual manner.

Signed-off-by Shanker Donthineni <[email protected]>
Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/include/asm/cputype.h | 4 ++++
 1 file changed, 4 insertions(+)
arch/arm64/include/asm/cputype.h +4 -0
index 26a68dd..ee60561 100644
@@ -71,6 +71,7 @@
 #define ARM_CPU_IMP_APM			0x50
 #define ARM_CPU_IMP_CAVIUM		0x43
 #define ARM_CPU_IMP_BRCM		0x42
+#define ARM_CPU_IMP_QCOM		0x51
 
 #define ARM_CPU_PART_AEM_V8		0xD0F
 #define ARM_CPU_PART_FOUNDATION		0xD00

@@ -84,10 +85,13 @@
 
 #define BRCM_CPU_PART_VULCAN		0x516
 
+#define QCOM_CPU_PART_FALKOR_V1		0x800
+
 #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)
 #define MIDR_THUNDERX	MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)
 #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX)
+#define MIDR_QCOM_FALKOR_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, QCOM_CPU_PART_FALKOR_V1)
 
 #ifndef __ASSEMBLY__
Signature · 3 lines
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.
author
#1 lore raw

PATCH v3 2/5 arm64: Work around Falkor erratum 1003

From: Shanker Donthineni <[email protected]>

On the Qualcomm Datacenter Technologies Falkor v1 CPU, memory accesses may allocate TLB entries using an incorrect ASID when TTBRx_EL1 is being updated. Changing the TTBRx_EL1[ASID] and TTBRx_EL1[BADDR] fields separately using a reserved ASID will ensure that there are no TLB entries with incorrect ASID after changing the the ASID.

Pseudo code:
write TTBRx_EL1[ASID] to a reserved value
ISB
write TTBRx_EL1[BADDR] to a desired value
ISB
write TTBRx_EL1[ASID] to a desired value
ISB

EL2 and EL3 code changing the EL1&0 ASID is not subject to this erratum because hardware is prohibited from performing translations from an out-of-context translation regime.

Signed-off-by Shanker Donthineni <[email protected]>
Signed-off-by Christopher Covington <[email protected]>
---
 Documentation/arm64/silicon-errata.txt | 43 +++++++++++++++++-----------------
 arch/arm64/Kconfig                     | 11 +++++++++
 arch/arm64/include/asm/cpucaps.h       |  3 ++-
 arch/arm64/include/asm/mmu_context.h   |  8 ++++++-
 arch/arm64/kernel/cpu_errata.c         |  7 ++++++
 arch/arm64/mm/context.c                | 10 ++++++++
 arch/arm64/mm/proc.S                   | 13 ++++++++++
 7 files changed, 72 insertions(+), 23 deletions(-)
Documentation/arm64/silicon-errata.txt +22 -21
index 405da11..7151aed 100644
@@ -42,24 +42,25 @@ file acts as a registry of software workarounds in the Linux Kernel and
 will be updated when new workarounds are committed and backported to
 stable kernels.
 
-| Implementor    | Component       | Erratum ID      | Kconfig                 |
-+----------------+-----------------+-----------------+-------------------------+
-| ARM            | Cortex-A53      | #826319         | ARM64_ERRATUM_826319    |
-| ARM            | Cortex-A53      | #827319         | ARM64_ERRATUM_827319    |
-| ARM            | Cortex-A53      | #824069         | ARM64_ERRATUM_824069    |
-| ARM            | Cortex-A53      | #819472         | ARM64_ERRATUM_819472    |
-| ARM            | Cortex-A53      | #845719         | ARM64_ERRATUM_845719    |
-| ARM            | Cortex-A53      | #843419         | ARM64_ERRATUM_843419    |
-| ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075    |
-| ARM            | Cortex-A57      | #852523         | N/A                     |
-| ARM            | Cortex-A57      | #834220         | ARM64_ERRATUM_834220    |
-| ARM            | Cortex-A72      | #853709         | N/A                     |
-| ARM            | MMU-500         | #841119,#826419 | N/A                     |
-|                |                 |                 |                         |
-| Cavium         | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375    |
-| Cavium         | ThunderX ITS    | #23144          | CAVIUM_ERRATUM_23144    |
-| Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154    |
-| Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456    |
-| Cavium         | ThunderX SMMUv2 | #27704          | N/A		       |
-|                |                 |                 |                         |
-| Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585     |
+| Implementor   | Component       | Erratum ID      | Kconfig                  |
++---------------+-----------------+-----------------+--------------------------+
+| ARM           | Cortex-A53      | #826319         | ARM64_ERRATUM_826319     |
+| ARM           | Cortex-A53      | #827319         | ARM64_ERRATUM_827319     |
+| ARM           | Cortex-A53      | #824069         | ARM64_ERRATUM_824069     |
+| ARM           | Cortex-A53      | #819472         | ARM64_ERRATUM_819472     |
+| ARM           | Cortex-A53      | #845719         | ARM64_ERRATUM_845719     |
+| ARM           | Cortex-A53      | #843419         | ARM64_ERRATUM_843419     |
+| ARM           | Cortex-A57      | #832075         | ARM64_ERRATUM_832075     |
+| ARM           | Cortex-A57      | #852523         | N/A                      |
+| ARM           | Cortex-A57      | #834220         | ARM64_ERRATUM_834220     |
+| ARM           | Cortex-A72      | #853709         | N/A                      |
+| ARM           | MMU-500         | #841119,#826419 | N/A                      |
+|               |                 |                 |                          |
+| Cavium        | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375     |
+| Cavium        | ThunderX ITS    | #23144          | CAVIUM_ERRATUM_23144     |
+| Cavium        | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154     |
+| Cavium        | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456     |
+| Cavium        | ThunderX SMMUv2 | #27704          | N/A                      |
+|               |                 |                 |                          |
+| Freescale/NXP | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585      |
+| Qualcomm      | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003 |
arch/arm64/Kconfig +11 -0
index 1117421..2a80ac9 100644
@@ -479,6 +479,17 @@ config CAVIUM_ERRATUM_27456
 
 	  If unsure, say Y.
 
+config QCOM_FALKOR_ERRATUM_1003
+	bool "Falkor E1003: Incorrect translation due to ASID change"
+	default y
+	help
+	  An incorrect translation TLBI entry may be created while changing the
+	  ASID and translation table address together for TTBR0_EL1. The
+	  workaround for this issue is to use a reserved ASID in
+	  cpu_do_switch_mm() before switching to the target ASID.
+
+	  If unsure, say Y.
+
 endmenu
arch/arm64/include/asm/cpucaps.h +2 -1
index 4174f09..5aaf7ee 100644
@@ -35,7 +35,8 @@
 #define ARM64_HYP_OFFSET_LOW			14
 #define ARM64_MISMATCHED_CACHE_LINE_SIZE	15
 #define ARM64_HAS_NO_FPSIMD			16
+#define ARM64_WORKAROUND_QCOM_FALKOR_E1003	17
 
-#define ARM64_NCAPS				17
+#define ARM64_NCAPS				18
 
 #endif /* __ASM_CPUCAPS_H */
arch/arm64/include/asm/mmu_context.h +7 -1
index 0363fe8..9632b05 100644
@@ -19,6 +19,10 @@
 #ifndef __ASM_MMU_CONTEXT_H
 #define __ASM_MMU_CONTEXT_H
 
+#define FALKOR_RESERVED_ASID	1
+
+#ifndef __ASSEMBLY__
+
 #include <linux/compiler.h>
 #include <linux/sched.h>
 

@@ -220,4 +224,6 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 
 void verify_cpu_asid_bits(void);
 
-#endif
+#endif /* !__ASSEMBLY__ */
+
+#endif /* !__ASM_MMU_CONTEXT_H */
arch/arm64/kernel/cpu_errata.c +7 -0
index b75e917..787b542 100644
@@ -130,6 +130,13 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 		.def_scope = SCOPE_LOCAL_CPU,
 		.enable = cpu_enable_trap_ctr_access,
 	},
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+	{
+		.desc = "Qualcomm Falkor erratum 1003",
+		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1003,
+		MIDR_RANGE(MIDR_QCOM_FALKOR_V1, 0x00, 0x00),
+	},
+#endif
 	{
 	}
 };
arch/arm64/mm/context.c +10 -0
index 4c63cb1..5a0a82a 100644
@@ -87,6 +87,11 @@ static void flush_context(unsigned int cpu)
 	/* Update the list of reserved ASIDs and the ASID bitmap. */
 	bitmap_clear(asid_map, 0, NUM_USER_ASIDS);
 
+	/* Reserve ASID for Falkor erratum 1003 */
+	if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) &&
+	    cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003))
+		__set_bit(FALKOR_RESERVED_ASID, asid_map);
+
 	/*
 	 * Ensure the generation bump is observed before we xchg the
 	 * active_asids.

@@ -244,6 +249,11 @@ static int asids_init(void)
 		panic("Failed to allocate bitmap for %lu ASIDs\n",
 		      NUM_USER_ASIDS);
 
+	/* Reserve ASID for Falkor erratum 1003 */
+	if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) &&
+	    cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003))
+		__set_bit(FALKOR_RESERVED_ASID, asid_map);
+
 	pr_info("ASID allocator initialised with %lu entries\n", NUM_USER_ASIDS);
 	return 0;
 }
arch/arm64/mm/proc.S +13 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround
Signature · 3 lines
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.
maintainer
#2 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Some minor comments below, nothing fundamental (as long as you say the new sequence doesn't have the speculative TLB load problem I mentioned on a previous version).

Quoted 41 lines · Christopher Covington
Documentation/arm64/silicon-errata.txt +12 -21
index 405da11..7151aed 100644
@@ -42,24 +42,25 @@ file acts as a registry of software workarounds in the Linux Kernel and
 will be updated when new workarounds are committed and backported to
 stable kernels.
 
-| Implementor    | Component       | Erratum ID      | Kconfig                 |
-+----------------+-----------------+-----------------+-------------------------+
-| ARM            | Cortex-A53      | #826319         | ARM64_ERRATUM_826319    |
-| ARM            | Cortex-A53      | #827319         | ARM64_ERRATUM_827319    |
-| ARM            | Cortex-A53      | #824069         | ARM64_ERRATUM_824069    |
-| ARM            | Cortex-A53      | #819472         | ARM64_ERRATUM_819472    |
-| ARM            | Cortex-A53      | #845719         | ARM64_ERRATUM_845719    |
-| ARM            | Cortex-A53      | #843419         | ARM64_ERRATUM_843419    |
-| ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075    |
-| ARM            | Cortex-A57      | #852523         | N/A                     |
-| ARM            | Cortex-A57      | #834220         | ARM64_ERRATUM_834220    |
-| ARM            | Cortex-A72      | #853709         | N/A                     |
-| ARM            | MMU-500         | #841119,#826419 | N/A                     |
-|                |                 |                 |                         |
-| Cavium         | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375    |
-| Cavium         | ThunderX ITS    | #23144          | CAVIUM_ERRATUM_23144    |
-| Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154    |
-| Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456    |
-| Cavium         | ThunderX SMMUv2 | #27704          | N/A		       |
-|                |                 |                 |                         |
-| Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585     |
+| Implementor   | Component       | Erratum ID      | Kconfig                  |
++---------------+-----------------+-----------------+--------------------------+
+| ARM           | Cortex-A53      | #826319         | ARM64_ERRATUM_826319     |
+| ARM           | Cortex-A53      | #827319         | ARM64_ERRATUM_827319     |
+| ARM           | Cortex-A53      | #824069         | ARM64_ERRATUM_824069     |
+| ARM           | Cortex-A53      | #819472         | ARM64_ERRATUM_819472     |
+| ARM           | Cortex-A53      | #845719         | ARM64_ERRATUM_845719     |
+| ARM           | Cortex-A53      | #843419         | ARM64_ERRATUM_843419     |
+| ARM           | Cortex-A57      | #832075         | ARM64_ERRATUM_832075     |
+| ARM           | Cortex-A57      | #852523         | N/A                      |
+| ARM           | Cortex-A57      | #834220         | ARM64_ERRATUM_834220     |
+| ARM           | Cortex-A72      | #853709         | N/A                      |

Documentation/arm64/silicon-errata.txt file acts as a registry of software workarounds in the Linux Kernel and · line 66

Quoted 10 lines
Documentation/arm64/silicon-errata.txt +10 -0
+| ARM           | MMU-500         | #841119,#826419 | N/A                      |
+|               |                 |                 |                          |
+| Cavium        | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375     |
+| Cavium        | ThunderX ITS    | #23144          | CAVIUM_ERRATUM_23144     |
+| Cavium        | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154     |
+| Cavium        | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456     |
+| Cavium        | ThunderX SMMUv2 | #27704          | N/A                      |
+|               |                 |                 |                          |
+| Freescale/NXP | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585      |
+| Qualcomm      | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003 |

Please don't change the "Implementor" column width, there is no point and it makes the patch harder to read (i.e. this hunk should only have one line).

Quoted 18 lines
arch/arm64/mm/context.c +5 -0
index 4c63cb1..5a0a82a 100644
@@ -87,6 +87,11 @@ static void flush_context(unsigned int cpu)
 	/* Update the list of reserved ASIDs and the ASID bitmap. */
 	bitmap_clear(asid_map, 0, NUM_USER_ASIDS);
 
+	/* Reserve ASID for Falkor erratum 1003 */
+	if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) &&
+	    cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003))
+		__set_bit(FALKOR_RESERVED_ASID, asid_map);
+
 	/*
 	 * Ensure the generation bump is observed before we xchg the
 	 * active_asids.

@@ -244,6 +249,11 @@ static int asids_init(void)
 		panic("Failed to allocate bitmap for %lu ASIDs\n",

arch/arm64/mm/context.c static int asids_init(void) · line 259

Quoted 10 lines
arch/arm64/mm/context.c +5 -0
 		      NUM_USER_ASIDS);
 
+	/* Reserve ASID for Falkor erratum 1003 */
+	if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) &&
+	    cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003))
+		__set_bit(FALKOR_RESERVED_ASID, asid_map);
+
 	pr_info("ASID allocator initialised with %lu entries\n", NUM_USER_ASIDS);
 	return 0;
 }

You could as well write a small static function in this file and call it twice.

Quoted 21 lines
arch/arm64/mm/proc.S +6 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR

arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 158

Quoted 10 lines
arch/arm64/mm/proc.S +7 -0
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround

Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.

Signature · 1 line
Catalin
#3 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 119 lines · Catalin Marinas

Some minor comments below, nothing fundamental (as long as you say the new sequence doesn't have the speculative TLB load problem I mentioned on a previous version).

Quoted 41 lines · Christopher Covington
Documentation/arm64/silicon-errata.txt +12 -21
index 405da11..7151aed 100644
@@ -42,24 +42,25 @@ file acts as a registry of software workarounds in the Linux Kernel and
 will be updated when new workarounds are committed and backported to
 stable kernels.
 
-| Implementor    | Component       | Erratum ID      | Kconfig                 |
-+----------------+-----------------+-----------------+-------------------------+
-| ARM            | Cortex-A53      | #826319         | ARM64_ERRATUM_826319    |
-| ARM            | Cortex-A53      | #827319         | ARM64_ERRATUM_827319    |
-| ARM            | Cortex-A53      | #824069         | ARM64_ERRATUM_824069    |
-| ARM            | Cortex-A53      | #819472         | ARM64_ERRATUM_819472    |
-| ARM            | Cortex-A53      | #845719         | ARM64_ERRATUM_845719    |
-| ARM            | Cortex-A53      | #843419         | ARM64_ERRATUM_843419    |
-| ARM            | Cortex-A57      | #832075         | ARM64_ERRATUM_832075    |
-| ARM            | Cortex-A57      | #852523         | N/A                     |
-| ARM            | Cortex-A57      | #834220         | ARM64_ERRATUM_834220    |
-| ARM            | Cortex-A72      | #853709         | N/A                     |
-| ARM            | MMU-500         | #841119,#826419 | N/A                     |
-|                |                 |                 |                         |
-| Cavium         | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375    |
-| Cavium         | ThunderX ITS    | #23144          | CAVIUM_ERRATUM_23144    |
-| Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154    |
-| Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456    |
-| Cavium         | ThunderX SMMUv2 | #27704          | N/A		       |
-|                |                 |                 |                         |
-| Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585     |
+| Implementor   | Component       | Erratum ID      | Kconfig                  |
++---------------+-----------------+-----------------+--------------------------+
+| ARM           | Cortex-A53      | #826319         | ARM64_ERRATUM_826319     |
+| ARM           | Cortex-A53      | #827319         | ARM64_ERRATUM_827319     |
+| ARM           | Cortex-A53      | #824069         | ARM64_ERRATUM_824069     |
+| ARM           | Cortex-A53      | #819472         | ARM64_ERRATUM_819472     |
+| ARM           | Cortex-A53      | #845719         | ARM64_ERRATUM_845719     |
+| ARM           | Cortex-A53      | #843419         | ARM64_ERRATUM_843419     |
+| ARM           | Cortex-A57      | #832075         | ARM64_ERRATUM_832075     |
+| ARM           | Cortex-A57      | #852523         | N/A                      |
+| ARM           | Cortex-A57      | #834220         | ARM64_ERRATUM_834220     |
+| ARM           | Cortex-A72      | #853709         | N/A                      |

Documentation/arm64/silicon-errata.txt file acts as a registry of software workarounds in the Linux Kernel and · line 66

Quoted 10 lines
Documentation/arm64/silicon-errata.txt +10 -0
+| ARM           | MMU-500         | #841119,#826419 | N/A                      |
+|               |                 |                 |                          |
+| Cavium        | ThunderX ITS    | #22375, #24313  | CAVIUM_ERRATUM_22375     |
+| Cavium        | ThunderX ITS    | #23144          | CAVIUM_ERRATUM_23144     |
+| Cavium        | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154     |
+| Cavium        | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456     |
+| Cavium        | ThunderX SMMUv2 | #27704          | N/A                      |
+|               |                 |                 |                          |
+| Freescale/NXP | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585      |
+| Qualcomm      | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003 |

Please don't change the "Implementor" column width, there is no point and it makes the patch harder to read (i.e. this hunk should only have one line).

Quoted 18 lines
arch/arm64/mm/context.c +5 -0
index 4c63cb1..5a0a82a 100644
@@ -87,6 +87,11 @@ static void flush_context(unsigned int cpu)
 	/* Update the list of reserved ASIDs and the ASID bitmap. */
 	bitmap_clear(asid_map, 0, NUM_USER_ASIDS);
 
+	/* Reserve ASID for Falkor erratum 1003 */
+	if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) &&
+	    cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003))
+		__set_bit(FALKOR_RESERVED_ASID, asid_map);
+
 	/*
 	 * Ensure the generation bump is observed before we xchg the
 	 * active_asids.

@@ -244,6 +249,11 @@ static int asids_init(void)
 		panic("Failed to allocate bitmap for %lu ASIDs\n",

arch/arm64/mm/context.c static int asids_init(void) · line 259

Quoted 10 lines
arch/arm64/mm/context.c +5 -0
 		      NUM_USER_ASIDS);
 
+	/* Reserve ASID for Falkor erratum 1003 */
+	if (IS_ENABLED(CONFIG_QCOM_FALKOR_ERRATUM_1003) &&
+	    cpus_have_cap(ARM64_WORKAROUND_QCOM_FALKOR_E1003))
+		__set_bit(FALKOR_RESERVED_ASID, asid_map);
+
 	pr_info("ASID allocator initialised with %lu entries\n", NUM_USER_ASIDS);
 	return 0;
 }

You could as well write a small static function in this file and call it twice.

Quoted 26 lines
arch/arm64/mm/proc.S +11 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
Quoted 8 lines

arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 158

Quoted 5 lines
arch/arm64/mm/proc.S +2 -0
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround

Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.

In which case (and also for consistency), should we add that pre_ttbr0 macro to entry.S, just before __uaccess_ttbr0_enable? It may not be needed in the SW pan case, but it is probably worth entertaining the idea that there may be something to do there...

Thanks,

	M.
Signature · 1 line
Jazz is not dead. It just smells funny...
reviewer
#4 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 37 lines · Marc Zyngier
Quoted 27 lines · Catalin Marinas
Quoted 26 lines · Christopher Covington
arch/arm64/mm/proc.S +11 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
Quoted 8 lines

arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 158

Quoted 5 lines
arch/arm64/mm/proc.S +2 -0
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround

Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.

Quoted 4 lines

In which case (and also for consistency), should we add that pre_ttbr0 macro to entry.S, just before __uaccess_ttbr0_enable? It may not be needed in the SW pan case, but it is probably worth entertaining the idea that there may be something to do there...

Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0().

Thanks,
Mark.

maintainer
#5 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 38 lines · Mark Rutland
Quoted 37 lines · Marc Zyngier
Quoted 35 lines · Catalin Marinas
Quoted 21 lines · Christopher Covington
arch/arm64/mm/proc.S +6 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR

arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 158

Quoted 10 lines
arch/arm64/mm/proc.S +7 -0
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround

Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.

Quoted 6 lines
Quoted 4 lines

In which case (and also for consistency), should we add that pre_ttbr0 macro to entry.S, just before __uaccess_ttbr0_enable? It may not be needed in the SW pan case, but it is probably worth entertaining the idea that there may be something to do there...

Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0().

This may be fine if my assumptions about this erratum are correct. In the cpu_set_reserved_ttbr0() case we set TTBR0_EL1 to a table without any entries, so no new entries could be tagged with the old ASID.

Signature · 1 line
Catalin
reviewer
#6 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 7 lines · Catalin Marinas
Quoted 1 line · Mark Rutland

Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0().

This may be fine if my assumptions about this erratum are correct. In the cpu_set_reserved_ttbr0() case we set TTBR0_EL1 to a table without any entries, so no new entries could be tagged with the old ASID.

For some reason, I was under the impression that the issue was old table entries being allocated to the new ASID. Looking over the series again, it's not clear to me precisely which cases can occur.

It would be good to see that clarified.

Thanks,
Mark.

author
#7 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 9 lines · Mark Rutland
Quoted 7 lines · Catalin Marinas
Quoted 1 line · Mark Rutland

Likewise, I beleive we may need to modify cpu_set_reserved_ttbr0().

This may be fine if my assumptions about this erratum are correct. In the cpu_set_reserved_ttbr0() case we set TTBR0_EL1 to a table without any entries, so no new entries could be tagged with the old ASID.

Quoted 5 lines

For some reason, I was under the impression that the issue was old table entries being allocated to the new ASID. Looking over the series again, it's not clear to me precisely which cases can occur.

It would be good to see that clarified.

I'll add more general background the commit message in the next revision and I've also asked directly about this. The answer is that page table entries using the new translation table base address will be allocated into the TLB using the old ASID. If employing the workaround, it's possible for page table entries using the new translation table base to be allocated into the TLB using the reserved ASID.

Cov

Signature · 3 lines
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.
maintainer
#8 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

In reply to Marc Zyngier
Quoted 37 lines · Marc Zyngier
Quoted 27 lines · Catalin Marinas
Quoted 26 lines · Christopher Covington
arch/arm64/mm/proc.S +11 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
Quoted 8 lines

arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 158

Quoted 5 lines
arch/arm64/mm/proc.S +2 -0
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround

Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.

Quoted 4 lines

In which case (and also for consistency), should we add that pre_ttbr0 macro to entry.S, just before __uaccess_ttbr0_enable? It may not be needed in the SW pan case, but it is probably worth entertaining the idea that there may be something to do there...

It may actually be needed in entry.S as well. With SW PAN, we move the context switching from cpu_do_switch_mm to the kernel_exit macro when returning to user. In this case we are switching from the reserved ASID 0 and reserved TTBR0_EL1 (pointing to a zeroed page) to the user's TTBR0_EL1 and ASID. If the ASID switch isn't taken into account, we may end up with new TLB entries being tagged with the reserved ASID. Apart from a potential loss of protection with TTBR0 PAN, is there anything else that could go wrong? Maybe a TLB conflict if we mix TLBs from multiple address spaces tagged with the same reserved ASID.

If the above is an issue, we would need to patch __uaccess_ttbr0_enable() as well, though I'm more inclined to make this erratum not selectable when TTBR0 PAN is enabled.

Signature · 1 line
Catalin
#9 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 53 lines · Catalin Marinas
Quoted 37 lines · Marc Zyngier
Quoted 27 lines · Catalin Marinas
Quoted 26 lines · Christopher Covington
arch/arm64/mm/proc.S +11 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
Quoted 8 lines

arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 158

Quoted 5 lines
arch/arm64/mm/proc.S +2 -0
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround

Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.

Quoted 4 lines

In which case (and also for consistency), should we add that pre_ttbr0 macro to entry.S, just before __uaccess_ttbr0_enable? It may not be needed in the SW pan case, but it is probably worth entertaining the idea that there may be something to do there...

It may actually be needed in entry.S as well. With SW PAN, we move the context switching from cpu_do_switch_mm to the kernel_exit macro when returning to user. In this case we are switching from the reserved ASID 0 and reserved TTBR0_EL1 (pointing to a zeroed page) to the user's TTBR0_EL1 and ASID. If the ASID switch isn't taken into account, we may end up with new TLB entries being tagged with the reserved ASID. Apart from a potential loss of protection with TTBR0 PAN, is there anything else that could go wrong? Maybe a TLB conflict if we mix TLBs from multiple address spaces tagged with the same reserved ASID.

Quoted 3 lines

If the above is an issue, we would need to patch __uaccess_ttbr0_enable() as well, though I'm more inclined to make this erratum not selectable when TTBR0 PAN is enabled.

I don't think that's a reasonable approach. By all means change the default, but we need to support kernel images with both of these kconfig options enabled.

Will

reviewer
#10 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

In reply to Catalin Marinas
Quoted 11 lines · Catalin Marinas
Quoted 5 lines · Christopher Covington

-| Implementor | Component | Erratum ID | Kconfig |

+| Implementor | Component | Erratum ID | Kconfig |

+| Qualcomm | Falkor v1 | E1003 | QCOM_FALKOR_ERRATUM_1003 |

Please don't change the "Implementor" column width, there is no point and it makes the patch harder to read (i.e. this hunk should only have one line).

It'll need to affect all lines since the kconfig column needs to expand by at least one character to fit QCOM_FALKOR_ERRATUM_1003.

I beleive the intent here was to keep the table fitting into a width of 80 characters.

IMO we should allow the table to expand past 80 chars (everyone reading this file should be able to resize tehir terminal), and only expand the kconfig column.

Thanks,
Mark.

#11 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 2 lines · Mark Rutland

It'll need to affect all lines since the kconfig column needs to expand by at least one character to fit QCOM_FALKOR_ERRATUM_1003.

Or we can make the macro shorter.

Signature · 3 lines
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.
reviewer
#12 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 5 lines · Timur Tabi
Quoted 2 lines · Mark Rutland

It'll need to affect all lines since the kconfig column needs to expand by at least one character to fit QCOM_FALKOR_ERRATUM_1003.

Or we can make the macro shorter.

The name, as it is, is perfectly descriptive.

Let's not sacrifice legibility over a non-issue.

Thanks,
Mark.

#13 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 3 lines · Mark Rutland

The name, as it is, is perfectly descriptive.

Let's not sacrifice legibility over a non-issue.

I don't want to kick a dead horse or anything, but changing it to QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems without sacrificing anything.

Signature · 3 lines
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.
reviewer
#14 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Quoted 8 lines · Timur Tabi
Quoted 3 lines · Mark Rutland

The name, as it is, is perfectly descriptive.

Let's not sacrifice legibility over a non-issue.

I don't want to kick a dead horse or anything, but changing it to QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems without sacrificing anything.

The CPU is called "Falkor", not "FLKR", and we're not coming up with an ACPI table name...

The ARM Ltd. erratum numbers are global to all parts, so we don't include the part name. Is the 1003 erratum number specific to Falkor?

If it's global, you could use QCOM_ERRATUM_1003 instead.

Otherwise, QCOM_FALKOR_ERRATUM_1003 is preferable.

Thanks,
Mark.

author
#15 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

Hi Mark,

Quoted 10 lines · Mark Rutland
Quoted 8 lines · Timur Tabi
Quoted 3 lines · Mark Rutland

The name, as it is, is perfectly descriptive.

Let's not sacrifice legibility over a non-issue.

I don't want to kick a dead horse or anything, but changing it to QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems without sacrificing anything.

Quoted 7 lines

The CPU is called "Falkor", not "FLKR", and we're not coming up with an ACPI table name...

The ARM Ltd. erratum numbers are global to all parts, so we don't include the part name. Is the 1003 erratum number specific to Falkor?

If it's global, you could use QCOM_ERRATUM_1003 instead.

E1003 is specific to Falkor, and hopefully just its first major revision. Qualcomm Technology's first/previous generation ARMv8 custom microarchitecture used errata numbers below 1000. I am not aware of global coordination in the numbering, unfortunately.

Quoted 1 line

Otherwise, QCOM_FALKOR_ERRATUM_1003 is preferable.

Thanks,
Cov

Signature · 3 lines
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.
#16 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

In reply to Timur Tabi

[finally, some proper bikeshedding]

Quoted 8 lines · Timur Tabi
Quoted 3 lines · Mark Rutland

The name, as it is, is perfectly descriptive.

Let's not sacrifice legibility over a non-issue.

I don't want to kick a dead horse or anything, but changing it to QCOM_FLKR_ERRATUM_1003 would eliminate all the spacing problems without sacrificing anything.

Other than not being able to grep for the core name in the source tree, how do you suggest we pronounce FLKR? Because so far, it rolls off the tongue in an interesting way...

Thanks,

	M.
Signature · 1 line
Jazz is not dead. It just smells funny...
maintainer
#17 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

In reply to Mark Rutland
Quoted 10 lines · Mark Rutland
Quoted 5 lines · Timur Tabi
Quoted 2 lines · Mark Rutland

It'll need to affect all lines since the kconfig column needs to expand by at least one character to fit QCOM_FALKOR_ERRATUM_1003.

Or we can make the macro shorter.

The name, as it is, is perfectly descriptive.

Let's not sacrifice legibility over a non-issue.

I agree, I didn't realise that the we expand the last column already. It's a non-issue indeed.

Signature · 1 line
Catalin
author
#18 lore raw

Re: [PATCH v3 2/5] arm64: Work around Falkor erratum 1003

In reply to Catalin Marinas

Hi Catalin,

Quoted 3 lines · Catalin Marinas

Some minor comments below, nothing fundamental (as long as you say the new sequence doesn't have the speculative TLB load problem I mentioned on a previous version).

This workaround is documented as providing functional correctness for both explicit and speculative memory accesses.

Quoted 26 lines
Quoted 26 lines
arch/arm64/mm/proc.S +11 -0
index 32682be..9ee46df 100644
@@ -23,6 +23,7 @@
 #include <asm/assembler.h>
 #include <asm/asm-offsets.h>
 #include <asm/hwcap.h>
+#include <asm/mmu_context.h>
 #include <asm/pgtable.h>
 #include <asm/pgtable-hwdef.h>
 #include <asm/cpufeature.h>

@@ -140,6 +141,18 @@ ENDPROC(cpu_do_resume)
 ENTRY(cpu_do_switch_mm)
 	mmid	x1, x1				// get mm->context.id
 	bfi	x0, x1, #48, #16		// set the ASID
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
+alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1003
+	mrs     x2, ttbr0_el1
+	mov     x3, #FALKOR_RESERVED_ASID
+	bfi     x2, x3, #48, #16                // reserved ASID + old BADDR
+	msr     ttbr0_el1, x2
+	isb
+	bfi     x2, x0, #0, #48                 // reserved ASID + new BADDR
+	msr     ttbr0_el1, x2
+	isb
Quoted 8 lines

arch/arm64/mm/proc.S ENDPROC(cpu_do_resume) · line 158

Quoted 5 lines
arch/arm64/mm/proc.S +2 -0
+alternative_else_nop_endif
+#endif
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
 	post_ttbr0_update_workaround

Please move the above hunk to a pre_ttbr0_update_workaround macro for consistency with post_ttbr0_update_workaround.

How should I deal with inputs to the macro?

A) Use no input parameters and hard code x0 usage
B) Use a macro input parameter for the new TTBR value (x0)
C) Something else

Thanks,
Cov

Signature · 3 lines
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.
author
#19 lore raw

PATCH v3 3/5 arm64: Create and use __tlbi_dsb() macros

In reply to Christopher Covington

This refactoring will allow an errata workaround that repeats tlbi dsb sequences to only change one location. This is not intended to change the generated assembly and comparison of before and after preprocessor output of arch/arm64/mm/mmu.c and vmlinux objdump shows no functional changes.

Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/include/asm/tlbflush.h | 104 +++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 35 deletions(-)
arch/arm64/include/asm/tlbflush.h +69 -35
index deab523..f28813c 100644
@@ -25,22 +25,69 @@
 #include <asm/cputype.h>
 
 /*
- * Raw TLBI operations.
+ * Raw TLBI, DSB operations
  *
- * Where necessary, use the __tlbi() macro to avoid asm()
- * boilerplate. Drivers and most kernel code should use the TLB
- * management routines in preference to the macro below.
+ * Where necessary, use __tlbi_*dsb() macros to avoid asm() boilerplate.
+ * Drivers and most kernel code should use the TLB management routines in
+ * preference to the macros below.
  *
- * The macro can be used as __tlbi(op) or __tlbi(op, arg), depending
- * on whether a particular TLBI operation takes an argument or
- * not. The macros handles invoking the asm with or without the
- * register argument as appropriate.
+ * The __tlbi_dsb() macro handles invoking the asm without any register
+ * argument, with a single register argument, and with start (included)
+ * and end (excluded) range of register arguments. For example:
+ *
+ * __tlbi_dsb(op, attr)
+ *
+ * 	tlbi op
+ *	dsb attr
+ *
+ * __tlbi_dsb(op, attr, addr)
+ *
+ *	mov %[addr], =addr
+ *	tlbi op, %[addr]
+ *	dsb attr
+ *
+ * __tlbi_range_dsb(op, attr, start, end)
+ *
+ * 	mov %[arg], =start
+ *	mov %[end], =end
+ * for:
+ * 	tlbi op, %[addr]
+ * 	add %[addr], %[addr], #(1 << (PAGE_SHIFT - 12))
+ * 	cmp %[addr], %[end]
+ * 	b.ne for
+ * 	dsb attr
  */
-#define __TLBI_0(op, arg)		asm ("tlbi " #op)
-#define __TLBI_1(op, arg)		asm ("tlbi " #op ", %0" : : "r" (arg))
-#define __TLBI_N(op, arg, n, ...)	__TLBI_##n(op, arg)
 
-#define __tlbi(op, ...)		__TLBI_N(op, ##__VA_ARGS__, 1, 0)
+#define __TLBI_FOR_0(ig0, ig1, ig2)
+#define __TLBI_INSTR_0(op, ig1, ig2)	"tlbi " #op
+#define __TLBI_IO_0(ig0, ig1, ig2)	: :
+
+#define __TLBI_FOR_1(ig0, ig1, ig2)
+#define __TLBI_INSTR_1(op, ig0, ig1)	"tlbi " #op ", %0"
+#define __TLBI_IO_1(ig0, arg, ig1)	: : "r" (arg)
+
+#define __TLBI_FOR_2(ig0, start, ig1)	unsigned long addr;		       \
+					for (addr = start; addr < end;	       \
+						addr += 1 << (PAGE_SHIFT - 12))
+#define __TLBI_INSTR_2(op, ig0, ig1)	"tlbi " #op ", %0"
+#define __TLBI_IO_2(ig0, ig1, ig2)	: : "r" (addr)
+
+#define __TLBI_FOR_N(op, a1, a2, n, ...)	__TLBI_FOR_##n(op, a1, a2)
+#define __TLBI_INSTR_N(op, a1, a2, n, ...)	__TLBI_INSTR_##n(op, a1, a2)
+#define __TLBI_IO_N(op, a1, a2, n, ...)	__TLBI_IO_##n(op, a1, a2)
+
+#define __TLBI_FOR(op, ...)		__TLBI_FOR_N(op, ##__VA_ARGS__, 2, 1, 0)
+#define __TLBI_INSTR(op, ...)		__TLBI_INSTR_N(op, ##__VA_ARGS__, 2, 1, 0)
+#define __TLBI_IO(op, ...)		__TLBI_IO_N(op, ##__VA_ARGS__, 2, 1, 0)
+
+#define __tlbi_asm_dsb(as, op, attr, ...) do {				       \
+		__TLBI_FOR(op, ##__VA_ARGS__)				       \
+			asm (__TLBI_INSTR(op, ##__VA_ARGS__)		       \
+			__TLBI_IO(op, ##__VA_ARGS__));			       \
+		asm volatile (	     as			"\ndsb " #attr "\n"    \
+		: : : "memory"); } while (0)
+
+#define __tlbi_dsb(...)	__tlbi_asm_dsb("", ##__VA_ARGS__)
 
 /*
  *	TLB Management

@@ -84,16 +131,14 @@
 static inline void local_flush_tlb_all(void)
 {
 	dsb(nshst);
-	__tlbi(vmalle1);
-	dsb(nsh);
+	__tlbi_dsb(vmalle1, nsh);
 	isb();
 }
 
 static inline void flush_tlb_all(void)
 {
 	dsb(ishst);
-	__tlbi(vmalle1is);
-	dsb(ish);
+	__tlbi_dsb(vmalle1is, ish);
 	isb();
 }
 

@@ -102,8 +147,7 @@ static inline void flush_tlb_mm(struct mm_struct *mm)
 	unsigned long asid = ASID(mm) << 48;
 
 	dsb(ishst);
-	__tlbi(aside1is, asid);
-	dsb(ish);
+	__tlbi_dsb(aside1is, ish, asid);
 }
 
 static inline void flush_tlb_page(struct vm_area_struct *vma,

@@ -112,8 +156,7 @@ static inline void flush_tlb_page(struct vm_area_struct *vma,
 	unsigned long addr = uaddr >> 12 | (ASID(vma->vm_mm) << 48);
 
 	dsb(ishst);
-	__tlbi(vale1is, addr);
-	dsb(ish);
+	__tlbi_dsb(vale1is, ish, addr);
 }
 
 /*

@@ -127,7 +170,6 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma,
 				     bool last_level)
 {
 	unsigned long asid = ASID(vma->vm_mm) << 48;
-	unsigned long addr;
 
 	if ((end - start) > MAX_TLB_RANGE) {
 		flush_tlb_mm(vma->vm_mm);

@@ -138,13 +180,10 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma,
 	end = asid | (end >> 12);
 
 	dsb(ishst);
-	for (addr = start; addr < end; addr += 1 << (PAGE_SHIFT - 12)) {
-		if (last_level)
-			__tlbi(vale1is, addr);
-		else
-			__tlbi(vae1is, addr);
-	}
-	dsb(ish);
+	if (last_level)
+		__tlbi_dsb(vale1is, ish, start, end);
+	else
+		__tlbi_dsb(vae1is, ish, start, end);
 }
 
 static inline void flush_tlb_range(struct vm_area_struct *vma,

@@ -155,8 +194,6 @@ static inline void flush_tlb_range(struct vm_area_struct *vma,
 
 static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end)
 {
-	unsigned long addr;
-
 	if ((end - start) > MAX_TLB_RANGE) {
 		flush_tlb_all();
 		return;

@@ -166,9 +203,7 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end
 	end >>= 12;
 
 	dsb(ishst);
-	for (addr = start; addr < end; addr += 1 << (PAGE_SHIFT - 12))
-		__tlbi(vaae1is, addr);
-	dsb(ish);
+	__tlbi_dsb(vaae1is, ish, start, end);
 	isb();
 }
 

@@ -181,8 +216,7 @@ static inline void __flush_tlb_pgtable(struct mm_struct *mm,
 {
 	unsigned long addr = uaddr >> 12 | (ASID(mm) << 48);
 
-	__tlbi(vae1is, addr);
-	dsb(ish);
+	__tlbi_dsb(vae1is, ish, addr);
 }
 
 #endif
Signature · 3 lines
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.
#20 lore raw

Re: [PATCH v3 3/5] arm64: Create and use __tlbi_dsb() macros

Hi Christopher,

Quoted 83 lines · Christopher Covington

This refactoring will allow an errata workaround that repeats tlbi dsb sequences to only change one location. This is not intended to change the generated assembly and comparison of before and after preprocessor output of arch/arm64/mm/mmu.c and vmlinux objdump shows no functional changes.

Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/include/asm/tlbflush.h | 104 +++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 35 deletions(-)
arch/arm64/include/asm/tlbflush.h +49 -12
index deab523..f28813c 100644
@@ -25,22 +25,69 @@
 #include <asm/cputype.h>
 
 /*
- * Raw TLBI operations.
+ * Raw TLBI, DSB operations
  *
- * Where necessary, use the __tlbi() macro to avoid asm()
- * boilerplate. Drivers and most kernel code should use the TLB
- * management routines in preference to the macro below.
+ * Where necessary, use __tlbi_*dsb() macros to avoid asm() boilerplate.
+ * Drivers and most kernel code should use the TLB management routines in
+ * preference to the macros below.
  *
- * The macro can be used as __tlbi(op) or __tlbi(op, arg), depending
- * on whether a particular TLBI operation takes an argument or
- * not. The macros handles invoking the asm with or without the
- * register argument as appropriate.
+ * The __tlbi_dsb() macro handles invoking the asm without any register
+ * argument, with a single register argument, and with start (included)
+ * and end (excluded) range of register arguments. For example:
+ *
+ * __tlbi_dsb(op, attr)
+ *
+ * 	tlbi op
+ *	dsb attr
+ *
+ * __tlbi_dsb(op, attr, addr)
+ *
+ *	mov %[addr], =addr
+ *	tlbi op, %[addr]
+ *	dsb attr
+ *
+ * __tlbi_range_dsb(op, attr, start, end)
+ *
+ * 	mov %[arg], =start
+ *	mov %[end], =end
+ * for:
+ * 	tlbi op, %[addr]
+ * 	add %[addr], %[addr], #(1 << (PAGE_SHIFT - 12))
+ * 	cmp %[addr], %[end]
+ * 	b.ne for
+ * 	dsb attr
  */
-#define __TLBI_0(op, arg)		asm ("tlbi " #op)
-#define __TLBI_1(op, arg)		asm ("tlbi " #op ", %0" : : "r" (arg))
-#define __TLBI_N(op, arg, n, ...)	__TLBI_##n(op, arg)
 
-#define __tlbi(op, ...)		__TLBI_N(op, ##__VA_ARGS__, 1, 0)
+#define __TLBI_FOR_0(ig0, ig1, ig2)
+#define __TLBI_INSTR_0(op, ig1, ig2)	"tlbi " #op
+#define __TLBI_IO_0(ig0, ig1, ig2)	: :
+
+#define __TLBI_FOR_1(ig0, ig1, ig2)
+#define __TLBI_INSTR_1(op, ig0, ig1)	"tlbi " #op ", %0"
+#define __TLBI_IO_1(ig0, arg, ig1)	: : "r" (arg)
+
+#define __TLBI_FOR_2(ig0, start, ig1)	unsigned long addr;		       \
+					for (addr = start; addr < end;	       \
+						addr += 1 << (PAGE_SHIFT - 12))
+#define __TLBI_INSTR_2(op, ig0, ig1)	"tlbi " #op ", %0"
+#define __TLBI_IO_2(ig0, ig1, ig2)	: : "r" (addr)
+
+#define __TLBI_FOR_N(op, a1, a2, n, ...)	__TLBI_FOR_##n(op, a1, a2)
+#define __TLBI_INSTR_N(op, a1, a2, n, ...)	__TLBI_INSTR_##n(op, a1, a2)
+#define __TLBI_IO_N(op, a1, a2, n, ...)	__TLBI_IO_##n(op, a1, a2)
+
+#define __TLBI_FOR(op, ...)		__TLBI_FOR_N(op, ##__VA_ARGS__, 2, 1, 0)
+#define __TLBI_INSTR(op, ...)		__TLBI_INSTR_N(op, ##__VA_ARGS__, 2, 1, 0)

arch/arm64/include/asm/tlbflush.h line 90

Quoted 10 lines
arch/arm64/include/asm/tlbflush.h +10 -0
+#define __TLBI_IO(op, ...)		__TLBI_IO_N(op, ##__VA_ARGS__, 2, 1, 0)
+
+#define __tlbi_asm_dsb(as, op, attr, ...) do {				       \
+		__TLBI_FOR(op, ##__VA_ARGS__)				       \
+			asm (__TLBI_INSTR(op, ##__VA_ARGS__)		       \
+			__TLBI_IO(op, ##__VA_ARGS__));			       \
+		asm volatile (	     as			"\ndsb " #attr "\n"    \
+		: : : "memory"); } while (0)
+
+#define __tlbi_dsb(...)	__tlbi_asm_dsb("", ##__VA_ARGS__)

I can't deny that this is cool, but ultimately it's completely unreadable. What I was thinking you'd do would be make __tlbi expand to:

tlbi
dsb
tlbi
dsb

for Falkor, and:

tlbi
nop
nop
nop

for everybody else.

Wouldn't that localise this change sufficiently that you wouldn't need to change all the callers and encode the looping in your cpp macros?

I realise you get an extra dsb in some places with that change, but I'd like to see numbers for the impact of that on top of the workaround. If it's an issue, then an alternative sequence would be:

tlbi
dsb
tlbi

and you'd rely on the existing dsb to complete that.

Having said that, I don't understand how your current loop code works when the workaround is applied. AFAICT, you end up emitting something like:

dsb ishst
for i in 0 to n
tlbi va+i
dsb
tlbi va+n
dsb

which looks wrong to me. Am I misreading something here?

Will

author
#21 lore raw

Re: [PATCH v3 3/5] arm64: Create and use __tlbi_dsb() macros

Hi Will,

Quoted 105 lines · Will Deacon

Hi Christopher,

Quoted 83 lines · Christopher Covington

This refactoring will allow an errata workaround that repeats tlbi dsb sequences to only change one location. This is not intended to change the generated assembly and comparison of before and after preprocessor output of arch/arm64/mm/mmu.c and vmlinux objdump shows no functional changes.

Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/include/asm/tlbflush.h | 104 +++++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 35 deletions(-)
arch/arm64/include/asm/tlbflush.h +49 -12
index deab523..f28813c 100644
@@ -25,22 +25,69 @@
 #include <asm/cputype.h>
 
 /*
- * Raw TLBI operations.
+ * Raw TLBI, DSB operations
  *
- * Where necessary, use the __tlbi() macro to avoid asm()
- * boilerplate. Drivers and most kernel code should use the TLB
- * management routines in preference to the macro below.
+ * Where necessary, use __tlbi_*dsb() macros to avoid asm() boilerplate.
+ * Drivers and most kernel code should use the TLB management routines in
+ * preference to the macros below.
  *
- * The macro can be used as __tlbi(op) or __tlbi(op, arg), depending
- * on whether a particular TLBI operation takes an argument or
- * not. The macros handles invoking the asm with or without the
- * register argument as appropriate.
+ * The __tlbi_dsb() macro handles invoking the asm without any register
+ * argument, with a single register argument, and with start (included)
+ * and end (excluded) range of register arguments. For example:
+ *
+ * __tlbi_dsb(op, attr)
+ *
+ * 	tlbi op
+ *	dsb attr
+ *
+ * __tlbi_dsb(op, attr, addr)
+ *
+ *	mov %[addr], =addr
+ *	tlbi op, %[addr]
+ *	dsb attr
+ *
+ * __tlbi_range_dsb(op, attr, start, end)
+ *
+ * 	mov %[arg], =start
+ *	mov %[end], =end
+ * for:
+ * 	tlbi op, %[addr]
+ * 	add %[addr], %[addr], #(1 << (PAGE_SHIFT - 12))
+ * 	cmp %[addr], %[end]
+ * 	b.ne for
+ * 	dsb attr
  */
-#define __TLBI_0(op, arg)		asm ("tlbi " #op)
-#define __TLBI_1(op, arg)		asm ("tlbi " #op ", %0" : : "r" (arg))
-#define __TLBI_N(op, arg, n, ...)	__TLBI_##n(op, arg)
 
-#define __tlbi(op, ...)		__TLBI_N(op, ##__VA_ARGS__, 1, 0)
+#define __TLBI_FOR_0(ig0, ig1, ig2)
+#define __TLBI_INSTR_0(op, ig1, ig2)	"tlbi " #op
+#define __TLBI_IO_0(ig0, ig1, ig2)	: :
+
+#define __TLBI_FOR_1(ig0, ig1, ig2)
+#define __TLBI_INSTR_1(op, ig0, ig1)	"tlbi " #op ", %0"
+#define __TLBI_IO_1(ig0, arg, ig1)	: : "r" (arg)
+
+#define __TLBI_FOR_2(ig0, start, ig1)	unsigned long addr;		       \
+					for (addr = start; addr < end;	       \
+						addr += 1 << (PAGE_SHIFT - 12))
+#define __TLBI_INSTR_2(op, ig0, ig1)	"tlbi " #op ", %0"
+#define __TLBI_IO_2(ig0, ig1, ig2)	: : "r" (addr)
+
+#define __TLBI_FOR_N(op, a1, a2, n, ...)	__TLBI_FOR_##n(op, a1, a2)
+#define __TLBI_INSTR_N(op, a1, a2, n, ...)	__TLBI_INSTR_##n(op, a1, a2)
+#define __TLBI_IO_N(op, a1, a2, n, ...)	__TLBI_IO_##n(op, a1, a2)
+
+#define __TLBI_FOR(op, ...)		__TLBI_FOR_N(op, ##__VA_ARGS__, 2, 1, 0)
+#define __TLBI_INSTR(op, ...)		__TLBI_INSTR_N(op, ##__VA_ARGS__, 2, 1, 0)

arch/arm64/include/asm/tlbflush.h line 90

Quoted 10 lines
arch/arm64/include/asm/tlbflush.h +10 -0
+#define __TLBI_IO(op, ...)		__TLBI_IO_N(op, ##__VA_ARGS__, 2, 1, 0)
+
+#define __tlbi_asm_dsb(as, op, attr, ...) do {				       \
+		__TLBI_FOR(op, ##__VA_ARGS__)				       \
+			asm (__TLBI_INSTR(op, ##__VA_ARGS__)		       \
+			__TLBI_IO(op, ##__VA_ARGS__));			       \
+		asm volatile (	     as			"\ndsb " #attr "\n"    \
+		: : : "memory"); } while (0)
+
+#define __tlbi_dsb(...)	__tlbi_asm_dsb("", ##__VA_ARGS__)

I can't deny that this is cool, but ultimately it's completely unreadable. What I was thinking you'd do would be make __tlbi expand to:

tlbi
dsb
tlbi
dsb

Quoted 8 lines

for Falkor, and:

tlbi
nop
nop
nop

for everybody else.

Thanks for the suggestion. So would __tlbi take a dsb sharability argument in your proposal? Or would it be communicated in some other fashion, maybe inferred from the tlbi argument? Or would the workaround dsbs all be the worst/broadest case?

Quoted 17 lines

Wouldn't that localise this change sufficiently that you wouldn't need to change all the callers and encode the looping in your cpp macros?

I realise you get an extra dsb in some places with that change, but I'd like to see numbers for the impact of that on top of the workaround. If it's an issue, then an alternative sequence would be:

tlbi
dsb
tlbi

and you'd rely on the existing dsb to complete that.

Having said that, I don't understand how your current loop code works when the workaround is applied. AFAICT, you end up emitting something like:

Quoted 8 lines

dsb ishst
for i in 0 to n
tlbi va+i
dsb
tlbi va+n
dsb

which looks wrong to me. Am I misreading something here?

You're right, I am off by 1 << (PAGE_SHIFT - 12) here. I would need to increment, compare, not take the loop branch (regular for loop stuff), then decrement (missing) and perform TLB invalidation again (present but using incorrect value).

Thanks,
Cov

Signature · 3 lines
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.
#22 lore raw

Re: [PATCH v3 3/5] arm64: Create and use __tlbi_dsb() macros

Quoted 26 lines · Christopher Covington
Quoted 25 lines · Will Deacon

arch/arm64/include/asm/tlbflush.h line 90

Quoted 8 lines · Christopher Covington
arch/arm64/include/asm/tlbflush.h +8 -0
+#define __tlbi_asm_dsb(as, op, attr, ...) do {				       \
+		__TLBI_FOR(op, ##__VA_ARGS__)				       \
+			asm (__TLBI_INSTR(op, ##__VA_ARGS__)		       \
+			__TLBI_IO(op, ##__VA_ARGS__));			       \
+		asm volatile (	     as			"\ndsb " #attr "\n"    \
+		: : : "memory"); } while (0)
+
+#define __tlbi_dsb(...)	__tlbi_asm_dsb("", ##__VA_ARGS__)

I can't deny that this is cool, but ultimately it's completely unreadable. What I was thinking you'd do would be make __tlbi expand to:

tlbi
dsb
tlbi
dsb

for Falkor, and:

tlbi
nop
nop
nop

Quoted 6 lines
Quoted 1 line

for everybody else.

Thanks for the suggestion. So would __tlbi take a dsb sharability argument in your proposal? Or would it be communicated in some other fashion, maybe inferred from the tlbi argument? Or would the workaround dsbs all be the worst/broadest case?

I think always using inner-shareable should be ok. If you wanted to optimise this, you'd want to avoid the workaround altogether for non-shareable invalidation, but that's fairly rare and I doubt you'd be able to measure the impact.

Quoted 24 lines
Quoted 24 lines

Wouldn't that localise this change sufficiently that you wouldn't need to change all the callers and encode the looping in your cpp macros?

I realise you get an extra dsb in some places with that change, but I'd like to see numbers for the impact of that on top of the workaround. If it's an issue, then an alternative sequence would be:

tlbi
dsb
tlbi

and you'd rely on the existing dsb to complete that.

Having said that, I don't understand how your current loop code works when the workaround is applied. AFAICT, you end up emitting something like:

dsb ishst
for i in 0 to n
tlbi va+i
dsb
tlbi va+n
dsb

Quoted 6 lines
Quoted 1 line

which looks wrong to me. Am I misreading something here?

You're right, I am off by 1 << (PAGE_SHIFT - 12) here. I would need to increment, compare, not take the loop branch (regular for loop stuff), then decrement (missing) and perform TLB invalidation again (present but using incorrect value).

It also strikes me as odd that you only need one extra TLBI after the loop has finished, as opposed to a tlbi; dsb; tlbi loop body (which is what you'd get if you modified __tlbi as I suggest).

Is it sufficient to have one extra TLBI after the loop and, if so, is the performance impact of my suggestion therefore unacceptable?

Will

author
#23 lore raw

Re: [PATCH v3 3/5] arm64: Create and use __tlbi_dsb() macros

Hi Will,

Quoted 20 lines · Will Deacon
Quoted 19 lines · Christopher Covington
Quoted 18 lines · Will Deacon

arch/arm64/include/asm/tlbflush.h line 90

Quoted 8 lines · Christopher Covington
arch/arm64/include/asm/tlbflush.h +8 -0
+#define __tlbi_asm_dsb(as, op, attr, ...) do {				       \
+		__TLBI_FOR(op, ##__VA_ARGS__)				       \
+			asm (__TLBI_INSTR(op, ##__VA_ARGS__)		       \
+			__TLBI_IO(op, ##__VA_ARGS__));			       \
+		asm volatile (	     as			"\ndsb " #attr "\n"    \
+		: : : "memory"); } while (0)
+
+#define __tlbi_dsb(...)	__tlbi_asm_dsb("", ##__VA_ARGS__)

I can't deny that this is cool, but ultimately it's completely unreadable. What I was thinking you'd do would be make __tlbi expand to:

tlbi
dsb
tlbi
dsb

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

for Falkor, and:

tlbi
nop
nop
nop

for everybody else.

I've implemented this (minus the last dsb / nop) in the next revision.

Quoted 9 lines
Quoted 4 lines

Thanks for the suggestion. So would __tlbi take a dsb sharability argument in your proposal? Or would it be communicated in some other fashion, maybe inferred from the tlbi argument? Or would the workaround dsbs all be the worst/broadest case?

I think always using inner-shareable should be ok. If you wanted to optimise this, you'd want to avoid the workaround altogether for non-shareable invalidation, but that's fairly rare and I doubt you'd be able to measure the impact.

I did not originally notice that Shanker's original workaround implementation unnecessarily applies the workaround to non-shareable invalidations. They're not affected by the erratum. But as you say, it's simpler to modify __tlbi for all cases. I'm not currently worried about that performance impact.

Quoted 31 lines
Quoted 24 lines
Quoted 24 lines

Wouldn't that localise this change sufficiently that you wouldn't need to change all the callers and encode the looping in your cpp macros?

I realise you get an extra dsb in some places with that change, but I'd like to see numbers for the impact of that on top of the workaround. If it's an issue, then an alternative sequence would be:

tlbi
dsb
tlbi

and you'd rely on the existing dsb to complete that.

Having said that, I don't understand how your current loop code works when the workaround is applied. AFAICT, you end up emitting something like:

dsb ishst
for i in 0 to n
tlbi va+i
dsb
tlbi va+n
dsb

Quoted 6 lines
Quoted 1 line

which looks wrong to me. Am I misreading something here?

You're right, I am off by 1 << (PAGE_SHIFT - 12) here. I would need to increment, compare, not take the loop branch (regular for loop stuff), then decrement (missing) and perform TLB invalidation again (present but using incorrect value).

Quoted 6 lines

It also strikes me as odd that you only need one extra TLBI after the loop has finished, as opposed to a tlbi; dsb; tlbi loop body (which is what you'd get if you modified __tlbi as I suggest).

Is it sufficient to have one extra TLBI after the loop and, if so, is the performance impact of my suggestion therefore unacceptable?

One is sufficient according to the errata documentation. I've described that aspect in the commit message of the next revision.

I've suggested colleagues follow up regarding performance. But reliable functionality comes first.

Thanks,
Cov

Signature · 3 lines
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.
author
#24 lore raw

PATCH v3 4/5 arm64: Use __tlbi_dsb() macros in KVM code

In reply to Christopher Covington

Refactor the KVM code to use the newly introduced __tlbi_dsb macros, which will allow an errata workaround that repeats tlbi dsb sequences to only change one location. This is not intended to change the generated assembly and comparing before and after vmlinux objdump shows no functional changes.

Signed-off-by Christopher Covington <[email protected]>
---
 arch/arm64/kvm/hyp/tlb.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)
arch/arm64/kvm/hyp/tlb.c +11 -18
index 88e2f2b..9669e4b 100644
@@ -16,6 +16,7 @@
  */
 
 #include <asm/kvm_hyp.h>
+#include <asm/tlbflush.h>
 
 void __hyp_text __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa)
 {

@@ -30,19 +31,15 @@ void __hyp_text __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa)
 	 * We could do so much better if we had the VA as well.
 	 * Instead, we invalidate Stage-2 for this IPA, and the
 	 * whole of Stage-1. Weep...
+	 *
+	 * We have to ensure completion of the invalidation at Stage-2 with a
+	 * DSB, since a table walk on another CPU could refill a TLB with a
+	 * complete (S1 + S2) walk based on the old Stage-2 mapping if the
+	 * Stage-1 invalidation happened first.
 	 */
 	ipa >>= 12;
-	asm volatile("tlbi ipas2e1is, %0" : : "r" (ipa));
-
-	/*
-	 * We have to ensure completion of the invalidation at Stage-2,
-	 * since a table walk on another CPU could refill a TLB with a
-	 * complete (S1 + S2) walk based on the old Stage-2 mapping if
-	 * the Stage-1 invalidation happened first.
-	 */
-	dsb(ish);
-	asm volatile("tlbi vmalle1is" : : );
-	dsb(ish);
+	__tlbi_dsb(ipas2e1is, ish, ipa);
+	__tlbi_dsb(vmalle1is, ish);
 	isb();
 
 	write_sysreg(0, vttbr_el2);

@@ -57,8 +54,7 @@ void __hyp_text __kvm_tlb_flush_vmid(struct kvm *kvm)
 	write_sysreg(kvm->arch.vttbr, vttbr_el2);
 	isb();
 
-	asm volatile("tlbi vmalls12e1is" : : );
-	dsb(ish);
+	__tlbi_dsb(vmalls12e1is, ish);
 	isb();
 
 	write_sysreg(0, vttbr_el2);

@@ -72,8 +68,7 @@ void __hyp_text __kvm_tlb_flush_local_vmid(struct kvm_vcpu *vcpu)
 	write_sysreg(kvm->arch.vttbr, vttbr_el2);
 	isb();
 
-	asm volatile("tlbi vmalle1" : : );
-	dsb(nsh);
+	__tlbi_dsb(vmalle1, nsh);
 	isb();
 
 	write_sysreg(0, vttbr_el2);

@@ -82,7 +77,5 @@ void __hyp_text __kvm_tlb_flush_local_vmid(struct kvm_vcpu *vcpu)
 void __hyp_text __kvm_flush_vm_context(void)
 {
 	dsb(ishst);
-	asm volatile("tlbi alle1is	\n"
-		     "ic ialluis	  ": : );
-	dsb(ish);
+	__tlbi_asm_dsb("ic ialluis", alle1is, ish);
 }
Signature · 3 lines
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.
author
#25 lore raw

PATCH v3 5/5 arm64: Work around Falkor erratum 1009

In reply to Christopher Covington

During a TLB invalidate sequence targeting the inner shareable domain, Falkor may prematurely complete the DSB before all loads and stores using the old translation are observed; instruction fetches are not subject to the conditions of this erratum.

Signed-off-by Christopher Covington <[email protected]>
---
 Documentation/arm64/silicon-errata.txt |  1 +
 arch/arm64/Kconfig                     | 10 ++++++++++
 arch/arm64/include/asm/cpucaps.h       |  3 ++-
 arch/arm64/include/asm/tlbflush.h      |  5 ++++-
 arch/arm64/kernel/cpu_errata.c         |  7 +++++++
 5 files changed, 24 insertions(+), 2 deletions(-)
Documentation/arm64/silicon-errata.txt +1 -0
index 7151aed..98bef2a 100644
@@ -64,3 +64,4 @@ stable kernels.
 |               |                 |                 |                          |
 | Freescale/NXP | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585      |
 | Qualcomm      | Falkor v1       | E1003           | QCOM_FALKOR_ERRATUM_1003 |
+| Qualcomm      | Falkor v1       | E1009           | QCOM_FALKOR_ERRATUM_1009 |
arch/arm64/Kconfig +10 -0
index 2a80ac9..d13e903 100644
@@ -490,6 +490,16 @@ config QCOM_FALKOR_ERRATUM_1003
 
 	  If unsure, say Y.
 
+config QCOM_FALKOR_ERRATUM_1009
+	bool "Falkor E1009: Prematurely complete a DSB after a TLBI"
+	default y
+	help
+	  Falkor CPU may prematurely complete a DSB following a TLBI xxIS
+	  invalidate maintenance operations. Repeat the TLBI operation one
+	  more time to fix the issue.
+
+	  If unsure, say Y.
+
 endmenu
arch/arm64/include/asm/cpucaps.h +2 -1
index 5aaf7ee..55bcd02 100644
@@ -36,7 +36,8 @@
 #define ARM64_MISMATCHED_CACHE_LINE_SIZE	15
 #define ARM64_HAS_NO_FPSIMD			16
 #define ARM64_WORKAROUND_QCOM_FALKOR_E1003	17
+#define ARM64_WORKAROUND_REPEAT_TLBI		18
 
-#define ARM64_NCAPS				18
+#define ARM64_NCAPS				19
 
 #endif /* __ASM_CPUCAPS_H */
arch/arm64/include/asm/tlbflush.h +4 -1
index f28813c..7313cd3 100644
@@ -85,7 +85,10 @@
 			asm (__TLBI_INSTR(op, ##__VA_ARGS__)		       \
 			__TLBI_IO(op, ##__VA_ARGS__));			       \
 		asm volatile (	     as			"\ndsb " #attr "\n"    \
-		: : : "memory"); } while (0)
+			ALTERNATIVE("nop"		"\nnop"	       "\n",   \
+			__TLBI_INSTR(op, ##__VA_ARGS__)	"\ndsb " #attr "\n",   \
+			ARM64_WORKAROUND_REPEAT_TLBI)			       \
+		__TLBI_IO(op, ##__VA_ARGS__) : "memory"); } while (0)
 
 #define __tlbi_dsb(...)	__tlbi_asm_dsb("", ##__VA_ARGS__)
arch/arm64/kernel/cpu_errata.c +7 -0
index 787b542..e644364 100644
@@ -137,6 +137,13 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 		MIDR_RANGE(MIDR_QCOM_FALKOR_V1, 0x00, 0x00),
 	},
 #endif
+#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1009
+	{
+		.desc = "Qualcomm Falkor erratum 1009",
+		.capability = ARM64_WORKAROUND_REPEAT_TLBI,
+		MIDR_RANGE(MIDR_QCOM_FALKOR_V1, 0x00, 0x00),
+	},
+#endif
 	{
 	}
 };
Signature · 3 lines
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.