Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1239757 > unrolled thread

[PATCH v2 07/22] arm64: Keep track of CPU feature registers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 07/22] arm64: Keep track of CPU feature registers "Suzuki K. Poulose" <suzuki.poulose@arm.com> - 2015-10-05 19:10 +0200
    Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers Catalin Marinas <catalin.marinas@arm.com> - 2015-10-07 19:20 +0200
      Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2015-10-08 12:00 +0200
        Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers Catalin Marinas <catalin.marinas@arm.com> - 2015-10-08 17:10 +0200
          Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2015-10-09 15:10 +0200
          Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2015-10-12 19:10 +0200
            Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers Mark Rutland <mark.rutland@arm.com> - 2015-10-12 19:30 +0200
              Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers Catalin Marinas <catalin.marinas@arm.com> - 2015-10-13 11:50 +0200
        Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers "Suzuki K. Poulose" <Suzuki.Poulose@arm.com> - 2015-10-09 13:00 +0200
          Re: [PATCH v2 07/22] arm64: Keep track of CPU feature registers Catalin Marinas <catalin.marinas@arm.com> - 2015-10-09 16:20 +0200

#1239757 — [PATCH v2 07/22] arm64: Keep track of CPU feature registers

This patch adds an infrastructure to keep track of the CPU feature
registers on the system. For each register, the infrastructure keeps
track of the system wide safe value of the feature bits. Also, tracks
the which fields of a register should be matched strictly across all
the CPUs on the system for the SANITY check infrastructure.

The feature bits are classified as one of SCALAR_MIN, SCALAR_MAX and DISCRETE
depending on the implication of the possible values. This information
is used to decide the safe value for a feature.

SCALAR_MIN - The smaller value is safer
SCALAR_MAX - The bigger value is safer
DISCRETE - We can't decide between the two, so a predefined safe_value is used.

This infrastructure will be later used to make better decisions for:

 - Kernel features (e.g, KVM, Debug)
 - SANITY Check
 - CPU capability
 - ELF HWCAP
 - Exposing CPU Feature register to userspace.

Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpu.h        |    1 +
 arch/arm64/include/asm/cpufeature.h |   48 ++++
 arch/arm64/include/asm/sysreg.h     |  120 ++++++++++
 arch/arm64/kernel/cpufeature.c      |  434 +++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/cpuinfo.c         |    3 +-
 5 files changed, 605 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index 30db691..704c17b 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -63,6 +63,7 @@ DECLARE_PER_CPU(struct cpuinfo_arm64, cpu_data);
 void cpuinfo_store_cpu(void);
 void __init cpuinfo_store_boot_cpu(void);
 
+void __init init_cpu_features(struct cpuinfo_arm64 *info);
 void update_cpu_features(struct cpuinfo_arm64 *info);
 
 #endif /* __ASM_CPU_H */
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index b5f313d..7ed92f2 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -35,6 +35,38 @@
 
 #include <linux/kernel.h>
 
+/* CPU feature register tracking */
+enum ftr_type {
+	FTR_DISCRETE,	/* Use a predefined safe value */
+	FTR_SCALAR_MIN,	/* Smaller value is safer */
+	FTR_SCALAR_MAX,	/* Bigger value is safer */
+};
+
+#define FTR_STRICT	true
+#define FTR_NONSTRICT	false
+
+struct arm64_ftr_bits {
+	bool		strict;		/* CPU Sanity check
+					 *  strict matching required ? */
+	enum ftr_type	type;
+	u8		shift;
+	u8		width;
+	s64		safe_val;	/* safe value for discrete features */
+};
+
+/*
+ * @arm64_ftr_reg - Feature register
+ * @strict_mask 	Bits which should match across all CPUs for sanity.
+ * @sys_val		Safe value across the CPUs (system view)
+ */
+struct arm64_ftr_reg {
+	u32			sys_id;
+	const char*		name;
+	u64			strict_mask;
+	u64			sys_val;
+	struct arm64_ftr_bits*	ftr_bits;
+};
+
 struct arm64_cpu_capabilities {
 	const char *desc;
 	u16 capability;
@@ -82,6 +114,22 @@ static inline int __attribute_const__ cpuid_feature_extract_field(u64 features,
 	return (s64)(features << (64 - 4 - field)) >> (64 - 4);
 }
 
+static inline s64 __attribute_const__
+cpuid_feature_extract_field_width(u64 features, int field, u8 width)
+{
+	return (s64)(features << (64 - width - field)) >> (64 - width);
+}
+
+static inline u64 ftr_mask(struct arm64_ftr_bits *ftrp)
+{
+	return (u64) GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift);
+}
+
+static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
+{
+	return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width);
+}
+
 static inline bool id_aa64mmfr0_mixed_endian_el0(u64 mmfr0)
 {
 	return cpuid_feature_extract_field(mmfr0, ID_AA64MMFR0_BIGENDEL_SHIFT) == 0x1 ||
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 04e11b1..dc72fc6 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -59,6 +59,46 @@
 	 ((crn) << CRn_shift) | ((crm) << CRm_shift) | ((op2) << Op2_shift))
 
 
+#define SYS_MIDR_EL1			sys_reg(3, 0, 0, 0, 0)
+#define SYS_MPIDR_EL1			sys_reg(3, 0, 0, 0, 5)
+#define SYS_REVIDR_EL1			sys_reg(3, 0, 0, 0, 6)
+
+#define SYS_ID_PFR0_EL1			sys_reg(3, 0, 0, 1, 0)
+#define SYS_ID_PFR1_EL1			sys_reg(3, 0, 0, 1, 1)
+#define SYS_ID_DFR0_EL1			sys_reg(3, 0, 0, 1, 2)
+#define SYS_ID_MMFR0_EL1		sys_reg(3, 0, 0, 1, 4)
+#define SYS_ID_MMFR1_EL1		sys_reg(3, 0, 0, 1, 5)
+#define SYS_ID_MMFR2_EL1		sys_reg(3, 0, 0, 1, 6)
+#define SYS_ID_MMFR3_EL1		sys_reg(3, 0, 0, 1, 7)
+
+#define SYS_ID_ISAR0_EL1		sys_reg(3, 0, 0, 2, 0)
+#define SYS_ID_ISAR1_EL1		sys_reg(3, 0, 0, 2, 1)
+#define SYS_ID_ISAR2_EL1		sys_reg(3, 0, 0, 2, 2)
+#define SYS_ID_ISAR3_EL1		sys_reg(3, 0, 0, 2, 3)
+#define SYS_ID_ISAR4_EL1		sys_reg(3, 0, 0, 2, 4)
+#define SYS_ID_ISAR5_EL1		sys_reg(3, 0, 0, 2, 5)
+#define SYS_ID_MMFR4_EL1		sys_reg(3, 0, 0, 2, 6)
+
+#define SYS_MVFR0_EL1			sys_reg(3, 0, 0, 3, 0)
+#define SYS_MVFR1_EL1			sys_reg(3, 0, 0, 3, 1)
+#define SYS_MVFR2_EL1			sys_reg(3, 0, 0, 3, 2)
+
+#define SYS_ID_AA64PFR0_EL1		sys_reg(3, 0, 0, 4, 0)
+#define SYS_ID_AA64PFR1_EL1		sys_reg(3, 0, 0, 4, 1)
+
+#define SYS_ID_AA64DFR0_EL1		sys_reg(3, 0, 0, 5, 0)
+#define SYS_ID_AA64DFR1_EL1		sys_reg(3, 0, 0, 5, 1)
+
+#define SYS_ID_AA64ISAR0_EL1		sys_reg(3, 0, 0, 6, 0)
+#define SYS_ID_AA64ISAR1_EL1		sys_reg(3, 0, 0, 6, 1)
+
+#define SYS_ID_AA64MMFR0_EL1		sys_reg(3, 0, 0, 7, 0)
+#define SYS_ID_AA64MMFR1_EL1		sys_reg(3, 0, 0, 7, 1)
+
+#define SYS_CNTFRQ_EL0			sys_reg(3, 3, 14, 0, 0)
+#define SYS_CTR_EL0			sys_reg(3, 3, 0, 0, 1)
+#define SYS_DCZID_EL0			sys_reg(3, 3, 0, 0, 7)
+
 #define REG_PSTATE_PAN_IMM	sys_reg(0, 0, 4, 0, 4)
 #define SET_PSTATE_PAN(x)	__inst_arm(0xd5000000 |\
 					   (REG_PSTATE_PAN_IMM << 5) |\
@@ -69,8 +109,88 @@
 #define SCTLR_EL1_SED		(0x1 << 8)
 #define SCTLR_EL1_SPAN		(0x1 << 23)
 
