Relax sanity checking for mismatched AArch32 EL1

PATCH Superseded Will Deacon 4 participants 32 messages Apr 14, 2020 – Apr 17, 2020

Subsystems (2)

arm64 KVM arm64

lore

Share & tools

lore

Will Deacon

author maintainer

2020-04-14 21:31 UTC #0 lore raw

cover Relax sanity checking for mismatched AArch32 EL1

Hi all,

For better or worse, there are SoCs in production where some, but not all of the CPUs, support AArch32 at EL1 and above. Right now, that results in "SANITY CHECK" warnings during boot and an unconditional kernel taint.

This patch series tries to do a bit better: the only time we care about AArch32 at EL1 is for KVM, so rather than throw our toys out of the pram, we can instead just disable support for 32-bit guests on these systems. In the unlikely scenario of a late CPU hotplug being the first time we notice that AArch32 is not available, then we fail the hotplug (right now we let the thing come online, which leads to hilarious results for any pre-existing 32-bit guests).

Feedback welcome,

Will

Cc Suzuki K Poulose <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Anshuman Khandual <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Sai Prakash Ranjan <[email protected]>

Cc Doug Anderson <[email protected]>

Cc [email protected]

--->8

Sai Prakash Ranjan (1):
arm64: cpufeature: Relax check for IESB support

Will Deacon (7):
arm64: cpufeature: Spell out register fields for ID_ISAR4 and ID_PFR1
arm64: cpufeature: Add CPU capability for AArch32 EL1 support
arm64: cpufeature: Remove redundant call to id_aa64pfr0_32bit_el0()
arm64: cpufeature: Factor out checking of AArch32 features
arm64: cpufeature: Relax AArch32 system checks if EL1 is 64-bit only
arm64: cpufeature: Relax checks for AArch32 support at EL[0-2]
arm64: cpufeature: Add an overview comment for the cpufeature
framework

 arch/arm64/include/asm/cpucaps.h    |   3 +-
 arch/arm64/include/asm/cpufeature.h |   7 +
 arch/arm64/include/asm/sysreg.h     |  18 +++
 arch/arm64/kernel/cpufeature.c      | 236 +++++++++++++++++++++-------
 arch/arm64/kvm/reset.c              |  12 +-
 5 files changed, 206 insertions(+), 70 deletions(-)

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Will Deacon

author maintainer

2020-04-14 21:31 UTC #1 lore raw

PATCH 1/8 arm64: cpufeature: Relax check for IESB support

From: Sai Prakash Ranjan <[email protected]>

We don't care if IESB is supported or not as we always set SCTLR_ELx.IESB and, if it works, that's really great.

Relax the ID_AA64MMFR2.IESB cpufeature check so that we don't warn and taint if it's mismatched.

Signed-off-by Sai Prakash Ranjan <[email protected]>

[will: rewrote commit message]

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/cpufeature.c +1 -1

index 9fac745aa7bb..63df28e6a425 100644
@@ -247,7 +247,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_FWB_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_AT_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LVA_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LSM_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_UAO_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_CNP_SHIFT, 4, 0),

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Suzuki K Poulose

reviewer Reviewed-by on v1 1/8

2020-04-15 09:57 UTC #2 lore raw

Re: [PATCH 1/8] arm64: cpufeature: Relax check for IESB support

Hi Will

Quoted 11 lines · Will Deacon

From: Sai Prakash Ranjan <[email protected]>
We don't care if IESB is supported or not as we always set SCTLR_ELx.IESB and, if it works, that's really great.
Relax the ID_AA64MMFR2.IESB cpufeature check so that we don't warn and taint if it's mismatched.
Signed-off-by Sai Prakash Ranjan <[email protected]>
[will: rewrote commit message]
Signed-off-by Will Deacon <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

Quoted 10 lines

---
  arch/arm64/kernel/cpufeature.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/cpufeature.c +0 -0

index 9fac745aa7bb..63df28e6a425 100644

arch/arm64/kernel/cpufeature.c static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { · line 253

Quoted 8 lines

arch/arm64/kernel/cpufeature.c +1 -1

@@ -247,7 +247,7 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_FWB_SHIFT, 4, 0),
  	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_AT_SHIFT, 4, 0),
  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LVA_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_IESB_SHIFT, 4, 0),
  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_LSM_SHIFT, 4, 0),
  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_UAO_SHIFT, 4, 0),
  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64MMFR2_CNP_SHIFT, 4, 0),

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

Will Deacon

author maintainer

2020-04-14 21:31 UTC #3 lore raw

PATCH 2/8 arm64: cpufeature: Spell out register fields for ID_ISAR4 and ID_PFR1

In reply to Will Deacon

In preparation for runtime updates to the strictness of some AArch32 features, spell out the register fields for ID_ISAR4 and ID_PFR1 to make things clearer to read. Note that this isn't functionally necessary, as the feature arrays themselves are not modified dynamically and remain 'const'.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/sysreg.h | 17 +++++++++++++++++
 arch/arm64/kernel/cpufeature.c  | 28 ++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

arch/arm64/include/asm/sysreg.h +17 -0

index ebc622432831..139cd24c181b 100644
@@ -750,6 +750,15 @@
 
 #define ID_DFR0_PERFMON_8_1		0x4
 
+#define ID_ISAR4_SWP_FRAC_SHIFT		28
+#define ID_ISAR4_PSR_M_SHIFT		24
+#define ID_ISAR4_SYNCH_PRIM_FRAC_SHIFT	20
+#define ID_ISAR4_BARRIER_SHIFT		16
+#define ID_ISAR4_SMC_SHIFT		12
+#define ID_ISAR4_WRITEBACK_SHIFT	8
+#define ID_ISAR4_WITHSHIFTS_SHIFT	4
+#define ID_ISAR4_UNPRIV_SHIFT		0
+
 #define ID_ISAR5_RDM_SHIFT		24
 #define ID_ISAR5_CRC32_SHIFT		16
 #define ID_ISAR5_SHA2_SHIFT		12

@@ -783,6 +792,14 @@
 #define MVFR1_FPDNAN_SHIFT		4
 #define MVFR1_FPFTZ_SHIFT		0
 
+#define ID_PFR1_GIC_SHIFT		28
+#define ID_PFR1_VIRT_FRAC_SHIFT		24
+#define ID_PFR1_SEC_FRAC_SHIFT		20
+#define ID_PFR1_GENTIMER_SHIFT		16
+#define ID_PFR1_VIRTUALIZATION_SHIFT	12
+#define ID_PFR1_MPROGMOD_SHIFT		8
+#define ID_PFR1_SECURITY_SHIFT		4
+#define ID_PFR1_PROGMOD_SHIFT		0
 
 #define ID_AA64MMFR0_TGRAN4_SHIFT	28
 #define ID_AA64MMFR0_TGRAN64_SHIFT	24

arch/arm64/kernel/cpufeature.c +26 -2

index 63df28e6a425..b143f8bc6c52 100644
@@ -332,6 +332,18 @@ static const struct arm64_ftr_bits ftr_id_mmfr4[] = {
 	ARM64_FTR_END,
 };
 
+static const struct arm64_ftr_bits ftr_id_isar4[] = {
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_SWP_FRAC_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_PSR_M_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_SYNCH_PRIM_FRAC_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_BARRIER_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_SMC_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_WRITEBACK_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_WITHSHIFTS_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR4_UNPRIV_SHIFT, 4, 0),
+	ARM64_FTR_END,
+};
+
 static const struct arm64_ftr_bits ftr_id_isar6[] = {
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR6_I8MM_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR6_BF16_SHIFT, 4, 0),

@@ -351,6 +363,18 @@ static const struct arm64_ftr_bits ftr_id_pfr0[] = {
 	ARM64_FTR_END,
 };
 
