[PATCH 0/5] arm64: cpufeature: Fixes for 4.4-rc1

[PATCH 0/5] arm64: cpufeature: Fixes for 4.4-rc1

[Suzuki K. Poulose]

This series contains fixes for two issues.

 Patches 1 - 3:	Handling of unsigned feature values, issu reported
		by AKASHI Takahiro.

 Patches 4 - 5:	Adds a check to make sure all the secondary CPUs have
		compatible ASIDBits to prevent system crashes.

Suzuki K. Poulose (5):
  arm64: cpufeature: Add helpers for extracting unsigned values
  arm64: cpufeature: Track unsigned fields
  arm64: debug: Treat the BRPs/WRPs as unsigned
  arm64: Make fail_incapable_cpu reusable
  arm64: Ensure the secondary CPUs have safe ASIDBits size

 arch/arm64/include/asm/cpufeature.h    |   22 +++++-
 arch/arm64/include/asm/hw_breakpoint.h |    6 +-
 arch/arm64/kernel/cpufeature.c         |  132 ++++++++++++++++++++++----------
 3 files changed, 116 insertions(+), 44 deletions(-)

-- 
1.7.9.5

[PATCH 0/5] arm64: cpufeature: Fixes for 4.4-rc1

[Suzuki K. Poulose]

This series contains fixes for two issues.

 Patches 1 - 3:	Handling of unsigned feature values, issu reported
		by AKASHI Takahiro.

 Patches 4 - 5:	Adds a check to make sure all the secondary CPUs have
		compatible ASIDBits to prevent system crashes.

Suzuki K. Poulose (5):
  arm64: cpufeature: Add helpers for extracting unsigned values
  arm64: cpufeature: Track unsigned fields
  arm64: debug: Treat the BRPs/WRPs as unsigned
  arm64: Make fail_incapable_cpu reusable
  arm64: Ensure the secondary CPUs have safe ASIDBits size

 arch/arm64/include/asm/cpufeature.h    |   22 +++++-
 arch/arm64/include/asm/hw_breakpoint.h |    6 +-
 arch/arm64/kernel/cpufeature.c         |  132 ++++++++++++++++++++++----------
 3 files changed, 116 insertions(+), 44 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] arm64: cpufeature: Add helpers for extracting unsigned values

[Suzuki K. Poulose]

The cpuid_feature_extract_field() extracts the feature value
as a signed integer. This could be problematic for features
whose values are unsigned. e.g, ID_AA64DFR0_EL1:BRPs. Add
an unsigned variant for the unsigned fields.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 11d5bb0f..7a16102 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -123,6 +123,18 @@ cpuid_feature_extract_field(u64 features, int field)
 	return cpuid_feature_extract_field_width(features, field, 4);
 }
 
+static inline unsigned int __attribute_const__
+cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width)
+{
+	return (u64)(features << (64 - width - field)) >> (64 - width);
+}
+
+static inline unsigned int __attribute_const__
+cpuid_feature_extract_unsigned_field(u64 features, int field)
+{
+	return cpuid_feature_extract_unsigned_field_width(features, field, 4);
+}
+
 static inline u64 arm64_ftr_mask(struct arm64_ftr_bits *ftrp)
 {
 	return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift);
-- 
1.7.9.5

[PATCH 1/5] arm64: cpufeature: Add helpers for extracting unsigned values

[Suzuki K. Poulose]

The cpuid_feature_extract_field() extracts the feature value
as a signed integer. This could be problematic for features
whose values are unsigned. e.g, ID_AA64DFR0_EL1:BRPs. Add
an unsigned variant for the unsigned fields.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 11d5bb0f..7a16102 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -123,6 +123,18 @@ cpuid_feature_extract_field(u64 features, int field)
 	return cpuid_feature_extract_field_width(features, field, 4);
 }
 
+static inline unsigned int __attribute_const__
+cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width)
+{
+	return (u64)(features << (64 - width - field)) >> (64 - width);
+}
+
+static inline unsigned int __attribute_const__
+cpuid_feature_extract_unsigned_field(u64 features, int field)
+{
+	return cpuid_feature_extract_unsigned_field_width(features, field, 4);
+}
+
 static inline u64 arm64_ftr_mask(struct arm64_ftr_bits *ftrp)
 {
 	return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift);
-- 
1.7.9.5

[PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

Some of the feature bits have unsigned values and need
to be treated accordingly to avoid errors. Adds the property
to the feature bits and use the appropriate field extract helpers.

Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |   10 ++++++++--
 arch/arm64/kernel/cpufeature.c      |   37 ++++++++++++++++++++++-------------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 7a16102..29c3f5d 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -46,8 +46,12 @@ enum ftr_type {
 #define FTR_STRICT	true	/* SANITY check strict matching required */
 #define FTR_NONSTRICT	false	/* SANITY check ignored */
 
+#define FTR_SIGNED	true	/* Value should be treated as signed */
+#define FTR_UNSIGNED	false	/* Value should be treated as unsigned */
+
 struct arm64_ftr_bits {
-	bool		strict;	  /* CPU Sanity check: strict matching required ? */
+	bool		sign;	/* Value is signed ? */
+	bool		strict;	/* CPU Sanity check: strict matching required ? */
 	enum ftr_type	type;
 	u8		shift;
 	u8		width;
@@ -142,7 +146,9 @@ static inline u64 arm64_ftr_mask(struct arm64_ftr_bits *ftrp)
 
 static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
 {
-	return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width);
+	return ftrp->sign ?
+		cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width) :
+		cpuid_feature_extract_unsigned_field_width(val, ftrp->shift, ftrp->width);
 }
 
 static inline bool id_aa64mmfr0_mixed_endian_el0(u64 mmfr0)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index c8cf892..0669c63 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -44,8 +44,9 @@ unsigned int compat_elf_hwcap2 __read_mostly;
 
 DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 
-#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
+#define __ARM64_FTR_BITS(SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
 	{						\
+		.sign = SIGNED,				\
 		.strict = STRICT,			\
 		.type = TYPE,				\
 		.shift = SHIFT,				\
@@ -53,6 +54,14 @@ DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 		.safe_val = SAFE_VAL,			\
 	}
 
+/* Define a feature with signed values */
+#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
+	__ARM64_FTR_BITS(FTR_SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
+
+/* Define a feature with unsigned value */
+#define U_ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
+	__ARM64_FTR_BITS(FTR_UNSIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
+
 #define ARM64_FTR_END					\
 	{						\
 		.width = 0,				\
@@ -99,7 +108,7 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
 	 * 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_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
 	ARM64_FTR_END,
 };
 
@@ -115,18 +124,18 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
 };
 
 static struct arm64_ftr_bits ftr_ctr[] = {
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
 	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 28, 3, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
 	/*
 	 * Linux can handle differing I-cache policies. Userspace JITs will
 	 * make use of *minLine
 	 */
-	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
+	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
 	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 4, 10, 0),	/* RAZ */
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
 	ARM64_FTR_END,
 };
 
@@ -144,12 +153,12 @@ static struct arm64_ftr_bits ftr_id_mmfr0[] = {
 
 static struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
 	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 32, 32, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
 	ARM64_FTR_END,
 };
 
-- 
1.7.9.5

[PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

Some of the feature bits have unsigned values and need
to be treated accordingly to avoid errors. Adds the property
to the feature bits and use the appropriate field extract helpers.

Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/cpufeature.h |   10 ++++++++--
 arch/arm64/kernel/cpufeature.c      |   37 ++++++++++++++++++++++-------------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 7a16102..29c3f5d 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -46,8 +46,12 @@ enum ftr_type {
 #define FTR_STRICT	true	/* SANITY check strict matching required */
 #define FTR_NONSTRICT	false	/* SANITY check ignored */
 
+#define FTR_SIGNED	true	/* Value should be treated as signed */
+#define FTR_UNSIGNED	false	/* Value should be treated as unsigned */
+
 struct arm64_ftr_bits {
-	bool		strict;	  /* CPU Sanity check: strict matching required ? */
+	bool		sign;	/* Value is signed ? */
+	bool		strict;	/* CPU Sanity check: strict matching required ? */
 	enum ftr_type	type;
 	u8		shift;
 	u8		width;
@@ -142,7 +146,9 @@ static inline u64 arm64_ftr_mask(struct arm64_ftr_bits *ftrp)
 
 static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
 {
-	return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width);
+	return ftrp->sign ?
+		cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width) :
+		cpuid_feature_extract_unsigned_field_width(val, ftrp->shift, ftrp->width);
 }
 
 static inline bool id_aa64mmfr0_mixed_endian_el0(u64 mmfr0)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index c8cf892..0669c63 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -44,8 +44,9 @@ unsigned int compat_elf_hwcap2 __read_mostly;
 
 DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 
-#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
+#define __ARM64_FTR_BITS(SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
 	{						\
+		.sign = SIGNED,				\
 		.strict = STRICT,			\
 		.type = TYPE,				\
 		.shift = SHIFT,				\
@@ -53,6 +54,14 @@ DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
 		.safe_val = SAFE_VAL,			\
 	}
 
+/* Define a feature with signed values */
+#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
+	__ARM64_FTR_BITS(FTR_SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
+
+/* Define a feature with unsigned value */
+#define U_ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
+	__ARM64_FTR_BITS(FTR_UNSIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
+
 #define ARM64_FTR_END					\
 	{						\
 		.width = 0,				\
@@ -99,7 +108,7 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
 	 * 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_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
 	ARM64_FTR_END,
 };
 
@@ -115,18 +124,18 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
 };
 
 static struct arm64_ftr_bits ftr_ctr[] = {
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
 	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 28, 3, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
 	/*
 	 * Linux can handle differing I-cache policies. Userspace JITs will
 	 * make use of *minLine
 	 */
-	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
+	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
 	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 4, 10, 0),	/* RAZ */
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
 	ARM64_FTR_END,
 };
 
@@ -144,12 +153,12 @@ static struct arm64_ftr_bits ftr_id_mmfr0[] = {
 
 static struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
 	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 32, 32, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
-	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
+	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
 	ARM64_FTR_END,
 };
 
-- 
1.7.9.5

[PATCH 3/5] arm64: debug: Treat the BRPs/WRPs as unsigned

[Suzuki K. Poulose]

IDAA64DFR0_EL1: BRPs and WRPs are unsigned values. Use
the appropriate helpers to extract those fields.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/hw_breakpoint.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index e54415e..9732908 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -138,16 +138,18 @@ extern struct pmu perf_ops_bp;
 /* Determine number of BRP registers available. */
 static inline int get_num_brps(void)
 {
+	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
 	return 1 +
-		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
+		cpuid_feature_extract_unsigned_field(dfr0,
 						ID_AA64DFR0_BRPS_SHIFT);
 }
 
 /* Determine number of WRP registers available. */
 static inline int get_num_wrps(void)
 {
+	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
 	return 1 +
-		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
+		cpuid_feature_extract_unsigned_field(dfr0,
 						ID_AA64DFR0_WRPS_SHIFT);
 }
 
-- 
1.7.9.5

[PATCH 3/5] arm64: debug: Treat the BRPs/WRPs as unsigned

[Suzuki K. Poulose]

IDAA64DFR0_EL1: BRPs and WRPs are unsigned values. Use
the appropriate helpers to extract those fields.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/include/asm/hw_breakpoint.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index e54415e..9732908 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -138,16 +138,18 @@ extern struct pmu perf_ops_bp;
 /* Determine number of BRP registers available. */
 static inline int get_num_brps(void)
 {
+	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
 	return 1 +
-		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
+		cpuid_feature_extract_unsigned_field(dfr0,
 						ID_AA64DFR0_BRPS_SHIFT);
 }
 
 /* Determine number of WRP registers available. */
 static inline int get_num_wrps(void)
 {
+	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
 	return 1 +
-		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
+		cpuid_feature_extract_unsigned_field(dfr0,
 						ID_AA64DFR0_WRPS_SHIFT);
 }
 
-- 
1.7.9.5

[PATCH 4/5] arm64: Make fail_incapable_cpu reusable

[Suzuki K. Poulose]

Refactors the code to get rid of the depenency on the capability
for fail_incapable_cpu(), so that it can be used by other checks.

Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 0669c63..5629f2c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -826,15 +826,15 @@ static u64 __raw_read_system_reg(u32 sys_id)
 }
 
 /*
- * Park the CPU which doesn't have the capability as advertised
- * by the system.
+ * Park the calling CPU which doesn't have the capability
+ * as advertised by the system.
  */
-static void fail_incapable_cpu(char *cap_type,
-				 const struct arm64_cpu_capabilities *cap)
+static void fail_incapable_cpu(void)
 {
 	int cpu = smp_processor_id();
 
-	pr_crit("CPU%d: missing %s : %s\n", cpu, cap_type, cap->desc);
+	pr_crit("CPU%d: will not boot\n", cpu);
+
 	/* Mark this CPU absent */
 	set_cpu_present(cpu, 0);
 
@@ -875,8 +875,11 @@ void verify_local_cpu_capabilities(void)
 		 * If the new CPU misses an advertised feature, we cannot proceed
 		 * further, park the cpu.
 		 */
-		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))
-			fail_incapable_cpu("arm64_features", &caps[i]);
+		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i])) {
+			pr_crit("CPU%d: missing feature: %s\n",
+					smp_processor_id(), caps[i].desc);
+			fail_incapable_cpu();
+		}
 		if (caps[i].enable)
 			caps[i].enable(NULL);
 	}
@@ -884,8 +887,11 @@ void verify_local_cpu_capabilities(void)
 	for (i = 0, caps = arm64_hwcaps; caps[i].desc; i++) {
 		if (!cpus_have_hwcap(&caps[i]))
 			continue;
-		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))
-			fail_incapable_cpu("arm64_hwcaps", &caps[i]);
+		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i])) {
+			pr_crit("CPU%d: missing HWCAP: %s\n",
+					smp_processor_id(), caps[i].desc);
+			fail_incapable_cpu();
+		}
 	}
 }
 
-- 
1.7.9.5

[PATCH 4/5] arm64: Make fail_incapable_cpu reusable

[Suzuki K. Poulose]

Refactors the code to get rid of the depenency on the capability
for fail_incapable_cpu(), so that it can be used by other checks.

Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 0669c63..5629f2c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -826,15 +826,15 @@ static u64 __raw_read_system_reg(u32 sys_id)
 }
 
 /*
- * Park the CPU which doesn't have the capability as advertised
- * by the system.
+ * Park the calling CPU which doesn't have the capability
+ * as advertised by the system.
  */
-static void fail_incapable_cpu(char *cap_type,
-				 const struct arm64_cpu_capabilities *cap)
+static void fail_incapable_cpu(void)
 {
 	int cpu = smp_processor_id();
 
-	pr_crit("CPU%d: missing %s : %s\n", cpu, cap_type, cap->desc);
+	pr_crit("CPU%d: will not boot\n", cpu);
+
 	/* Mark this CPU absent */
 	set_cpu_present(cpu, 0);
 
@@ -875,8 +875,11 @@ void verify_local_cpu_capabilities(void)
 		 * If the new CPU misses an advertised feature, we cannot proceed
 		 * further, park the cpu.
 		 */
-		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))
-			fail_incapable_cpu("arm64_features", &caps[i]);
+		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i])) {
+			pr_crit("CPU%d: missing feature: %s\n",
+					smp_processor_id(), caps[i].desc);
+			fail_incapable_cpu();
+		}
 		if (caps[i].enable)
 			caps[i].enable(NULL);
 	}
@@ -884,8 +887,11 @@ void verify_local_cpu_capabilities(void)
 	for (i = 0, caps = arm64_hwcaps; caps[i].desc; i++) {
 		if (!cpus_have_hwcap(&caps[i]))
 			continue;
-		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i]))
-			fail_incapable_cpu("arm64_hwcaps", &caps[i]);
+		if (!feature_matches(__raw_read_system_reg(caps[i].sys_reg), &caps[i])) {
+			pr_crit("CPU%d: missing HWCAP: %s\n",
+					smp_processor_id(), caps[i].desc);
+			fail_incapable_cpu();
+		}
 	}
 }
 
-- 
1.7.9.5

[PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Suzuki K. Poulose]

The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
id and is used in the early boot to make decisions. The value is
picked up from the Boot CPU and cannot be delayed until other CPUs
are up. If a secondary CPU has a smaller size than that of the Boot
CPU, things will break horribly and the usual SANITY check is not good
enough to prevent the system from crashing. Prevent this by failing CPUs with
ASID smaller than that of the boot CPU.

Also moves the fail_incapable_cpu() out of the CONFIG_HOTPLUG_CPU.

Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c |   81 +++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 5629f2c..769782a 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -293,6 +293,28 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {
 	ARM64_FTR_REG(SYS_CNTFRQ_EL0, ftr_generic32),
 };
 
+/*
+ * Park the calling CPU which doesn't have the capability
+ * as advertised by the system.
+ */
+static void fail_incapable_cpu(void)
+{
+	int cpu = smp_processor_id();
+
+	pr_crit("CPU%d: will not boot\n", cpu);
+
+	/* Mark this CPU absent */
+	set_cpu_present(cpu, 0);
+
+	/* Check if we can park ourselves */
+	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
+		cpu_ops[cpu]->cpu_die(cpu);
+	asm(
+	"1:	wfe\n"
+	"	wfi\n"
+	"	b	1b");
+}
+
 static int search_cmp_ftr_reg(const void *id, const void *regp)
 {
 	return (int)(unsigned long)id - (int)((const struct arm64_ftr_reg *)regp)->sys_id;
@@ -459,6 +481,40 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
 }
 
 /*
+ * The asid_bits, which determine the width of the mm context
+ * id, is based on the boot CPU value. If the new CPU doesn't
+ * have an ASID >= boot CPU, we are in trouble. Fail this CPU.
+ */
+static void check_cpu_asid_bits(int cpu,
+				struct cpuinfo_arm64 *info,
+				struct cpuinfo_arm64 *boot)
+{
+	u32 asid_boot = cpuid_feature_extract_unsigned_field(boot->reg_id_aa64mmfr0,
+							ID_AA64MMFR0_ASID_SHIFT);
+	u32 asid_cur = cpuid_feature_extract_unsigned_field(info->reg_id_aa64mmfr0,
+							ID_AA64MMFR0_ASID_SHIFT);
+	if (asid_cur < asid_boot) {
+		pr_crit("CPU%d: has incompatible ASIDBits: %u vs Boot CPU:%u\n",
+				cpu, asid_cur, asid_boot);
+		fail_incapable_cpu();
+	}
+	return;
+}
+
+/*
+ * Checks whether the cpu is missing any of the features
+ * the kernel has already started using at early boot,
+ * before the other CPUs are brought up. This is intended
+ * for checking features where variations can be fatal.
+ */
+static void check_early_cpu_features(int cpu,
+				struct cpuinfo_arm64 *info,
+				struct cpuinfo_arm64 *boot)
+{
+	check_cpu_asid_bits(cpu, info, boot);
+}
+
+/*
  * Update system wide CPU feature registers with the values from a
  * non-boot CPU. Also performs SANITY checks to make sure that there
  * aren't any insane variations from that of the boot CPU.
@@ -469,6 +525,9 @@ void update_cpu_features(int cpu,
 {
 	int taint = 0;
 
+	/* Make sure there are no fatal feature variations for this cpu */
+	check_early_cpu_features(cpu, info, boot);
+
 	/*
 	 * The kernel can handle differing I-cache policies, but otherwise
 	 * caches should look identical. Userspace JITs will make use of
@@ -826,28 +885,6 @@ static u64 __raw_read_system_reg(u32 sys_id)
 }
 
 /*
- * Park the calling CPU which doesn't have the capability
- * as advertised by the system.
- */
-static void fail_incapable_cpu(void)
-{
-	int cpu = smp_processor_id();
-
-	pr_crit("CPU%d: will not boot\n", cpu);
-
-	/* Mark this CPU absent */
-	set_cpu_present(cpu, 0);
-
-	/* Check if we can park ourselves */
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
-	asm(
-	"1:	wfe\n"
-	"	wfi\n"
-	"	b	1b");
-}
-
-/*
  * Run through the enabled system capabilities and enable() it on this CPU.
  * The capabilities were decided based on the available CPUs at the boot time.
  * Any new CPU should match the system wide status of the capability. If the
-- 
1.7.9.5

[PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Suzuki K. Poulose]

The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
id and is used in the early boot to make decisions. The value is
picked up from the Boot CPU and cannot be delayed until other CPUs
are up. If a secondary CPU has a smaller size than that of the Boot
CPU, things will break horribly and the usual SANITY check is not good
enough to prevent the system from crashing. Prevent this by failing CPUs with
ASID smaller than that of the boot CPU.

Also moves the fail_incapable_cpu() out of the CONFIG_HOTPLUG_CPU.

Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kernel/cpufeature.c |   81 +++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 5629f2c..769782a 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -293,6 +293,28 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {
 	ARM64_FTR_REG(SYS_CNTFRQ_EL0, ftr_generic32),
 };
 
+/*
+ * Park the calling CPU which doesn't have the capability
+ * as advertised by the system.
+ */
+static void fail_incapable_cpu(void)
+{
+	int cpu = smp_processor_id();
+
+	pr_crit("CPU%d: will not boot\n", cpu);
+
+	/* Mark this CPU absent */
+	set_cpu_present(cpu, 0);
+
+	/* Check if we can park ourselves */
+	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
+		cpu_ops[cpu]->cpu_die(cpu);
+	asm(
+	"1:	wfe\n"
+	"	wfi\n"
+	"	b	1b");
+}
+
 static int search_cmp_ftr_reg(const void *id, const void *regp)
 {
 	return (int)(unsigned long)id - (int)((const struct arm64_ftr_reg *)regp)->sys_id;
@@ -459,6 +481,40 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
 }
 
 /*
+ * The asid_bits, which determine the width of the mm context
+ * id, is based on the boot CPU value. If the new CPU doesn't
+ * have an ASID >= boot CPU, we are in trouble. Fail this CPU.
+ */
+static void check_cpu_asid_bits(int cpu,
+				struct cpuinfo_arm64 *info,
+				struct cpuinfo_arm64 *boot)
+{
+	u32 asid_boot = cpuid_feature_extract_unsigned_field(boot->reg_id_aa64mmfr0,
+							ID_AA64MMFR0_ASID_SHIFT);
+	u32 asid_cur = cpuid_feature_extract_unsigned_field(info->reg_id_aa64mmfr0,
+							ID_AA64MMFR0_ASID_SHIFT);
+	if (asid_cur < asid_boot) {
+		pr_crit("CPU%d: has incompatible ASIDBits: %u vs Boot CPU:%u\n",
+				cpu, asid_cur, asid_boot);
+		fail_incapable_cpu();
+	}
+	return;
+}
+
+/*
+ * Checks whether the cpu is missing any of the features
+ * the kernel has already started using@early boot,
+ * before the other CPUs are brought up. This is intended
+ * for checking features where variations can be fatal.
+ */
+static void check_early_cpu_features(int cpu,
+				struct cpuinfo_arm64 *info,
+				struct cpuinfo_arm64 *boot)
+{
+	check_cpu_asid_bits(cpu, info, boot);
+}
+
+/*
  * Update system wide CPU feature registers with the values from a
  * non-boot CPU. Also performs SANITY checks to make sure that there
  * aren't any insane variations from that of the boot CPU.
@@ -469,6 +525,9 @@ void update_cpu_features(int cpu,
 {
 	int taint = 0;
 
+	/* Make sure there are no fatal feature variations for this cpu */
+	check_early_cpu_features(cpu, info, boot);
+
 	/*
 	 * The kernel can handle differing I-cache policies, but otherwise
 	 * caches should look identical. Userspace JITs will make use of
@@ -826,28 +885,6 @@ static u64 __raw_read_system_reg(u32 sys_id)
 }
 
 /*
- * Park the calling CPU which doesn't have the capability
- * as advertised by the system.
- */
-static void fail_incapable_cpu(void)
-{
-	int cpu = smp_processor_id();
-
-	pr_crit("CPU%d: will not boot\n", cpu);
-
-	/* Mark this CPU absent */
-	set_cpu_present(cpu, 0);
-
-	/* Check if we can park ourselves */
-	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
-	asm(
-	"1:	wfe\n"
-	"	wfi\n"
-	"	b	1b");
-}
-
-/*
  * Run through the enabled system capabilities and enable() it on this CPU.
  * The capabilities were decided based on the available CPUs at the boot time.
  * Any new CPU should match the system wide status of the capability. If the
-- 
1.7.9.5

Re: [PATCH 2/5] arm64: cpufeature: Track unsigned fields

[AKASHI Takahiro]

Hi

 From my curiosity,
can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,
but it seems to be not always true looking at your patch.
Anyhow, for example,

On 11/19/2015 02:08 AM, Suzuki K. Poulose wrote:
> Some of the feature bits have unsigned values and need
> to be treated accordingly to avoid errors. Adds the property
> to the feature bits and use the appropriate field extract helpers.
>
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>   arch/arm64/include/asm/cpufeature.h |   10 ++++++++--
>   arch/arm64/kernel/cpufeature.c      |   37 ++++++++++++++++++++++-------------
>   2 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 7a16102..29c3f5d 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -46,8 +46,12 @@ enum ftr_type {
>   #define FTR_STRICT	true	/* SANITY check strict matching required */
>   #define FTR_NONSTRICT	false	/* SANITY check ignored */
>
> +#define FTR_SIGNED	true	/* Value should be treated as signed */
> +#define FTR_UNSIGNED	false	/* Value should be treated as unsigned */
> +
>   struct arm64_ftr_bits {
> -	bool		strict;	  /* CPU Sanity check: strict matching required ? */
> +	bool		sign;	/* Value is signed ? */
> +	bool		strict;	/* CPU Sanity check: strict matching required ? */
>   	enum ftr_type	type;
>   	u8		shift;
>   	u8		width;
> @@ -142,7 +146,9 @@ static inline u64 arm64_ftr_mask(struct arm64_ftr_bits *ftrp)
>
>   static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
>   {
> -	return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width);
> +	return ftrp->sign ?
> +		cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width) :
> +		cpuid_feature_extract_unsigned_field_width(val, ftrp->shift, ftrp->width);
>   }
>
>   static inline bool id_aa64mmfr0_mixed_endian_el0(u64 mmfr0)
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index c8cf892..0669c63 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -44,8 +44,9 @@ unsigned int compat_elf_hwcap2 __read_mostly;
>
>   DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>
> -#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
> +#define __ARM64_FTR_BITS(SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
>   	{						\
> +		.sign = SIGNED,				\
>   		.strict = STRICT,			\
>   		.type = TYPE,				\
>   		.shift = SHIFT,				\
> @@ -53,6 +54,14 @@ DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>   		.safe_val = SAFE_VAL,			\
>   	}
>
> +/* Define a feature with signed values */
> +#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
> +	__ARM64_FTR_BITS(FTR_SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
> +
> +/* Define a feature with unsigned value */
> +#define U_ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
> +	__ARM64_FTR_BITS(FTR_UNSIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
> +
>   #define ARM64_FTR_END					\
>   	{						\
>   		.width = 0,				\
> @@ -99,7 +108,7 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
>   	 * 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_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
>   	ARM64_FTR_END,
>   };

BigEnd, bits[11:8], is 0b0000 for "No mixed-endian support", and 0b0001 for
"Mixed-endian support". But can any other value be possible? If not, why signed?
If there are some hidden (or undocumented) specifications, as Ard mentioned, that's fine.
Please ignore my comments.

Thanks,
-Takahiro AKASHI


> @@ -115,18 +124,18 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
>   };
>
>   static struct arm64_ftr_bits ftr_ctr[] = {
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
>   	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 28, 3, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
>   	/*
>   	 * Linux can handle differing I-cache policies. Userspace JITs will
>   	 * make use of *minLine
>   	 */
> -	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
> +	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
>   	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 4, 10, 0),	/* RAZ */
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
>   	ARM64_FTR_END,
>   };
>
> @@ -144,12 +153,12 @@ static struct arm64_ftr_bits ftr_id_mmfr0[] = {
>
>   static struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>   	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 32, 32, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
>   	ARM64_FTR_END,
>   };
>
>

[PATCH 2/5] arm64: cpufeature: Track unsigned fields

[AKASHI Takahiro]

Hi

 From my curiosity,
can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,
but it seems to be not always true looking at your patch.
Anyhow, for example,

On 11/19/2015 02:08 AM, Suzuki K. Poulose wrote:
> Some of the feature bits have unsigned values and need
> to be treated accordingly to avoid errors. Adds the property
> to the feature bits and use the appropriate field extract helpers.
>
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>   arch/arm64/include/asm/cpufeature.h |   10 ++++++++--
>   arch/arm64/kernel/cpufeature.c      |   37 ++++++++++++++++++++++-------------
>   2 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 7a16102..29c3f5d 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -46,8 +46,12 @@ enum ftr_type {
>   #define FTR_STRICT	true	/* SANITY check strict matching required */
>   #define FTR_NONSTRICT	false	/* SANITY check ignored */
>
> +#define FTR_SIGNED	true	/* Value should be treated as signed */
> +#define FTR_UNSIGNED	false	/* Value should be treated as unsigned */
> +
>   struct arm64_ftr_bits {
> -	bool		strict;	  /* CPU Sanity check: strict matching required ? */
> +	bool		sign;	/* Value is signed ? */
> +	bool		strict;	/* CPU Sanity check: strict matching required ? */
>   	enum ftr_type	type;
>   	u8		shift;
>   	u8		width;
> @@ -142,7 +146,9 @@ static inline u64 arm64_ftr_mask(struct arm64_ftr_bits *ftrp)
>
>   static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
>   {
> -	return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width);
> +	return ftrp->sign ?
> +		cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width) :
> +		cpuid_feature_extract_unsigned_field_width(val, ftrp->shift, ftrp->width);
>   }
>
>   static inline bool id_aa64mmfr0_mixed_endian_el0(u64 mmfr0)
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index c8cf892..0669c63 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -44,8 +44,9 @@ unsigned int compat_elf_hwcap2 __read_mostly;
>
>   DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>
> -#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
> +#define __ARM64_FTR_BITS(SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
>   	{						\
> +		.sign = SIGNED,				\
>   		.strict = STRICT,			\
>   		.type = TYPE,				\
>   		.shift = SHIFT,				\
> @@ -53,6 +54,14 @@ DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>   		.safe_val = SAFE_VAL,			\
>   	}
>
> +/* Define a feature with signed values */
> +#define ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
> +	__ARM64_FTR_BITS(FTR_SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
> +
> +/* Define a feature with unsigned value */
> +#define U_ARM64_FTR_BITS(STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
> +	__ARM64_FTR_BITS(FTR_UNSIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL)
> +
>   #define ARM64_FTR_END					\
>   	{						\
>   		.width = 0,				\
> @@ -99,7 +108,7 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr0[] = {
>   	 * 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_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
>   	ARM64_FTR_END,
>   };

BigEnd, bits[11:8], is 0b0000 for "No mixed-endian support", and 0b0001 for
"Mixed-endian support". But can any other value be possible? If not, why signed?
If there are some hidden (or undocumented) specifications, as Ard mentioned, that's fine.
Please ignore my comments.

Thanks,
-Takahiro AKASHI


> @@ -115,18 +124,18 @@ static struct arm64_ftr_bits ftr_id_aa64mmfr1[] = {
>   };
>
>   static struct arm64_ftr_bits ftr_ctr[] = {
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 31, 1, 1),	/* RAO */
>   	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 28, 3, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_HIGHER_SAFE, 24, 4, 0),	/* CWG */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 20, 4, 0),	/* ERG */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 16, 4, 1),	/* DminLine */
>   	/*
>   	 * Linux can handle differing I-cache policies. Userspace JITs will
>   	 * make use of *minLine
>   	 */
> -	ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
> +	U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_EXACT, 14, 2, 0),	/* L1Ip */
>   	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 4, 10, 0),	/* RAZ */
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, 0, 4, 0),	/* IminLine */
>   	ARM64_FTR_END,
>   };
>
> @@ -144,12 +153,12 @@ static struct arm64_ftr_bits ftr_id_mmfr0[] = {
>
>   static struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>   	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, 32, 32, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
> -	ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_CTX_CMPS_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_WRPS_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_BRPS_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_PMUVER_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_TRACEVER_SHIFT, 4, 0),
> +	U_ARM64_FTR_BITS(FTR_STRICT, FTR_EXACT, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6),
>   	ARM64_FTR_END,
>   };
>
>

Re: [PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

On 19/11/15 04:57, AKASHI Takahiro wrote:
> Hi
>
>  From my curiosity,
> can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
> I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,
> but it seems to be not always true looking at your patch.
> Anyhow, for example,

...

>> -    ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
>> +    U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
>>       ARM64_FTR_END,
>>   };
>
> BigEnd, bits[11:8], is 0b0000 for "No mixed-endian support", and 0b0001 for
> "Mixed-endian support". But can any other value be possible? If not, why signed?
> If there are some hidden (or undocumented) specifications, as Ard mentioned, that's fine.
> Please ignore my comments.

There are no hidden specifications, but just that they are undocumented. To be precise,
the criteria I selected was based on the meaning of their values.

1) If value represents something which cannot be negative and hence should be treated as
    unsigned.
    e.g, number of break points ID_AA64DFR0:BRPs.

    
2) If the individual values are mapped to some other values which cannot be negative, but have
    LOWER_SAFE/HIGHER_SAFE relation.
     e.g,  CTR_EL0:WRG - Log2 of the write granule size.
	  ID_AA64MMFR0:PARANGE - Supported PA Size


Thanks
Suzuki

[PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

On 19/11/15 04:57, AKASHI Takahiro wrote:
> Hi
>
>  From my curiosity,
> can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
> I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,
> but it seems to be not always true looking at your patch.
> Anyhow, for example,

...

>> -    ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
>> +    U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
>>       ARM64_FTR_END,
>>   };
>
> BigEnd, bits[11:8], is 0b0000 for "No mixed-endian support", and 0b0001 for
> "Mixed-endian support". But can any other value be possible? If not, why signed?
> If there are some hidden (or undocumented) specifications, as Ard mentioned, that's fine.
> Please ignore my comments.

There are no hidden specifications, but just that they are undocumented. To be precise,
the criteria I selected was based on the meaning of their values.

1) If value represents something which cannot be negative and hence should be treated as
    unsigned.
    e.g, number of break points ID_AA64DFR0:BRPs.

    
2) If the individual values are mapped to some other values which cannot be negative, but have
    LOWER_SAFE/HIGHER_SAFE relation.
     e.g,  CTR_EL0:WRG - Log2 of the write granule size.
	  ID_AA64MMFR0:PARANGE - Supported PA Size


Thanks
Suzuki

Re: [PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

On 19/11/15 10:03, Suzuki K. Poulose wrote:
> On 19/11/15 04:57, AKASHI Takahiro wrote:
>> Hi
>>
>>  From my curiosity,
>> can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
>> I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,

Also, the unsigned values are not restricted to LOWER_SAFE/HIGHER_SAFE, but also applies for
FTR_EXACT, so that we use the appropriate methods to extract them.


Suzuki

[PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

On 19/11/15 10:03, Suzuki K. Poulose wrote:
> On 19/11/15 04:57, AKASHI Takahiro wrote:
>> Hi
>>
>>  From my curiosity,
>> can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
>> I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,

Also, the unsigned values are not restricted to LOWER_SAFE/HIGHER_SAFE, but also applies for
FTR_EXACT, so that we use the appropriate methods to extract them.


Suzuki

Re: [PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Catalin Marinas]

On Thu, Nov 19, 2015 at 10:03:13AM +0000, Suzuki K. Poulose wrote:
> On 19/11/15 04:57, AKASHI Takahiro wrote:
> > From my curiosity,
> >can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
> >I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,
> >but it seems to be not always true looking at your patch.
> >Anyhow, for example,
> 
> ...
> 
> >>-    ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
> >>+    U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
> >>      ARM64_FTR_END,
> >>  };
> >
> >BigEnd, bits[11:8], is 0b0000 for "No mixed-endian support", and 0b0001 for
> >"Mixed-endian support". But can any other value be possible? If not, why signed?
> >If there are some hidden (or undocumented) specifications, as Ard mentioned, that's fine.
> >Please ignore my comments.
> 
> There are no hidden specifications, but just that they are
> undocumented. To be precise, the criteria I selected was based on the
> meaning of their values.
> 
> 1) If value represents something which cannot be negative and hence
>    should be treated as unsigned. e.g, number of break points
>    ID_AA64DFR0:BRPs.
> 
> 2) If the individual values are mapped to some other values which
>    cannot be negative, but have LOWER_SAFE/HIGHER_SAFE relation. e.g,
>    CTR_EL0:WRG - Log2 of the write granule size. ID_AA64MMFR0:PARANGE
>    - Supported PA Size

I asked for a clarification to be added to the ARM ARM but, in the
meantime, we have three types of fields:

a) A precise value (number of breakpoint registers) or a value from
   which you derive some precise value. You mentioned these above

b) Fields defining the presence of a feature (1, 2, 3). These would
   always be positive since the absence of such feature would mean a
   value of 0

c) Fields defining the absence of a feature by setting 0xf. These are
   usually fields that were initial RAZ and turned to -1. I don't expect
   such field be greater than 0, nor smaller than -1.

So I think we can treat (a) and (b) as unsigned permanently. We could
treat (c) as unsigned as well with a value of 0xf though I'm not sure
how it matches your LOWER/HIGHER_SAFE definitions.

If we go for all unsigned, we no longer need the sign extension of a
4-bit value.

-- 
Catalin

[PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Catalin Marinas]

On Thu, Nov 19, 2015 at 10:03:13AM +0000, Suzuki K. Poulose wrote:
> On 19/11/15 04:57, AKASHI Takahiro wrote:
> > From my curiosity,
> >can you please clarify your criteria regarding which fields of a register should be signed or unsigned?
> >I guessed that the fields marked with FTR_LOWER_SAFE or FTR_HIGHER_SAFE could be unsigned,
> >but it seems to be not always true looking at your patch.
> >Anyhow, for example,
> 
> ...
> 
> >>-    ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
> >>+    U_ARM64_FTR_BITS(FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64MMFR0_PARANGE_SHIFT, 4, 0),
> >>      ARM64_FTR_END,
> >>  };
> >
> >BigEnd, bits[11:8], is 0b0000 for "No mixed-endian support", and 0b0001 for
> >"Mixed-endian support". But can any other value be possible? If not, why signed?
> >If there are some hidden (or undocumented) specifications, as Ard mentioned, that's fine.
> >Please ignore my comments.
> 
> There are no hidden specifications, but just that they are
> undocumented. To be precise, the criteria I selected was based on the
> meaning of their values.
> 
> 1) If value represents something which cannot be negative and hence
>    should be treated as unsigned. e.g, number of break points
>    ID_AA64DFR0:BRPs.
> 
> 2) If the individual values are mapped to some other values which
>    cannot be negative, but have LOWER_SAFE/HIGHER_SAFE relation. e.g,
>    CTR_EL0:WRG - Log2 of the write granule size. ID_AA64MMFR0:PARANGE
>    - Supported PA Size

I asked for a clarification to be added to the ARM ARM but, in the
meantime, we have three types of fields:

a) A precise value (number of breakpoint registers) or a value from
   which you derive some precise value. You mentioned these above

b) Fields defining the presence of a feature (1, 2, 3). These would
   always be positive since the absence of such feature would mean a
   value of 0

c) Fields defining the absence of a feature by setting 0xf. These are
   usually fields that were initial RAZ and turned to -1. I don't expect
   such field be greater than 0, nor smaller than -1.

So I think we can treat (a) and (b) as unsigned permanently. We could
treat (c) as unsigned as well with a value of 0xf though I'm not sure
how it matches your LOWER/HIGHER_SAFE definitions.

If we go for all unsigned, we no longer need the sign extension of a
4-bit value.

-- 
Catalin

Re: [PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

On 19/11/15 18:45, Catalin Marinas wrote:
> On Thu, Nov 19, 2015 at 10:03:13AM +0000, Suzuki K. Poulose wrote:
>> On 19/11/15 04:57, AKASHI Takahiro wrote:


> a) A precise value (number of breakpoint registers) or a value from
>     which you derive some precise value. You mentioned these above
>
> b) Fields defining the presence of a feature (1, 2, 3). These would
>     always be positive since the absence of such feature would mean a
>     value of 0
>
> c) Fields defining the absence of a feature by setting 0xf. These are
>     usually fields that were initial RAZ and turned to -1. I don't expect
>     such field be greater than 0, nor smaller than -1.
>
> So I think we can treat (a) and (b) as unsigned permanently.

Agreed.

> We could treat (c) as unsigned as well with a value of 0xf though I'm not sure
> how it matches your LOWER/HIGHER_SAFE definitions.

I think we should treat (c) as signed, as we never know what could change,
given that meaning of (0xf - implies unsupported) < meaning of (0 - supported).
Treating them unsigned could break the LOWER/HIGHER_SAFE definitions and makes
the safe value selection ugly.

Cheers
Suzuki

[PATCH 2/5] arm64: cpufeature: Track unsigned fields

[Suzuki K. Poulose]

On 19/11/15 18:45, Catalin Marinas wrote:
> On Thu, Nov 19, 2015 at 10:03:13AM +0000, Suzuki K. Poulose wrote:
>> On 19/11/15 04:57, AKASHI Takahiro wrote:


> a) A precise value (number of breakpoint registers) or a value from
>     which you derive some precise value. You mentioned these above
>
> b) Fields defining the presence of a feature (1, 2, 3). These would
>     always be positive since the absence of such feature would mean a
>     value of 0
>
> c) Fields defining the absence of a feature by setting 0xf. These are
>     usually fields that were initial RAZ and turned to -1. I don't expect
>     such field be greater than 0, nor smaller than -1.
>
> So I think we can treat (a) and (b) as unsigned permanently.

Agreed.

> We could treat (c) as unsigned as well with a value of 0xf though I'm not sure
> how it matches your LOWER/HIGHER_SAFE definitions.

I think we should treat (c) as signed, as we never know what could change,
given that meaning of (0xf - implies unsupported) < meaning of (0 - supported).
Treating them unsigned could break the LOWER/HIGHER_SAFE definitions and makes
the safe value selection ugly.

Cheers
Suzuki

Re: [PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Will Deacon]

On Wed, Nov 18, 2015 at 05:09:00PM +0000, Suzuki K. Poulose wrote:
> The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
> id and is used in the early boot to make decisions. The value is
> picked up from the Boot CPU and cannot be delayed until other CPUs
> are up. If a secondary CPU has a smaller size than that of the Boot
> CPU, things will break horribly and the usual SANITY check is not good
> enough to prevent the system from crashing. Prevent this by failing CPUs with
> ASID smaller than that of the boot CPU.
> 
> Also moves the fail_incapable_cpu() out of the CONFIG_HOTPLUG_CPU.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/cpufeature.c |   81 +++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 5629f2c..769782a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -293,6 +293,28 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {
>  	ARM64_FTR_REG(SYS_CNTFRQ_EL0, ftr_generic32),
>  };
>  
> +/*
> + * Park the calling CPU which doesn't have the capability
> + * as advertised by the system.
> + */
> +static void fail_incapable_cpu(void)
> +{
> +	int cpu = smp_processor_id();
> +
> +	pr_crit("CPU%d: will not boot\n", cpu);

This is less informative than the current message (whcih describes the
missing capability).

> +
> +	/* Mark this CPU absent */
> +	set_cpu_present(cpu, 0);
> +
> +	/* Check if we can park ourselves */
> +	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
> +		cpu_ops[cpu]->cpu_die(cpu);
> +	asm(
> +	"1:	wfe\n"
> +	"	wfi\n"
> +	"	b	1b");
> +}
> +
>  static int search_cmp_ftr_reg(const void *id, const void *regp)
>  {
>  	return (int)(unsigned long)id - (int)((const struct arm64_ftr_reg *)regp)->sys_id;
> @@ -459,6 +481,40 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
>  }
>  
>  /*
> + * The asid_bits, which determine the width of the mm context
> + * id, is based on the boot CPU value. If the new CPU doesn't
> + * have an ASID >= boot CPU, we are in trouble. Fail this CPU.
> + */
> +static void check_cpu_asid_bits(int cpu,
> +				struct cpuinfo_arm64 *info,
> +				struct cpuinfo_arm64 *boot)
> +{
> +	u32 asid_boot = cpuid_feature_extract_unsigned_field(boot->reg_id_aa64mmfr0,
> +							ID_AA64MMFR0_ASID_SHIFT);
> +	u32 asid_cur = cpuid_feature_extract_unsigned_field(info->reg_id_aa64mmfr0,
> +							ID_AA64MMFR0_ASID_SHIFT);
> +	if (asid_cur < asid_boot) {
> +		pr_crit("CPU%d: has incompatible ASIDBits: %u vs Boot CPU:%u\n",
> +				cpu, asid_cur, asid_boot);
> +		fail_incapable_cpu();
> +	}

Hmm. Whilst we want to ensure that secondary CPUs don't have a smaller
ASID size than the boot CPU, can we actually guarantee that a smaller
value for ID_AA64MMFR0.ASIDBits corresponds to fewer bits? We're
probably better off assuming 8-bit ASIDs unless ASIDBits == 2 (which is
what the ASID allocator does).

Will

[PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Will Deacon]

On Wed, Nov 18, 2015 at 05:09:00PM +0000, Suzuki K. Poulose wrote:
> The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
> id and is used in the early boot to make decisions. The value is
> picked up from the Boot CPU and cannot be delayed until other CPUs
> are up. If a secondary CPU has a smaller size than that of the Boot
> CPU, things will break horribly and the usual SANITY check is not good
> enough to prevent the system from crashing. Prevent this by failing CPUs with
> ASID smaller than that of the boot CPU.
> 
> Also moves the fail_incapable_cpu() out of the CONFIG_HOTPLUG_CPU.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kernel/cpufeature.c |   81 +++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 5629f2c..769782a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -293,6 +293,28 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {
>  	ARM64_FTR_REG(SYS_CNTFRQ_EL0, ftr_generic32),
>  };
>  
> +/*
> + * Park the calling CPU which doesn't have the capability
> + * as advertised by the system.
> + */
> +static void fail_incapable_cpu(void)
> +{
> +	int cpu = smp_processor_id();
> +
> +	pr_crit("CPU%d: will not boot\n", cpu);

This is less informative than the current message (whcih describes the
missing capability).

> +
> +	/* Mark this CPU absent */
> +	set_cpu_present(cpu, 0);
> +
> +	/* Check if we can park ourselves */
> +	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
> +		cpu_ops[cpu]->cpu_die(cpu);
> +	asm(
> +	"1:	wfe\n"
> +	"	wfi\n"
> +	"	b	1b");
> +}
> +
>  static int search_cmp_ftr_reg(const void *id, const void *regp)
>  {
>  	return (int)(unsigned long)id - (int)((const struct arm64_ftr_reg *)regp)->sys_id;
> @@ -459,6 +481,40 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
>  }
>  
>  /*
> + * The asid_bits, which determine the width of the mm context
> + * id, is based on the boot CPU value. If the new CPU doesn't
> + * have an ASID >= boot CPU, we are in trouble. Fail this CPU.
> + */
> +static void check_cpu_asid_bits(int cpu,
> +				struct cpuinfo_arm64 *info,
> +				struct cpuinfo_arm64 *boot)
> +{
> +	u32 asid_boot = cpuid_feature_extract_unsigned_field(boot->reg_id_aa64mmfr0,
> +							ID_AA64MMFR0_ASID_SHIFT);
> +	u32 asid_cur = cpuid_feature_extract_unsigned_field(info->reg_id_aa64mmfr0,
> +							ID_AA64MMFR0_ASID_SHIFT);
> +	if (asid_cur < asid_boot) {
> +		pr_crit("CPU%d: has incompatible ASIDBits: %u vs Boot CPU:%u\n",
> +				cpu, asid_cur, asid_boot);
> +		fail_incapable_cpu();
> +	}

Hmm. Whilst we want to ensure that secondary CPUs don't have a smaller
ASID size than the boot CPU, can we actually guarantee that a smaller
value for ID_AA64MMFR0.ASIDBits corresponds to fewer bits? We're
probably better off assuming 8-bit ASIDs unless ASIDBits == 2 (which is
what the ASID allocator does).

Will

Re: [PATCH 3/5] arm64: debug: Treat the BRPs/WRPs as unsigned

[Will Deacon]

On Wed, Nov 18, 2015 at 05:08:58PM +0000, Suzuki K. Poulose wrote:
> IDAA64DFR0_EL1: BRPs and WRPs are unsigned values. Use
> the appropriate helpers to extract those fields.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/hw_breakpoint.h |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Once you've got the dependencies sorted out:

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index e54415e..9732908 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -138,16 +138,18 @@ extern struct pmu perf_ops_bp;
>  /* Determine number of BRP registers available. */
>  static inline int get_num_brps(void)
>  {
> +	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
>  	return 1 +
> -		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
> +		cpuid_feature_extract_unsigned_field(dfr0,
>  						ID_AA64DFR0_BRPS_SHIFT);
>  }
>  
>  /* Determine number of WRP registers available. */
>  static inline int get_num_wrps(void)
>  {
> +	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
>  	return 1 +
> -		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
> +		cpuid_feature_extract_unsigned_field(dfr0,
>  						ID_AA64DFR0_WRPS_SHIFT);
>  }
>  
> -- 
> 1.7.9.5
> 

[PATCH 3/5] arm64: debug: Treat the BRPs/WRPs as unsigned

[Will Deacon]

On Wed, Nov 18, 2015 at 05:08:58PM +0000, Suzuki K. Poulose wrote:
> IDAA64DFR0_EL1: BRPs and WRPs are unsigned values. Use
> the appropriate helpers to extract those fields.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/hw_breakpoint.h |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Once you've got the dependencies sorted out:

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index e54415e..9732908 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -138,16 +138,18 @@ extern struct pmu perf_ops_bp;
>  /* Determine number of BRP registers available. */
>  static inline int get_num_brps(void)
>  {
> +	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
>  	return 1 +
> -		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
> +		cpuid_feature_extract_unsigned_field(dfr0,
>  						ID_AA64DFR0_BRPS_SHIFT);
>  }
>  
>  /* Determine number of WRP registers available. */
>  static inline int get_num_wrps(void)
>  {
> +	u64 dfr0 = read_system_reg(SYS_ID_AA64DFR0_EL1);
>  	return 1 +
> -		cpuid_feature_extract_field(read_system_reg(SYS_ID_AA64DFR0_EL1),
> +		cpuid_feature_extract_unsigned_field(dfr0,
>  						ID_AA64DFR0_WRPS_SHIFT);
>  }
>  
> -- 
> 1.7.9.5
> 

Re: [PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Suzuki K. Poulose]

On 23/11/15 17:29, Will Deacon wrote:
> On Wed, Nov 18, 2015 at 05:09:00PM +0000, Suzuki K. Poulose wrote:
>> The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
>> id and is used in the early boot to make decisions. The value is
>> picked up from the Boot CPU and cannot be delayed until other CPUs
>> are up. If a secondary CPU has a smaller size than that of the Boot
>> CPU, things will break horribly and the usual SANITY check is not good
>> enough to prevent the system from crashing. Prevent this by failing CPUs with
>> ASID smaller than that of the boot CPU.

...

>> +	pr_crit("CPU%d: will not boot\n", cpu);
>
> This is less informative than the current message (whcih describes the
> missing capability).

The missing capability is printed by the caller (Patch 4/5) and see below
for the new user.


	ID_AA64MMFR0_ASID_SHIFT);
>> +	if (asid_cur < asid_boot) {
>> +		pr_crit("CPU%d: has incompatible ASIDBits: %u vs Boot CPU:%u\n",
>> +				cpu, asid_cur, asid_boot);
>> +		fail_incapable_cpu();
>> +	}
>
> Hmm. Whilst we want to ensure that secondary CPUs don't have a smaller
> ASID size than the boot CPU, can we actually guarantee that a smaller
> value for ID_AA64MMFR0.ASIDBits corresponds to fewer bits? We're
> probably better off assuming 8-bit ASIDs unless ASIDBits == 2 (which is
> what the ASID allocator does).

Right, I already have a reworked version, which does the same and moves it
back to verify_local_cpu_capabilities(). I will send it tomorrow.

Cheers
Suzuki

[PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Suzuki K. Poulose]

On 23/11/15 17:29, Will Deacon wrote:
> On Wed, Nov 18, 2015 at 05:09:00PM +0000, Suzuki K. Poulose wrote:
>> The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
>> id and is used in the early boot to make decisions. The value is
>> picked up from the Boot CPU and cannot be delayed until other CPUs
>> are up. If a secondary CPU has a smaller size than that of the Boot
>> CPU, things will break horribly and the usual SANITY check is not good
>> enough to prevent the system from crashing. Prevent this by failing CPUs with
>> ASID smaller than that of the boot CPU.

...

>> +	pr_crit("CPU%d: will not boot\n", cpu);
>
> This is less informative than the current message (whcih describes the
> missing capability).

The missing capability is printed by the caller (Patch 4/5) and see below
for the new user.


	ID_AA64MMFR0_ASID_SHIFT);
>> +	if (asid_cur < asid_boot) {
>> +		pr_crit("CPU%d: has incompatible ASIDBits: %u vs Boot CPU:%u\n",
>> +				cpu, asid_cur, asid_boot);
>> +		fail_incapable_cpu();
>> +	}
>
> Hmm. Whilst we want to ensure that secondary CPUs don't have a smaller
> ASID size than the boot CPU, can we actually guarantee that a smaller
> value for ID_AA64MMFR0.ASIDBits corresponds to fewer bits? We're
> probably better off assuming 8-bit ASIDs unless ASIDBits == 2 (which is
> what the ASID allocator does).

Right, I already have a reworked version, which does the same and moves it
back to verify_local_cpu_capabilities(). I will send it tomorrow.

Cheers
Suzuki

Re: [PATCH 0/5] arm64: cpufeature: Fixes for 4.4-rc1

[Catalin Marinas]

On Wed, Nov 18, 2015 at 05:03:53PM +0000, Suzuki K. Poulose wrote:
> This series contains fixes for two issues.
> 
>  Patches 1 - 3:	Handling of unsigned feature values, issu reported
> 		by AKASHI Takahiro.
> 
>  Patches 4 - 5:	Adds a check to make sure all the secondary CPUs have
> 		compatible ASIDBits to prevent system crashes.
> 
> Suzuki K. Poulose (5):
>   arm64: cpufeature: Add helpers for extracting unsigned values
>   arm64: cpufeature: Track unsigned fields
>   arm64: debug: Treat the BRPs/WRPs as unsigned

I merged the first three patches, there are still discussions on the
last two. Thanks.

-- 
Catalin

[PATCH 0/5] arm64: cpufeature: Fixes for 4.4-rc1

[Catalin Marinas]

On Wed, Nov 18, 2015 at 05:03:53PM +0000, Suzuki K. Poulose wrote:
> This series contains fixes for two issues.
> 
>  Patches 1 - 3:	Handling of unsigned feature values, issu reported
> 		by AKASHI Takahiro.
> 
>  Patches 4 - 5:	Adds a check to make sure all the secondary CPUs have
> 		compatible ASIDBits to prevent system crashes.
> 
> Suzuki K. Poulose (5):
>   arm64: cpufeature: Add helpers for extracting unsigned values
>   arm64: cpufeature: Track unsigned fields
>   arm64: debug: Treat the BRPs/WRPs as unsigned

I merged the first three patches, there are still discussions on the
last two. Thanks.

-- 
Catalin