+/* id_aa64isar0 */
+#define ID_AA64ISAR0_RDM_SHIFT		28
+#define ID_AA64ISAR0_ATOMICS_SHIFT	20
+#define ID_AA64ISAR0_CRC32_SHIFT	16
+#define ID_AA64ISAR0_SHA2_SHIFT		12
+#define ID_AA64ISAR0_SHA1_SHIFT		8
+#define ID_AA64ISAR0_AES_SHIFT		4
+
+/* id_aa64pfr0 */
+#define ID_AA64PFR0_GIC_SHIFT		24
+#define ID_AA64PFR0_ASIMD_SHIFT		20
+#define ID_AA64PFR0_FP_SHIFT		16
+#define ID_AA64PFR0_EL3_SHIFT		12
+#define ID_AA64PFR0_EL2_SHIFT		8
+#define ID_AA64PFR0_EL1_SHIFT		4
+#define ID_AA64PFR0_EL0_SHIFT		0
+
+#define ID_AA64PFR0_FP_NI		0xf
+#define ID_AA64PFR0_FP_ON		0x0
+#define ID_AA64PFR0_ASIMD_NI		0xf
+#define ID_AA64PFR0_ASIMD_ON		0x0
+#define ID_AA64PFR0_EL1_64BIT_ONLY	0x1
+#define ID_AA64PFR0_EL0_64BIT_ONLY	0x1
+
+/* id_aa64mmfr0 */
+#define ID_AA64MMFR0_TGRAN4_SHIFT	28
+#define ID_AA64MMFR0_TGRAN64_SHIFT	24
+#define ID_AA64MMFR0_TGRAN16_SHIFT	20
 #define ID_AA64MMFR0_BIGENDEL0_SHIFT	16
+#define ID_AA64MMFR0_SNSMEM_SHIFT	12
 #define ID_AA64MMFR0_BIGENDEL_SHIFT	8
+#define ID_AA64MMFR0_ASID_SHIFT		4
+#define ID_AA64MMFR0_PARANGE_SHIFT	0
+
+#define ID_AA64MMFR0_TGRAN4_NI		0xf
+#define ID_AA64MMFR0_TGRAN4_ON		0x0
+#define ID_AA64MMFR0_TGRAN64_NI		0xf
+#define ID_AA64MMFR0_TGRAN64_ON		0x0
+#define ID_AA64MMFR0_TGRAN16_NI		0x0
+#define ID_AA64MMFR0_TGRAN16_ON		0x1
+
+/* id_aa64mmfr1 */
+#define ID_AA64MMFR1_PAN_SHIFT		20
+#define ID_AA64MMFR1_LOR_SHIFT		16
+#define ID_AA64MMFR1_HPD_SHIFT		12
+#define ID_AA64MMFR1_VHE_SHIFT		8
+#define ID_AA64MMFR1_VMIDBITS_SHIFT	4
+#define ID_AA64MMFR1_HADBS_SHIFT	0
+
+/* id_aa64dfr0 */
+#define ID_AA64DFR0_CTX_CMPS_SHIFT	28
+#define ID_AA64DFR0_WRPS_SHIFT		20
+#define ID_AA64DFR0_BRPS_SHIFT		12
+#define ID_AA64DFR0_PMUVER_SHIFT	8
+#define ID_AA64DFR0_TRACEVER_SHIFT	4
+#define ID_AA64DFR0_DEBUGVER_SHIFT	0
+
+#define ID_ISAR5_RDM_SHIFT		24
+#define ID_ISAR5_CRC32_SHIFT		16
+#define ID_ISAR5_SHA2_SHIFT		12
+#define ID_ISAR5_SHA1_SHIFT		8
+#define ID_ISAR5_AES_SHIFT		4
+#define ID_ISAR5_SEVL_SHIFT		0
+
+#define MVFR0_FPROUND_SHIFT		28
+#define MVFR0_FPSHVEC_SHIFT		24
+#define MVFR0_FPSQRT_SHIFT		20
+#define MVFR0_FPDIVIDE_SHIFT		16
+#define MVFR0_FPTRAP_SHIFT		12
+#define MVFR0_FPDP_SHIFT		8
+#define MVFR0_FPSP_SHIFT		4
+#define MVFR0_SIMD_SHIFT		0
+
+#define MVFR1_SIMDFMAC_SHIFT		28
+#define MVFR1_FPHP_SHIFT		24
+#define MVFR1_SIMDHP_SHIFT		20
+#define MVFR1_SIMDSP_SHIFT		16
+#define MVFR1_SIMDINT_SHIFT		12
+#define MVFR1_SIMDLS_SHIFT		8
+#define MVFR1_FPDNAN_SHIFT		4
+#define MVFR1_FPFTZ_SHIFT		0
+
 
 #ifdef __ASSEMBLY__
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 1ae8b24..d42ad90 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -58,8 +58,442 @@ static void update_mixed_endian_el0_support(struct cpuinfo_arm64 *info)
 	mixed_endian_el0 &= id_aa64mmfr0_mixed_endian_el0(info->reg_id_aa64mmfr0);
 }
 
+#define ARM64_FTR_BITS(ftr_strict, ftr_type, ftr_shift, ftr_width, ftr_safe_val) \
+	{							\
+		.strict = ftr_strict,				\
+		.type = ftr_type,				\
+		.shift = ftr_shift,				\
+		.width = ftr_width,				\
+		.safe_val = ftr_safe_val,			\
+	}
+
+#define ARM64_FTR_END					\
+	{						\
+		.width = 0,				\
+	}
+
+static struct arm64_ftr_bits ftr_id_aa64isar0[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 32, 32, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64ISAR0_RDM_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 24, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64ISAR0_ATOMICS_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64ISAR0_CRC32_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64ISAR0_SHA2_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64ISAR0_SHA1_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64ISAR0_AES_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// RAZ
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 32, 32, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 28, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64PFR0_GIC_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64PFR0_ASIMD_SHIFT, 4, ID_AA64PFR0_ASIMD_NI),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64PFR0_FP_SHIFT, 4, ID_AA64PFR0_FP_NI),
+	/* Linux doesn't care about the EL3 */
+	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_DISCRETE, ID_AA64PFR0_EL3_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64PFR0_EL2_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64PFR0_EL1_SHIFT, 4, ID_AA64PFR0_EL1_64BIT_ONLY),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64PFR0_EL0_SHIFT, 4, ID_AA64PFR0_EL0_64BIT_ONLY),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 32, 32, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR0_TGRAN4_SHIFT, 4, ID_AA64MMFR0_TGRAN4_NI),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR0_TGRAN64_SHIFT, 4, ID_AA64MMFR0_TGRAN64_NI),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR0_TGRAN16_SHIFT, 4, ID_AA64MMFR0_TGRAN16_NI),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR0_BIGENDEL0_SHIFT, 4, 0),
+	/* Linux shouldn't care about secure memory */
+	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_DISCRETE, ID_AA64MMFR0_SNSMEM_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR0_BIGENDEL_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR0_ASID_SHIFT, 4, 0),
+	/*
+	 * Differing PARange is fine as long as all peripherals and memory are mapped
+	 * within the minimum PARange of all CPUs
+	 */
+	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_SCALAR_MIN, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 32, 32, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64MMFR1_PAN_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR1_LOR_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR1_HPD_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR1_VHE_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR1_VMIDBITS_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64MMFR1_HADBS_SHIFT, 4, 0),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_ctr[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 31, 1, 1),	// RAO
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 28, 3, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MAX, 24, 4, 0),	// CWG
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 20, 4, 0),	// ERG
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 16, 4, 1),	// DminLine
+	/*
+	 * Linux can handle differing I-cache policies. Userspace JITs will
+	 * make use of *minLine
+	 */
+	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_DISCRETE, 14, 2, 0),	// L1Ip
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 10, 0),	// RAZ
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 0, 4, 0),	// IminLine
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_id_mmfr0[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 28, 4, 0),	// InnerShr
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 24, 4, 0),	// FCSE
+	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_SCALAR_MIN, 20, 4, 0),	// AuxReg
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 16, 4, 0),	// TCM
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 12, 4, 0),	// ShareLvl
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 8, 4, 0),	// OuterShr
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 4, 0),	// PMSA
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// VMSA
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 32, 32, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_mvfr2[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 8, 24, 0),	// RAZ
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 4, 0),	// FPMisc
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// SIMDMisc
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_dczid[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 5, 27, 0),// RAZ
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 1, 1),	// DZP
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 0, 4, 0),	// BS
+	ARM64_FTR_END,
+};
+
+
+static struct arm64_ftr_bits ftr_id_isar5[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_ISAR5_RDM_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 20, 4, 0),	// RAZ
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_ISAR5_CRC32_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_ISAR5_SHA2_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_ISAR5_SHA1_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_ISAR5_AES_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, ID_ISAR5_SEVL_SHIFT, 4, 0),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_id_mmfr4[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 8, 24, 0),	// RAZ
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 4, 0),	// ac2
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// RAZ
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_id_pfr0[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 16, 16, 0),	// RAZ
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 12, 4, 0),	// State3
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 8, 4, 0),	// State2
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 4, 0),	// State1
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// State0
+	ARM64_FTR_END,
+};
+
+/*
+ * Common ftr bits for a 32bit register with all hidden, strict
+ * attributes, with 4bit feature fields and a default safe value of
+ * 0. Covers the following 32bit registers:
+ * id_isar[0-4], id_mmfr[1-3], id_pfr1, mvfr[0-1]
+ */
+static struct arm64_ftr_bits ftr_generic_scalar_32bit[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 28, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 24, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 20, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 16, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 12, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 8, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 4, 4, 0),
+	ARM64_FTR_BITS(FTR_STRICT, FTR_SCALAR_MIN, 0, 4, 0),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_generic[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 64, 0),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_generic32[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 32, 0),
+	ARM64_FTR_END,
+};
+
+static struct arm64_ftr_bits ftr_aa64raz[] = {
+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 64, 0),
+	ARM64_FTR_END,
+};
+
+#define ARM64_FTR_REG(id, ftr_table)			\
+	[ sys_reg_Op2(id) ] = {				\
+		.sys_id = id,				\
+		.name = #id,				\
+		.ftr_bits = &((ftr_table)[0]),		\
+	}
+
+static struct arm64_ftr_reg crm_1[] = {
+	ARM64_FTR_REG(SYS_ID_PFR0_EL1, ftr_id_pfr0),
+	ARM64_FTR_REG(SYS_ID_PFR1_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_DFR0_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_MMFR0_EL1, ftr_id_mmfr0),
+	ARM64_FTR_REG(SYS_ID_MMFR1_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_MMFR2_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_MMFR3_EL1, ftr_generic_scalar_32bit),
+};
+
+static struct arm64_ftr_reg crm_2[] = {
+	ARM64_FTR_REG(SYS_ID_ISAR0_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_ISAR1_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_ISAR2_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_ISAR3_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5),
+	ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4),
+};
+
+static struct arm64_ftr_reg crm_3[] = {
+	ARM64_FTR_REG(SYS_MVFR0_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_MVFR1_EL1, ftr_generic_scalar_32bit),
+	ARM64_FTR_REG(SYS_MVFR2_EL1, ftr_mvfr2),
+};
+
+static struct arm64_ftr_reg crm_4[] = {
+	ARM64_FTR_REG(SYS_ID_AA64PFR0_EL1, ftr_id_aa64pfr0),
+	ARM64_FTR_REG(SYS_ID_AA64PFR1_EL1, ftr_aa64raz),
+};
+
+static struct arm64_ftr_reg crm_5[] = {
+	ARM64_FTR_REG(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0),
+	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_generic),
+};
+
+static struct arm64_ftr_reg crm_6[] = {
+	ARM64_FTR_REG(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0),
+	ARM64_FTR_REG(SYS_ID_AA64ISAR1_EL1, ftr_aa64raz),
+};
+
+static struct arm64_ftr_reg crm_7[] = {
+	ARM64_FTR_REG(SYS_ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0),
+	ARM64_FTR_REG(SYS_ID_AA64MMFR1_EL1, ftr_id_aa64mmfr1),
+};
+
+static struct arm64_ftr_reg op1_3[] = {
+	ARM64_FTR_REG(SYS_CTR_EL0, ftr_ctr),
+	ARM64_FTR_REG(SYS_DCZID_EL0, ftr_dczid),
+	ARM64_FTR_REG(SYS_CNTFRQ_EL0, ftr_generic32),
+};
+
+#define ARM64_REG_TABLE(table) 		\
+	{ .n = ARRAY_SIZE(table), .regs = table }
+static struct arm64_reg_table {
+	int n;
+	struct arm64_ftr_reg *regs;
+} op1_0[] = {
+	ARM64_REG_TABLE(crm_1),
+	ARM64_REG_TABLE(crm_2),
+	ARM64_REG_TABLE(crm_3),
+	ARM64_REG_TABLE(crm_4),
+	ARM64_REG_TABLE(crm_5),
+	ARM64_REG_TABLE(crm_6),
+	ARM64_REG_TABLE(crm_7),
+};
+
+/*
+ * get_arm6_sys_reg - Lookup a feature register entry using its
+ * sys_reg() encoding.
+ *
+ * We track only the following space:
+ * Op0 = 3, Op1 = 0, CRn = 0, CRm = [1 - 7], Op2 = [0 - 7]
+ * Op0 = 3, Op1 = 3, CRn = 0, CRm = 0, Op2 = { 1, 7 } 	(CTR, DCZID)
+ * Op0 = 3, Op1 = 3, CRn = 14, CRm = 0, Op2 = 0		(CNTFRQ)
+ *
+ * The space (3, 0, 0, {1-7}, {0-7}) is arranged in a 2D array op1_0,
+ * indexed by CRm and Op2. Since not all CRm's have fully allocated Op2's
+ * arm64_reg_table[CRm-1].n indicates the largest Op2 tracked for CRm.
+ *
+ * Since we have limited number of entries with Op1 = 3, we use linear search
+ * to find the reg.
+ *
+ */
+static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)
+{
+	int i;
+	u8 op2, crn, crm;
+	u8 op1 = sys_reg_Op1(sys_id);
+
+	if (sys_reg_Op0(sys_id) != 3)
+		return NULL;
+	switch (op1) {
+	case 0:
+
+		crm = sys_reg_CRm(sys_id);
+		op2 = sys_reg_Op2(sys_id);
+		crn = sys_reg_CRn(sys_id);
+		if (crn || !crm || crm > 7)
+			return NULL;
+		if (op2 < op1_0[crm - 1].n &&
+			op1_0[crm - 1].regs[op2].sys_id == sys_id)
+			return &op1_0[crm - 1].regs[op2];
+		return NULL;
+	case 3:
+		for (i = 0; i < ARRAY_SIZE(op1_3); i++)
+			if (op1_3[i].sys_id == sys_id)
+				return &op1_3[i];
+	}
+	return NULL;
+}
+
+static u64 arm64_ftr_set_value(struct arm64_ftr_bits *ftrp, s64 reg, s64 ftr_val)
+{
+	u64 mask = ftr_mask(ftrp);
+
+	reg &= ~mask;
+	reg |= (ftr_val << ftrp->shift) & mask;
+	return reg;
+}
+
+static s64 arm64_ftr_safe_value(struct arm64_ftr_bits *ftrp, s64 new, s64 cur)
+{
+	switch(ftrp->type) {
+	case FTR_DISCRETE:
+		return ftrp->safe_val;
+	case FTR_SCALAR_MIN:
+		return new < cur ? new : cur;
+	case FTR_SCALAR_MAX:
+		return new > cur ? new : cur;
+	}
+
+	BUG();
+	return 0;
+}
+
+/*
+ * Initialise the CPU feature register from Boot CPU values.
+ * Also initiliases the strict_mask for the register.
+ */
+static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
+{
+	u64 val = 0;
+	u64 strict_mask = ~0x0ULL;
+	struct arm64_ftr_bits *ftrp;
+	struct arm64_ftr_reg *reg = get_arm64_sys_reg(sys_reg);
+
+	BUG_ON(!reg);
+
+	for(ftrp  = reg->ftr_bits; ftrp->width; ftrp++) {
+		s64 ftr_new = arm64_ftr_value(ftrp, new);
+
+		val = arm64_ftr_set_value(ftrp, val, ftr_new);
+		if (!ftrp->strict)
+			strict_mask &= ~ftr_mask(ftrp);
+	}
+	reg->sys_val = val;
+	reg->strict_mask = strict_mask;
+}
+
+void __init init_cpu_features(struct cpuinfo_arm64 *info)
+{
+	init_cpu_ftr_reg(SYS_CTR_EL0, info->reg_ctr);
+	init_cpu_ftr_reg(SYS_DCZID_EL0, info->reg_dczid);
+	init_cpu_ftr_reg(SYS_CNTFRQ_EL0, info->reg_cntfrq);
+	init_cpu_ftr_reg(SYS_ID_AA64DFR0_EL1, info->reg_id_aa64dfr0);
+	init_cpu_ftr_reg(SYS_ID_AA64DFR1_EL1, info->reg_id_aa64dfr1);
+	init_cpu_ftr_reg(SYS_ID_AA64ISAR0_EL1, info->reg_id_aa64isar0);
+	init_cpu_ftr_reg(SYS_ID_AA64ISAR1_EL1, info->reg_id_aa64isar1);
+	init_cpu_ftr_reg(SYS_ID_AA64MMFR0_EL1, info->reg_id_aa64mmfr0);
+	init_cpu_ftr_reg(SYS_ID_AA64MMFR1_EL1, info->reg_id_aa64mmfr1);
+	init_cpu_ftr_reg(SYS_ID_AA64PFR0_EL1, info->reg_id_aa64pfr0);
+	init_cpu_ftr_reg(SYS_ID_AA64PFR1_EL1, info->reg_id_aa64pfr1);
+	init_cpu_ftr_reg(SYS_ID_DFR0_EL1, info->reg_id_dfr0);
+	init_cpu_ftr_reg(SYS_ID_ISAR0_EL1, info->reg_id_isar0);
+	init_cpu_ftr_reg(SYS_ID_ISAR1_EL1, info->reg_id_isar1);
+	init_cpu_ftr_reg(SYS_ID_ISAR2_EL1, info->reg_id_isar2);
+	init_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3);
+	init_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4);
+	init_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5);
+	init_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0);
+	init_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1);
+	init_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2);
+	init_cpu_ftr_reg(SYS_ID_MMFR3_EL1, info->reg_id_mmfr3);
+	init_cpu_ftr_reg(SYS_ID_PFR0_EL1, info->reg_id_pfr0);
+	init_cpu_ftr_reg(SYS_ID_PFR1_EL1, info->reg_id_pfr1);
+	init_cpu_ftr_reg(SYS_MVFR0_EL1, info->reg_mvfr0);
+	init_cpu_ftr_reg(SYS_MVFR1_EL1, info->reg_mvfr1);
+	init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2);
+
+	/* This will be removed later, once we start using the infrastructure */
+	update_mixed_endian_el0_support(info);
+}
+
+static void update_cpu_ftr_reg(u32 sys_reg, u64 new)
+{
+	struct arm64_ftr_bits *ftrp;
+	struct arm64_ftr_reg *reg = get_arm64_sys_reg(sys_reg);
+
+	BUG_ON(!reg);
+
+	for(ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
+		s64 ftr_cur = arm64_ftr_value(ftrp, reg->sys_val);
+		s64 ftr_new = arm64_ftr_value(ftrp, new);
+
+		if (ftr_cur == ftr_new)
+			continue;
+		/* Find a safe value */
+		ftr_new = arm64_ftr_safe_value(ftrp, ftr_new, ftr_cur);
+		reg->sys_val = arm64_ftr_set_value(ftrp, reg->sys_val, ftr_new);
+	}
+
+}
+
+/* Update CPU feature register from non-boot CPU */
 void update_cpu_features(struct cpuinfo_arm64 *info)
 {
+	update_cpu_ftr_reg(SYS_CTR_EL0, info->reg_ctr);
+	update_cpu_ftr_reg(SYS_DCZID_EL0, info->reg_dczid);
+	update_cpu_ftr_reg(SYS_CNTFRQ_EL0, info->reg_cntfrq);
+	update_cpu_ftr_reg(SYS_ID_AA64DFR0_EL1, info->reg_id_aa64dfr0);
+	update_cpu_ftr_reg(SYS_ID_AA64DFR1_EL1, info->reg_id_aa64dfr1);
+	update_cpu_ftr_reg(SYS_ID_AA64ISAR0_EL1, info->reg_id_aa64isar0);
+	update_cpu_ftr_reg(SYS_ID_AA64ISAR1_EL1, info->reg_id_aa64isar1);
+	update_cpu_ftr_reg(SYS_ID_AA64MMFR0_EL1, info->reg_id_aa64mmfr0);
+	update_cpu_ftr_reg(SYS_ID_AA64MMFR1_EL1, info->reg_id_aa64mmfr1);
+	update_cpu_ftr_reg(SYS_ID_AA64PFR0_EL1, info->reg_id_aa64pfr0);
+	update_cpu_ftr_reg(SYS_ID_AA64PFR1_EL1, info->reg_id_aa64pfr1);
+	update_cpu_ftr_reg(SYS_ID_DFR0_EL1, info->reg_id_dfr0);
+	update_cpu_ftr_reg(SYS_ID_ISAR0_EL1, info->reg_id_isar0);
+	update_cpu_ftr_reg(SYS_ID_ISAR1_EL1, info->reg_id_isar1);
+	update_cpu_ftr_reg(SYS_ID_ISAR2_EL1, info->reg_id_isar2);
+	update_cpu_ftr_reg(SYS_ID_ISAR3_EL1, info->reg_id_isar3);
+	update_cpu_ftr_reg(SYS_ID_ISAR4_EL1, info->reg_id_isar4);
+	update_cpu_ftr_reg(SYS_ID_ISAR5_EL1, info->reg_id_isar5);
+	update_cpu_ftr_reg(SYS_ID_MMFR0_EL1, info->reg_id_mmfr0);
+	update_cpu_ftr_reg(SYS_ID_MMFR1_EL1, info->reg_id_mmfr1);
+	update_cpu_ftr_reg(SYS_ID_MMFR2_EL1, info->reg_id_mmfr2);
+	update_cpu_ftr_reg(SYS_ID_MMFR3_EL1, info->reg_id_mmfr3);
+	update_cpu_ftr_reg(SYS_ID_PFR0_EL1, info->reg_id_pfr0);
+	update_cpu_ftr_reg(SYS_ID_PFR1_EL1, info->reg_id_pfr1);
+	update_cpu_ftr_reg(SYS_MVFR0_EL1, info->reg_mvfr0);
+	update_cpu_ftr_reg(SYS_MVFR1_EL1, info->reg_mvfr1);
+	update_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2);
+
 	update_mixed_endian_el0_support(info);
 }
 
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 0dadb69..857aaf0 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -340,7 +340,6 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 
 	check_local_cpu_errata();
 	check_local_cpu_features();
-	update_cpu_features(info);
 }
 
 void cpuinfo_store_cpu(void)
@@ -348,6 +347,7 @@ void cpuinfo_store_cpu(void)
 	struct cpuinfo_arm64 *info = this_cpu_ptr(&cpu_data);
 	__cpuinfo_store_cpu(info);
 	cpuinfo_sanity_check(info);
+	update_cpu_features(info);
 }
 
 void __init cpuinfo_store_boot_cpu(void)
@@ -356,4 +356,5 @@ void __init cpuinfo_store_boot_cpu(void)
 	__cpuinfo_store_cpu(info);
 
 	boot_cpu_data = *info;
+	init_cpu_features(&boot_cpu_data);
 }
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1241696

On Mon, Oct 05, 2015 at 06:01:56PM +0100, Suzuki K. Poulose wrote:
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index b5f313d..7ed92f2 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -35,6 +35,38 @@
>  
>  #include <linux/kernel.h>
>  
> +/* CPU feature register tracking */
> +enum ftr_type {
> +	FTR_DISCRETE,	/* Use a predefined safe value */
> +	FTR_SCALAR_MIN,	/* Smaller value is safer */
> +	FTR_SCALAR_MAX,	/* Bigger value is safer */
> +};

I think s/safer/safe/ sounds better, otherwise it looks like we are not
entirely sure.

BTW, I don't fully understand the name choosing here (e.g. discrete vs.
scalar; a scalar type is also a discrete type). "MIN" looks to me like
any higher value should be safe but instead the comment states "smaller
value is safe". Maybe something like:

	FTR_EXACT
	FTR_LOWER_SAFE
	FTR_HIGHER_SAFE

> +
> +#define FTR_STRICT	true
> +#define FTR_NONSTRICT	false
> +
> +struct arm64_ftr_bits {
> +	bool		strict;		/* CPU Sanity check
> +					 *  strict matching required ? */

I don't care about 80 characters line, especially around comments. So
please place the comment on a single line.

> +	enum ftr_type	type;
> +	u8		shift;
> +	u8		width;
> +	s64		safe_val;	/* safe value for discrete features */
> +};
> +
> +/*
> + * @arm64_ftr_reg - Feature register
> + * @strict_mask 	Bits which should match across all CPUs for sanity.
> + * @sys_val		Safe value across the CPUs (system view)
> + */
> +struct arm64_ftr_reg {
> +	u32			sys_id;
> +	const char*		name;
> +	u64			strict_mask;
> +	u64			sys_val;
> +	struct arm64_ftr_bits*	ftr_bits;
> +};

Move '*' near the struct member names, not the type.

> +
>  struct arm64_cpu_capabilities {
>  	const char *desc;
>  	u16 capability;
> @@ -82,6 +114,22 @@ static inline int __attribute_const__ cpuid_feature_extract_field(u64 features,
>  	return (s64)(features << (64 - 4 - field)) >> (64 - 4);
>  }
>  
> +static inline s64 __attribute_const__
> +cpuid_feature_extract_field_width(u64 features, int field, u8 width)
> +{
> +	return (s64)(features << (64 - width - field)) >> (64 - width);
> +}

I think you should rewrite cpuid_feature_extract_field() in terms of the
_width one (the latter being more generic).

> +
> +static inline u64 ftr_mask(struct arm64_ftr_bits *ftrp)
> +{
> +	return (u64) GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift);
> +}

Nitpick: remove the space after (u64).

> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 1ae8b24..d42ad90 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -58,8 +58,442 @@ static void update_mixed_endian_el0_support(struct cpuinfo_arm64 *info)
>  	mixed_endian_el0 &= id_aa64mmfr0_mixed_endian_el0(info->reg_id_aa64mmfr0);
>  }
>  
> +#define ARM64_FTR_BITS(ftr_strict, ftr_type, ftr_shift, ftr_width, ftr_safe_val) \
> +	{							\
> +		.strict = ftr_strict,				\
> +		.type = ftr_type,				\
> +		.shift = ftr_shift,				\
> +		.width = ftr_width,				\
> +		.safe_val = ftr_safe_val,			\
> +	}

You can drop "ftr_" from all the arguments, it makes the macro
definition shorter.

[...]
> +static struct arm64_ftr_bits ftr_id_pfr0[] = {
> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 16, 16, 0),	// RAZ
> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 12, 4, 0),	// State3
> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 8, 4, 0),	// State2
> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 4, 0),	// State1
> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// State0
> +	ARM64_FTR_END,
> +};

Do we care about the RAZ/RAO fields? Or we use this later to check a new
CPU's compatibility with the overall features?

Also, you captured lots of fields that Linux does not care about. Is it
possible to ignore them altogether, only keep those which are relevant.

> +static struct arm64_ftr_reg crm_1[] = {
> +	ARM64_FTR_REG(SYS_ID_PFR0_EL1, ftr_id_pfr0),
> +	ARM64_FTR_REG(SYS_ID_PFR1_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_DFR0_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_MMFR0_EL1, ftr_id_mmfr0),
> +	ARM64_FTR_REG(SYS_ID_MMFR1_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_MMFR2_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_MMFR3_EL1, ftr_generic_scalar_32bit),
> +};
> +
> +static struct arm64_ftr_reg crm_2[] = {
> +	ARM64_FTR_REG(SYS_ID_ISAR0_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_ISAR1_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_ISAR2_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_ISAR3_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5),
> +	ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4),
> +};
> +
> +static struct arm64_ftr_reg crm_3[] = {
> +	ARM64_FTR_REG(SYS_MVFR0_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_MVFR1_EL1, ftr_generic_scalar_32bit),
> +	ARM64_FTR_REG(SYS_MVFR2_EL1, ftr_mvfr2),
> +};
> +
> +static struct arm64_ftr_reg crm_4[] = {
> +	ARM64_FTR_REG(SYS_ID_AA64PFR0_EL1, ftr_id_aa64pfr0),
> +	ARM64_FTR_REG(SYS_ID_AA64PFR1_EL1, ftr_aa64raz),
> +};
> +
> +static struct arm64_ftr_reg crm_5[] = {
> +	ARM64_FTR_REG(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0),
> +	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_generic),
> +};
> +
> +static struct arm64_ftr_reg crm_6[] = {
> +	ARM64_FTR_REG(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0),
> +	ARM64_FTR_REG(SYS_ID_AA64ISAR1_EL1, ftr_aa64raz),
> +};
> +
> +static struct arm64_ftr_reg crm_7[] = {
> +	ARM64_FTR_REG(SYS_ID_AA64MMFR0_EL1, ftr_id_aa64mmfr0),
> +	ARM64_FTR_REG(SYS_ID_AA64MMFR1_EL1, ftr_id_aa64mmfr1),
> +};
> +
> +static struct arm64_ftr_reg op1_3[] = {
> +	ARM64_FTR_REG(SYS_CTR_EL0, ftr_ctr),
> +	ARM64_FTR_REG(SYS_DCZID_EL0, ftr_dczid),
> +	ARM64_FTR_REG(SYS_CNTFRQ_EL0, ftr_generic32),
> +};
> +
> +#define ARM64_REG_TABLE(table) 		\
> +	{ .n = ARRAY_SIZE(table), .regs = table }
> +static struct arm64_reg_table {
> +	int n;
> +	struct arm64_ftr_reg *regs;
> +} op1_0[] = {
> +	ARM64_REG_TABLE(crm_1),
> +	ARM64_REG_TABLE(crm_2),
> +	ARM64_REG_TABLE(crm_3),
> +	ARM64_REG_TABLE(crm_4),
> +	ARM64_REG_TABLE(crm_5),
> +	ARM64_REG_TABLE(crm_6),
> +	ARM64_REG_TABLE(crm_7),
> +};
> +
> +/*
> + * get_arm6_sys_reg - Lookup a feature register entry using its

arm64_...

> + * sys_reg() encoding.
> + *
> + * We track only the following space:
> + * Op0 = 3, Op1 = 0, CRn = 0, CRm = [1 - 7], Op2 = [0 - 7]
> + * Op0 = 3, Op1 = 3, CRn = 0, CRm = 0, Op2 = { 1, 7 } 	(CTR, DCZID)
> + * Op0 = 3, Op1 = 3, CRn = 14, CRm = 0, Op2 = 0		(CNTFRQ)
> + *
> + * The space (3, 0, 0, {1-7}, {0-7}) is arranged in a 2D array op1_0,
> + * indexed by CRm and Op2. Since not all CRm's have fully allocated Op2's
> + * arm64_reg_table[CRm-1].n indicates the largest Op2 tracked for CRm.
> + *
> + * Since we have limited number of entries with Op1 = 3, we use linear search
> + * to find the reg.
> + *
> + */
> +static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)
> +{
> +	int i;
> +	u8 op2, crn, crm;
> +	u8 op1 = sys_reg_Op1(sys_id);
> +
> +	if (sys_reg_Op0(sys_id) != 3)
> +		return NULL;
> +	switch (op1) {
> +	case 0:
> +
> +		crm = sys_reg_CRm(sys_id);
> +		op2 = sys_reg_Op2(sys_id);
> +		crn = sys_reg_CRn(sys_id);
> +		if (crn || !crm || crm > 7)
> +			return NULL;
> +		if (op2 < op1_0[crm - 1].n &&
> +			op1_0[crm - 1].regs[op2].sys_id == sys_id)
> +			return &op1_0[crm - 1].regs[op2];
> +		return NULL;
> +	case 3:
> +		for (i = 0; i < ARRAY_SIZE(op1_3); i++)
> +			if (op1_3[i].sys_id == sys_id)
> +				return &op1_3[i];
> +	}
> +	return NULL;
> +}

For this function, do we ever expect to be called with an invalid
sys_id? You could add a BUG_ON(!ret) here.

Is this function ever called on a hot path? If not, just keep everything
in an array and do a linear search rather than having different arrays
based on op*. Especially if we managed to limit the number of registers
to only those that Linux cares about.

As a general coding style comment, I would much prefer to have a "ret"
local variable and one or two points of return from a function rather
than multiple returns. I've seen this as a general trend in your
patches, so please fix.

> +
> +static u64 arm64_ftr_set_value(struct arm64_ftr_bits *ftrp, s64 reg, s64 ftr_val)
> +{
> +	u64 mask = ftr_mask(ftrp);
> +
> +	reg &= ~mask;
> +	reg |= (ftr_val << ftrp->shift) & mask;
> +	return reg;
> +}
> +
> +static s64 arm64_ftr_safe_value(struct arm64_ftr_bits *ftrp, s64 new, s64 cur)
> +{
> +	switch(ftrp->type) {
> +	case FTR_DISCRETE:
> +		return ftrp->safe_val;
> +	case FTR_SCALAR_MIN:
> +		return new < cur ? new : cur;
> +	case FTR_SCALAR_MAX:
> +		return new > cur ? new : cur;
> +	}

Same here about the returns.

> +static void update_cpu_ftr_reg(u32 sys_reg, u64 new)
> +{
> +	struct arm64_ftr_bits *ftrp;
> +	struct arm64_ftr_reg *reg = get_arm64_sys_reg(sys_reg);
> +
> +	BUG_ON(!reg);
> +
> +	for(ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
> +		s64 ftr_cur = arm64_ftr_value(ftrp, reg->sys_val);
> +		s64 ftr_new = arm64_ftr_value(ftrp, new);
> +
> +		if (ftr_cur == ftr_new)
> +			continue;
> +		/* Find a safe value */
> +		ftr_new = arm64_ftr_safe_value(ftrp, ftr_new, ftr_cur);
> +		reg->sys_val = arm64_ftr_set_value(ftrp, reg->sys_val, ftr_new);
> +	}
> +
> +}
> +
> +/* Update CPU feature register from non-boot CPU */

s/register from/registers for/

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242136

On 07/10/15 18:16, Catalin Marinas wrote:
> On Mon, Oct 05, 2015 at 06:01:56PM +0100, Suzuki K. Poulose wrote:
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index b5f313d..7ed92f2 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -35,6 +35,38 @@
>>
>>   #include <linux/kernel.h>
>>
>> +/* CPU feature register tracking */
>> +enum ftr_type {
>> +	FTR_DISCRETE,	/* Use a predefined safe value */
>> +	FTR_SCALAR_MIN,	/* Smaller value is safer */
>> +	FTR_SCALAR_MAX,	/* Bigger value is safer */
>> +};
>
> I think s/safer/safe/ sounds better, otherwise it looks like we are not
> entirely sure.

>
> BTW, I don't fully understand the name choosing here (e.g. discrete vs.
> scalar; a scalar type is also a discrete type). "MIN" looks to me like
> any higher value should be safe but instead the comment states "smaller
> value is safe". Maybe something like:
>
> 	FTR_EXACT
> 	FTR_LOWER_SAFE
> 	FTR_HIGHER_SAFE

OK, I will change them.


>
>> +
>> +#define FTR_STRICT	true
>> +#define FTR_NONSTRICT	false
>> +
>> +struct arm64_ftr_bits {
>> +	bool		strict;		/* CPU Sanity check
>> +					 *  strict matching required ? */
>
> I don't care about 80 characters line, especially around comments. So
> please place the comment on a single line.
>

OK

>> +struct arm64_ftr_reg {
>> +	u32			sys_id;
>> +	const char*		name;
>> +	u64			strict_mask;
>> +	u64			sys_val;
>> +	struct arm64_ftr_bits*	ftr_bits;
>> +};
>
> Move '*' near the struct member names, not the type.
>

Ok

>> +
>>   struct arm64_cpu_capabilities {
>>   	const char *desc;
>>   	u16 capability;
>> @@ -82,6 +114,22 @@ static inline int __attribute_const__ cpuid_feature_extract_field(u64 features,
>>   	return (s64)(features << (64 - 4 - field)) >> (64 - 4);
>>   }
>>
>> +static inline s64 __attribute_const__
>> +cpuid_feature_extract_field_width(u64 features, int field, u8 width)
>> +{
>> +	return (s64)(features << (64 - width - field)) >> (64 - width);
>> +}
>
> I think you should rewrite cpuid_feature_extract_field() in terms of the
> _width one (the latter being more generic).
>

OK, somehow, I was thinking that cpuid_feature_extract_field() could be
optimised by the compiler for a fixed width of for. Hence didn't change it.


>> +
>> +static inline u64 ftr_mask(struct arm64_ftr_bits *ftrp)
>> +{
>> +	return (u64) GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift);
>> +}
>
> Nitpick: remove the space after (u64).

OK

>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 1ae8b24..d42ad90 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -58,8 +58,442 @@ static void update_mixed_endian_el0_support(struct cpuinfo_arm64 *info)
>>   	mixed_endian_el0 &= id_aa64mmfr0_mixed_endian_el0(info->reg_id_aa64mmfr0);
>>   }
>>
>> +#define ARM64_FTR_BITS(ftr_strict, ftr_type, ftr_shift, ftr_width, ftr_safe_val) \
>> +	{							\
>> +		.strict = ftr_strict,				\
>> +		.type = ftr_type,				\
>> +		.shift = ftr_shift,				\
>> +		.width = ftr_width,				\
>> +		.safe_val = ftr_safe_val,			\
>> +	}
>
> You can drop "ftr_" from all the arguments, it makes the macro
> definition shorter.

In fact I tried that before, but then the macro expansion will replace the
field names with the supplied values and hence won't compile. Either we
should change the field names or the values.

>
> [...]
>> +static struct arm64_ftr_bits ftr_id_pfr0[] = {
>> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 16, 16, 0),	// RAZ
>> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 12, 4, 0),	// State3
>> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 8, 4, 0),	// State2
>> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 4, 0),	// State1
>> +	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// State0
>> +	ARM64_FTR_END,
>> +};
>
> Do we care about the RAZ/RAO fields? Or we use this later to check a new
> CPU's compatibility with the overall features?

Its just for sanity checks.

> Also, you captured lots of fields that Linux does not care about. Is it
> possible to ignore them altogether, only keep those which are relevant.
>

The list is entierly from the SANITY check. If there are any registers
that we think need not be cross checked, we could get rid of them.

  
>> +
>> +/*
>> + * get_arm6_sys_reg - Lookup a feature register entry using its
>
> arm64_...

yikes, thanks for spotting.

>
>> + * sys_reg() encoding.
>> + *
>> + * We track only the following space:
>> + * Op0 = 3, Op1 = 0, CRn = 0, CRm = [1 - 7], Op2 = [0 - 7]
>> + * Op0 = 3, Op1 = 3, CRn = 0, CRm = 0, Op2 = { 1, 7 } 	(CTR, DCZID)
>> + * Op0 = 3, Op1 = 3, CRn = 14, CRm = 0, Op2 = 0		(CNTFRQ)
>> + *
>> + * The space (3, 0, 0, {1-7}, {0-7}) is arranged in a 2D array op1_0,
>> + * indexed by CRm and Op2. Since not all CRm's have fully allocated Op2's
>> + * arm64_reg_table[CRm-1].n indicates the largest Op2 tracked for CRm.
>> + *
>> + * Since we have limited number of entries with Op1 = 3, we use linear search
>> + * to find the reg.
>> + *
>> + */
>> +static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)
>> +{
>> +	int i;
>> +	u8 op2, crn, crm;
>> +	u8 op1 = sys_reg_Op1(sys_id);
>> +
>> +	if (sys_reg_Op0(sys_id) != 3)
>> +		return NULL;
>> +	switch (op1) {
>> +	case 0:
>> +
>> +		crm = sys_reg_CRm(sys_id);
>> +		op2 = sys_reg_Op2(sys_id);
>> +		crn = sys_reg_CRn(sys_id);
>> +		if (crn || !crm || crm > 7)
>> +			return NULL;
>> +		if (op2 < op1_0[crm - 1].n &&
>> +			op1_0[crm - 1].regs[op2].sys_id == sys_id)
>> +			return &op1_0[crm - 1].regs[op2];
>> +		return NULL;
>> +	case 3:
>> +		for (i = 0; i < ARRAY_SIZE(op1_3); i++)
>> +			if (op1_3[i].sys_id == sys_id)
>> +				return &op1_3[i];
>> +	}
>> +	return NULL;
>> +}
>
> For this function, do we ever expect to be called with an invalid
> sys_id? You could add a BUG_ON(!ret) here.
>

It could be called for an id which Reserved RAZ in the id range, we
plan to emulate. i.e, (3, 0, 0, [0-7], [0-7]).
See emulate_sys_reg(u32 id, u64 *valp) in Patch 20/22.
Since we don't track them, we return NULL here..
We could BUG_ON() all the other cases (e.g, MIDR and the other
classes).

Thanks for pointing that out.

> Is this function ever called on a hot path? If not, just keep everything
> in an array and do a linear search rather than having different arrays
> based on op*. Especially if we managed to limit the number of registers
> to only those that Linux cares about.

I started with linear array in the RFC post. But since then the number of
users for the API has gone up. Hence thought of optimising it. The only
'intensive' user is SANITY check for each register at CPU bring up.

>
> As a general coding style comment, I would much prefer to have a "ret"
> local variable and one or two points of return from a function rather
> than multiple returns. I've seen this as a general trend in your
> patches, so please fix.
>

...

>> +
>> +static s64 arm64_ftr_safe_value(struct arm64_ftr_bits *ftrp, s64 new, s64 cur)
>> +{
>> +	switch(ftrp->type) {
>> +	case FTR_DISCRETE:
>> +		return ftrp->safe_val;
>> +	case FTR_SCALAR_MIN:
>> +		return new < cur ? new : cur;
>> +	case FTR_SCALAR_MAX:
>> +		return new > cur ? new : cur;
>> +	}
>
> Same here about the returns.

Will fix it everywhere. Thanks for pointing out.

>
>> +static void update_cpu_ftr_reg(u32 sys_reg, u64 new)
>> +{
>> +	struct arm64_ftr_bits *ftrp;
>> +	struct arm64_ftr_reg *reg = get_arm64_sys_reg(sys_reg);
>> +
>> +	BUG_ON(!reg);
>> +
>> +	for(ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>> +		s64 ftr_cur = arm64_ftr_value(ftrp, reg->sys_val);
>> +		s64 ftr_new = arm64_ftr_value(ftrp, new);
>> +
>> +		if (ftr_cur == ftr_new)
>> +			continue;
>> +		/* Find a safe value */
>> +		ftr_new = arm64_ftr_safe_value(ftrp, ftr_new, ftr_cur);
>> +		reg->sys_val = arm64_ftr_set_value(ftrp, reg->sys_val, ftr_new);
>> +	}
>> +
>> +}
>> +
>> +/* Update CPU feature register from non-boot CPU */
>
> s/register from/registers for/

OK

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242500

On Thu, Oct 08, 2015 at 10:55:11AM +0100, Suzuki K. Poulose wrote:
> >>@@ -82,6 +114,22 @@ static inline int __attribute_const__ cpuid_feature_extract_field(u64 features,
> >>  	return (s64)(features << (64 - 4 - field)) >> (64 - 4);
> >>  }
> >>
> >>+static inline s64 __attribute_const__
> >>+cpuid_feature_extract_field_width(u64 features, int field, u8 width)
> >>+{
> >>+	return (s64)(features << (64 - width - field)) >> (64 - width);
> >>+}
> >
> >I think you should rewrite cpuid_feature_extract_field() in terms of the
> >_width one (the latter being more generic).
> >
> 
> OK, somehow, I was thinking that cpuid_feature_extract_field() could be
> optimised by the compiler for a fixed width of for. Hence didn't change it.

Since both are static inline, the compiler should be smart enough to
optimise it already.

> >>diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>index 1ae8b24..d42ad90 100644
> >>--- a/arch/arm64/kernel/cpufeature.c
> >>+++ b/arch/arm64/kernel/cpufeature.c
> >>@@ -58,8 +58,442 @@ static void update_mixed_endian_el0_support(struct cpuinfo_arm64 *info)
> >>  	mixed_endian_el0 &= id_aa64mmfr0_mixed_endian_el0(info->reg_id_aa64mmfr0);
> >>  }
> >>
> >>+#define ARM64_FTR_BITS(ftr_strict, ftr_type, ftr_shift, ftr_width, ftr_safe_val) \
> >>+	{							\
> >>+		.strict = ftr_strict,				\
> >>+		.type = ftr_type,				\
> >>+		.shift = ftr_shift,				\
> >>+		.width = ftr_width,				\
> >>+		.safe_val = ftr_safe_val,			\
> >>+	}
> >
> >You can drop "ftr_" from all the arguments, it makes the macro
> >definition shorter.
> 
> In fact I tried that before, but then the macro expansion will replace the
> field names with the supplied values and hence won't compile. Either we
> should change the field names or the values.

OK, keep them in this case.

> >[...]
> >>+static struct arm64_ftr_bits ftr_id_pfr0[] = {
> >>+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 16, 16, 0),	// RAZ
> >>+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 12, 4, 0),	// State3
> >>+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 8, 4, 0),	// State2
> >>+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 4, 4, 0),	// State1
> >>+	ARM64_FTR_BITS(FTR_STRICT, FTR_DISCRETE, 0, 4, 0),	// State0
> >>+	ARM64_FTR_END,
> >>+};
> >
> >Do we care about the RAZ/RAO fields? Or we use this later to check a new
> >CPU's compatibility with the overall features?
> 
> Its just for sanity checks.
> 
> >Also, you captured lots of fields that Linux does not care about. Is it
> >possible to ignore them altogether, only keep those which are relevant.
> >
> 
> The list is entierly from the SANITY check. If there are any registers
> that we think need not be cross checked, we could get rid of them.

So we have three types of fields in these registers:

a) features defined but not something we care about in Linux
b) reserved fields
c) features important to Linux

I guess for (a), Linux may not even care if they don't match (though we
need to be careful which fields we ignore). As for (b), even if they
differ, since we don't know the meaning at this point, I think we should
just ignore them. If, for example, they add a feature that Linux doesn't
care about, they practically fall under the (a) category.

Regarding exposing reserved CPUID fields to user, I assume we would
always return 0.

> >>+ * sys_reg() encoding.
> >>+ *
> >>+ * We track only the following space:
> >>+ * Op0 = 3, Op1 = 0, CRn = 0, CRm = [1 - 7], Op2 = [0 - 7]
> >>+ * Op0 = 3, Op1 = 3, CRn = 0, CRm = 0, Op2 = { 1, 7 } 	(CTR, DCZID)
> >>+ * Op0 = 3, Op1 = 3, CRn = 14, CRm = 0, Op2 = 0		(CNTFRQ)
> >>+ *
> >>+ * The space (3, 0, 0, {1-7}, {0-7}) is arranged in a 2D array op1_0,
> >>+ * indexed by CRm and Op2. Since not all CRm's have fully allocated Op2's
> >>+ * arm64_reg_table[CRm-1].n indicates the largest Op2 tracked for CRm.
> >>+ *
> >>+ * Since we have limited number of entries with Op1 = 3, we use linear search
> >>+ * to find the reg.
> >>+ *
> >>+ */
> >>+static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)
> >>+{
> >>+	int i;
> >>+	u8 op2, crn, crm;
> >>+	u8 op1 = sys_reg_Op1(sys_id);
> >>+
> >>+	if (sys_reg_Op0(sys_id) != 3)
> >>+		return NULL;
> >>+	switch (op1) {
> >>+	case 0:
> >>+
> >>+		crm = sys_reg_CRm(sys_id);
> >>+		op2 = sys_reg_Op2(sys_id);
> >>+		crn = sys_reg_CRn(sys_id);
> >>+		if (crn || !crm || crm > 7)
> >>+			return NULL;
> >>+		if (op2 < op1_0[crm - 1].n &&
> >>+			op1_0[crm - 1].regs[op2].sys_id == sys_id)
> >>+			return &op1_0[crm - 1].regs[op2];
> >>+		return NULL;
> >>+	case 3:
> >>+		for (i = 0; i < ARRAY_SIZE(op1_3); i++)
> >>+			if (op1_3[i].sys_id == sys_id)
> >>+				return &op1_3[i];
> >>+	}
> >>+	return NULL;
> >>+}
[...]
> >Is this function ever called on a hot path? If not, just keep everything
> >in an array and do a linear search rather than having different arrays
> >based on op*. Especially if we managed to limit the number of registers
> >to only those that Linux cares about.
> 
> I started with linear array in the RFC post. But since then the number of
> users for the API has gone up. Hence thought of optimising it. The only
> 'intensive' user is SANITY check for each register at CPU bring up.

This shouldn't be that bad since it's not happening very often. However,
do we need this thing for MRS emulation (not many registers though)? You
could use a binary search (something like radix tree seems overkill)

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243333

On 08/10/15 16:03, Catalin Marinas wrote:
> On Thu, Oct 08, 2015 at 10:55:11AM +0100, Suzuki K. Poulose wrote:
>>>> +#define ARM64_FTR_BITS(ftr_strict, ftr_type, ftr_shift, ftr_width, ftr_safe_val) \

>>>
>>> You can drop "ftr_" from all the arguments, it makes the macro
>>> definition shorter.
>>
>> In fact I tried that before, but then the macro expansion will replace the
>> field names with the supplied values and hence won't compile. Either we
>> should change the field names or the values.
>
> OK, keep them in this case.

I have changed it to :

ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)

>>
>>> Also, you captured lots of fields that Linux does not care about. Is it
>>> possible to ignore them altogether, only keep those which are relevant.
>>>
>>
>> The list is entierly from the SANITY check. If there are any registers
>> that we think need not be cross checked, we could get rid of them.
>
> So we have three types of fields in these registers:
>
> a) features defined but not something we care about in Linux
> b) reserved fields
> c) features important to Linux
>
> I guess for (a), Linux may not even care if they don't match (though we
> need to be careful which fields we ignore). As for (b), even if they
> differ, since we don't know the meaning at this point, I think we should
> just ignore them. If, for example, they add a feature that Linux doesn't
> care about, they practically fall under the (a) category.

OK. So we can pack the consecutive features of type (a) and make it NONSTRICT.

>
> Regarding exposing reserved CPUID fields to user, I assume we would
> always return 0.

Ideally, the architecturally reserved value (i.e, 0 for RAZ and 1 for RES1).

>>> Is this function ever called on a hot path? If not, just keep everything
>>> in an array and do a linear search rather than having different arrays
>>> based on op*. Especially if we managed to limit the number of registers
>>> to only those that Linux cares about.
>>
>> I started with linear array in the RFC post. But since then the number of
>> users for the API has gone up. Hence thought of optimising it. The only
>> 'intensive' user is SANITY check for each register at CPU bring up.
>
> This shouldn't be that bad since it's not happening very often. However,
> do we need this thing for MRS emulation (not many registers though)? You
> could use a binary search (something like radix tree seems overkill)

Yes we do need this for MRS emulation. I will change it to binary search.

Thanks
Suzuki

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244929

On 08/10/15 16:03, Catalin Marinas wrote:
> On Thu, Oct 08, 2015 at 10:55:11AM +0100, Suzuki K. Poulose wrote:

...
>
> So we have three types of fields in these registers:
>
> a) features defined but not something we care about in Linux
> b) reserved fields
> c) features important to Linux
>
> I guess for (a), Linux may not even care if they don't match (though we
> need to be careful which fields we ignore). As for (b), even if they
> differ, since we don't know the meaning at this point, I think we should
> just ignore them. If, for example, they add a feature that Linux doesn't
> care about, they practically fall under the (a) category.
>
> Regarding exposing reserved CPUID fields to user, I assume we would
> always return 0.

Mark,

Do you have any comments on this ? The list I have here is what you came
up with in SANITY checks.

Thanks
Suzuki

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244945

Hi,

Thanks for the heads-up.

On Mon, Oct 12, 2015 at 06:01:25PM +0100, Suzuki K. Poulose wrote:
> On 08/10/15 16:03, Catalin Marinas wrote:
> >On Thu, Oct 08, 2015 at 10:55:11AM +0100, Suzuki K. Poulose wrote:
> 
> ...
> >
> >So we have three types of fields in these registers:
> >
> >a) features defined but not something we care about in Linux
> >b) reserved fields
> >c) features important to Linux
> >
> >I guess for (a), Linux may not even care if they don't match (though we
> >need to be careful which fields we ignore). As for (b), even if they
> >differ, since we don't know the meaning at this point, I think we should
> >just ignore them. If, for example, they add a feature that Linux doesn't
> >care about, they practically fall under the (a) category.
> >
> >Regarding exposing reserved CPUID fields to user, I assume we would
> >always return 0.
> 
> Mark,
> 
> Do you have any comments on this ? The list I have here is what you came
> up with in SANITY checks.

My feeling was that we should play it safe with fields which are
currently reserved (warning if they differ for now).

If they turn out to be irrelevant, it's simple to backport a patch to
ignore them, whereas if they matter we get instant visibility, which is
the entire point of the sanity checks.

So I think we should warn if reserved fields differ. I'd rather have a
few spurious warnings until kernels get updated than miss an issue that
could have been dealt with and avoided.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245517

On Mon, Oct 12, 2015 at 06:21:04PM +0100, Mark Rutland wrote:
> On Mon, Oct 12, 2015 at 06:01:25PM +0100, Suzuki K. Poulose wrote:
> > On 08/10/15 16:03, Catalin Marinas wrote:
> > >On Thu, Oct 08, 2015 at 10:55:11AM +0100, Suzuki K. Poulose wrote:
> > 
> > ...
> > >
> > >So we have three types of fields in these registers:
> > >
> > >a) features defined but not something we care about in Linux
> > >b) reserved fields
> > >c) features important to Linux
> > >
> > >I guess for (a), Linux may not even care if they don't match (though we
> > >need to be careful which fields we ignore). As for (b), even if they
> > >differ, since we don't know the meaning at this point, I think we should
> > >just ignore them. If, for example, they add a feature that Linux doesn't
> > >care about, they practically fall under the (a) category.
> > >
> > >Regarding exposing reserved CPUID fields to user, I assume we would
> > >always return 0.
> > 
> > Mark,
> > 
> > Do you have any comments on this ? The list I have here is what you came
> > up with in SANITY checks.
> 
> My feeling was that we should play it safe with fields which are
> currently reserved (warning if they differ for now).
> 
> If they turn out to be irrelevant, it's simple to backport a patch to
> ignore them, whereas if they matter we get instant visibility, which is
> the entire point of the sanity checks.
> 
> So I think we should warn if reserved fields differ. I'd rather have a
> few spurious warnings until kernels get updated than miss an issue that
> could have been dealt with and avoided.

The warnings are indeed harmless. I think the main danger is when a
field goes negative which means an existing feature without CPUID field
allocated is removed (though I don't expect this for ARMv8). But you are
right, let the warnings in for now, let's re-assess when/if a difference
happens.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243239

On 08/10/15 10:55, Suzuki K. Poulose wrote:
> On 07/10/15 18:16, Catalin Marinas wrote:
>> On Mon, Oct 05, 2015 at 06:01:56PM +0100, Suzuki K. Poulose wrote:
>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>> index 1ae8b24..d42ad90 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -58,8 +58,442 @@ static void update_mixed_endian_el0_support(struct cpuinfo_arm64 *info)
>>>       mixed_endian_el0 &= id_aa64mmfr0_mixed_endian_el0(info->reg_id_aa64mmfr0);

>>> + * sys_reg() encoding.
>>> + *
>>> + * We track only the following space:
>>> + * Op0 = 3, Op1 = 0, CRn = 0, CRm = [1 - 7], Op2 = [0 - 7]
>>> + * Op0 = 3, Op1 = 3, CRn = 0, CRm = 0, Op2 = { 1, 7 }     (CTR, DCZID)
>>> + * Op0 = 3, Op1 = 3, CRn = 14, CRm = 0, Op2 = 0        (CNTFRQ)
>>> + *
>>> + * The space (3, 0, 0, {1-7}, {0-7}) is arranged in a 2D array op1_0,
>>> + * indexed by CRm and Op2. Since not all CRm's have fully allocated Op2's
>>> + * arm64_reg_table[CRm-1].n indicates the largest Op2 tracked for CRm.
>>> + *
>>> + * Since we have limited number of entries with Op1 = 3, we use linear search
>>> + * to find the reg.
>>> + *
>>> + */
>>> +static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)
>>> +{
>>> +    int i;
>>> +    u8 op2, crn, crm;
>>> +    u8 op1 = sys_reg_Op1(sys_id);
>>> +
>>> +    if (sys_reg_Op0(sys_id) != 3)
>>> +        return NULL;
>>> +    switch (op1) {
>>> +    case 0:
>>> +
>>> +        crm = sys_reg_CRm(sys_id);
>>> +        op2 = sys_reg_Op2(sys_id);
>>> +        crn = sys_reg_CRn(sys_id);
>>> +        if (crn || !crm || crm > 7)
>>> +            return NULL;
>>> +        if (op2 < op1_0[crm - 1].n &&
>>> +            op1_0[crm - 1].regs[op2].sys_id == sys_id)
>>> +            return &op1_0[crm - 1].regs[op2];
>>> +        return NULL;
>>> +    case 3:
>>> +        for (i = 0; i < ARRAY_SIZE(op1_3); i++)
>>> +            if (op1_3[i].sys_id == sys_id)
>>> +                return &op1_3[i];
>>> +    }
>>> +    return NULL;
>>> +}
>>
>> For this function, do we ever expect to be called with an invalid
>> sys_id? You could add a BUG_ON(!ret) here.
>>
>
> It could be called for an id which Reserved RAZ in the id range, we
> plan to emulate. i.e, (3, 0, 0, [0-7], [0-7]).
> See emulate_sys_reg(u32 id, u64 *valp) in Patch 20/22.
> Since we don't track them, we return NULL here..
> We could BUG_ON() all the other cases (e.g, MIDR and the other
> classes).
>
> Thanks for pointing that out.

Actually, the error handling is left to the users of the function.
We do a BUG_ON() in the caller. e.g, init/update_cpu_ftr_reg can't
accept a NULL and BUGs. While the emulate_sys_reg() issues the call
only for the emualted feature registers(excluding MIDR/REVIDR etc),
so a NULL is perfectly acceptable for them.

Thanks
Suzuki


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243403

On Fri, Oct 09, 2015 at 11:56:14AM +0100, Suzuki K. Poulose wrote:
> On 08/10/15 10:55, Suzuki K. Poulose wrote:
> >On 07/10/15 18:16, Catalin Marinas wrote:
> >>On Mon, Oct 05, 2015 at 06:01:56PM +0100, Suzuki K. Poulose wrote:
> >>>diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>>index 1ae8b24..d42ad90 100644
> >>>--- a/arch/arm64/kernel/cpufeature.c
> >>>+++ b/arch/arm64/kernel/cpufeature.c
> >>>@@ -58,8 +58,442 @@ static void update_mixed_endian_el0_support(struct cpuinfo_arm64 *info)
> >>>      mixed_endian_el0 &= id_aa64mmfr0_mixed_endian_el0(info->reg_id_aa64mmfr0);
> 
> >>>+ * sys_reg() encoding.
> >>>+ *
> >>>+ * We track only the following space:
> >>>+ * Op0 = 3, Op1 = 0, CRn = 0, CRm = [1 - 7], Op2 = [0 - 7]
> >>>+ * Op0 = 3, Op1 = 3, CRn = 0, CRm = 0, Op2 = { 1, 7 }     (CTR, DCZID)
> >>>+ * Op0 = 3, Op1 = 3, CRn = 14, CRm = 0, Op2 = 0        (CNTFRQ)
> >>>+ *
> >>>+ * The space (3, 0, 0, {1-7}, {0-7}) is arranged in a 2D array op1_0,
> >>>+ * indexed by CRm and Op2. Since not all CRm's have fully allocated Op2's
> >>>+ * arm64_reg_table[CRm-1].n indicates the largest Op2 tracked for CRm.
> >>>+ *
> >>>+ * Since we have limited number of entries with Op1 = 3, we use linear search
> >>>+ * to find the reg.
> >>>+ *
> >>>+ */
> >>>+static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)
> >>>+{
> >>>+    int i;
> >>>+    u8 op2, crn, crm;
> >>>+    u8 op1 = sys_reg_Op1(sys_id);
> >>>+
> >>>+    if (sys_reg_Op0(sys_id) != 3)
> >>>+        return NULL;
> >>>+    switch (op1) {
> >>>+    case 0:
> >>>+
> >>>+        crm = sys_reg_CRm(sys_id);
> >>>+        op2 = sys_reg_Op2(sys_id);
> >>>+        crn = sys_reg_CRn(sys_id);
> >>>+        if (crn || !crm || crm > 7)
> >>>+            return NULL;
> >>>+        if (op2 < op1_0[crm - 1].n &&
> >>>+            op1_0[crm - 1].regs[op2].sys_id == sys_id)
> >>>+            return &op1_0[crm - 1].regs[op2];
> >>>+        return NULL;
> >>>+    case 3:
> >>>+        for (i = 0; i < ARRAY_SIZE(op1_3); i++)
> >>>+            if (op1_3[i].sys_id == sys_id)
> >>>+                return &op1_3[i];
> >>>+    }
> >>>+    return NULL;
> >>>+}
> >>
> >>For this function, do we ever expect to be called with an invalid
> >>sys_id? You could add a BUG_ON(!ret) here.
> >>
> >
> >It could be called for an id which Reserved RAZ in the id range, we
> >plan to emulate. i.e, (3, 0, 0, [0-7], [0-7]).
> >See emulate_sys_reg(u32 id, u64 *valp) in Patch 20/22.
> >Since we don't track them, we return NULL here..
> >We could BUG_ON() all the other cases (e.g, MIDR and the other
> >classes).
> >
> >Thanks for pointing that out.
> 
> Actually, the error handling is left to the users of the function.
> We do a BUG_ON() in the caller. e.g, init/update_cpu_ftr_reg can't
> accept a NULL and BUGs. While the emulate_sys_reg() issues the call
> only for the emualted feature registers(excluding MIDR/REVIDR etc),
> so a NULL is perfectly acceptable for them.

OK, please leave the NULL checking in the caller then.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web