+static const struct arm64_ftr_bits ftr_id_pfr1[] = {
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_GIC_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_VIRT_FRAC_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_SEC_FRAC_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_GENTIMER_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_VIRTUALIZATION_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_MPROGMOD_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_SECURITY_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_PFR1_PROGMOD_SHIFT, 4, 0),
+	ARM64_FTR_END,
+};
+
 static const struct arm64_ftr_bits ftr_id_dfr0[] = {
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 28, 4, 0),
 	S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, 24, 4, 0xf),	/* PerfMon */

@@ -411,7 +435,7 @@ static const struct __ftr_reg_entry {
 
 	/* Op1 = 0, CRn = 0, CRm = 1 */
 	ARM64_FTR_REG(SYS_ID_PFR0_EL1, ftr_id_pfr0),
-	ARM64_FTR_REG(SYS_ID_PFR1_EL1, ftr_generic_32bits),
+	ARM64_FTR_REG(SYS_ID_PFR1_EL1, ftr_id_pfr1),
 	ARM64_FTR_REG(SYS_ID_DFR0_EL1, ftr_id_dfr0),
 	ARM64_FTR_REG(SYS_ID_MMFR0_EL1, ftr_id_mmfr0),
 	ARM64_FTR_REG(SYS_ID_MMFR1_EL1, ftr_generic_32bits),

@@ -423,7 +447,7 @@ static const struct __ftr_reg_entry {
 	ARM64_FTR_REG(SYS_ID_ISAR1_EL1, ftr_generic_32bits),
 	ARM64_FTR_REG(SYS_ID_ISAR2_EL1, ftr_generic_32bits),
 	ARM64_FTR_REG(SYS_ID_ISAR3_EL1, ftr_generic_32bits),
-	ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits),
+	ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_id_isar4),
 	ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5),
 	ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4),
 	ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_id_isar6),

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Suzuki K Poulose

reviewer Reviewed-by on v1 2/8

2020-04-15 10:04 UTC #4 lore raw

Re: [PATCH 2/8] arm64: cpufeature: Spell out register fields for ID_ISAR4 and ID_PFR1

Quoted 11 lines · Will Deacon

In preparation for runtime updates to the strictness of some AArch32 features, spell out the register fields for ID_ISAR4 and ID_PFR1 to make things clearer to read. Note that this isn't functionally necessary, as the feature arrays themselves are not modified dynamically and remain 'const'.
Signed-off-by Will Deacon <[email protected]>
---
  arch/arm64/include/asm/sysreg.h | 17 +++++++++++++++++
  arch/arm64/kernel/cpufeature.c  | 28 ++++++++++++++++++++++++++--
  2 files changed, 43 insertions(+), 2 deletions(-)

Reviewed-by Suzuki K Poulose <[email protected]>

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

Will Deacon

author maintainer

2020-04-14 21:31 UTC #5 lore raw

PATCH 3/8 arm64: cpufeature: Add CPU capability for AArch32 EL1 support

In reply to Will Deacon

Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.

Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/cpucaps.h |  3 ++-
 arch/arm64/include/asm/sysreg.h  |  1 +
 arch/arm64/kernel/cpufeature.c   | 12 ++++++++++++
 arch/arm64/kvm/reset.c           | 12 ++----------
 4 files changed, 17 insertions(+), 11 deletions(-)

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

index 8eb5a088ae65..c54c674e6c21 100644
@@ -61,7 +61,8 @@
 #define ARM64_HAS_AMU_EXTN			51
 #define ARM64_HAS_ADDRESS_AUTH			52
 #define ARM64_HAS_GENERIC_AUTH			53
+#define ARM64_HAS_32BIT_EL1			54
 
-#define ARM64_NCAPS				54
+#define ARM64_NCAPS				55
 
 #endif /* __ASM_CPUCAPS_H */

arch/arm64/include/asm/sysreg.h +1 -0

index 139cd24c181b..3d8e2f0347c4 100644
@@ -653,6 +653,7 @@
 #define ID_AA64PFR0_ASIMD_NI		0xf
 #define ID_AA64PFR0_ASIMD_SUPPORTED	0x0
 #define ID_AA64PFR0_EL1_64BIT_ONLY	0x1
+#define ID_AA64PFR0_EL1_32BIT_64BIT	0x2
 #define ID_AA64PFR0_EL0_64BIT_ONLY	0x1
 #define ID_AA64PFR0_EL0_32BIT_64BIT	0x2

arch/arm64/kernel/cpufeature.c +12 -0

index b143f8bc6c52..838fe5cc8d7e 100644
@@ -1535,6 +1535,18 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.field_pos = ID_AA64PFR0_EL0_SHIFT,
 		.min_field_value = ID_AA64PFR0_EL0_32BIT_64BIT,
 	},
+#ifdef CONFIG_KVM
+	{
+		.desc = "32-bit EL1 Support",
+		.capability = ARM64_HAS_32BIT_EL1,
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.matches = has_cpuid_feature,
+		.sys_reg = SYS_ID_AA64PFR0_EL1,
+		.sign = FTR_UNSIGNED,
+		.field_pos = ID_AA64PFR0_EL1_SHIFT,
+		.min_field_value = ID_AA64PFR0_EL1_32BIT_64BIT,
+	},
+#endif
 	{
 		.desc = "Kernel page table isolation (KPTI)",
 		.capability = ARM64_UNMAP_KERNEL_AT_EL0,

arch/arm64/kvm/reset.c +2 ~~-10~~

index 30b7ea680f66..102e5c4e01a0 100644
@@ -46,14 +46,6 @@ static const struct kvm_regs default_regs_reset32 = {
 			PSR_AA32_I_BIT | PSR_AA32_F_BIT),
 };
 
-static bool cpu_has_32bit_el1(void)
-{
-	u64 pfr0;
-
-	pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
-	return !!(pfr0 & 0x20);
-}
-
 /**
  * kvm_arch_vm_ioctl_check_extension
  *

@@ -66,7 +58,7 @@ int kvm_arch_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 
 	switch (ext) {
 	case KVM_CAP_ARM_EL1_32BIT:
-		r = cpu_has_32bit_el1();
+		r = cpus_have_const_cap(ARM64_HAS_32BIT_EL1);
 		break;
 	case KVM_CAP_GUEST_DEBUG_HW_BPS:
 		r = get_num_brps();

@@ -288,7 +280,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	switch (vcpu->arch.target) {
 	default:
 		if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
-			if (!cpu_has_32bit_el1())
+			if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1))
 				goto out;
 			cpu_reset = &default_regs_reset32;
 		} else {

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Marc Zyngier

maintainer Acked-by on v1 3/8

2020-04-15 08:56 UTC #6 lore raw

Re: [PATCH 3/8] arm64: cpufeature: Add CPU capability for AArch32 EL1 support

Hi Will,

Quoted 8 lines · Will Deacon

Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.
Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.
Signed-off-by Will Deacon <[email protected]>

Definitely an improvement over the current situation, as the direct read of ID_AA64PFR0 was always a bit dodgy. Given that I'm pretty sure these new braindead SoCs are going to run an older version of the kernel, should we Cc stable for this?

Otherwise:

Acked-by Marc Zyngier <[email protected]>

M.

Signature · 6 lines

Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Will Deacon

author maintainer

2020-04-15 17:00 UTC #7 lore raw

Re: [PATCH 3/8] arm64: cpufeature: Add CPU capability for AArch32 EL1 support

Hi Marc,

Quoted 8 lines · Marc Zyngier

Quoted 7 lines · Will Deacon
Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.
Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.

Quoted 6 lines

Quoted 1 line
Signed-off-by Will Deacon <[email protected]>
Definitely an improvement over the current situation, as the direct read of ID_AA64PFR0 was always a bit dodgy. Given that I'm pretty sure these new braindead SoCs are going to run an older version of the kernel, should we Cc stable for this?

I don't think there's a real need for -stable given that we do at least taint the kernel. That's likely to annoy vendors enough to backport this themselves ;)

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

reviewer Reviewed-by on v1 3/8

2020-04-15 10:09 UTC #8 lore raw

Re: [PATCH 3/8] arm64: cpufeature: Add CPU capability for AArch32 EL1 support

In reply to Will Deacon

Quoted 8 lines · Will Deacon

Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.
Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.
Signed-off-by Will Deacon <[email protected]>

One of the other important missing sanity check for KVM is the VMID width check. I will code something up.

Reviewed-by Suzuki K Poulose <[email protected]>

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Will Deacon

author maintainer

2020-04-15 10:14 UTC #9 lore raw

Re: [PATCH 3/8] arm64: cpufeature: Add CPU capability for AArch32 EL1 support

Quoted 5 lines · Suzuki K Poulose

Quoted 4 lines · Will Deacon
Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.

Quoted 7 lines

Quoted 4 lines
Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.
Signed-off-by Will Deacon <[email protected]>
One of the other important missing sanity check for KVM is the VMID width check. I will code something up.

Cheers! Do we handle things like the IPA size already?

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

reviewer

2020-04-15 13:11 UTC #10 lore raw

Re: [PATCH 3/8] arm64: cpufeature: Add CPU capability for AArch32 EL1 support

Quoted 9 lines · Will Deacon

Quoted 8 lines · Suzuki K Poulose
Quoted 7 lines · Will Deacon
Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.
Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Will Deacon <[email protected]>
One of the other important missing sanity check for KVM is the VMID width check. I will code something up.
Cheers! Do we handle things like the IPA size already?

Good point. No, we don't. I will include this too.

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

Marc Zyngier

maintainer

2020-04-15 13:22 UTC #11 lore raw

Re: [PATCH 3/8] arm64: cpufeature: Add CPU capability for AArch32 EL1 support

Quoted 10 lines · On Wed, 15 Apr 2020 14:15:51 +0100 Suzuki K Poulose <[email protected]>…

Quoted 9 lines · Will Deacon
Quoted 8 lines · Suzuki K Poulose
Quoted 7 lines · Will Deacon
Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.
Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Will Deacon <[email protected]>
One of the other important missing sanity check for KVM is the VMID width check. I will code something up.
Cheers! Do we handle things like the IPA size already?
Good point. No, we don't. I will include this too.

There is also the question of the ARMv8.5-GTG extension. I have a patch that treats it as non-strict, but that approach would fail with KVM if we online a late CPU without support for the right page size at S2.

M.

Signature · 6 lines

Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

reviewer

2020-04-17 09:39 UTC #12 lore raw

Re: [PATCH 3/8] arm64: cpufeature: Add CPU capability for AArch32 EL1 support

Quoted 18 lines · Marc Zyngier

Quoted 15 lines · On Wed, 15 Apr 2020 14:15:51 +0100 Suzuki K Poulose <[email protected]>…
Quoted 14 lines · Will Deacon
Quoted 12 lines · Suzuki K Poulose
Quoted 8 lines · Will Deacon
Although we emit a "SANITY CHECK" warning and taint the kernel if we detect a CPU mismatch for AArch32 support at EL1, we still online the CPU with disastrous consequences for any running 32-bit VMs.
Introduce a capability for AArch32 support at EL1 so that late onlining of incompatible CPUs is forbidden.
Signed-off-by Will Deacon <[email protected]>
One of the other important missing sanity check for KVM is the VMID width check. I will code something up.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Cheers! Do we handle things like the IPA size already?
Good point. No, we don't. I will include this too.
There is also the question of the ARMv8.5-GTG extension. I have a patch that treats it as non-strict, but that approach would fail with KVM if we online a late CPU without support for the right page size at S2.

Good point. Again this can be added to the list of checks performed on the hot-plugged CPUs along with IPA, VMID width.

Cheers
Suzuki

Will Deacon

author maintainer

2020-04-14 21:31 UTC #13 lore raw

PATCH 4/8 arm64: cpufeature: Remove redundant call to id_aa64pfr0_32bit_el0()

In reply to Will Deacon

There's no need to call id_aa64pfr0_32bit_el0() twice because the sanitised value of ID_AA64PFR0_EL1 has already been updated for the CPU being onlined.

Remove the redundant function call.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

arch/arm64/kernel/cpufeature.c +1 -3

index 838fe5cc8d7e..7dfcdd9e75c1 100644
@@ -792,9 +792,7 @@ void update_cpu_features(int cpu,
 	 * If we have AArch32, we care about 32-bit features for compat.
 	 * If the system doesn't support AArch32, don't update them.
 	 */
-	if (id_aa64pfr0_32bit_el0(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1)) &&
-		id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) {
-
+	if (id_aa64pfr0_32bit_el0(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1))) {
 		taint |= check_update_ftr_reg(SYS_ID_DFR0_EL1, cpu,
 					info->reg_id_dfr0, boot->reg_id_dfr0);
 		taint |= check_update_ftr_reg(SYS_ID_ISAR0_EL1, cpu,

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Suzuki K Poulose

reviewer Reviewed-by on v1 4/8

2020-04-15 10:20 UTC #14 lore raw

Re: [PATCH 4/8] arm64: cpufeature: Remove redundant call to id_aa64pfr0_32bit_el0()

Quoted 7 lines · Will Deacon

There's no need to call id_aa64pfr0_32bit_el0() twice because the sanitised value of ID_AA64PFR0_EL1 has already been updated for the CPU being onlined.
Remove the redundant function call.
Signed-off-by Will Deacon <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

Quoted 10 lines

---
  arch/arm64/kernel/cpufeature.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

arch/arm64/kernel/cpufeature.c +0 -0

index 838fe5cc8d7e..7dfcdd9e75c1 100644

arch/arm64/kernel/cpufeature.c void update_cpu_features(int cpu, · line 798

Quoted 10 lines

arch/arm64/kernel/cpufeature.c +1 -3

@@ -792,9 +792,7 @@ void update_cpu_features(int cpu,
  	 * If we have AArch32, we care about 32-bit features for compat.
  	 * If the system doesn't support AArch32, don't update them.
  	 */
-	if (id_aa64pfr0_32bit_el0(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1)) &&
-		id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0)) {
-
+	if (id_aa64pfr0_32bit_el0(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1))) {
  		taint |= check_update_ftr_reg(SYS_ID_DFR0_EL1, cpu,
  					info->reg_id_dfr0, boot->reg_id_dfr0);
  		taint |= check_update_ftr_reg(SYS_ID_ISAR0_EL1, cpu,

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

Will Deacon

author maintainer

2020-04-14 21:31 UTC #15 lore raw

PATCH 5/8 arm64: cpufeature: Factor out checking of AArch32 features

In reply to Will Deacon

update_cpu_features() is pretty large, so split out the checking of the AArch32 features into a separate function and call it after checking the AArch64 features.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 108 +++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 47 deletions(-)

arch/arm64/kernel/cpufeature.c +61 ~~-47~~

index 7dfcdd9e75c1..32828a77acc3 100644
@@ -715,6 +715,65 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
 	return 1;
 }
 
+static int update_32bit_cpu_features(int cpu, struct cpuinfo_arm64 *info,
+				     struct cpuinfo_arm64 *boot)
+{
+	int taint = 0;
+	u64 pfr0 = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
+
+	/*
+	 * If we don't have AArch32 at all then skip the checks entirely
+	 * as the register values may be UNKNOWN and we're not going to be
+	 * using them for anything.
+	 */
+	if (!id_aa64pfr0_32bit_el0(pfr0))
+		return taint;
+
+	taint |= check_update_ftr_reg(SYS_ID_DFR0_EL1, cpu,
+				      info->reg_id_dfr0, boot->reg_id_dfr0);
+	taint |= check_update_ftr_reg(SYS_ID_ISAR0_EL1, cpu,
+				      info->reg_id_isar0, boot->reg_id_isar0);
+	taint |= check_update_ftr_reg(SYS_ID_ISAR1_EL1, cpu,
+				      info->reg_id_isar1, boot->reg_id_isar1);
+	taint |= check_update_ftr_reg(SYS_ID_ISAR2_EL1, cpu,
+				      info->reg_id_isar2, boot->reg_id_isar2);
+	taint |= check_update_ftr_reg(SYS_ID_ISAR3_EL1, cpu,
+				      info->reg_id_isar3, boot->reg_id_isar3);
+	taint |= check_update_ftr_reg(SYS_ID_ISAR4_EL1, cpu,
+				      info->reg_id_isar4, boot->reg_id_isar4);
+	taint |= check_update_ftr_reg(SYS_ID_ISAR5_EL1, cpu,
+				      info->reg_id_isar5, boot->reg_id_isar5);
+	taint |= check_update_ftr_reg(SYS_ID_ISAR6_EL1, cpu,
+				      info->reg_id_isar6, boot->reg_id_isar6);
+
+	/*
+	 * Regardless of the value of the AuxReg field, the AIFSR, ADFSR, and
+	 * ACTLR formats could differ across CPUs and therefore would have to
+	 * be trapped for virtualization anyway.
+	 */
+	taint |= check_update_ftr_reg(SYS_ID_MMFR0_EL1, cpu,
+				      info->reg_id_mmfr0, boot->reg_id_mmfr0);
+	taint |= check_update_ftr_reg(SYS_ID_MMFR1_EL1, cpu,
+				      info->reg_id_mmfr1, boot->reg_id_mmfr1);
+	taint |= check_update_ftr_reg(SYS_ID_MMFR2_EL1, cpu,
+				      info->reg_id_mmfr2, boot->reg_id_mmfr2);
+	taint |= check_update_ftr_reg(SYS_ID_MMFR3_EL1, cpu,
+				      info->reg_id_mmfr3, boot->reg_id_mmfr3);
+	taint |= check_update_ftr_reg(SYS_ID_PFR0_EL1, cpu,
+				      info->reg_id_pfr0, boot->reg_id_pfr0);
+	taint |= check_update_ftr_reg(SYS_ID_PFR1_EL1, cpu,
+				      info->reg_id_pfr1, boot->reg_id_pfr1);
+	taint |= check_update_ftr_reg(SYS_MVFR0_EL1, cpu,
+				      info->reg_mvfr0, boot->reg_mvfr0);
+	taint |= check_update_ftr_reg(SYS_MVFR1_EL1, cpu,
+				      info->reg_mvfr1, boot->reg_mvfr1);
+	taint |= check_update_ftr_reg(SYS_MVFR2_EL1, cpu,
+				      info->reg_mvfr2, boot->reg_mvfr2);
+
+	return taint;
+}
+
+
 /*
  * Update system wide CPU feature registers with the values from a
  * non-boot CPU. Also performs SANITY checks to make sure that there

@@ -788,53 +847,6 @@ void update_cpu_features(int cpu,
 	taint |= check_update_ftr_reg(SYS_ID_AA64ZFR0_EL1, cpu,
 				      info->reg_id_aa64zfr0, boot->reg_id_aa64zfr0);
 
-	/*
-	 * If we have AArch32, we care about 32-bit features for compat.
-	 * If the system doesn't support AArch32, don't update them.
-	 */
-	if (id_aa64pfr0_32bit_el0(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1))) {
-		taint |= check_update_ftr_reg(SYS_ID_DFR0_EL1, cpu,
-					info->reg_id_dfr0, boot->reg_id_dfr0);
-		taint |= check_update_ftr_reg(SYS_ID_ISAR0_EL1, cpu,
-					info->reg_id_isar0, boot->reg_id_isar0);
-		taint |= check_update_ftr_reg(SYS_ID_ISAR1_EL1, cpu,
-					info->reg_id_isar1, boot->reg_id_isar1);
-		taint |= check_update_ftr_reg(SYS_ID_ISAR2_EL1, cpu,
-					info->reg_id_isar2, boot->reg_id_isar2);
-		taint |= check_update_ftr_reg(SYS_ID_ISAR3_EL1, cpu,
-					info->reg_id_isar3, boot->reg_id_isar3);
-		taint |= check_update_ftr_reg(SYS_ID_ISAR4_EL1, cpu,
-					info->reg_id_isar4, boot->reg_id_isar4);
-		taint |= check_update_ftr_reg(SYS_ID_ISAR5_EL1, cpu,
-					info->reg_id_isar5, boot->reg_id_isar5);
-		taint |= check_update_ftr_reg(SYS_ID_ISAR6_EL1, cpu,
-					info->reg_id_isar6, boot->reg_id_isar6);
-
-		/*
-		 * Regardless of the value of the AuxReg field, the AIFSR, ADFSR, and
-		 * ACTLR formats could differ across CPUs and therefore would have to
-		 * be trapped for virtualization anyway.
-		 */
-		taint |= check_update_ftr_reg(SYS_ID_MMFR0_EL1, cpu,
-					info->reg_id_mmfr0, boot->reg_id_mmfr0);
-		taint |= check_update_ftr_reg(SYS_ID_MMFR1_EL1, cpu,
-					info->reg_id_mmfr1, boot->reg_id_mmfr1);
-		taint |= check_update_ftr_reg(SYS_ID_MMFR2_EL1, cpu,
-					info->reg_id_mmfr2, boot->reg_id_mmfr2);
-		taint |= check_update_ftr_reg(SYS_ID_MMFR3_EL1, cpu,
-					info->reg_id_mmfr3, boot->reg_id_mmfr3);
-		taint |= check_update_ftr_reg(SYS_ID_PFR0_EL1, cpu,
-					info->reg_id_pfr0, boot->reg_id_pfr0);
-		taint |= check_update_ftr_reg(SYS_ID_PFR1_EL1, cpu,
-					info->reg_id_pfr1, boot->reg_id_pfr1);
-		taint |= check_update_ftr_reg(SYS_MVFR0_EL1, cpu,
-					info->reg_mvfr0, boot->reg_mvfr0);
-		taint |= check_update_ftr_reg(SYS_MVFR1_EL1, cpu,
-					info->reg_mvfr1, boot->reg_mvfr1);
-		taint |= check_update_ftr_reg(SYS_MVFR2_EL1, cpu,
-					info->reg_mvfr2, boot->reg_mvfr2);
-	}
-
 	if (id_aa64pfr0_sve(info->reg_id_aa64pfr0)) {
 		taint |= check_update_ftr_reg(SYS_ZCR_EL1, cpu,
 					info->reg_zcr, boot->reg_zcr);

@@ -845,6 +857,8 @@ void update_cpu_features(int cpu,
 			sve_update_vq_map();
 	}
 
+	taint |= update_32bit_cpu_features(cpu, info, boot);
+
 	/*
 	 * Mismatched CPU features are a recipe for disaster. Don't even
 	 * pretend to support them.

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Suzuki K Poulose

reviewer Reviewed-by on v1 5/8

2020-04-15 10:32 UTC #16 lore raw

Re: [PATCH 5/8] arm64: cpufeature: Factor out checking of AArch32 features

Quoted 20 lines · Will Deacon

update_cpu_features() is pretty large, so split out the checking of the AArch32 features into a separate function and call it after checking the AArch64 features.
Signed-off-by Will Deacon <[email protected]>
---
  arch/arm64/kernel/cpufeature.c | 108 +++++++++++++++++++--------------
  1 file changed, 61 insertions(+), 47 deletions(-)
arch/arm64/kernel/cpufeature.c +3 -0
index 7dfcdd9e75c1..32828a77acc3 100644
@@ -715,6 +715,65 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
  	return 1;
  }
 
+static int update_32bit_cpu_features(int cpu, struct cpuinfo_arm64 *info,
+				     struct cpuinfo_arm64 *boot)
+{

...

arch/arm64/kernel/cpufeature.c void update_cpu_features(int cpu, · line 861

Quoted 10 lines

arch/arm64/kernel/cpufeature.c +2 -1

-
  	if (id_aa64pfr0_sve(info->reg_id_aa64pfr0)) {
  		taint |= check_update_ftr_reg(SYS_ZCR_EL1, cpu,
  					info->reg_zcr, boot->reg_zcr);

@@ -845,6 +857,8 @@ void update_cpu_features(int cpu,
  			sve_update_vq_map();
  	}
 
+	taint |= update_32bit_cpu_features(cpu, info, boot);
+

This relies on the assumption that the id_aa64pfr0 has been sanitised. It may be worth adding a comment to make sure people (hacking the kernel) don't move this around and break that dependency.

Either ways:

Reviewed-by Suzuki K Poulose <[email protected]>

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

Will Deacon

author maintainer

2020-04-14 21:31 UTC #17 lore raw

PATCH 6/8 arm64: cpufeature: Relax AArch32 system checks if EL1 is 64-bit only

In reply to Will Deacon

If AArch32 is not supported at EL1, the AArch32 feature register fields no longer advertise support for some system features:

* ISAR4.SMC
* PFR1.{Virt_frac, Sec_frac, Virtualization, Security, ProgMod}

In which case, we don't need to emit "SANITY CHECK" failures for all of them.

Add logic to relax the strictness of individual feature register fields at runtime and use this for the fields above if 32-bit EL1 is not supported.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h |  7 ++++++
 arch/arm64/kernel/cpufeature.c      | 33 ++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

arch/arm64/include/asm/cpufeature.h +7 -0

index afe08251ff95..f5c4672e498b 100644
@@ -551,6 +551,13 @@ static inline bool id_aa64mmfr0_mixed_endian_el0(u64 mmfr0)
 		cpuid_feature_extract_unsigned_field(mmfr0, ID_AA64MMFR0_BIGENDEL0_SHIFT) == 0x1;
 }
 
+static inline bool id_aa64pfr0_32bit_el1(u64 pfr0)
+{
+	u32 val = cpuid_feature_extract_unsigned_field(pfr0, ID_AA64PFR0_EL1_SHIFT);
+
+	return val == ID_AA64PFR0_EL1_32BIT_64BIT;
+}
+
 static inline bool id_aa64pfr0_32bit_el0(u64 pfr0)
 {
 	u32 val = cpuid_feature_extract_unsigned_field(pfr0, ID_AA64PFR0_EL0_SHIFT);

arch/arm64/kernel/cpufeature.c +32 -1

index 32828a77acc3..9e0321e3e581 100644
@@ -715,6 +715,25 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
 	return 1;
 }
 
+static void relax_cpu_ftr_reg(u32 sys_id, int field)
+{
+	const struct arm64_ftr_bits *ftrp;
+	struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
+
+	if (WARN_ON(!regp))
+		return;
+
+	for (ftrp = regp->ftr_bits; ftrp->width; ftrp++) {
+		if (ftrp->shift == field) {
+			regp->strict_mask &= ~arm64_ftr_mask(ftrp);
+			break;
+		}
+	}
+
+	/* Bogus field? */
+	WARN_ON(!ftrp->width);
+}
+
 static int update_32bit_cpu_features(int cpu, struct cpuinfo_arm64 *info,
 				     struct cpuinfo_arm64 *boot)
 {

@@ -729,6 +748,19 @@ static int update_32bit_cpu_features(int cpu, struct cpuinfo_arm64 *info,
 	if (!id_aa64pfr0_32bit_el0(pfr0))
 		return taint;
 
+	/*
+	 * If we don't have AArch32 at EL1, then relax the strictness of
+	 * EL1-dependent register fields to avoid spurious sanity check fails.
+	 */
+	if (!id_aa64pfr0_32bit_el1(pfr0)) {
+		relax_cpu_ftr_reg(SYS_ID_ISAR4_EL1, ID_ISAR4_SMC_SHIFT);
+		relax_cpu_ftr_reg(SYS_ID_PFR1_EL1, ID_PFR1_VIRT_FRAC_SHIFT);
+		relax_cpu_ftr_reg(SYS_ID_PFR1_EL1, ID_PFR1_SEC_FRAC_SHIFT);
+		relax_cpu_ftr_reg(SYS_ID_PFR1_EL1, ID_PFR1_VIRTUALIZATION_SHIFT);
+		relax_cpu_ftr_reg(SYS_ID_PFR1_EL1, ID_PFR1_SECURITY_SHIFT);
+		relax_cpu_ftr_reg(SYS_ID_PFR1_EL1, ID_PFR1_PROGMOD_SHIFT);
+	}
+
 	taint |= check_update_ftr_reg(SYS_ID_DFR0_EL1, cpu,
 				      info->reg_id_dfr0, boot->reg_id_dfr0);
 	taint |= check_update_ftr_reg(SYS_ID_ISAR0_EL1, cpu,

@@ -773,7 +805,6 @@ static int update_32bit_cpu_features(int cpu, struct cpuinfo_arm64 *info,
 	return taint;
 }
 
-
 /*
  * Update system wide CPU feature registers with the values from a
  * non-boot CPU. Also performs SANITY checks to make sure that there

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Suzuki K Poulose

reviewer Reviewed-by on v1 6/8

2020-04-15 10:38 UTC #18 lore raw

Re: [PATCH 6/8] arm64: cpufeature: Relax AArch32 system checks if EL1 is 64-bit only

Quoted 6 lines · Will Deacon

If AArch32 is not supported at EL1, the AArch32 feature register fields no longer advertise support for some system features:
* ISAR4.SMC
* PFR1.{Virt_frac, Sec_frac, Virtualization, Security, ProgMod}

Quoted 8 lines

In which case, we don't need to emit "SANITY CHECK" failures for all of them.
Add logic to relax the strictness of individual feature register fields at runtime and use this for the fields above if 32-bit EL1 is not supported.
Signed-off-by Will Deacon <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

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

Will Deacon

author maintainer

2020-04-14 21:31 UTC #19 lore raw

PATCH 7/8 arm64: cpufeature: Relax checks for AArch32 support at EL[0-2]

In reply to Will Deacon

We don't need to be quite as strict about mismatched AArch32 support, which is good because the friendly hardware folks have been busy mismatching this to their hearts' content.

* We don't care about EL2 or EL3 (there are silly comments concerning the latter, so remove those)

* EL1 support is gated by the ARM64_HAS_32BIT_EL1 capability and handled gracefully when a mismatch occurs

* EL1 support is gated by the ARM64_HAS_32BIT_EL0 capability and handled gracefully when a mismatch occurs

Relax the AArch32 checks to FTR_NONSTRICT.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

arch/arm64/kernel/cpufeature.c +3 -7

index 9e0321e3e581..680a453ca8c4 100644
@@ -172,11 +172,10 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_GIC_SHIFT, 4, 0),
 	S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_ASIMD_SHIFT, 4, ID_AA64PFR0_ASIMD_NI),
 	S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI),
-	/* Linux doesn't care about the EL3 */
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL3_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY),
-	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL2_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY),
 	ARM64_FTR_END,
 };

@@ -867,9 +866,6 @@ void update_cpu_features(int cpu,
 	taint |= check_update_ftr_reg(SYS_ID_AA64MMFR2_EL1, cpu,
 				      info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2);
 
-	/*
-	 * EL3 is not our concern.
-	 */
 	taint |= check_update_ftr_reg(SYS_ID_AA64PFR0_EL1, cpu,
 				      info->reg_id_aa64pfr0, boot->reg_id_aa64pfr0);
 	taint |= check_update_ftr_reg(SYS_ID_AA64PFR1_EL1, cpu,

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Suzuki K Poulose

reviewer Reviewed-by on v1 7/8

2020-04-15 10:46 UTC #20 lore raw

Re: [PATCH 7/8] arm64: cpufeature: Relax checks for AArch32 support at EL[0-2]

Quoted 11 lines · Will Deacon

We don't need to be quite as strict about mismatched AArch32 support, which is good because the friendly hardware folks have been busy mismatching this to their hearts' content.
* We don't care about EL2 or EL3 (there are silly comments concerning the latter, so remove those)
* EL1 support is gated by the ARM64_HAS_32BIT_EL1 capability and handled gracefully when a mismatch occurs
* EL1 support is gated by the ARM64_HAS_32BIT_EL0 capability and handled

s/EL1/EL0

Quoted 3 lines

gracefully when a mismatch occurs
Relax the AArch32 checks to FTR_NONSTRICT.

Agreed. We should do something similar for the features exposed by the ELF_HWCAP, of course in a separate series.

Quoted 2 lines

Signed-off-by Will Deacon <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

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

Will Deacon

author maintainer

2020-04-15 10:58 UTC #21 lore raw

Re: [PATCH 7/8] arm64: cpufeature: Relax checks for AArch32 support at EL[0-2]

Quoted 13 lines · Suzuki K Poulose

Quoted 11 lines · Will Deacon
We don't need to be quite as strict about mismatched AArch32 support, which is good because the friendly hardware folks have been busy mismatching this to their hearts' content.
* We don't care about EL2 or EL3 (there are silly comments concerning the latter, so remove those)
* EL1 support is gated by the ARM64_HAS_32BIT_EL1 capability and handled gracefully when a mismatch occurs
* EL1 support is gated by the ARM64_HAS_32BIT_EL0 capability and handled

Quoted 8 lines

s/EL1/EL0
Quoted 3 lines
gracefully when a mismatch occurs
Relax the AArch32 checks to FTR_NONSTRICT.
Agreed. We should do something similar for the features exposed by the ELF_HWCAP, of course in a separate series.

Hmm, I didn't think we needed to touch the HWCAPs, as they're derived from the sanitised feature register values. What am I missing?

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

reviewer

2020-04-15 11:32 UTC #22 lore raw

Re: [PATCH 7/8] arm64: cpufeature: Relax checks for AArch32 support at EL[0-2]

Quoted 18 lines · Will Deacon

Quoted 17 lines · Suzuki K Poulose
Quoted 11 lines · Will Deacon
We don't need to be quite as strict about mismatched AArch32 support, which is good because the friendly hardware folks have been busy mismatching this to their hearts' content.
* We don't care about EL2 or EL3 (there are silly comments concerning the latter, so remove those)
* EL1 support is gated by the ARM64_HAS_32BIT_EL1 capability and handled gracefully when a mismatch occurs
* EL1 support is gated by the ARM64_HAS_32BIT_EL0 capability and handled
s/EL1/EL0
Quoted 2 lines
gracefully when a mismatch occurs

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Relax the AArch32 checks to FTR_NONSTRICT.
Agreed. We should do something similar for the features exposed by the ELF_HWCAP, of course in a separate series.
Hmm, I didn't think we needed to touch the HWCAPs, as they're derived from the sanitised feature register values. What am I missing?

sorry, that was cryptic. I was suggesting to relax the ftr fields to NONSTRICT for the fields covered by ELF HWCAPs (and other CPU hwcaps).

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

Will Deacon

author maintainer

2020-04-15 12:29 UTC #23 lore raw

Re: [PATCH 7/8] arm64: cpufeature: Relax checks for AArch32 support at EL[0-2]

Quoted 21 lines · Suzuki K Poulose

Quoted 20 lines · Will Deacon
Quoted 19 lines · Suzuki K Poulose
Quoted 11 lines · Will Deacon
We don't need to be quite as strict about mismatched AArch32 support, which is good because the friendly hardware folks have been busy mismatching this to their hearts' content.
* We don't care about EL2 or EL3 (there are silly comments concerning the latter, so remove those)
* EL1 support is gated by the ARM64_HAS_32BIT_EL1 capability and handled gracefully when a mismatch occurs
* EL1 support is gated by the ARM64_HAS_32BIT_EL0 capability and handled
s/EL1/EL0
Quoted 3 lines
gracefully when a mismatch occurs
Relax the AArch32 checks to FTR_NONSTRICT.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Agreed. We should do something similar for the features exposed by the ELF_HWCAP, of course in a separate series.
Hmm, I didn't think we needed to touch the HWCAPs, as they're derived from the sanitised feature register values. What am I missing?
sorry, that was cryptic. I was suggesting to relax the ftr fields to NONSTRICT for the fields covered by ELF HWCAPs (and other CPU hwcaps).

Ah, gotcha. Given that the HWCAPs usually describe EL0 features, I say we can punt this down the road until people give us hardware with mismatched AArch32 at EL0.

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Suzuki K Poulose

reviewer

2020-04-17 09:33 UTC #24 lore raw

Re: [PATCH 7/8] arm64: cpufeature: Relax checks for AArch32 support at EL[0-2]

Quoted 28 lines · Will Deacon

Quoted 27 lines · Suzuki K Poulose
Quoted 25 lines · Will Deacon
Quoted 13 lines · Suzuki K Poulose
Quoted 11 lines · Will Deacon
We don't need to be quite as strict about mismatched AArch32 support, which is good because the friendly hardware folks have been busy mismatching this to their hearts' content.
* We don't care about EL2 or EL3 (there are silly comments concerning the latter, so remove those)
* EL1 support is gated by the ARM64_HAS_32BIT_EL1 capability and handled gracefully when a mismatch occurs
* EL1 support is gated by the ARM64_HAS_32BIT_EL0 capability and handled
Quoted 8 lines
s/EL1/EL0
Quoted 3 lines
gracefully when a mismatch occurs
Relax the AArch32 checks to FTR_NONSTRICT.
Agreed. We should do something similar for the features exposed by the ELF_HWCAP, of course in a separate series.
Hmm, I didn't think we needed to touch the HWCAPs, as they're derived from the sanitised feature register values. What am I missing?

Quoted 6 lines

Quoted 2 lines
sorry, that was cryptic. I was suggesting to relax the ftr fields to NONSTRICT for the fields covered by ELF HWCAPs (and other CPU hwcaps).
Ah, gotcha. Given that the HWCAPs usually describe EL0 features, I say we can punt this down the road until people give us hardware with mismatched AArch32 at EL0.

Btw, this is not just mismatched AArch32, but mismatched AArch64 HWCAPs too, which I believe exists. Anyways as you said, we can delay this until we get the reports :-)

Suzuki

Will Deacon

author maintainer

2020-04-14 21:31 UTC #25 lore raw

PATCH 8/8 arm64: cpufeature: Add an overview comment for the cpufeature framework

In reply to Will Deacon

Now that Suzuki isn't within throwing distance, I thought I'd better add a rough overview comment to cpufeature.c so that it doesn't take me days to remember how it works next time.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/cpufeature.c | 43 ++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

arch/arm64/kernel/cpufeature.c +43 -0

index 680a453ca8c4..421ca99dc8fc 100644
@@ -3,6 +3,49 @@
  * Contains CPU feature definitions
  *
  * Copyright (C) 2015 ARM Ltd.
+ *
+ * A note for the weary kernel hacker: the code here is confusing and hard to
+ * follow! That's partly because it's solving a nasty problem, but also because
+ * there's a little bit of over-abstraction that tends to obscure what's going
+ * on behind a maze of helper functions and macros.
+ *
+ * The basic problem is that hardware folks have started gluing together CPUs
+ * with distinct architectural features; in some cases even creating SoCs where
+ * user-visible instructions are available only on a subset of the available
+ * cores. We try to address this by snapshotting the feature registers of the
+ * boot CPU and comparing these with the feature registers of each secondary
+ * CPU when bringing them up. If there is a mismatch, then we update the
+ * snapshot state to indicate the lowest-common denominator of the feature,
+ * known as the "safe" value. This snapshot state can be queried to view the
+ * "sanitised" value of a feature register.
+ *
+ * The sanitised register values are used to decide which capabilities we
+ * have in the system. These may be in the form of traditional "hwcaps"
+ * advertised to userspace or internal "cpucaps" which are used to configure
+ * things like alternative patching and static keys. While a feature mismatch
+ * may result in a TAINT_CPU_OUT_OF_SPEC kernel taint, a capability mismatch
+ * may prevent a CPU from being onlined at all.
+ *
+ * Some implementation details worth remembering:
+ *
+ * - Mismatched features are *always* sanitised to a "safe" value, which
+ *   usually indicates that the feature is not supported.
+ *
+ * - A mismatched feature marked with FTR_STRICT will cause a "SANITY CHECK"
+ *   warning when onlining an offending CPU and the kernel will be tainted
+ *   with TAINT_CPU_OUT_OF_SPEC.
+ *
+ * - Features marked as FTR_VISIBLE have their sanitised value visible to
+ *   userspace. FTR_VISIBLE features in registers that are only visible
+ *   to EL0 by trapping *must* have a corresponding HWCAP so that late
+ *   onlining of CPUs cannot lead to features disappearing at runtime.
+ *
+ * - A "feature" is typically a 4-bit register field. A "capability" is the
+ *   high-level description derived from the sanitised field value.
+ *
+ * - Read the Arm ARM (DDI 0487F.a) section D13.1.3 ("Principles of the ID
+ *   scheme for fields in ID registers") to understand when feature fields
+ *   may be signed or unsigned (FTR_SIGNED and FTR_UNSIGNED accordingly).
  */
 
 #define pr_fmt(fmt) "CPU features: " fmt

Signature · 6 lines

2.26.0.110.g2183baf09c-goog

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

Will Deacon

author maintainer

2020-04-16 11:58 UTC #26 lore raw

Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework

Hi Suzuki,

Quoted 8 lines · Will Deacon

Now that Suzuki isn't within throwing distance, I thought I'd better add a rough overview comment to cpufeature.c so that it doesn't take me days to remember how it works next time.
Signed-off-by Will Deacon <[email protected]>
---
 arch/arm64/kernel/cpufeature.c | 43 ++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Any chance you can look at this one, please? I don't trust myself to get all of the details right here! I'm also wondering whether we should mention something about KVM and the guest view of the registers.

What do you think?

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

Suzuki K Poulose

reviewer

2020-04-16 14:54 UTC #27 lore raw

Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework

In reply to Will Deacon

Hi Will,

Quoted 14 lines · Will Deacon

Now that Suzuki isn't within throwing distance, I thought I'd better add a rough overview comment to cpufeature.c so that it doesn't take me days to remember how it works next time.
Signed-off-by Will Deacon <[email protected]>
---
  arch/arm64/kernel/cpufeature.c | 43 ++++++++++++++++++++++++++++++++++
  1 file changed, 43 insertions(+)
arch/arm64/kernel/cpufeature.c +0 -0
index 680a453ca8c4..421ca99dc8fc 100644

arch/arm64/kernel/cpufeature.c line 10

Quoted 8 lines

arch/arm64/kernel/cpufeature.c +5 -0

@@ -3,6 +3,49 @@
   * Contains CPU feature definitions
   *
   * Copyright (C) 2015 ARM Ltd.
+ *
+ * A note for the weary kernel hacker: the code here is confusing and hard to
+ * follow! That's partly because it's solving a nasty problem, but also because
+ * there's a little bit of over-abstraction that tends to obscure what's going
+ * on behind a maze of helper functions and macros.

Thanks for writing this up !

arch/arm64/kernel/cpufeature.c line 19

Quoted 9 lines

arch/arm64/kernel/cpufeature.c +9 -0

+ *
+ * The basic problem is that hardware folks have started gluing together CPUs
+ * with distinct architectural features; in some cases even creating SoCs where
+ * user-visible instructions are available only on a subset of the available
+ * cores. We try to address this by snapshotting the feature registers of the
+ * boot CPU and comparing these with the feature registers of each secondary
+ * CPU when bringing them up. If there is a mismatch, then we update the
+ * snapshot state to indicate the lowest-common denominator of the feature,
+ * known as the "safe" value. This snapshot state can be queried to view the

I am not sure if the following is implied above.

1) Against the "snapshot" state, where mismatches triggers updating the "snapshot" state to reflect the "safe" value.

2) Compared against the CPU feature registers of *the boot CPU* for "FTR_STRICT" fields and any mismatch triggers TAINT_CPU_OUT_OF_SPEC. This makes sure that warning is generated for each OUT_OF_SPEC secondary CPU.

Quoted 7 lines

arch/arm64/kernel/cpufeature.c +7 -0

+ * "sanitised" value of a feature register.
+ *
+ * The sanitised register values are used to decide which capabilities we
+ * have in the system. These may be in the form of traditional "hwcaps"
+ * advertised to userspace or internal "cpucaps" which are used to configure
+ * things like alternative patching and static keys. While a feature mismatch
+ * may result in a TAINT_CPU_OUT_OF_SPEC kernel taint, a capability mismatch

arch/arm64/kernel/cpufeature.c line 36

Quoted 10 lines

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

+ * may prevent a CPU from being onlined at all.
+ *
+ * Some implementation details worth remembering:
+ *
+ * - Mismatched features are *always* sanitised to a "safe" value, which
+ *   usually indicates that the feature is not supported.
+ *
+ * - A mismatched feature marked with FTR_STRICT will cause a "SANITY CHECK"
+ *   warning when onlining an offending CPU and the kernel will be tainted
+ *   with TAINT_CPU_OUT_OF_SPEC.

As mentioned above, this check is against that of the "boot CPU" register state, which may not be implicit from the statement.

arch/arm64/kernel/cpufeature.c line 42

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +6 -0

+ *
+ * - Features marked as FTR_VISIBLE have their sanitised value visible to
+ *   userspace. FTR_VISIBLE features in registers that are only visible
+ *   to EL0 by trapping *must* have a corresponding HWCAP so that late
+ *   onlining of CPUs cannot lead to features disappearing at runtime.
+ *

As you mentioned in the other response we could add information about the guest view, something like :

- KVM exposes the sanitised value of the feature registers to the guests and is not affected by the FTR_VISIBLE. However, depending on the individual feature support in the hypervisor, some of the fields may be capped/limited.

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

Marc Zyngier

maintainer

2020-04-16 15:26 UTC #28 lore raw

Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework

On 2020-04-16 15:59, Suzuki K Poulose wrote:

Hi Suzuki,

[...]

Quoted 7 lines

As you mentioned in the other response we could add information about the guest view, something like :
- KVM exposes the sanitised value of the feature registers to the guests and is not affected by the FTR_VISIBLE. However, depending on the individual feature support in the hypervisor, some of the fields may be capped/limited.

Although in most cases, what KVM exposes is indeed a strict subset of the host's features, there is a few corner cases where we expose features that do not necessarily exist on the host. For example ARMv8.5-GTG and ARMv8.4-TTL get exposed by the NV patches even if they don't exist on the host, as KVM will actually emulate them.

Not a big deal, but I just wanted to outline that it isn't as clear-cut as it may seem...

Thanks,

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Will Deacon

author maintainer

2020-04-16 18:12 UTC #29 lore raw

Re: [PATCH 8/8] arm64: cpufeature: Add an overview comment for the cpufeature framework

In reply to Suzuki K Poulose

Quoted 10 lines · Suzuki K Poulose

Quoted 9 lines · Will Deacon
Now that Suzuki isn't within throwing distance, I thought I'd better add a rough overview comment to cpufeature.c so that it doesn't take me days to remember how it works next time.
Signed-off-by Will Deacon <[email protected]>
---
  arch/arm64/kernel/cpufeature.c | 43 ++++++++++++++++++++++++++++++++++
  1 file changed, 43 insertions(+)

Quoted 15 lines

arch/arm64/kernel/cpufeature.c line 10

Quoted 13 lines

arch/arm64/kernel/cpufeature.c +5 -0

index 680a453ca8c4..421ca99dc8fc 100644
@@ -3,6 +3,49 @@
   * Contains CPU feature definitions
   *
   * Copyright (C) 2015 ARM Ltd.
+ *
+ * A note for the weary kernel hacker: the code here is confusing and hard to
+ * follow! That's partly because it's solving a nasty problem, but also because
+ * there's a little bit of over-abstraction that tends to obscure what's going
+ * on behind a maze of helper functions and macros.

Thanks for writing this up !

It's purely a selfish thing ;)

Quoted 11 lines

arch/arm64/kernel/cpufeature.c line 19

Quoted 8 lines

arch/arm64/kernel/cpufeature.c +8 -0

+ * The basic problem is that hardware folks have started gluing together CPUs
+ * with distinct architectural features; in some cases even creating SoCs where
+ * user-visible instructions are available only on a subset of the available
+ * cores. We try to address this by snapshotting the feature registers of the
+ * boot CPU and comparing these with the feature registers of each secondary
+ * CPU when bringing them up. If there is a mismatch, then we update the
+ * snapshot state to indicate the lowest-common denominator of the feature,
+ * known as the "safe" value. This snapshot state can be queried to view the

I am not sure if the following is implied above.

Quoted 7 lines

1) Against the "snapshot" state, where mismatches triggers updating the "snapshot" state to reflect the "safe" value.
2) Compared against the CPU feature registers of *the boot CPU* for "FTR_STRICT" fields and any mismatch triggers TAINT_CPU_OUT_OF_SPEC. This makes sure that warning is generated for each OUT_OF_SPEC secondary CPU.

I was trying to avoid talking about the consequences of a mismatch in that paragraph, and instead cover them below:

Quoted 6 lines

arch/arm64/kernel/cpufeature.c line 27

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +6 -0

+ * The sanitised register values are used to decide which capabilities we
+ * have in the system. These may be in the form of traditional "hwcaps"
+ * advertised to userspace or internal "cpucaps" which are used to configure
+ * things like alternative patching and static keys. While a feature mismatch
+ * may result in a TAINT_CPU_OUT_OF_SPEC kernel taint, a capability mismatch
+ * may prevent a CPU from being onlined at all.

Do you think something is missing here?

Quoted 10 lines

arch/arm64/kernel/cpufeature.c line 36

Quoted 9 lines

arch/arm64/kernel/cpufeature.c +9 -0

+ *
+ * Some implementation details worth remembering:
+ *
+ * - Mismatched features are *always* sanitised to a "safe" value, which
+ *   usually indicates that the feature is not supported.
+ *
+ * - A mismatched feature marked with FTR_STRICT will cause a "SANITY CHECK"
+ *   warning when onlining an offending CPU and the kernel will be tainted
+ *   with TAINT_CPU_OUT_OF_SPEC.

Quoted 2 lines

As mentioned above, this check is against that of the "boot CPU" register state, which may not be implicit from the statement.

Hmm, I'm trying to figure out if this matters. I suppose this means you get a SANITY CHECK warning for every mismatching secondary CPU, but that's implied by the above. Is there something else I'm missing?

Quoted 7 lines

arch/arm64/kernel/cpufeature.c line 42

Quoted 6 lines

arch/arm64/kernel/cpufeature.c +6 -0

+ *
+ * - Features marked as FTR_VISIBLE have their sanitised value visible to
+ *   userspace. FTR_VISIBLE features in registers that are only visible
+ *   to EL0 by trapping *must* have a corresponding HWCAP so that late
+ *   onlining of CPUs cannot lead to features disappearing at runtime.
+ *

Quoted 7 lines

As you mentioned in the other response we could add information about the guest view, something like :
- KVM exposes the sanitised value of the feature registers to the guests and is not affected by the FTR_VISIBLE. However, depending on the individual feature support in the hypervisor, some of the fields may be capped/limited.

In light of Marc's comment, I'll add something here along the lines of:

"KVM exposes its own view of the feature registers to guest operating
systems regardless of FTR_VISIBLE. This is typically driven from the sanitised register values to allow virtual CPUs to be migrated between arbitrary physical CPUs, but some features not present on the host are also advertised and emulated. Look at sys_reg_descs[] for the gory details."

Will

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Sai Prakash Ranjan

Tested-by on the v1 series

2020-04-16 08:39 UTC #30 lore raw

In reply to Will Deacon

Quoted 15 lines · Will Deacon

Hi all,
For better or worse, there are SoCs in production where some, but not all of the CPUs, support AArch32 at EL1 and above. Right now, that results in "SANITY CHECK" warnings during boot and an unconditional kernel taint.
This patch series tries to do a bit better: the only time we care about AArch32 at EL1 is for KVM, so rather than throw our toys out of the pram, we can instead just disable support for 32-bit guests on these systems. In the unlikely scenario of a late CPU hotplug being the first time we notice that AArch32 is not available, then we fail the hotplug (right now we let the thing come online, which leads to hilarious results for any pre-existing 32-bit guests).

Quoted 4 lines

Feedback welcome,
Will

Thanks Will, tested this series on QCOM SC7180 and SM8150 SoCs.

For the entire series,

Tested-by [email protected]

-Sai

Signature · 7 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Sai Prakash Ranjan

Tested-by

2020-04-16 10:26 UTC #31 lore raw

Quoted 18 lines · Sai Prakash Ranjan

Quoted 17 lines · Will Deacon
Hi all,
For better or worse, there are SoCs in production where some, but not all of the CPUs, support AArch32 at EL1 and above. Right now, that results in "SANITY CHECK" warnings during boot and an unconditional kernel taint.
This patch series tries to do a bit better: the only time we care about AArch32 at EL1 is for KVM, so rather than throw our toys out of the pram, we can instead just disable support for 32-bit guests on these systems. In the unlikely scenario of a late CPU hotplug being the first time we notice that AArch32 is not available, then we fail the hotplug (right now we let the thing come online, which leads to hilarious results for any pre-existing 32-bit guests).
Feedback welcome,

Quoted 9 lines

Quoted 2 lines
Will
Thanks Will, tested this series on QCOM SC7180 and SM8150 SoCs.
For the entire series,
Tested-by [email protected]

Urgh sorry, it should be

Tested-by Sai Prakash Ranjan <[email protected]>

-Sai

Signature · 7 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel