Cortex-A77 erratum 1508412 workaround

PATCH Superseded Rob Herring 4 participants 10 messages Jul 17, 2020 – Aug 3, 2020

Subsystems (3)

Documentation arm64 KVM arm64

lore

Share & tools

lore

Rob Herring

author

2020-07-17 20:52 UTC #0 lore raw

v3 cover Cortex-A77 erratum 1508412 workaround

This series implements the work-around for Cortex-A77 erratum 1508412. KVM guests which don't implement the workaround can still deadlock the system. This is also the case with the existing Cortex-A57 erratum 832075, so we add a warning message if an erratum can cause deadlock.

Changes detailed in patches.

Rob

v2: https://lore.kernel.org/linux-arm-kernel/[email protected]/
v1: https://lore.kernel.org/linux-arm-kernel/[email protected]/

Rob Herring (3):
KVM: arm64: Print warning when cpu erratum can cause guests to
deadlock
arm64: Add part number for Arm Cortex-A77
arm64: Add workaround for Arm Cortex-A77 erratum 1508412

 Documentation/arm64/silicon-errata.rst |  2 ++
 arch/arm64/Kconfig                     | 19 +++++++++++++++++++
 arch/arm64/include/asm/cpucaps.h       |  3 ++-
 arch/arm64/include/asm/cputype.h       |  2 ++
 arch/arm64/include/asm/kvm_hyp.h       | 11 +++++++++++
 arch/arm64/kernel/cpu_errata.c         | 10 ++++++++++
 arch/arm64/kvm/arm.c                   |  5 +++++
 arch/arm64/kvm/hyp/switch.c            |  7 ++++---
 arch/arm64/kvm/hyp/sysreg-sr.c         |  2 +-
 arch/arm64/kvm/sys_regs.c              |  8 +++++++-
 arch/arm64/mm/fault.c                  | 10 ++++++++++
 11 files changed, 73 insertions(+), 6 deletions(-)

--
2.25.1
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Rob Herring

author

2020-07-17 20:52 UTC #1 lore raw

PATCH v3 1/3 KVM: arm64: Print warning when cpu erratum can cause guests to deadlock

If guests don't have certain CPU erratum workarounds implemented, then there is a possibility a guest can deadlock the system. IOW, only trusted guests should be used on systems with the erratum.

This is the case for Cortex-A57 erratum 832075.

Cc Marc Zyngier <[email protected]>

Cc James Morse <[email protected]>

Cc Julien Thierry <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc [email protected]

Signed-off-by Rob Herring <[email protected]>

---
v3:
- s/work-arounds/workarounds/

 arch/arm64/kvm/arm.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kvm/arm.c +4 -0

index 90cb90561446..9b070b5e212b 100644
@@ -1653,6 +1653,10 @@ int kvm_arch_init(void *opaque)
 		return -ENODEV;
 	}
 
+	if (cpus_have_const_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE))
+		kvm_info("Guests without required CPU erratum workarounds can deadlock system!\n" \
+			 "Only trusted guests should be used on this system.\n");
+
 	for_each_online_cpu(cpu) {
 		smp_call_function_single(cpu, check_kvm_target_cpu, &ret, 1);
 		if (ret < 0) {

--
2.25.1
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Rob Herring

author

2020-07-17 20:52 UTC #2 lore raw

PATCH v3 2/3 arm64: Add part number for Arm Cortex-A77

In reply to Rob Herring

Add the MIDR part number info for the Arm Cortex-A77.

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
 arch/arm64/include/asm/cputype.h | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/include/asm/cputype.h +2 -0

index a87a93f67671..7a2d3c336579 100644
@@ -71,6 +71,7 @@
 #define ARM_CPU_PART_CORTEX_A55		0xD05
 #define ARM_CPU_PART_CORTEX_A76		0xD0B
 #define ARM_CPU_PART_NEOVERSE_N1	0xD0C
+#define ARM_CPU_PART_CORTEX_A77		0xD0D
 
 #define APM_CPU_PART_POTENZA		0x000

@@ -104,6 +105,7 @@
 #define MIDR_CORTEX_A55 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A55)
 #define MIDR_CORTEX_A76	MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A76)
 #define MIDR_NEOVERSE_N1 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_NEOVERSE_N1)
+#define MIDR_CORTEX_A77	MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A77)
 #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_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_83XX)

Signature · 6 lines

2.25.1

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Rob Herring

author

2020-07-17 20:52 UTC #3 lore raw

PATCH v3 3/3 arm64: Add workaround for Arm Cortex-A77 erratum 1508412

In reply to Rob Herring

On Cortex-A77 r0p0 and r1p0, a sequence of a non-cacheable or device load and a store exclusive or PAR_EL1 read can cause a deadlock.

The workaround requires a DMB SY before and after a PAR_EL1 register read. A deadlock is still possible with the workaround as KVM guests must also have the workaround. IOW, a malicious guest can deadlock an affected systems.

This workaround also depends on a firmware counterpart to enable the h/w to insert DMB SY after load and store exclusive instructions. See the errata document SDEN-1152370 v10 [1] for more information.

[1] https://static.docs.arm.com/101992/0010/Arm_Cortex_A77_MP074_Software_Developer_Errata_Notice_v10.pdf

Cc Catalin Marinas <[email protected]>

Cc James Morse <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Will Deacon <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Julien Thierry <[email protected]>

Cc [email protected]

Signed-off-by Rob Herring <[email protected]>

---
v3:
- Add dmbs around PAR reads in KVM code
- Clean-up 'work-around' and 'errata'

v2:
- Don't disable KVM, just print warning

---
 Documentation/arm64/silicon-errata.rst |  2 ++
 arch/arm64/Kconfig                     | 19 +++++++++++++++++++
 arch/arm64/include/asm/cpucaps.h       |  3 ++-
 arch/arm64/include/asm/kvm_hyp.h       | 11 +++++++++++
 arch/arm64/kernel/cpu_errata.c         | 10 ++++++++++
 arch/arm64/kvm/arm.c                   |  3 ++-
 arch/arm64/kvm/hyp/switch.c            |  7 ++++---
 arch/arm64/kvm/hyp/sysreg-sr.c         |  2 +-
 arch/arm64/kvm/sys_regs.c              |  8 +++++++-
 arch/arm64/mm/fault.c                  | 10 ++++++++++
 10 files changed, 68 insertions(+), 7 deletions(-)

Documentation/arm64/silicon-errata.rst +2 -0

index 936cf2a59ca4..716b279e3b33 100644
@@ -90,6 +90,8 @@ stable kernels.
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A76      | #1463225        | ARM64_ERRATUM_1463225       |
 +----------------+-----------------+-----------------+-----------------------------+
+| ARM            | Cortex-A77      | #1508412        | ARM64_ERRATUM_1508412       |
++----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Neoverse-N1     | #1188873,1418040| ARM64_ERRATUM_1418040       |
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Neoverse-N1     | #1349291        | N/A                         |

arch/arm64/Kconfig +19 -0

index a4a094bedcb2..6638444ce0d8 100644
@@ -626,6 +626,25 @@ config ARM64_ERRATUM_1542419
 
 	  If unsure, say Y.
 
+config ARM64_ERRATUM_1508412
+	bool "Cortex-A77: 1508412: workaround deadlock on sequence of NC/Device load and store exclusive or PAR read"
+	default y
+	help
+	  This option adds a workaround for Arm Cortex-A77 erratum 1508412.
+
+	  Affected Cortex-A77 cores (r0p0, r1p0) could deadlock on a sequence
+	  of a store-exclusive or read of PAR_EL1 and a load with device or
+	  non-cacheable memory attributes. The workaround depends on a firmware
+	  counterpart.
+
+	  KVM guests must also have the workaround implemented or they can
+	  deadlock the system.
+
+	  Work around the issue by inserting DMB SY barriers around PAR_EL1
+	  register reads and warning KVM users.
+
+	  If unsure, say Y.
+
 config CAVIUM_ERRATUM_22375
 	bool "Cavium erratum 22375, 24313"
 	default y

arch/arm64/include/asm/cpucaps.h +2 -1

index d7b3bb0cb180..2a2cdb4ced8b 100644
@@ -62,7 +62,8 @@
 #define ARM64_HAS_GENERIC_AUTH			52
 #define ARM64_HAS_32BIT_EL1			53
 #define ARM64_BTI				54
+#define ARM64_WORKAROUND_1508412		55
 
-#define ARM64_NCAPS				55
+#define ARM64_NCAPS				56
 
 #endif /* __ASM_CPUCAPS_H */

arch/arm64/include/asm/kvm_hyp.h +11 -0

index ce3080834bfa..ce5b0d9b12bf 100644
@@ -46,6 +46,17 @@
 #define read_sysreg_el2(r)	read_sysreg_elx(r, _EL2, _EL1)
 #define write_sysreg_el2(v,r)	write_sysreg_elx(v, r, _EL2, _EL1)
 
+static inline u64 __hyp_text read_sysreg_par(void)
+{
+	u64 par;
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	par = read_sysreg(par_el1);
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	return par;
+}
+
 /*
  * Without an __arch_swab32(), we fall back to ___constant_swab32(), but the
  * static inline can allow the compiler to out-of-line this. KVM always wants

arch/arm64/kernel/cpu_errata.c +10 -0

index ad06d6802d2e..5eee8a75540c 100644
@@ -938,6 +938,16 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 		.matches = has_neoverse_n1_erratum_1542419,
 		.cpu_enable = cpu_enable_trap_ctr_access,
 	},
+#endif
+#ifdef CONFIG_ARM64_ERRATUM_1508412
+	{
+		/* we depend on the firmware portion for correctness */
+		.desc = "ARM erratum 1508412 (kernel portion)",
+		.capability = ARM64_WORKAROUND_1508412,
+		ERRATA_MIDR_RANGE(MIDR_CORTEX_A77,
+				  0, 0,
+				  1, 0),
+	},
 #endif
 	{
 	}

arch/arm64/kvm/arm.c +2 -1

index 9b070b5e212b..21d8b3ca5bd7 100644
@@ -1653,7 +1653,8 @@ int kvm_arch_init(void *opaque)
 		return -ENODEV;
 	}
 
-	if (cpus_have_const_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE))
+	if (cpus_have_const_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) ||
+	    cpus_have_const_cap(ARM64_WORKAROUND_1508412))
 		kvm_info("Guests without required CPU erratum workarounds can deadlock system!\n" \
 			 "Only trusted guests should be used on this system.\n");

arch/arm64/kvm/hyp/switch.c +4 -3

index db1c4487d95d..d76b6638b705 100644
@@ -298,11 +298,12 @@ static bool __hyp_text __translate_far_to_hpfar(u64 far, u64 *hpfar)
 	 * We do need to save/restore PAR_EL1 though, as we haven't
 	 * saved the guest context yet, and we may return early...
 	 */
-	par = read_sysreg(par_el1);
+	par = read_sysreg_par();
+
 	asm volatile("at s1e1r, %0" : : "r" (far));
 	isb();
 
-	tmp = read_sysreg(par_el1);
+	tmp = read_sysreg_par();
 	write_sysreg(par, par_el1);
 
 	if (unlikely(tmp & SYS_PAR_EL1_F))

@@ -925,7 +926,7 @@ void __hyp_text __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt)
 {
 	u64 spsr = read_sysreg_el2(SYS_SPSR);
 	u64 elr = read_sysreg_el2(SYS_ELR);
-	u64 par = read_sysreg(par_el1);
+	u64 par = read_sysreg_par();
 
 	if (!has_vhe())
 		__hyp_call_panic_nvhe(spsr, elr, par, host_ctxt);

arch/arm64/kvm/hyp/sysreg-sr.c +1 -1

index cc7e957f5b2c..f522cbff291d 100644
@@ -52,7 +52,7 @@ static void __hyp_text __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
 	ctxt->sys_regs[CONTEXTIDR_EL1]	= read_sysreg_el1(SYS_CONTEXTIDR);
 	ctxt->sys_regs[AMAIR_EL1]	= read_sysreg_el1(SYS_AMAIR);
 	ctxt->sys_regs[CNTKCTL_EL1]	= read_sysreg_el1(SYS_CNTKCTL);
-	ctxt->sys_regs[PAR_EL1]		= read_sysreg(par_el1);
+	ctxt->sys_regs[PAR_EL1]		= read_sysreg_par();
 	ctxt->sys_regs[TPIDR_EL1]	= read_sysreg(tpidr_el1);
 
 	ctxt->gp_regs.sp_el1		= read_sysreg(sp_el1);

arch/arm64/kvm/sys_regs.c +7 -1

index baf5ce9225ce..3f798e0f1419 100644
@@ -94,10 +94,16 @@ static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
 	case TPIDR_EL1:		*val = read_sysreg_s(SYS_TPIDR_EL1);	break;
 	case AMAIR_EL1:		*val = read_sysreg_s(SYS_AMAIR_EL12);	break;
 	case CNTKCTL_EL1:	*val = read_sysreg_s(SYS_CNTKCTL_EL12);	break;
-	case PAR_EL1:		*val = read_sysreg_s(SYS_PAR_EL1);	break;
 	case DACR32_EL2:	*val = read_sysreg_s(SYS_DACR32_EL2);	break;
 	case IFSR32_EL2:	*val = read_sysreg_s(SYS_IFSR32_EL2);	break;
 	case DBGVCR32_EL2:	*val = read_sysreg_s(SYS_DBGVCR32_EL2);	break;
+	case PAR_EL1:
+		if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+			dmb(sy);
+		*val = read_sysreg_s(SYS_PAR_EL1);
+		if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+			dmb(sy);
+		break;
 	default:		return false;
 	}

arch/arm64/mm/fault.c +10 -0

index 8afb238ff335..98609532e61a 100644
@@ -260,7 +260,17 @@ static bool __kprobes is_spurious_el1_translation_fault(unsigned long addr,
 	local_irq_save(flags);
 	asm volatile("at s1e1r, %0" :: "r" (addr));
 	isb();
+	/*
+	 * Arm Erratum 1508412 requires dmb(sy) before and after reads of
+	 * PAR_EL1.
+	 * As this location is not a hot path, just condition it on the config
+	 * option.
+	 */
+	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+		dmb(sy);
 	par = read_sysreg(par_el1);
+	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+		dmb(sy);
 	local_irq_restore(flags);
 
 	/*

Signature · 6 lines

2.25.1

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Andrew Scull

2020-07-27 15:52 UTC #4 lore raw

Re: [PATCH v3 3/3] arm64: Add workaround for Arm Cortex-A77 erratum 1508412

Hi Rob, a couple of suggestions for way this erratum is gated, but I haven't delved into the details of the errata itself.

Quoted 147 lines · Rob Herring

On Cortex-A77 r0p0 and r1p0, a sequence of a non-cacheable or device load and a store exclusive or PAR_EL1 read can cause a deadlock.

[1] https://static.docs.arm.com/101992/0010/Arm_Cortex_A77_MP074_Software_Developer_Errata_Notice_v10.pdf

Cc Catalin Marinas <[email protected]>

Cc James Morse <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Will Deacon <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Julien Thierry <[email protected]>

Cc [email protected]

Signed-off-by Rob Herring <[email protected]>

---
v3:
- Add dmbs around PAR reads in KVM code
- Clean-up 'work-around' and 'errata'

v2:
- Don't disable KVM, just print warning

---
 Documentation/arm64/silicon-errata.rst |  2 ++
 arch/arm64/Kconfig                     | 19 +++++++++++++++++++
 arch/arm64/include/asm/cpucaps.h       |  3 ++-
 arch/arm64/include/asm/kvm_hyp.h       | 11 +++++++++++
 arch/arm64/kernel/cpu_errata.c         | 10 ++++++++++
 arch/arm64/kvm/arm.c                   |  3 ++-
 arch/arm64/kvm/hyp/switch.c            |  7 ++++---
 arch/arm64/kvm/hyp/sysreg-sr.c         |  2 +-
 arch/arm64/kvm/sys_regs.c              |  8 +++++++-
 arch/arm64/mm/fault.c                  | 10 ++++++++++
 10 files changed, 68 insertions(+), 7 deletions(-)

Documentation/arm64/silicon-errata.rst +2 -0

index 936cf2a59ca4..716b279e3b33 100644
@@ -90,6 +90,8 @@ stable kernels.
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Cortex-A76      | #1463225        | ARM64_ERRATUM_1463225       |
 +----------------+-----------------+-----------------+-----------------------------+
+| ARM            | Cortex-A77      | #1508412        | ARM64_ERRATUM_1508412       |
++----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Neoverse-N1     | #1188873,1418040| ARM64_ERRATUM_1418040       |
 +----------------+-----------------+-----------------+-----------------------------+
 | ARM            | Neoverse-N1     | #1349291        | N/A                         |

arch/arm64/Kconfig +19 -0

index a4a094bedcb2..6638444ce0d8 100644
@@ -626,6 +626,25 @@ config ARM64_ERRATUM_1542419
 
 	  If unsure, say Y.
 
+config ARM64_ERRATUM_1508412
+	bool "Cortex-A77: 1508412: workaround deadlock on sequence of NC/Device load and store exclusive or PAR read"
+	default y
+	help
+	  This option adds a workaround for Arm Cortex-A77 erratum 1508412.
+
+	  Affected Cortex-A77 cores (r0p0, r1p0) could deadlock on a sequence
+	  of a store-exclusive or read of PAR_EL1 and a load with device or
+	  non-cacheable memory attributes. The workaround depends on a firmware
+	  counterpart.
+
+	  KVM guests must also have the workaround implemented or they can
+	  deadlock the system.
+
+	  Work around the issue by inserting DMB SY barriers around PAR_EL1
+	  register reads and warning KVM users.
+
+	  If unsure, say Y.
+
 config CAVIUM_ERRATUM_22375
 	bool "Cavium erratum 22375, 24313"
 	default y

arch/arm64/include/asm/cpucaps.h +2 -1

index d7b3bb0cb180..2a2cdb4ced8b 100644
@@ -62,7 +62,8 @@
 #define ARM64_HAS_GENERIC_AUTH			52
 #define ARM64_HAS_32BIT_EL1			53
 #define ARM64_BTI				54
+#define ARM64_WORKAROUND_1508412		55
 
-#define ARM64_NCAPS				55
+#define ARM64_NCAPS				56
 
 #endif /* __ASM_CPUCAPS_H */

arch/arm64/include/asm/kvm_hyp.h +11 -0

index ce3080834bfa..ce5b0d9b12bf 100644
@@ -46,6 +46,17 @@
 #define read_sysreg_el2(r)	read_sysreg_elx(r, _EL2, _EL1)
 #define write_sysreg_el2(v,r)	write_sysreg_elx(v, r, _EL2, _EL1)
 
+static inline u64 __hyp_text read_sysreg_par(void)
+{
+	u64 par;
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	par = read_sysreg(par_el1);
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	return par;
+}
+
 /*
  * Without an __arch_swab32(), we fall back to ___constant_swab32(), but the
  * static inline can allow the compiler to out-of-line this. KVM always wants

arch/arm64/kernel/cpu_errata.c +10 -0

index ad06d6802d2e..5eee8a75540c 100644
@@ -938,6 +938,16 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
 		.matches = has_neoverse_n1_erratum_1542419,
 		.cpu_enable = cpu_enable_trap_ctr_access,
 	},
+#endif
+#ifdef CONFIG_ARM64_ERRATUM_1508412
+	{
+		/* we depend on the firmware portion for correctness */
+		.desc = "ARM erratum 1508412 (kernel portion)",
+		.capability = ARM64_WORKAROUND_1508412,
+		ERRATA_MIDR_RANGE(MIDR_CORTEX_A77,
+				  0, 0,
+				  1, 0),
+	},
 #endif
 	{
 	}

arch/arm64/kvm/arm.c +0 -0

index 9b070b5e212b..21d8b3ca5bd7 100644

arch/arm64/kvm/arm.c int kvm_arch_init(void *opaque) · line 1657

Quoted 6 lines

arch/arm64/kvm/arm.c +2 -1

@@ -1653,7 +1653,8 @@ int kvm_arch_init(void *opaque)
 		return -ENODEV;
 	}
 
-	if (cpus_have_const_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE))
+	if (cpus_have_const_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) ||
+	    cpus_have_const_cap(ARM64_WORKAROUND_1508412))

By the time KVM is initialized, it's safe to use cpus_have_final_cap rather than cpus_have_const_cap as the capabilities will have been finalized.

It looks as though the other places that the capabilities are checked happen after this point so they can also use cpus_have_final_cap.

Quoted 75 lines

arch/arm64/kvm/arm.c +0 -0

 		kvm_info("Guests without required CPU erratum workarounds can deadlock system!\n" \
 			 "Only trusted guests should be used on this system.\n");

arch/arm64/kvm/hyp/switch.c +4 -3

index db1c4487d95d..d76b6638b705 100644
@@ -298,11 +298,12 @@ static bool __hyp_text __translate_far_to_hpfar(u64 far, u64 *hpfar)
 	 * We do need to save/restore PAR_EL1 though, as we haven't
 	 * saved the guest context yet, and we may return early...
 	 */
-	par = read_sysreg(par_el1);
+	par = read_sysreg_par();
+
 	asm volatile("at s1e1r, %0" : : "r" (far));
 	isb();
 
-	tmp = read_sysreg(par_el1);
+	tmp = read_sysreg_par();
 	write_sysreg(par, par_el1);
 
 	if (unlikely(tmp & SYS_PAR_EL1_F))

@@ -925,7 +926,7 @@ void __hyp_text __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt)
 {
 	u64 spsr = read_sysreg_el2(SYS_SPSR);
 	u64 elr = read_sysreg_el2(SYS_ELR);
-	u64 par = read_sysreg(par_el1);
+	u64 par = read_sysreg_par();
 
 	if (!has_vhe())
 		__hyp_call_panic_nvhe(spsr, elr, par, host_ctxt);

arch/arm64/kvm/hyp/sysreg-sr.c +1 -1

index cc7e957f5b2c..f522cbff291d 100644
@@ -52,7 +52,7 @@ static void __hyp_text __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
 	ctxt->sys_regs[CONTEXTIDR_EL1]	= read_sysreg_el1(SYS_CONTEXTIDR);
 	ctxt->sys_regs[AMAIR_EL1]	= read_sysreg_el1(SYS_AMAIR);
 	ctxt->sys_regs[CNTKCTL_EL1]	= read_sysreg_el1(SYS_CNTKCTL);
-	ctxt->sys_regs[PAR_EL1]		= read_sysreg(par_el1);
+	ctxt->sys_regs[PAR_EL1]		= read_sysreg_par();
 	ctxt->sys_regs[TPIDR_EL1]	= read_sysreg(tpidr_el1);
 
 	ctxt->gp_regs.sp_el1		= read_sysreg(sp_el1);

arch/arm64/kvm/sys_regs.c +7 -1

index baf5ce9225ce..3f798e0f1419 100644
@@ -94,10 +94,16 @@ static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
 	case TPIDR_EL1:		*val = read_sysreg_s(SYS_TPIDR_EL1);	break;
 	case AMAIR_EL1:		*val = read_sysreg_s(SYS_AMAIR_EL12);	break;
 	case CNTKCTL_EL1:	*val = read_sysreg_s(SYS_CNTKCTL_EL12);	break;
-	case PAR_EL1:		*val = read_sysreg_s(SYS_PAR_EL1);	break;
 	case DACR32_EL2:	*val = read_sysreg_s(SYS_DACR32_EL2);	break;
 	case IFSR32_EL2:	*val = read_sysreg_s(SYS_IFSR32_EL2);	break;
 	case DBGVCR32_EL2:	*val = read_sysreg_s(SYS_DBGVCR32_EL2);	break;
+	case PAR_EL1:
+		if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+			dmb(sy);
+		*val = read_sysreg_s(SYS_PAR_EL1);
+		if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+			dmb(sy);
+		break;
 	default:		return false;
 	}

arch/arm64/mm/fault.c +1 -0

index 8afb238ff335..98609532e61a 100644
@@ -260,7 +260,17 @@ static bool __kprobes is_spurious_el1_translation_fault(unsigned long addr,
 	local_irq_save(flags);
 	asm volatile("at s1e1r, %0" :: "r" (addr));
 	isb();
+	/*

arch/arm64/mm/fault.c static bool __kprobes is_spurious_el1_translation_fault(unsigned long addr, · line 273

Quoted 10 lines

arch/arm64/mm/fault.c +9 -0

+	 * Arm Erratum 1508412 requires dmb(sy) before and after reads of
+	 * PAR_EL1.
+	 * As this location is not a hot path, just condition it on the config
+	 * option.
+	 */
+	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+		dmb(sy);
 	par = read_sysreg(par_el1);
+	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+		dmb(sy);

Having the condition simply based on the config can bring the additional DMBs even if the CPU isn't affected by this erratum. The config is default y so this doesn't seem like an unlikely situation.

The comment mentions that this is a hot path so would an alternative be applicable? That's the approach taken for the speculative AT errata.

  asm(ALTERNATIVE("nop", "dmb sy", ARM64_ERRATUM_1508412));

Quoted 10 lines

arch/arm64/mm/fault.c +0 -0

 	local_irq_restore(flags);
 
 	/*

Signature · 6 lines

2.25.1

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Catalin Marinas

maintainer

2020-07-29 16:38 UTC #5 lore raw

Re: [PATCH v3 3/3] arm64: Add workaround for Arm Cortex-A77 erratum 1508412

In reply to Rob Herring

Quoted 8 lines · Rob Herring

arch/arm64/include/asm/kvm_hyp.h +0 -0

index ce3080834bfa..ce5b0d9b12bf 100644
@@ -46,6 +46,17 @@
 #define read_sysreg_el2(r)	read_sysreg_elx(r, _EL2, _EL1)
 #define write_sysreg_el2(v,r)	write_sysreg_elx(v, r, _EL2, _EL1)

arch/arm64/include/asm/kvm_hyp.h line 58

Quoted 10 lines

arch/arm64/include/asm/kvm_hyp.h +10 -0

+static inline u64 __hyp_text read_sysreg_par(void)
+{
+	u64 par;
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	par = read_sysreg(par_el1);
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	return par;
+}

Even if that's not always called on a critical path, I agree with Andrew that we could use alternatives here for dmb(sy).

Quoted 11 lines

arch/arm64/kvm/sys_regs.c +0 -1

index baf5ce9225ce..3f798e0f1419 100644
@@ -94,10 +94,16 @@ static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
 	case TPIDR_EL1:		*val = read_sysreg_s(SYS_TPIDR_EL1);	break;
 	case AMAIR_EL1:		*val = read_sysreg_s(SYS_AMAIR_EL12);	break;
 	case CNTKCTL_EL1:	*val = read_sysreg_s(SYS_CNTKCTL_EL12);	break;
-	case PAR_EL1:		*val = read_sysreg_s(SYS_PAR_EL1);	break;
 	case DACR32_EL2:	*val = read_sysreg_s(SYS_DACR32_EL2);	break;
 	case IFSR32_EL2:	*val = read_sysreg_s(SYS_IFSR32_EL2);	break;

arch/arm64/kvm/sys_regs.c static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val) · line 108

Quoted 10 lines

arch/arm64/kvm/sys_regs.c +7 -0

 	case DBGVCR32_EL2:	*val = read_sysreg_s(SYS_DBGVCR32_EL2);	break;
+	case PAR_EL1:
+		if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+			dmb(sy);
+		*val = read_sysreg_s(SYS_PAR_EL1);
+		if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+			dmb(sy);
+		break;
 	default:		return false;
 	}

Can't we use read_sysreg_par() directly here?

Quoted 10 lines

arch/arm64/mm/fault.c +2 -0

index 8afb238ff335..98609532e61a 100644
@@ -260,7 +260,17 @@ static bool __kprobes is_spurious_el1_translation_fault(unsigned long addr,
 	local_irq_save(flags);
 	asm volatile("at s1e1r, %0" :: "r" (addr));
 	isb();
+	/*
+	 * Arm Erratum 1508412 requires dmb(sy) before and after reads of

arch/arm64/mm/fault.c static bool __kprobes is_spurious_el1_translation_fault(unsigned long addr, · line 274

Quoted 10 lines

arch/arm64/mm/fault.c +8 -0

+	 * PAR_EL1.
+	 * As this location is not a hot path, just condition it on the config
+	 * option.
+	 */
+	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+		dmb(sy);
 	par = read_sysreg(par_el1);
+	if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+		dmb(sy);
 	local_irq_restore(flags);

Why not read_sysreg_par()?

Signature · 5 lines

Catalin
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Will Deacon

maintainer

2020-07-30 08:22 UTC #6 lore raw

Re: [PATCH v3 3/3] arm64: Add workaround for Arm Cortex-A77 erratum 1508412

Quoted 9 lines · Catalin Marinas

Quoted 8 lines · Rob Herring

arch/arm64/include/asm/kvm_hyp.h +0 -0

index ce3080834bfa..ce5b0d9b12bf 100644
@@ -46,6 +46,17 @@
 #define read_sysreg_el2(r)	read_sysreg_elx(r, _EL2, _EL1)
 #define write_sysreg_el2(v,r)	write_sysreg_elx(v, r, _EL2, _EL1)

Quoted 13 lines

arch/arm64/include/asm/kvm_hyp.h line 58
Quoted 10 lines
arch/arm64/include/asm/kvm_hyp.h +10 -0
+static inline u64 __hyp_text read_sysreg_par(void)
+{
+	u64 par;
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	par = read_sysreg(par_el1);
+	if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+		dmb(sy);
+	return par;
+}
Even if that's not always called on a critical path, I agree with Andrew that we could use alternatives here for dmb(sy).

Even then, I'm not sure how this helps if the CPU can speculatively branch to the PAR access without executing the DMB. Is that not possible on A77?

Will
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Rob Herring

author

2020-07-31 22:55 UTC #7 lore raw

Re: [PATCH v3 3/3] arm64: Add workaround for Arm Cortex-A77 erratum 1508412

Quoted 25 lines · Will Deacon

Quoted 9 lines · Catalin Marinas
Quoted 8 lines · Rob Herring
arch/arm64/include/asm/kvm_hyp.h +0 -0
index ce3080834bfa..ce5b0d9b12bf 100644
@@ -46,6 +46,17 @@
 #define read_sysreg_el2(r) read_sysreg_elx(r, _EL2, _EL1)
 #define write_sysreg_el2(v,r)      write_sysreg_elx(v, r, _EL2, _EL1)
 
Quoted 13 lines
arch/arm64/include/asm/kvm_hyp.h line 58
Quoted 10 lines
arch/arm64/include/asm/kvm_hyp.h +10 -0
+static inline u64 __hyp_text read_sysreg_par(void)
+{
+   u64 par;
+   if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+           dmb(sy);
+   par = read_sysreg(par_el1);
+   if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+           dmb(sy);
+   return par;
+}
Even if that's not always called on a critical path, I agree with Andrew that we could use alternatives here for dmb(sy).

Quoted 2 lines

Even then, I'm not sure how this helps if the CPU can speculatively branch to the PAR access without executing the DMB. Is that not possible on A77?

I'm told by the h/w folks speculation is not possible in this case.

Rob
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Will Deacon

maintainer

2020-08-03 15:03 UTC #8 lore raw

Re: [PATCH v3 3/3] arm64: Add workaround for Arm Cortex-A77 erratum 1508412

Quoted 26 lines · Rob Herring

Quoted 25 lines · Will Deacon
Quoted 22 lines · Catalin Marinas
Quoted 8 lines · Rob Herring
arch/arm64/include/asm/kvm_hyp.h +0 -0
index ce3080834bfa..ce5b0d9b12bf 100644
@@ -46,6 +46,17 @@
 #define read_sysreg_el2(r) read_sysreg_elx(r, _EL2, _EL1)
 #define write_sysreg_el2(v,r)      write_sysreg_elx(v, r, _EL2, _EL1)
 
arch/arm64/include/asm/kvm_hyp.h line 58
Quoted 10 lines
arch/arm64/include/asm/kvm_hyp.h +10 -0
+static inline u64 __hyp_text read_sysreg_par(void)
+{
+   u64 par;
+   if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+           dmb(sy);
+   par = read_sysreg(par_el1);
+   if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+           dmb(sy);
+   return par;
+}
Even if that's not always called on a critical path, I agree with Andrew that we could use alternatives here for dmb(sy).

Quoted 4 lines

Quoted 2 lines
Even then, I'm not sure how this helps if the CPU can speculatively branch to the PAR access without executing the DMB. Is that not possible on A77?
I'm told by the h/w folks speculation is not possible in this case.

Thanks. Could you add a comment to that effect, please?

Will
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Rob Herring

author

2020-07-31 23:21 UTC #9 lore raw

Re: [PATCH v3 3/3] arm64: Add workaround for Arm Cortex-A77 erratum 1508412

In reply to Catalin Marinas

Quoted 10 lines · Catalin Marinas

Quoted 8 lines · Rob Herring

arch/arm64/include/asm/kvm_hyp.h +0 -0

index ce3080834bfa..ce5b0d9b12bf 100644
@@ -46,6 +46,17 @@
 #define read_sysreg_el2(r)   read_sysreg_elx(r, _EL2, _EL1)
 #define write_sysreg_el2(v,r)        write_sysreg_elx(v, r, _EL2, _EL1)

Quoted 13 lines

arch/arm64/include/asm/kvm_hyp.h line 58
Quoted 10 lines
arch/arm64/include/asm/kvm_hyp.h +10 -0
+static inline u64 __hyp_text read_sysreg_par(void)
+{
+     u64 par;
+     if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+             dmb(sy);
+     par = read_sysreg(par_el1);
+     if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+             dmb(sy);
+     return par;
+}
Even if that's not always called on a critical path, I agree with Andrew that we could use alternatives here for dmb(sy).

His suggestion in the KVM code was to use cpus_have_final_cap() rather than cpus_have_const_cap. But given it's just a dmb or nop, alternatives is a better choice for all of these?

Quoted 15 lines

arch/arm64/kvm/sys_regs.c +3 -1

index baf5ce9225ce..3f798e0f1419 100644
@@ -94,10 +94,16 @@ static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val)
      case TPIDR_EL1:         *val = read_sysreg_s(SYS_TPIDR_EL1);    break;
      case AMAIR_EL1:         *val = read_sysreg_s(SYS_AMAIR_EL12);   break;
      case CNTKCTL_EL1:       *val = read_sysreg_s(SYS_CNTKCTL_EL12); break;
-     case PAR_EL1:           *val = read_sysreg_s(SYS_PAR_EL1);      break;
      case DACR32_EL2:        *val = read_sysreg_s(SYS_DACR32_EL2);   break;
      case IFSR32_EL2:        *val = read_sysreg_s(SYS_IFSR32_EL2);   break;
      case DBGVCR32_EL2:      *val = read_sysreg_s(SYS_DBGVCR32_EL2); break;
+     case PAR_EL1:
+             if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+                     dmb(sy);

Quoted 8 lines

arch/arm64/kvm/sys_regs.c static bool __vcpu_read_sys_reg_from_cpu(int reg, u64 *val) · line 108
Quoted 6 lines
arch/arm64/kvm/sys_regs.c +4 -0
+             *val = read_sysreg_s(SYS_PAR_EL1);
+             if (cpus_have_const_cap(ARM64_WORKAROUND_1508412))
+                     dmb(sy);
+             break;
      default:                return false;
      }
Can't we use read_sysreg_par() directly here?

I assumed read_sysreg_s() was used here for some reason instead of read_sysreg()?

Quoted 14 lines

arch/arm64/mm/fault.c +6 -0

index 8afb238ff335..98609532e61a 100644
@@ -260,7 +260,17 @@ static bool __kprobes is_spurious_el1_translation_fault(unsigned long addr,
      local_irq_save(flags);
      asm volatile("at s1e1r, %0" :: "r" (addr));
      isb();
+     /*
+      * Arm Erratum 1508412 requires dmb(sy) before and after reads of
+      * PAR_EL1.
+      * As this location is not a hot path, just condition it on the config
+      * option.
+      */

Quoted 8 lines

arch/arm64/mm/fault.c static bool __kprobes is_spurious_el1_translation_fault(unsigned long addr, · line 274
Quoted 6 lines
arch/arm64/mm/fault.c +4 -0
+     if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+             dmb(sy);
      par = read_sysreg(par_el1);
+     if (IS_ENABLED(CONFIG_ARM64_ERRATUM_1508412))
+             dmb(sy);
      local_irq_restore(flags);
Why not read_sysreg_par()?

Okay with read_sysreg_par() going in asm/sysreg.h instead? I was hesitant to add it there as there didn't seem to be any other instances of a function for a specific register there.

Rob
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm