[PATCH 0/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[PATCH 0/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Anshuman Khandual]

This series enables FEAT_Debugv8p9 thus extending breakpoint and watchpoint
support upto 64. This series is based on v6.12-rc1 after applying the KVM
FEAT_FGT2 FGU series.

https://lore.kernel.org/all/20241001024356.1096072-1-anshuman.khandual@arm.com/

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
Cc: James Morse <james.morse@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.linux.dev
Cc: linux-kernel@vger.kernel.org

Changes in V1:

- Changed FTR_STRICT to FTR_NONSTRICT for the following ID_AA64DFR1_EL1
  register fields - ABL_CMPs, DPFZS, PMICNTR, CTX_CMPs, WRPs and BRPs

Changes in RFC V2:

https://lore.kernel.org/linux-arm-kernel/20240620092607.267132-1-anshuman.khandual@arm.com/

- This series has been split from RFC V1 dealing only with arm64 breakpoints
- Restored back DBG_MDSCR_MASK definition (unrelated change)
- Added preempt_disable()/enable() blocks between selecting banks and registers

Changes in RFC:

https://lore.kernel.org/all/20240405080008.1225223-1-anshuman.khandual@arm.com/

Anshuman Khandual (3):
  arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register
  arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9
  arm64/hw_breakpoint: Enable FEAT_Debugv8p9

 Documentation/arch/arm64/booting.rst    | 19 ++++++++++
 arch/arm64/include/asm/debug-monitors.h |  1 +
 arch/arm64/include/asm/el2_setup.h      | 27 +++++++++++++
 arch/arm64/include/asm/hw_breakpoint.h  | 50 ++++++++++++++++++++-----
 arch/arm64/include/asm/kvm_arm.h        |  1 +
 arch/arm64/kernel/cpufeature.c          | 21 ++++++++---
 arch/arm64/kernel/debug-monitors.c      | 16 ++++++--
 arch/arm64/kernel/hw_breakpoint.c       | 33 ++++++++++++++++
 8 files changed, 149 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Anshuman Khandual]

This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
ftr_raz[] array which is now redundant. These register fields will be used
to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
later.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
cc: Mark Brown <broonie@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/kernel/cpufeature.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 718728a85430..bd4d85f5dd92 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -530,6 +530,21 @@ static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
 	ARM64_FTR_END,
 };
 
+static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_EBEP_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ITE_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABLE_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_PMICNTR_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SPMU_SHIFT, 4, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_BRPs_SHIFT, 8, 0),
+	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SYSPMUID_SHIFT, 8, 0),
+	ARM64_FTR_END,
+};
+
 static const struct arm64_ftr_bits ftr_mvfr0[] = {
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, MVFR0_EL1_FPRound_SHIFT, 4, 0),
 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, MVFR0_EL1_FPShVec_SHIFT, 4, 0),
@@ -708,10 +723,6 @@ static const struct arm64_ftr_bits ftr_single32[] = {
 	ARM64_FTR_END,
 };
 
-static const struct arm64_ftr_bits ftr_raz[] = {
-	ARM64_FTR_END,
-};
-
 #define __ARM64_FTR_REG_OVERRIDE(id_str, id, table, ovr) {	\
 		.sys_id = id,					\
 		.reg = 	&(struct arm64_ftr_reg){		\
@@ -784,7 +795,7 @@ static const struct __ftr_reg_entry {
 
 	/* Op1 = 0, CRn = 0, CRm = 5 */
 	ARM64_FTR_REG(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0),
-	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_raz),
+	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_id_aa64dfr1),
 
 	/* Op1 = 0, CRn = 0, CRm = 6 */
 	ARM64_FTR_REG(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0),
-- 
2.25.1

[PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Anshuman Khandual]

Fine grained trap control for MDSELR_EL1 register needs to be configured in
HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2
is also present. This adds a new helper __init_el2_fgt2() initializing this
new FEAT_FGT2 based fine grained registers.

MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and
watchpoint exceptions when kernel enters at EL1, but EL2 is also present.
This updates __init_el2_debug() as required for FEAT_Debugv8p9.

While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: kvmarm@lists.linux.dev
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 Documentation/arch/arm64/booting.rst | 19 +++++++++++++++++++
 arch/arm64/include/asm/el2_setup.h   | 27 +++++++++++++++++++++++++++
 arch/arm64/include/asm/kvm_arm.h     |  1 +
 3 files changed, 47 insertions(+)

diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst
index b57776a68f15..e69d972018cf 100644
--- a/Documentation/arch/arm64/booting.rst
+++ b/Documentation/arch/arm64/booting.rst
@@ -285,6 +285,12 @@ Before jumping into the kernel, the following conditions must be met:
 
     - SCR_EL3.FGTEn (bit 27) must be initialised to 0b1.
 
+  For CPUs with the Fine Grained Traps (FEAT_FGT2) extension present:
+
+  - If EL3 is present and the kernel is entered at EL2:
+
+    - SCR_EL3.FGTEn2 (bit 59) must be initialised to 0b1.
+
   For CPUs with support for HCRX_EL2 (FEAT_HCX) present:
 
   - If EL3 is present and the kernel is entered at EL2:
@@ -319,6 +325,19 @@ Before jumping into the kernel, the following conditions must be met:
     - ZCR_EL2.LEN must be initialised to the same value for all CPUs the
       kernel will execute on.
 
+  For CPUs with FEAT_Debugv8p9 extension present:
+
+  - If the kernel is entered at EL1 and EL2 is present:
+
+    - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
+    - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
+    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
+
+  - If EL3 is present:
+
+    - MDCR_EL3.TDA (bit 9) must be initialized to 0b0
+    - MDCR_EL3.EBWE (bit 43) must be initialized to 0b1
+
   For CPUs with the Scalable Matrix Extension (FEAT_SME):
 
   - If EL3 is present:
diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index e0ffdf13a18b..2e6a8de5e4e4 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -97,6 +97,14 @@
 						// to own it.
 
 .Lskip_trace_\@:
+	mrs	x1, id_aa64dfr0_el1
+	ubfx	x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4
+	cmp	x1, #ID_AA64DFR0_EL1_DebugVer_V8P9
+	b.lt	.Lskip_dbg_v8p9_\@
+
+	mov	x0, #MDCR_EL2_EBWE
+	orr	x2, x2, x0
+.Lskip_dbg_v8p9_\@:
 	msr	mdcr_el2, x2			// Configure debug traps
 .endm
 
@@ -215,6 +223,24 @@
 .Lskip_fgt_\@:
 .endm
 
+.macro __init_el2_fgt2
+	mrs	x1, id_aa64mmfr0_el1
+	ubfx	x1, x1, #ID_AA64MMFR0_EL1_FGT_SHIFT, #4
+	cmp	x1, #ID_AA64MMFR0_EL1_FGT_FGT2
+	b.lt	.Lskip_fgt2_\@
+
+	mrs	x1, id_aa64dfr0_el1
+	ubfx	x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4
+	cmp	x1, #ID_AA64DFR0_EL1_DebugVer_V8P9
+	b.lt	.Lskip_dbg_v8p9_\@
+
+	mov_q   x0, HDFGWTR2_EL2_nMDSELR_EL1
+	msr_s	SYS_HDFGWTR2_EL2, x0
+	msr_s	SYS_HDFGRTR2_EL2, x0
+.Lskip_dbg_v8p9_\@:
+.Lskip_fgt2_\@:
+.endm
+
 .macro __init_el2_nvhe_prepare_eret
 	mov	x0, #INIT_PSTATE_EL1
 	msr	spsr_el2, x0
@@ -240,6 +266,7 @@
 	__init_el2_nvhe_idregs
 	__init_el2_cptr
 	__init_el2_fgt
+	__init_el2_fgt2
 .endm
 
 #ifndef __KVM_NVHE_HYPERVISOR__
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 850fac9a7840..18aeec43efc2 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -312,6 +312,7 @@
 				 GENMASK(15, 0))
 
 /* Hyp Debug Configuration Register bits */
+#define MDCR_EL2_EBWE		(UL(1) << 43)
 #define MDCR_EL2_E2TB_MASK	(UL(0x3))
 #define MDCR_EL2_E2TB_SHIFT	(UL(24))
 #define MDCR_EL2_HPMFZS		(UL(1) << 36)
-- 
2.25.1

[PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Anshuman Khandual]

Currently there can be maximum 16 breakpoints, and 16 watchpoints available
on a given platform - as detected from ID_AA64DFR0_EL1.[BRPs|WRPs] register
fields. But these breakpoint, and watchpoints can be extended further up to
64 via a new arch feature FEAT_Debugv8p9.

This first enables banked access for the breakpoint and watchpoint register
set via MDSELR_EL1, extended exceptions via MDSCR_EL1.EMBWE and determining
available breakpoints and watchpoints in the platform from ID_AA64DFR1_EL1,
when FEAT_Debugv8p9 is enabled.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/debug-monitors.h |  1 +
 arch/arm64/include/asm/hw_breakpoint.h  | 50 ++++++++++++++++++++-----
 arch/arm64/kernel/debug-monitors.c      | 16 ++++++--
 arch/arm64/kernel/hw_breakpoint.c       | 33 ++++++++++++++++
 4 files changed, 86 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 13d437bcbf58..a14097673ae0 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -20,6 +20,7 @@
 #define DBG_MDSCR_KDE		(1 << 13)
 #define DBG_MDSCR_MDE		(1 << 15)
 #define DBG_MDSCR_MASK		~(DBG_MDSCR_KDE | DBG_MDSCR_MDE)
+#define DBG_MDSCR_EMBWE		(1UL << 32)
 
 #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)
 
diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
index bd81cf17744a..362c4d4a64ac 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -79,8 +79,8 @@ static inline void decode_ctrl_reg(u32 reg,
  * Limits.
  * Changing these will require modifications to the register accessors.
  */
-#define ARM_MAX_BRP		16
-#define ARM_MAX_WRP		16
+#define ARM_MAX_BRP		64
+#define ARM_MAX_WRP		64
 
 /* Virtual debug register bases. */
 #define AARCH64_DBG_REG_BVR	0
@@ -94,13 +94,25 @@ static inline void decode_ctrl_reg(u32 reg,
 #define AARCH64_DBG_REG_NAME_WVR	wvr
 #define AARCH64_DBG_REG_NAME_WCR	wcr
 
+static inline bool is_debug_v8p9_enabled(void)
+{
+	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
+	int dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
+
+	return dver == ID_AA64DFR0_EL1_DebugVer_V8P9;
+}
+
 /* Accessor macros for the debug registers. */
 #define AARCH64_DBG_READ(N, REG, VAL) do {\
 	VAL = read_sysreg(dbg##REG##N##_el1);\
+	if (is_debug_v8p9_enabled())	\
+		preempt_enable();	\
 } while (0)
 
 #define AARCH64_DBG_WRITE(N, REG, VAL) do {\
 	write_sysreg(VAL, dbg##REG##N##_el1);\
+	if (is_debug_v8p9_enabled())	\
+		preempt_enable();	\
 } while (0)
 
 struct task_struct;
@@ -138,19 +150,37 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
 /* Determine number of BRP registers available. */
 static inline int get_num_brps(void)
 {
-	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
-	return 1 +
-		cpuid_feature_extract_unsigned_field(dfr0,
-						ID_AA64DFR0_EL1_BRPs_SHIFT);
+	u64 dfr0, dfr1;
+	int dver, brps;
+
+	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
+	dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
+	if (dver == ID_AA64DFR0_EL1_DebugVer_V8P9) {
+		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
+		brps = cpuid_feature_extract_unsigned_field_width(dfr1,
+								  ID_AA64DFR1_EL1_BRPs_SHIFT, 8);
+	} else {
+		brps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRPs_SHIFT);
+	}
+	return 1 + brps;
 }
 
 /* Determine number of WRP registers available. */
 static inline int get_num_wrps(void)
 {
-	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
-	return 1 +
-		cpuid_feature_extract_unsigned_field(dfr0,
-						ID_AA64DFR0_EL1_WRPs_SHIFT);
+	u64 dfr0, dfr1;
+	int dver, wrps;
+
+	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
+	dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
+	if (dver == ID_AA64DFR0_EL1_DebugVer_V8P9) {
+		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
+		wrps = cpuid_feature_extract_unsigned_field_width(dfr1,
+								  ID_AA64DFR1_EL1_WRPs_SHIFT, 8);
+	} else {
+		wrps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_WRPs_SHIFT);
+	}
+	return 1 + wrps;
 }
 
 #ifdef CONFIG_CPU_PM
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 024a7b245056..af643c935f2e 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -23,6 +23,7 @@
 #include <asm/debug-monitors.h>
 #include <asm/system_misc.h>
 #include <asm/traps.h>
+#include <asm/hw_breakpoint.h>
 
 /* Determine debug architecture. */
 u8 debug_monitors_arch(void)
@@ -34,7 +35,7 @@ u8 debug_monitors_arch(void)
 /*
  * MDSCR access routines.
  */
-static void mdscr_write(u32 mdscr)
+static void mdscr_write(u64 mdscr)
 {
 	unsigned long flags;
 	flags = local_daif_save();
@@ -43,7 +44,7 @@ static void mdscr_write(u32 mdscr)
 }
 NOKPROBE_SYMBOL(mdscr_write);
 
-static u32 mdscr_read(void)
+static u64 mdscr_read(void)
 {
 	return read_sysreg(mdscr_el1);
 }
@@ -76,10 +77,11 @@ early_param("nodebugmon", early_debug_disable);
  */
 static DEFINE_PER_CPU(int, mde_ref_count);
 static DEFINE_PER_CPU(int, kde_ref_count);
+static DEFINE_PER_CPU(int, embwe_ref_count);
 
 void enable_debug_monitors(enum dbg_active_el el)
 {
-	u32 mdscr, enable = 0;
+	u64 mdscr, enable = 0;
 
 	WARN_ON(preemptible());
 
@@ -90,6 +92,9 @@ void enable_debug_monitors(enum dbg_active_el el)
 	    this_cpu_inc_return(kde_ref_count) == 1)
 		enable |= DBG_MDSCR_KDE;
 
+	if (is_debug_v8p9_enabled() && this_cpu_inc_return(embwe_ref_count) == 1)
+		enable |= DBG_MDSCR_EMBWE;
+
 	if (enable && debug_enabled) {
 		mdscr = mdscr_read();
 		mdscr |= enable;
@@ -100,7 +105,7 @@ NOKPROBE_SYMBOL(enable_debug_monitors);
 
 void disable_debug_monitors(enum dbg_active_el el)
 {
-	u32 mdscr, disable = 0;
+	u64 mdscr, disable = 0;
 
 	WARN_ON(preemptible());
 
@@ -111,6 +116,9 @@ void disable_debug_monitors(enum dbg_active_el el)
 	    this_cpu_dec_return(kde_ref_count) == 0)
 		disable &= ~DBG_MDSCR_KDE;
 
+	if (is_debug_v8p9_enabled() && this_cpu_dec_return(embwe_ref_count) == 0)
+		disable &= ~DBG_MDSCR_EMBWE;
+
 	if (disable) {
 		mdscr = mdscr_read();
 		mdscr &= disable;
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 722ac45f9f7b..30156d732284 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -103,10 +103,40 @@ int hw_breakpoint_slots(int type)
 	WRITE_WB_REG_CASE(OFF, 14, REG, VAL);	\
 	WRITE_WB_REG_CASE(OFF, 15, REG, VAL)
 
+static int set_bank_index(int n)
+{
+	int mdsel_bank;
+	int bank = n / 16, index = n % 16;
+
+	switch (bank) {
+	case 0:
+		mdsel_bank = MDSELR_EL1_BANK_BANK_0;
+		break;
+	case 1:
+		mdsel_bank = MDSELR_EL1_BANK_BANK_1;
+		break;
+	case 2:
+		mdsel_bank = MDSELR_EL1_BANK_BANK_2;
+		break;
+	case 3:
+		mdsel_bank = MDSELR_EL1_BANK_BANK_3;
+		break;
+	default:
+		pr_warn("Unknown register bank %d\n", bank);
+	}
+	preempt_disable();
+	write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
+	isb();
+	return index;
+}
+
 static u64 read_wb_reg(int reg, int n)
 {
 	u64 val = 0;
 
+	if (is_debug_v8p9_enabled())
+		n = set_bank_index(n);
+
 	switch (reg + n) {
 	GEN_READ_WB_REG_CASES(AARCH64_DBG_REG_BVR, AARCH64_DBG_REG_NAME_BVR, val);
 	GEN_READ_WB_REG_CASES(AARCH64_DBG_REG_BCR, AARCH64_DBG_REG_NAME_BCR, val);
@@ -122,6 +152,9 @@ NOKPROBE_SYMBOL(read_wb_reg);
 
 static void write_wb_reg(int reg, int n, u64 val)
 {
+	if (is_debug_v8p9_enabled())
+		n = set_bank_index(n);
+
 	switch (reg + n) {
 	GEN_WRITE_WB_REG_CASES(AARCH64_DBG_REG_BVR, AARCH64_DBG_REG_NAME_BVR, val);
 	GEN_WRITE_WB_REG_CASES(AARCH64_DBG_REG_BCR, AARCH64_DBG_REG_NAME_BCR, val);
-- 
2.25.1

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[kernel test robot]

Hi Anshuman,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on kvmarm/next soc/for-next arm/for-next arm/fixes linus/master v6.12-rc1 next-20241002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/arm64-cpufeature-Add-field-details-for-ID_AA64DFR1_EL1-register/20241001-123752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20241001043602.1116991-3-anshuman.khandual%40arm.com
patch subject: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9
config: arm64-randconfig-002-20241003 (https://download.01.org/0day-ci/archive/20241003/202410030702.9xBCVi6s-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410030702.9xBCVi6s-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410030702.9xBCVi6s-lkp@intel.com/

All errors (new ones prefixed by >>):

>> <instantiation>:3:10: error: expected compatible register, symbol or integer in range [0, 4095]
    cmp x1, #ID_AA64MMFR0_EL1_FGT_FGT2
            ^
   <instantiation>:12:2: note: while in macro instantiation
    __init_el2_fgt2
    ^
   arch/arm64/kernel/head.S:317:2: note: while in macro instantiation
    init_el2_state
    ^
>> <instantiation>:1:5: error: expected absolute expression
   .if (((HDFGWTR2_EL2_nMDSELR_EL1) >> 31) == 0 || ((HDFGWTR2_EL2_nMDSELR_EL1) >> 31) == 0x1ffffffff)
       ^
   <instantiation>:11:2: note: while in macro instantiation
    mov_q x0, HDFGWTR2_EL2_nMDSELR_EL1
    ^
   <instantiation>:12:2: note: while in macro instantiation
    __init_el2_fgt2
    ^
   arch/arm64/kernel/head.S:317:2: note: while in macro instantiation
    init_el2_state
    ^
   <instantiation>:4:6: error: expected absolute expression
    .if (((HDFGWTR2_EL2_nMDSELR_EL1) >> 47) == 0 || ((HDFGWTR2_EL2_nMDSELR_EL1) >> 47) == 0x1ffff)
        ^
   <instantiation>:11:2: note: while in macro instantiation
    mov_q x0, HDFGWTR2_EL2_nMDSELR_EL1
    ^
   <instantiation>:12:2: note: while in macro instantiation
    __init_el2_fgt2
    ^
   arch/arm64/kernel/head.S:317:2: note: while in macro instantiation
    init_el2_state
    ^
>> <instantiation>:1:6: error: expected constant expression
   .inst(0xd5000000|(SYS_HDFGWTR2_EL2)|(.L__gpr_num_x0))
        ^
   <instantiation>:12:2: note: while in macro instantiation
    msr_s SYS_HDFGWTR2_EL2, x0
    ^
   <instantiation>:12:2: note: while in macro instantiation
    __init_el2_fgt2
    ^
   arch/arm64/kernel/head.S:317:2: note: while in macro instantiation
    init_el2_state
    ^
>> <instantiation>:1:6: error: expected constant expression
   .inst(0xd5000000|(SYS_HDFGRTR2_EL2)|(.L__gpr_num_x0))
        ^
   <instantiation>:13:2: note: while in macro instantiation
    msr_s SYS_HDFGRTR2_EL2, x0
    ^
   <instantiation>:12:2: note: while in macro instantiation
    __init_el2_fgt2
    ^
   arch/arm64/kernel/head.S:317:2: note: while in macro instantiation
    init_el2_state
    ^

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[kernel test robot]

Hi Anshuman,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on kvmarm/next soc/for-next arm/for-next arm/fixes linus/master v6.12-rc1 next-20241002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/arm64-cpufeature-Add-field-details-for-ID_AA64DFR1_EL1-register/20241001-123752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20241001043602.1116991-3-anshuman.khandual%40arm.com
patch subject: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9
config: arm64-allnoconfig (https://download.01.org/0day-ci/archive/20241003/202410030737.Mxx9Cvge-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410030737.Mxx9Cvge-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410030737.Mxx9Cvge-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/arm64/kernel/head.S: Assembler messages:
>> arch/arm64/kernel/head.S:550: Error: non-constant expression in ".if" statement
   arch/arm64/kernel/head.S:237:  Info: macro invoked from here
   arch/arm64/kernel/head.S:269:   Info: macro invoked from here
   arch/arm64/kernel/head.S:317:    Info: macro invoked from here
   arch/arm64/kernel/head.S:553: Error: non-constant expression in ".if" statement
   arch/arm64/kernel/head.S:237:  Info: macro invoked from here
   arch/arm64/kernel/head.S:269:   Info: macro invoked from here
   arch/arm64/kernel/head.S:317:    Info: macro invoked from here
>> arch/arm64/kernel/head.S:1094: Error: constant expression required
   arch/arm64/kernel/head.S:238:  Info: macro invoked from here
   arch/arm64/kernel/head.S:269:   Info: macro invoked from here
   arch/arm64/kernel/head.S:317:    Info: macro invoked from here
>> arch/arm64/kernel/head.S:1094: Error: constant expression required
   arch/arm64/kernel/head.S:239:  Info: macro invoked from here
   arch/arm64/kernel/head.S:269:   Info: macro invoked from here
   arch/arm64/kernel/head.S:317:    Info: macro invoked from here
>> arch/arm64/kernel/head.S:317: Error: undefined symbol ID_AA64MMFR0_EL1_FGT_FGT2 used as an immediate value


vim +317 arch/arm64/kernel/head.S

034edabe6cf1d0d Laura Abbott    2014-11-21  246  
034edabe6cf1d0d Laura Abbott    2014-11-21  247  /*
034edabe6cf1d0d Laura Abbott    2014-11-21  248   * end early head section, begin head code that is also used for
034edabe6cf1d0d Laura Abbott    2014-11-21  249   * hotplug and needs to have the same protections as the text region
034edabe6cf1d0d Laura Abbott    2014-11-21  250   */
d54170812ef1c80 Mark Rutland    2023-02-20  251  	.section ".idmap.text","a"
f80fb3a3d50843a Ard Biesheuvel  2016-01-26  252  
9703d9d7f77ce12 Catalin Marinas 2012-03-05  253  /*
ecbb11ab3ebc027 Mark Rutland    2020-11-13  254   * Starting from EL2 or EL1, configure the CPU to execute at the highest
ecbb11ab3ebc027 Mark Rutland    2020-11-13  255   * reachable EL supported by the kernel in a chosen default state. If dropping
ecbb11ab3ebc027 Mark Rutland    2020-11-13  256   * from EL2 to EL1, configure EL2 before configuring EL1.
828e9834e9a5b7e Matthew Leach   2013-10-11  257   *
d87a8e65b510112 Mark Rutland    2020-11-13  258   * Since we cannot always rely on ERET synchronizing writes to sysregs (e.g. if
d87a8e65b510112 Mark Rutland    2020-11-13  259   * SCTLR_ELx.EOS is clear), we place an ISB prior to ERET.
828e9834e9a5b7e Matthew Leach   2013-10-11  260   *
b65e411d6cc2f12 Marc Zyngier    2022-06-30  261   * Returns either BOOT_CPU_MODE_EL1 or BOOT_CPU_MODE_EL2 in x0 if
b65e411d6cc2f12 Marc Zyngier    2022-06-30  262   * booted in EL1 or EL2 respectively, with the top 32 bits containing
b65e411d6cc2f12 Marc Zyngier    2022-06-30  263   * potential context flags. These flags are *not* stored in __boot_cpu_mode.
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  264   *
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  265   * x0: whether we are being called from the primary boot path with the MMU on
9703d9d7f77ce12 Catalin Marinas 2012-03-05  266   */
ecbb11ab3ebc027 Mark Rutland    2020-11-13  267  SYM_FUNC_START(init_kernel_el)
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  268  	mrs	x1, CurrentEL
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  269  	cmp	x1, #CurrentEL_EL2
d87a8e65b510112 Mark Rutland    2020-11-13  270  	b.eq	init_el2
d87a8e65b510112 Mark Rutland    2020-11-13  271  
d87a8e65b510112 Mark Rutland    2020-11-13  272  SYM_INNER_LABEL(init_el1, SYM_L_LOCAL)
31a32b49b80f79c Marc Zyngier    2021-04-08  273  	mov_q	x0, INIT_SCTLR_EL1_MMU_OFF
9d7c13e5dde3127 Ard Biesheuvel  2023-01-11  274  	pre_disable_mmu_workaround
31a32b49b80f79c Marc Zyngier    2021-04-08  275  	msr	sctlr_el1, x0
9cf71728931a407 Matthew Leach   2013-10-11  276  	isb
d87a8e65b510112 Mark Rutland    2020-11-13  277  	mov_q	x0, INIT_PSTATE_EL1
d87a8e65b510112 Mark Rutland    2020-11-13  278  	msr	spsr_el1, x0
d87a8e65b510112 Mark Rutland    2020-11-13  279  	msr	elr_el1, lr
d87a8e65b510112 Mark Rutland    2020-11-13  280  	mov	w0, #BOOT_CPU_MODE_EL1
d87a8e65b510112 Mark Rutland    2020-11-13  281  	eret
9703d9d7f77ce12 Catalin Marinas 2012-03-05  282  
d87a8e65b510112 Mark Rutland    2020-11-13  283  SYM_INNER_LABEL(init_el2, SYM_L_LOCAL)
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  284  	msr	elr_el2, lr
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  285  
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  286  	// clean all HYP code to the PoC if we booted at EL2 with the MMU on
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  287  	cbz	x0, 0f
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  288  	adrp	x0, __hyp_idmap_text_start
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  289  	adr_l	x1, __hyp_text_end
d54170812ef1c80 Mark Rutland    2023-02-20  290  	adr_l	x2, dcache_clean_poc
d54170812ef1c80 Mark Rutland    2023-02-20  291  	blr	x2
34e526cb7d46726 Ard Biesheuvel  2024-04-15  292  
34e526cb7d46726 Ard Biesheuvel  2024-04-15  293  	mov_q	x0, INIT_SCTLR_EL2_MMU_OFF
34e526cb7d46726 Ard Biesheuvel  2024-04-15  294  	pre_disable_mmu_workaround
34e526cb7d46726 Ard Biesheuvel  2024-04-15  295  	msr	sctlr_el2, x0
34e526cb7d46726 Ard Biesheuvel  2024-04-15  296  	isb
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  297  0:
78869f0f0552d03 David Brazdil   2020-12-02  298  	mov_q	x0, HCR_HOST_NVHE_FLAGS
b3320142f3db9b3 Marc Zyngier    2024-03-21  299  
b3320142f3db9b3 Marc Zyngier    2024-03-21  300  	/*
b3320142f3db9b3 Marc Zyngier    2024-03-21  301  	 * Compliant CPUs advertise their VHE-onlyness with
b3320142f3db9b3 Marc Zyngier    2024-03-21  302  	 * ID_AA64MMFR4_EL1.E2H0 < 0. HCR_EL2.E2H can be
b3320142f3db9b3 Marc Zyngier    2024-03-21  303  	 * RES1 in that case. Publish the E2H bit early so that
b3320142f3db9b3 Marc Zyngier    2024-03-21  304  	 * it can be picked up by the init_el2_state macro.
b3320142f3db9b3 Marc Zyngier    2024-03-21  305  	 *
b3320142f3db9b3 Marc Zyngier    2024-03-21  306  	 * Fruity CPUs seem to have HCR_EL2.E2H set to RAO/WI, but
b3320142f3db9b3 Marc Zyngier    2024-03-21  307  	 * don't advertise it (they predate this relaxation).
b3320142f3db9b3 Marc Zyngier    2024-03-21  308  	 */
b3320142f3db9b3 Marc Zyngier    2024-03-21  309  	mrs_s	x1, SYS_ID_AA64MMFR4_EL1
b3320142f3db9b3 Marc Zyngier    2024-03-21  310  	tbz	x1, #(ID_AA64MMFR4_EL1_E2H0_SHIFT + ID_AA64MMFR4_EL1_E2H0_WIDTH - 1), 1f
b3320142f3db9b3 Marc Zyngier    2024-03-21  311  
b3320142f3db9b3 Marc Zyngier    2024-03-21  312  	orr	x0, x0, #HCR_E2H
b3320142f3db9b3 Marc Zyngier    2024-03-21  313  1:
78869f0f0552d03 David Brazdil   2020-12-02  314  	msr	hcr_el2, x0
22043a3c082a584 Dave Martin     2017-10-31  315  	isb
78869f0f0552d03 David Brazdil   2020-12-02  316  
e2df464173f0b58 Marc Zyngier    2021-02-08 @317  	init_el2_state
22043a3c082a584 Dave Martin     2017-10-31  318  
712c6ff4dba4917 Marc Zyngier    2012-10-19  319  	/* Hypervisor stub */
78869f0f0552d03 David Brazdil   2020-12-02  320  	adr_l	x0, __hyp_stub_vectors
712c6ff4dba4917 Marc Zyngier    2012-10-19  321  	msr	vbar_el2, x0
d87a8e65b510112 Mark Rutland    2020-11-13  322  	isb
78869f0f0552d03 David Brazdil   2020-12-02  323  
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  324  	mov_q	x1, INIT_SCTLR_EL1_MMU_OFF
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  325  
31a32b49b80f79c Marc Zyngier    2021-04-08  326  	mrs	x0, hcr_el2
31a32b49b80f79c Marc Zyngier    2021-04-08  327  	and	x0, x0, #HCR_E2H
3944382fa6f22b5 Marc Zyngier    2024-01-22  328  	cbz	x0, 2f
b3320142f3db9b3 Marc Zyngier    2024-03-21  329  
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  330  	/* Set a sane SCTLR_EL1, the VHE way */
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  331  	msr_s	SYS_SCTLR_EL12, x1
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  332  	mov	x2, #BOOT_CPU_FLAG_E2H
3944382fa6f22b5 Marc Zyngier    2024-01-22  333  	b	3f
31a32b49b80f79c Marc Zyngier    2021-04-08  334  
3944382fa6f22b5 Marc Zyngier    2024-01-22  335  2:
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  336  	msr	sctlr_el1, x1
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  337  	mov	x2, xzr
3944382fa6f22b5 Marc Zyngier    2024-01-22  338  3:
1700f89cb99aae1 Marc Zyngier    2023-06-14  339  	__init_el2_nvhe_prepare_eret
1700f89cb99aae1 Marc Zyngier    2023-06-14  340  
d87a8e65b510112 Mark Rutland    2020-11-13  341  	mov	w0, #BOOT_CPU_MODE_EL2
ae4b7e38e9a9479 Marc Zyngier    2022-06-30  342  	orr	x0, x0, x2
9703d9d7f77ce12 Catalin Marinas 2012-03-05  343  	eret
ecbb11ab3ebc027 Mark Rutland    2020-11-13  344  SYM_FUNC_END(init_kernel_el)
9703d9d7f77ce12 Catalin Marinas 2012-03-05  345  
9703d9d7f77ce12 Catalin Marinas 2012-03-05  346  	/*
9703d9d7f77ce12 Catalin Marinas 2012-03-05  347  	 * This provides a "holding pen" for platforms to hold all secondary
9703d9d7f77ce12 Catalin Marinas 2012-03-05  348  	 * cores are held until we're ready for them to initialise.
9703d9d7f77ce12 Catalin Marinas 2012-03-05  349  	 */
c63d9f82db94399 Mark Brown      2020-02-18  350  SYM_FUNC_START(secondary_holding_pen)
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  351  	mov	x0, xzr
ecbb11ab3ebc027 Mark Rutland    2020-11-13  352  	bl	init_kernel_el			// w0=cpu_boot_mode
005e12676af09a3 Ard Biesheuvel  2022-06-24  353  	mrs	x2, mpidr_el1
b03cc885328e3c0 Ard Biesheuvel  2016-04-18  354  	mov_q	x1, MPIDR_HWID_BITMASK
005e12676af09a3 Ard Biesheuvel  2022-06-24  355  	and	x2, x2, x1
b1c98297fe0c6e2 Ard Biesheuvel  2015-03-10  356  	adr_l	x3, secondary_holding_pen_release
9703d9d7f77ce12 Catalin Marinas 2012-03-05  357  pen:	ldr	x4, [x3]
005e12676af09a3 Ard Biesheuvel  2022-06-24  358  	cmp	x4, x2
9703d9d7f77ce12 Catalin Marinas 2012-03-05  359  	b.eq	secondary_startup
9703d9d7f77ce12 Catalin Marinas 2012-03-05  360  	wfe
9703d9d7f77ce12 Catalin Marinas 2012-03-05  361  	b	pen
c63d9f82db94399 Mark Brown      2020-02-18  362  SYM_FUNC_END(secondary_holding_pen)
652af8997993540 Mark Rutland    2013-10-24  363  
652af8997993540 Mark Rutland    2013-10-24  364  	/*
652af8997993540 Mark Rutland    2013-10-24  365  	 * Secondary entry point that jumps straight into the kernel. Only to
652af8997993540 Mark Rutland    2013-10-24  366  	 * be used where CPUs are brought online dynamically by the kernel.
652af8997993540 Mark Rutland    2013-10-24  367  	 */
c63d9f82db94399 Mark Brown      2020-02-18  368  SYM_FUNC_START(secondary_entry)
3dcf60bbfd284e5 Ard Biesheuvel  2023-01-11  369  	mov	x0, xzr
ecbb11ab3ebc027 Mark Rutland    2020-11-13  370  	bl	init_kernel_el			// w0=cpu_boot_mode
652af8997993540 Mark Rutland    2013-10-24  371  	b	secondary_startup
c63d9f82db94399 Mark Brown      2020-02-18  372  SYM_FUNC_END(secondary_entry)
9703d9d7f77ce12 Catalin Marinas 2012-03-05  373  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[kernel test robot]

Hi Anshuman,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on kvmarm/next soc/for-next arm/for-next arm/fixes linus/master v6.12-rc1 next-20241002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/arm64-cpufeature-Add-field-details-for-ID_AA64DFR1_EL1-register/20241001-123752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20241001043602.1116991-4-anshuman.khandual%40arm.com
patch subject: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9
config: arm64-randconfig-004-20241003 (https://download.01.org/0day-ci/archive/20241003/202410030700.kZSan6G6-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410030700.kZSan6G6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410030700.kZSan6G6-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/arm64/kernel/hw_breakpoint.c: In function 'set_bank_index':
>> arch/arm64/kernel/hw_breakpoint.c:113:30: error: 'MDSELR_EL1_BANK_BANK_0' undeclared (first use in this function)
     113 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_0;
         |                              ^~~~~~~~~~~~~~~~~~~~~~
   arch/arm64/kernel/hw_breakpoint.c:113:30: note: each undeclared identifier is reported only once for each function it appears in
>> arch/arm64/kernel/hw_breakpoint.c:116:30: error: 'MDSELR_EL1_BANK_BANK_1' undeclared (first use in this function)
     116 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_1;
         |                              ^~~~~~~~~~~~~~~~~~~~~~
>> arch/arm64/kernel/hw_breakpoint.c:119:30: error: 'MDSELR_EL1_BANK_BANK_2' undeclared (first use in this function)
     119 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_2;
         |                              ^~~~~~~~~~~~~~~~~~~~~~
>> arch/arm64/kernel/hw_breakpoint.c:122:30: error: 'MDSELR_EL1_BANK_BANK_3' undeclared (first use in this function)
     122 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_3;
         |                              ^~~~~~~~~~~~~~~~~~~~~~
   In file included from arch/arm64/include/asm/cputype.h:226,
                    from arch/arm64/include/asm/cache.h:43,
                    from include/linux/cache.h:6,
                    from include/linux/time.h:5,
                    from include/linux/compat.h:10,
                    from arch/arm64/kernel/hw_breakpoint.c:12:
>> arch/arm64/kernel/hw_breakpoint.c:128:38: error: 'MDSELR_EL1_BANK_SHIFT' undeclared (first use in this function); did you mean 'CSSELR_EL1_InD_SHIFT'?
     128 |         write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
         |                                      ^~~~~~~~~~~~~~~~~~~~~
   arch/arm64/include/asm/sysreg.h:1168:27: note: in definition of macro 'write_sysreg_s'
    1168 |         u64 __val = (u64)(v);                                           \
         |                           ^
>> arch/arm64/kernel/hw_breakpoint.c:128:61: error: 'SYS_MDSELR_EL1' undeclared (first use in this function); did you mean 'SYS_MDSCR_EL1'?
     128 |         write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
         |                                                             ^~~~~~~~~~~~~~
   arch/arm64/include/asm/sysreg.h:1169:46: note: in definition of macro 'write_sysreg_s'
    1169 |         u32 __maybe_unused __check_r = (u32)(r);                        \
         |                                              ^


vim +/MDSELR_EL1_BANK_BANK_0 +113 arch/arm64/kernel/hw_breakpoint.c

    59	
    60	#define READ_WB_REG_CASE(OFF, N, REG, VAL)	\
    61		case (OFF + N):				\
    62			AARCH64_DBG_READ(N, REG, VAL);	\
    63			break
    64	
    65	#define WRITE_WB_REG_CASE(OFF, N, REG, VAL)	\
    66		case (OFF + N):				\
    67			AARCH64_DBG_WRITE(N, REG, VAL);	\
    68			break
    69	
    70	#define GEN_READ_WB_REG_CASES(OFF, REG, VAL)	\
    71		READ_WB_REG_CASE(OFF,  0, REG, VAL);	\
    72		READ_WB_REG_CASE(OFF,  1, REG, VAL);	\
    73		READ_WB_REG_CASE(OFF,  2, REG, VAL);	\
    74		READ_WB_REG_CASE(OFF,  3, REG, VAL);	\
    75		READ_WB_REG_CASE(OFF,  4, REG, VAL);	\
    76		READ_WB_REG_CASE(OFF,  5, REG, VAL);	\
    77		READ_WB_REG_CASE(OFF,  6, REG, VAL);	\
    78		READ_WB_REG_CASE(OFF,  7, REG, VAL);	\
    79		READ_WB_REG_CASE(OFF,  8, REG, VAL);	\
    80		READ_WB_REG_CASE(OFF,  9, REG, VAL);	\
    81		READ_WB_REG_CASE(OFF, 10, REG, VAL);	\
    82		READ_WB_REG_CASE(OFF, 11, REG, VAL);	\
    83		READ_WB_REG_CASE(OFF, 12, REG, VAL);	\
    84		READ_WB_REG_CASE(OFF, 13, REG, VAL);	\
    85		READ_WB_REG_CASE(OFF, 14, REG, VAL);	\
    86		READ_WB_REG_CASE(OFF, 15, REG, VAL)
    87	
    88	#define GEN_WRITE_WB_REG_CASES(OFF, REG, VAL)	\
    89		WRITE_WB_REG_CASE(OFF,  0, REG, VAL);	\
    90		WRITE_WB_REG_CASE(OFF,  1, REG, VAL);	\
    91		WRITE_WB_REG_CASE(OFF,  2, REG, VAL);	\
    92		WRITE_WB_REG_CASE(OFF,  3, REG, VAL);	\
    93		WRITE_WB_REG_CASE(OFF,  4, REG, VAL);	\
    94		WRITE_WB_REG_CASE(OFF,  5, REG, VAL);	\
    95		WRITE_WB_REG_CASE(OFF,  6, REG, VAL);	\
    96		WRITE_WB_REG_CASE(OFF,  7, REG, VAL);	\
    97		WRITE_WB_REG_CASE(OFF,  8, REG, VAL);	\
    98		WRITE_WB_REG_CASE(OFF,  9, REG, VAL);	\
    99		WRITE_WB_REG_CASE(OFF, 10, REG, VAL);	\
   100		WRITE_WB_REG_CASE(OFF, 11, REG, VAL);	\
   101		WRITE_WB_REG_CASE(OFF, 12, REG, VAL);	\
   102		WRITE_WB_REG_CASE(OFF, 13, REG, VAL);	\
   103		WRITE_WB_REG_CASE(OFF, 14, REG, VAL);	\
   104		WRITE_WB_REG_CASE(OFF, 15, REG, VAL)
   105	
   106	static int set_bank_index(int n)
   107	{
   108		int mdsel_bank;
   109		int bank = n / 16, index = n % 16;
   110	
   111		switch (bank) {
   112		case 0:
 > 113			mdsel_bank = MDSELR_EL1_BANK_BANK_0;
   114			break;
   115		case 1:
 > 116			mdsel_bank = MDSELR_EL1_BANK_BANK_1;
   117			break;
   118		case 2:
 > 119			mdsel_bank = MDSELR_EL1_BANK_BANK_2;
   120			break;
   121		case 3:
 > 122			mdsel_bank = MDSELR_EL1_BANK_BANK_3;
   123			break;
   124		default:
   125			pr_warn("Unknown register bank %d\n", bank);
   126		}
   127		preempt_disable();
 > 128		write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
   129		isb();
   130		return index;
   131	}
   132	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Anshuman Khandual]

On 10/3/24 05:06, kernel test robot wrote:
> Hi Anshuman,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on arm64/for-next/core]
> [also build test ERROR on kvmarm/next soc/for-next arm/for-next arm/fixes linus/master v6.12-rc1 next-20241002]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/arm64-cpufeature-Add-field-details-for-ID_AA64DFR1_EL1-register/20241001-123752
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
> patch link:    https://lore.kernel.org/r/20241001043602.1116991-4-anshuman.khandual%40arm.com
> patch subject: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9
> config: arm64-randconfig-004-20241003 (https://download.01.org/0day-ci/archive/20241003/202410030700.kZSan6G6-lkp@intel.com/config)
> compiler: aarch64-linux-gcc (GCC) 14.1.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410030700.kZSan6G6-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410030700.kZSan6G6-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    arch/arm64/kernel/hw_breakpoint.c: In function 'set_bank_index':
>>> arch/arm64/kernel/hw_breakpoint.c:113:30: error: 'MDSELR_EL1_BANK_BANK_0' undeclared (first use in this function)
>      113 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_0;
>          |                              ^~~~~~~~~~~~~~~~~~~~~~
>    arch/arm64/kernel/hw_breakpoint.c:113:30: note: each undeclared identifier is reported only once for each function it appears in
>>> arch/arm64/kernel/hw_breakpoint.c:116:30: error: 'MDSELR_EL1_BANK_BANK_1' undeclared (first use in this function)
>      116 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_1;
>          |                              ^~~~~~~~~~~~~~~~~~~~~~
>>> arch/arm64/kernel/hw_breakpoint.c:119:30: error: 'MDSELR_EL1_BANK_BANK_2' undeclared (first use in this function)
>      119 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_2;
>          |                              ^~~~~~~~~~~~~~~~~~~~~~
>>> arch/arm64/kernel/hw_breakpoint.c:122:30: error: 'MDSELR_EL1_BANK_BANK_3' undeclared (first use in this function)
>      122 |                 mdsel_bank = MDSELR_EL1_BANK_BANK_3;
>          |                              ^~~~~~~~~~~~~~~~~~~~~~
>    In file included from arch/arm64/include/asm/cputype.h:226,
>                     from arch/arm64/include/asm/cache.h:43,
>                     from include/linux/cache.h:6,
>                     from include/linux/time.h:5,
>                     from include/linux/compat.h:10,
>                     from arch/arm64/kernel/hw_breakpoint.c:12:
>>> arch/arm64/kernel/hw_breakpoint.c:128:38: error: 'MDSELR_EL1_BANK_SHIFT' undeclared (first use in this function); did you mean 'CSSELR_EL1_InD_SHIFT'?
>      128 |         write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
>          |                                      ^~~~~~~~~~~~~~~~~~~~~
>    arch/arm64/include/asm/sysreg.h:1168:27: note: in definition of macro 'write_sysreg_s'
>     1168 |         u64 __val = (u64)(v);                                           \
>          |                           ^
>>> arch/arm64/kernel/hw_breakpoint.c:128:61: error: 'SYS_MDSELR_EL1' undeclared (first use in this function); did you mean 'SYS_MDSCR_EL1'?
>      128 |         write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
>          |                                                             ^~~~~~~~~~~~~~
>    arch/arm64/include/asm/sysreg.h:1169:46: note: in definition of macro 'write_sysreg_s'
>     1169 |         u32 __maybe_unused __check_r = (u32)(r);                        \
>          |                                              ^
> 
> 
> vim +/MDSELR_EL1_BANK_BANK_0 +113 arch/arm64/kernel/hw_breakpoint.c
> 
>     59	
>     60	#define READ_WB_REG_CASE(OFF, N, REG, VAL)	\
>     61		case (OFF + N):				\
>     62			AARCH64_DBG_READ(N, REG, VAL);	\
>     63			break
>     64	
>     65	#define WRITE_WB_REG_CASE(OFF, N, REG, VAL)	\
>     66		case (OFF + N):				\
>     67			AARCH64_DBG_WRITE(N, REG, VAL);	\
>     68			break
>     69	
>     70	#define GEN_READ_WB_REG_CASES(OFF, REG, VAL)	\
>     71		READ_WB_REG_CASE(OFF,  0, REG, VAL);	\
>     72		READ_WB_REG_CASE(OFF,  1, REG, VAL);	\
>     73		READ_WB_REG_CASE(OFF,  2, REG, VAL);	\
>     74		READ_WB_REG_CASE(OFF,  3, REG, VAL);	\
>     75		READ_WB_REG_CASE(OFF,  4, REG, VAL);	\
>     76		READ_WB_REG_CASE(OFF,  5, REG, VAL);	\
>     77		READ_WB_REG_CASE(OFF,  6, REG, VAL);	\
>     78		READ_WB_REG_CASE(OFF,  7, REG, VAL);	\
>     79		READ_WB_REG_CASE(OFF,  8, REG, VAL);	\
>     80		READ_WB_REG_CASE(OFF,  9, REG, VAL);	\
>     81		READ_WB_REG_CASE(OFF, 10, REG, VAL);	\
>     82		READ_WB_REG_CASE(OFF, 11, REG, VAL);	\
>     83		READ_WB_REG_CASE(OFF, 12, REG, VAL);	\
>     84		READ_WB_REG_CASE(OFF, 13, REG, VAL);	\
>     85		READ_WB_REG_CASE(OFF, 14, REG, VAL);	\
>     86		READ_WB_REG_CASE(OFF, 15, REG, VAL)
>     87	
>     88	#define GEN_WRITE_WB_REG_CASES(OFF, REG, VAL)	\
>     89		WRITE_WB_REG_CASE(OFF,  0, REG, VAL);	\
>     90		WRITE_WB_REG_CASE(OFF,  1, REG, VAL);	\
>     91		WRITE_WB_REG_CASE(OFF,  2, REG, VAL);	\
>     92		WRITE_WB_REG_CASE(OFF,  3, REG, VAL);	\
>     93		WRITE_WB_REG_CASE(OFF,  4, REG, VAL);	\
>     94		WRITE_WB_REG_CASE(OFF,  5, REG, VAL);	\
>     95		WRITE_WB_REG_CASE(OFF,  6, REG, VAL);	\
>     96		WRITE_WB_REG_CASE(OFF,  7, REG, VAL);	\
>     97		WRITE_WB_REG_CASE(OFF,  8, REG, VAL);	\
>     98		WRITE_WB_REG_CASE(OFF,  9, REG, VAL);	\
>     99		WRITE_WB_REG_CASE(OFF, 10, REG, VAL);	\
>    100		WRITE_WB_REG_CASE(OFF, 11, REG, VAL);	\
>    101		WRITE_WB_REG_CASE(OFF, 12, REG, VAL);	\
>    102		WRITE_WB_REG_CASE(OFF, 13, REG, VAL);	\
>    103		WRITE_WB_REG_CASE(OFF, 14, REG, VAL);	\
>    104		WRITE_WB_REG_CASE(OFF, 15, REG, VAL)
>    105	
>    106	static int set_bank_index(int n)
>    107	{
>    108		int mdsel_bank;
>    109		int bank = n / 16, index = n % 16;
>    110	
>    111		switch (bank) {
>    112		case 0:
>  > 113			mdsel_bank = MDSELR_EL1_BANK_BANK_0;
>    114			break;
>    115		case 1:
>  > 116			mdsel_bank = MDSELR_EL1_BANK_BANK_1;
>    117			break;
>    118		case 2:
>  > 119			mdsel_bank = MDSELR_EL1_BANK_BANK_2;
>    120			break;
>    121		case 3:
>  > 122			mdsel_bank = MDSELR_EL1_BANK_BANK_3;
>    123			break;
>    124		default:
>    125			pr_warn("Unknown register bank %d\n", bank);
>    126		}
>    127		preempt_disable();
>  > 128		write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
>    129		isb();
>    130		return index;
>    131	}
>    132	
> 

This build failure and also the other two on the series here are false positives
caused by non-availability of used register field definitions which are provided
via the dependent KVM FEAT_FGT2 FGU series.

Re: [PATCH 0/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Anshuman Khandual]

On 10/1/24 10:05, Anshuman Khandual wrote:
> This series enables FEAT_Debugv8p9 thus extending breakpoint and watchpoint
> support upto 64. This series is based on v6.12-rc1 after applying the KVM
> FEAT_FGT2 FGU series.
> 
> https://lore.kernel.org/all/20241001024356.1096072-1-anshuman.khandual@arm.com/
> 
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: kvmarm@lists.linux.dev
> Cc: linux-kernel@vger.kernel.org
> 
> Changes in V1:
> 
> - Changed FTR_STRICT to FTR_NONSTRICT for the following ID_AA64DFR1_EL1
>   register fields - ABL_CMPs, DPFZS, PMICNTR, CTX_CMPs, WRPs and BRPs
> 
> Changes in RFC V2:
> 
> https://lore.kernel.org/linux-arm-kernel/20240620092607.267132-1-anshuman.khandual@arm.com/
> 
> - This series has been split from RFC V1 dealing only with arm64 breakpoints
> - Restored back DBG_MDSCR_MASK definition (unrelated change)
> - Added preempt_disable()/enable() blocks between selecting banks and registers
> 
> Changes in RFC:
> 
> https://lore.kernel.org/all/20240405080008.1225223-1-anshuman.khandual@arm.com/
> 
> Anshuman Khandual (3):
>   arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register
>   arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9
>   arm64/hw_breakpoint: Enable FEAT_Debugv8p9
> 
>  Documentation/arch/arm64/booting.rst    | 19 ++++++++++
>  arch/arm64/include/asm/debug-monitors.h |  1 +
>  arch/arm64/include/asm/el2_setup.h      | 27 +++++++++++++
>  arch/arm64/include/asm/hw_breakpoint.h  | 50 ++++++++++++++++++++-----
>  arch/arm64/include/asm/kvm_arm.h        |  1 +
>  arch/arm64/kernel/cpufeature.c          | 21 ++++++++---
>  arch/arm64/kernel/debug-monitors.c      | 16 ++++++--
>  arch/arm64/kernel/hw_breakpoint.c       | 33 ++++++++++++++++
>  8 files changed, 149 insertions(+), 19 deletions(-)
> 

Hello,

Gentle ping ! Any updates on this series ? Although I realize that this series is
dependent on the other FEAT_FGT2 series [1] for MDSELR_EL1 access handling in KVM
and sysreg definitions, but just wondering if proposed breakpoint and watchpoint
changes are okay or something needs to be changed. Thank you.

[1] https://lore.kernel.org/all/20241001024356.1096072-1-anshuman.khandual@arm.com/

- Anshuman

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Mark Rutland]

On Tue, Oct 01, 2024 at 10:06:02AM +0530, Anshuman Khandual wrote:
> Currently there can be maximum 16 breakpoints, and 16 watchpoints available
> on a given platform - as detected from ID_AA64DFR0_EL1.[BRPs|WRPs] register
> fields. But these breakpoint, and watchpoints can be extended further up to
> 64 via a new arch feature FEAT_Debugv8p9.
> 
> This first enables banked access for the breakpoint and watchpoint register
> set via MDSELR_EL1, extended exceptions via MDSCR_EL1.EMBWE and determining
> available breakpoints and watchpoints in the platform from ID_AA64DFR1_EL1,
> when FEAT_Debugv8p9 is enabled.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/include/asm/debug-monitors.h |  1 +
>  arch/arm64/include/asm/hw_breakpoint.h  | 50 ++++++++++++++++++++-----
>  arch/arm64/kernel/debug-monitors.c      | 16 ++++++--
>  arch/arm64/kernel/hw_breakpoint.c       | 33 ++++++++++++++++
>  4 files changed, 86 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 13d437bcbf58..a14097673ae0 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -20,6 +20,7 @@
>  #define DBG_MDSCR_KDE		(1 << 13)
>  #define DBG_MDSCR_MDE		(1 << 15)
>  #define DBG_MDSCR_MASK		~(DBG_MDSCR_KDE | DBG_MDSCR_MDE)
> +#define DBG_MDSCR_EMBWE		(1UL << 32)
>  
>  #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)
>  
> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
> index bd81cf17744a..362c4d4a64ac 100644
> --- a/arch/arm64/include/asm/hw_breakpoint.h
> +++ b/arch/arm64/include/asm/hw_breakpoint.h
> @@ -79,8 +79,8 @@ static inline void decode_ctrl_reg(u32 reg,
>   * Limits.
>   * Changing these will require modifications to the register accessors.
>   */
> -#define ARM_MAX_BRP		16
> -#define ARM_MAX_WRP		16
> +#define ARM_MAX_BRP		64
> +#define ARM_MAX_WRP		64
>  
>  /* Virtual debug register bases. */
>  #define AARCH64_DBG_REG_BVR	0
> @@ -94,13 +94,25 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define AARCH64_DBG_REG_NAME_WVR	wvr
>  #define AARCH64_DBG_REG_NAME_WCR	wcr
>  
> +static inline bool is_debug_v8p9_enabled(void)
> +{
> +	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> +	int dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
> +
> +	return dver == ID_AA64DFR0_EL1_DebugVer_V8P9;
> +}
> +
>  /* Accessor macros for the debug registers. */
>  #define AARCH64_DBG_READ(N, REG, VAL) do {\
>  	VAL = read_sysreg(dbg##REG##N##_el1);\
> +	if (is_debug_v8p9_enabled())	\
> +		preempt_enable();	\
>  } while (0)
>  
>  #define AARCH64_DBG_WRITE(N, REG, VAL) do {\
>  	write_sysreg(VAL, dbg##REG##N##_el1);\
> +	if (is_debug_v8p9_enabled())	\
> +		preempt_enable();	\
>  } while (0)

Without looking any further in this patch, this is clearly the wrong
level of abstraction. Any disable/enable of preemption should be clearly
balanced in a caller rather than half of that being hidden away in a
low-level primitive.

Wherever this lives it needs a comment explaining what it is doing and
why. I assume this is intended to protect the bank in sequences like:

	MSR	MDSELR, <...>
	ISB
	MRS	<..._, BANKED_REGISTER

... but is theat suffucient for mutual exclusion against
exception handlers, or does that come from somewhere else?

>  struct task_struct;
> @@ -138,19 +150,37 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>  /* Determine number of BRP registers available. */
>  static inline int get_num_brps(void)
>  {
> -	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> -	return 1 +
> -		cpuid_feature_extract_unsigned_field(dfr0,
> -						ID_AA64DFR0_EL1_BRPs_SHIFT);
> +	u64 dfr0, dfr1;
> +	int dver, brps;
> +
> +	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> +	dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
> +	if (dver == ID_AA64DFR0_EL1_DebugVer_V8P9) {
> +		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
> +		brps = cpuid_feature_extract_unsigned_field_width(dfr1,
> +								  ID_AA64DFR1_EL1_BRPs_SHIFT, 8);
> +	} else {
> +		brps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRPs_SHIFT);
> +	}
> +	return 1 + brps;
>  }
>  
>  /* Determine number of WRP registers available. */
>  static inline int get_num_wrps(void)
>  {
> -	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> -	return 1 +
> -		cpuid_feature_extract_unsigned_field(dfr0,
> -						ID_AA64DFR0_EL1_WRPs_SHIFT);
> +	u64 dfr0, dfr1;
> +	int dver, wrps;
> +
> +	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> +	dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
> +	if (dver == ID_AA64DFR0_EL1_DebugVer_V8P9) {
> +		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
> +		wrps = cpuid_feature_extract_unsigned_field_width(dfr1,
> +								  ID_AA64DFR1_EL1_WRPs_SHIFT, 8);
> +	} else {
> +		wrps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_WRPs_SHIFT);
> +	}
> +	return 1 + wrps;
>  }
>  
>  #ifdef CONFIG_CPU_PM
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 024a7b245056..af643c935f2e 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -23,6 +23,7 @@
>  #include <asm/debug-monitors.h>
>  #include <asm/system_misc.h>
>  #include <asm/traps.h>
> +#include <asm/hw_breakpoint.h>

Nit: these are ordered alphabetically, please keep them that way.

>  
>  /* Determine debug architecture. */
>  u8 debug_monitors_arch(void)
> @@ -34,7 +35,7 @@ u8 debug_monitors_arch(void)
>  /*
>   * MDSCR access routines.
>   */
> -static void mdscr_write(u32 mdscr)
> +static void mdscr_write(u64 mdscr)
>  {
>  	unsigned long flags;
>  	flags = local_daif_save();
> @@ -43,7 +44,7 @@ static void mdscr_write(u32 mdscr)
>  }
>  NOKPROBE_SYMBOL(mdscr_write);
>  
> -static u32 mdscr_read(void)
> +static u64 mdscr_read(void)
>  {
>  	return read_sysreg(mdscr_el1);
>  }
> @@ -76,10 +77,11 @@ early_param("nodebugmon", early_debug_disable);
>   */
>  static DEFINE_PER_CPU(int, mde_ref_count);
>  static DEFINE_PER_CPU(int, kde_ref_count);
> +static DEFINE_PER_CPU(int, embwe_ref_count);

We have refcounting for MDE and KDE because they enable debug exceptions
to be taken (and e.g. require a hypervisor to do more work when they're
enabled), but AFAICT that's not true for EMBWE.

Do we need to refcount EMBWE?

>  void enable_debug_monitors(enum dbg_active_el el)
>  {
> -	u32 mdscr, enable = 0;
> +	u64 mdscr, enable = 0;
>  
>  	WARN_ON(preemptible());
>  
> @@ -90,6 +92,9 @@ void enable_debug_monitors(enum dbg_active_el el)
>  	    this_cpu_inc_return(kde_ref_count) == 1)
>  		enable |= DBG_MDSCR_KDE;
>  
> +	if (is_debug_v8p9_enabled() && this_cpu_inc_return(embwe_ref_count) == 1)
> +		enable |= DBG_MDSCR_EMBWE;

... which suggests that this could simplified to be:

	if (is_debug_v8p9_enabled())
		enable != DBG_MDSCR_EMBWE;

... and likewise below, unless I'm missing some reason why we must
refcount this?

> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index 722ac45f9f7b..30156d732284 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -103,10 +103,40 @@ int hw_breakpoint_slots(int type)
>  	WRITE_WB_REG_CASE(OFF, 14, REG, VAL);	\
>  	WRITE_WB_REG_CASE(OFF, 15, REG, VAL)
>  
> +static int set_bank_index(int n)
> +{
> +	int mdsel_bank;
> +	int bank = n / 16, index = n % 16;
> +
> +	switch (bank) {
> +	case 0:
> +		mdsel_bank = MDSELR_EL1_BANK_BANK_0;
> +		break;
> +	case 1:
> +		mdsel_bank = MDSELR_EL1_BANK_BANK_1;
> +		break;
> +	case 2:
> +		mdsel_bank = MDSELR_EL1_BANK_BANK_2;
> +		break;
> +	case 3:
> +		mdsel_bank = MDSELR_EL1_BANK_BANK_3;
> +		break;

Since this is a trivial mapping, do we actually need the switch?

> +	default:
> +		pr_warn("Unknown register bank %d\n", bank);
> +	}
> +	preempt_disable();
> +	write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
> +	isb();
> +	return index;
> +}
> +
>  static u64 read_wb_reg(int reg, int n)
>  {
>  	u64 val = 0;
>  
> +	if (is_debug_v8p9_enabled())
> +		n = set_bank_index(n);
> +
>  	switch (reg + n) {
>  	GEN_READ_WB_REG_CASES(AARCH64_DBG_REG_BVR, AARCH64_DBG_REG_NAME_BVR, val);
>  	GEN_READ_WB_REG_CASES(AARCH64_DBG_REG_BCR, AARCH64_DBG_REG_NAME_BCR, val);

As above, this would be better as something like:

	// rename the existing read_wb_reg(), unchanged
	static u64 __read_wb_reg(int reg, int n);

	static u64 read_wb_reg(int reg, int n)
	{
		u64 val;

		if (!is_debug_v8p9_enabled())
			return __read_wb_reg(reg, n);

		/*
		 * TODO: explain here
		 */
		preempt_disable();
		write_sysreg_s(...); // MDSELR
		isb();
		val = __read_wb_reg(reg, idx_within_bank);
		preempt_enable();

		return val;
	}

... or:

	static u64 read_wb_reg(int reg, int n)
	{
		u64 val;

		if (is_debug_v8p9_enabled()) {
			/*
			 * TODO: explain here
			 */
			preempt_disable();
			write_sysreg_s(...); // MDSELR
			isb();
			val = __read_wb_reg(reg, idx_within_bank);
			preempt_enable();
		} else {
			val = __read_wb_reg(reg, n);
		}

		return val;
	}

... which is more lines but *vastly* clearer. 

Likewise for the write case.

Mark.

Re: [PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Mark Rutland]

On Tue, Oct 01, 2024 at 10:06:00AM +0530, Anshuman Khandual wrote:
> This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
> ftr_raz[] array which is now redundant. These register fields will be used
> to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
> later.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> cc: Mark Brown <broonie@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  arch/arm64/kernel/cpufeature.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 718728a85430..bd4d85f5dd92 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -530,6 +530,21 @@ static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>  	ARM64_FTR_END,
>  };
>  
> +static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_EBEP_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ITE_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABLE_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_PMICNTR_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SPMU_SHIFT, 4, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_BRPs_SHIFT, 8, 0),
> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SYSPMUID_SHIFT, 8, 0),
> +	ARM64_FTR_END,
> +};
> +

Is there some general principle that has been applied here? e.g. is this
STRICT unless we know of variation in practice?

e.g. it seems a bit odd that ABLE cannot vary while the number of
breakpoints can...

I suspect we will see systems with mismatched EBEP too, but maybe I'm
wrong.

Mark.

>  static const struct arm64_ftr_bits ftr_mvfr0[] = {
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, MVFR0_EL1_FPRound_SHIFT, 4, 0),
>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, MVFR0_EL1_FPShVec_SHIFT, 4, 0),
> @@ -708,10 +723,6 @@ static const struct arm64_ftr_bits ftr_single32[] = {
>  	ARM64_FTR_END,
>  };
>  
> -static const struct arm64_ftr_bits ftr_raz[] = {
> -	ARM64_FTR_END,
> -};
> -
>  #define __ARM64_FTR_REG_OVERRIDE(id_str, id, table, ovr) {	\
>  		.sys_id = id,					\
>  		.reg = 	&(struct arm64_ftr_reg){		\
> @@ -784,7 +795,7 @@ static const struct __ftr_reg_entry {
>  
>  	/* Op1 = 0, CRn = 0, CRm = 5 */
>  	ARM64_FTR_REG(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0),
> -	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_raz),
> +	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_id_aa64dfr1),
>  
>  	/* Op1 = 0, CRn = 0, CRm = 6 */
>  	ARM64_FTR_REG(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0),
> -- 
> 2.25.1
> 

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Mark Rutland]

On Tue, Oct 01, 2024 at 10:06:01AM +0530, Anshuman Khandual wrote:
> Fine grained trap control for MDSELR_EL1 register needs to be configured in
> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2
> is also present. This adds a new helper __init_el2_fgt2() initializing this
> new FEAT_FGT2 based fine grained registers.
> 
> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and
> watchpoint exceptions when kernel enters at EL1, but EL2 is also present.
> This updates __init_el2_debug() as required for FEAT_Debugv8p9.
> 
> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements.

[...]

> +  For CPUs with FEAT_Debugv8p9 extension present:
> +
> +  - If the kernel is entered at EL1 and EL2 is present:
> +
> +    - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
> +    - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
> +    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
> +
> +  - If EL3 is present:
> +
> +    - MDCR_EL3.TDA (bit 9) must be initialized to 0b0

AFAICT we need TDA==0 this regardless of FEAT_Debugv8p9 (and e.g. we need
MDCR_EL3.TPM==0 where FEAT_PMUv3 is implemented), so we should probably
check if there's anything else we haven't yet documented in MDCR_EL3.

[...]

>  .Lskip_trace_\@:
> +	mrs	x1, id_aa64dfr0_el1
> +	ubfx	x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4
> +	cmp	x1, #ID_AA64DFR0_EL1_DebugVer_V8P9
> +	b.lt	.Lskip_dbg_v8p9_\@
> +
> +	mov	x0, #MDCR_EL2_EBWE
> +	orr	x2, x2, x0

That can be:

	orr	x2, x2, #MDCR_EL2_EBWE

Mark.

Re: [PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Anshuman Khandual]

On 10/22/24 21:26, Mark Rutland wrote:
> On Tue, Oct 01, 2024 at 10:06:00AM +0530, Anshuman Khandual wrote:
>> This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
>> ftr_raz[] array which is now redundant. These register fields will be used
>> to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
>> later.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> cc: Mark Brown <broonie@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/kernel/cpufeature.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 718728a85430..bd4d85f5dd92 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -530,6 +530,21 @@ static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>>  	ARM64_FTR_END,
>>  };
>>  
>> +static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_EBEP_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ITE_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABLE_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_PMICNTR_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SPMU_SHIFT, 4, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_BRPs_SHIFT, 8, 0),
>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SYSPMUID_SHIFT, 8, 0),
>> +	ARM64_FTR_END,
>> +};
>> +
> 
> Is there some general principle that has been applied here? e.g. is this
> STRICT unless we know of variation in practice?

Yes, that's correct. STRICT unless there is a known variation in practice.

> 
> e.g. it seems a bit odd that ABLE cannot vary while the number of
> breakpoints can...
But all these (ABL_CMPs, CTX_CMPs, WRPs, BRPs) are marked as FTR_NONSTRICT.
Would not that allow ABL_CMPs to vary as well ?

Although the existing break-point numbers are currently marked FTR_STRICT,
should they be changed first ?

static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
	...................
	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_WRPs_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRPs_SHIFT, 4, 0),
	...................
}

> 
> I suspect we will see systems with mismatched EBEP too, but maybe I'm
> wrong.

Should that be marked FTR_NONSTRICT as well ? But is there a definite way
to confirm if systems could/might come up with such variations in features
between different cpus ?

> 
> Mark.
> 
>>  static const struct arm64_ftr_bits ftr_mvfr0[] = {
>>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, MVFR0_EL1_FPRound_SHIFT, 4, 0),
>>  	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, MVFR0_EL1_FPShVec_SHIFT, 4, 0),
>> @@ -708,10 +723,6 @@ static const struct arm64_ftr_bits ftr_single32[] = {
>>  	ARM64_FTR_END,
>>  };
>>  
>> -static const struct arm64_ftr_bits ftr_raz[] = {
>> -	ARM64_FTR_END,
>> -};
>> -
>>  #define __ARM64_FTR_REG_OVERRIDE(id_str, id, table, ovr) {	\
>>  		.sys_id = id,					\
>>  		.reg = 	&(struct arm64_ftr_reg){		\
>> @@ -784,7 +795,7 @@ static const struct __ftr_reg_entry {
>>  
>>  	/* Op1 = 0, CRn = 0, CRm = 5 */
>>  	ARM64_FTR_REG(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0),
>> -	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_raz),
>> +	ARM64_FTR_REG(SYS_ID_AA64DFR1_EL1, ftr_id_aa64dfr1),
>>  
>>  	/* Op1 = 0, CRn = 0, CRm = 6 */
>>  	ARM64_FTR_REG(SYS_ID_AA64ISAR0_EL1, ftr_id_aa64isar0),
>> -- 
>> 2.25.1
>>

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Anshuman Khandual]

On 10/22/24 21:40, Mark Rutland wrote:
> On Tue, Oct 01, 2024 at 10:06:01AM +0530, Anshuman Khandual wrote:
>> Fine grained trap control for MDSELR_EL1 register needs to be configured in
>> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2
>> is also present. This adds a new helper __init_el2_fgt2() initializing this
>> new FEAT_FGT2 based fine grained registers.
>>
>> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and
>> watchpoint exceptions when kernel enters at EL1, but EL2 is also present.
>> This updates __init_el2_debug() as required for FEAT_Debugv8p9.
>>
>> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements.
> 
> [...]
> 
>> +  For CPUs with FEAT_Debugv8p9 extension present:
>> +
>> +  - If the kernel is entered at EL1 and EL2 is present:
>> +
>> +    - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
>> +    - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
>> +    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
>> +
>> +  - If EL3 is present:
>> +
>> +    - MDCR_EL3.TDA (bit 9) must be initialized to 0b0
> 
> AFAICT we need TDA==0 this regardless of FEAT_Debugv8p9 (and e.g. we need

That's because MDCR_EL3.TDA=0, enables access to many other debug registers
beside FEAT_Debugv8p9, which are currently used and hence this MDCR_EL3.TDA
=0 requirement is a not a new one but rather a missing one instead ?

> MDCR_EL3.TPM==0 where FEAT_PMUv3 is implemented), so we should probably
> check if there's anything else we haven't yet documented in MDCR_EL3.

Will scan through MDCR_EL3 register and match it with existing documentation
i.e Documentation/arch/arm64/booting.rst. If there are some missing MDCR_EL3
fields which should be mentioned, will add them via a separate pre-requisite
patch ?

> 
> [...]
> 
>>  .Lskip_trace_\@:
>> +	mrs	x1, id_aa64dfr0_el1
>> +	ubfx	x1, x1, #ID_AA64DFR0_EL1_DebugVer_SHIFT, #4
>> +	cmp	x1, #ID_AA64DFR0_EL1_DebugVer_V8P9
>> +	b.lt	.Lskip_dbg_v8p9_\@
>> +
>> +	mov	x0, #MDCR_EL2_EBWE
>> +	orr	x2, x2, x0
> 
> That can be:
> 
> 	orr	x2, x2, #MDCR_EL2_EBWE

Right, will change.

> 
> Mark.

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Anshuman Khandual]

On 10/22/24 21:04, Mark Rutland wrote:
> On Tue, Oct 01, 2024 at 10:06:02AM +0530, Anshuman Khandual wrote:
>> Currently there can be maximum 16 breakpoints, and 16 watchpoints available
>> on a given platform - as detected from ID_AA64DFR0_EL1.[BRPs|WRPs] register
>> fields. But these breakpoint, and watchpoints can be extended further up to
>> 64 via a new arch feature FEAT_Debugv8p9.
>>
>> This first enables banked access for the breakpoint and watchpoint register
>> set via MDSELR_EL1, extended exceptions via MDSCR_EL1.EMBWE and determining
>> available breakpoints and watchpoints in the platform from ID_AA64DFR1_EL1,
>> when FEAT_Debugv8p9 is enabled.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  arch/arm64/include/asm/debug-monitors.h |  1 +
>>  arch/arm64/include/asm/hw_breakpoint.h  | 50 ++++++++++++++++++++-----
>>  arch/arm64/kernel/debug-monitors.c      | 16 ++++++--
>>  arch/arm64/kernel/hw_breakpoint.c       | 33 ++++++++++++++++
>>  4 files changed, 86 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
>> index 13d437bcbf58..a14097673ae0 100644
>> --- a/arch/arm64/include/asm/debug-monitors.h
>> +++ b/arch/arm64/include/asm/debug-monitors.h
>> @@ -20,6 +20,7 @@
>>  #define DBG_MDSCR_KDE		(1 << 13)
>>  #define DBG_MDSCR_MDE		(1 << 15)
>>  #define DBG_MDSCR_MASK		~(DBG_MDSCR_KDE | DBG_MDSCR_MDE)
>> +#define DBG_MDSCR_EMBWE		(1UL << 32)
>>  
>>  #define	DBG_ESR_EVT(x)		(((x) >> 27) & 0x7)
>>  
>> diff --git a/arch/arm64/include/asm/hw_breakpoint.h b/arch/arm64/include/asm/hw_breakpoint.h
>> index bd81cf17744a..362c4d4a64ac 100644
>> --- a/arch/arm64/include/asm/hw_breakpoint.h
>> +++ b/arch/arm64/include/asm/hw_breakpoint.h
>> @@ -79,8 +79,8 @@ static inline void decode_ctrl_reg(u32 reg,
>>   * Limits.
>>   * Changing these will require modifications to the register accessors.
>>   */
>> -#define ARM_MAX_BRP		16
>> -#define ARM_MAX_WRP		16
>> +#define ARM_MAX_BRP		64
>> +#define ARM_MAX_WRP		64
>>  
>>  /* Virtual debug register bases. */
>>  #define AARCH64_DBG_REG_BVR	0
>> @@ -94,13 +94,25 @@ static inline void decode_ctrl_reg(u32 reg,
>>  #define AARCH64_DBG_REG_NAME_WVR	wvr
>>  #define AARCH64_DBG_REG_NAME_WCR	wcr
>>  
>> +static inline bool is_debug_v8p9_enabled(void)
>> +{
>> +	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>> +	int dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
>> +
>> +	return dver == ID_AA64DFR0_EL1_DebugVer_V8P9;
>> +}
>> +
>>  /* Accessor macros for the debug registers. */
>>  #define AARCH64_DBG_READ(N, REG, VAL) do {\
>>  	VAL = read_sysreg(dbg##REG##N##_el1);\
>> +	if (is_debug_v8p9_enabled())	\
>> +		preempt_enable();	\
>>  } while (0)
>>  
>>  #define AARCH64_DBG_WRITE(N, REG, VAL) do {\
>>  	write_sysreg(VAL, dbg##REG##N##_el1);\
>> +	if (is_debug_v8p9_enabled())	\
>> +		preempt_enable();	\
>>  } while (0)
> 
> Without looking any further in this patch, this is clearly the wrong
> level of abstraction. Any disable/enable of preemption should be clearly
> balanced in a caller rather than half of that being hidden away in a
> low-level primitive.

Agreed, this was not the most optimal method from readability perspective
as well but could not come up with a better way without creating too much
code churn. But sure, will improve upon this (as you have suggested later).

> 
> Wherever this lives it needs a comment explaining what it is doing and
> why. I assume this is intended to protect the bank in sequences like:
> 
> 	MSR	MDSELR, <...>
> 	ISB
> 	MRS	<..._, BANKED_REGISTER

Correct, it is protecting the above sequence.

> 
> ... but is theat suffucient for mutual exclusion against
> exception handlers, or does that come from somewhere else?

Looking at all existing use cases for breakpoint/watchpoints, it should
be sufficient to protect against mutual exclusion. But thinking, do you
have a particular exception handler scenario in mind where this might
still be problematic ? Will keep looking into it.

> 
>>  struct task_struct;
>> @@ -138,19 +150,37 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>>  /* Determine number of BRP registers available. */
>>  static inline int get_num_brps(void)
>>  {
>> -	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>> -	return 1 +
>> -		cpuid_feature_extract_unsigned_field(dfr0,
>> -						ID_AA64DFR0_EL1_BRPs_SHIFT);
>> +	u64 dfr0, dfr1;
>> +	int dver, brps;
>> +
>> +	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>> +	dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
>> +	if (dver == ID_AA64DFR0_EL1_DebugVer_V8P9) {
>> +		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
>> +		brps = cpuid_feature_extract_unsigned_field_width(dfr1,
>> +								  ID_AA64DFR1_EL1_BRPs_SHIFT, 8);
>> +	} else {
>> +		brps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRPs_SHIFT);
>> +	}
>> +	return 1 + brps;
>>  }
>>  
>>  /* Determine number of WRP registers available. */
>>  static inline int get_num_wrps(void)
>>  {
>> -	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>> -	return 1 +
>> -		cpuid_feature_extract_unsigned_field(dfr0,
>> -						ID_AA64DFR0_EL1_WRPs_SHIFT);
>> +	u64 dfr0, dfr1;
>> +	int dver, wrps;
>> +
>> +	dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>> +	dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
>> +	if (dver == ID_AA64DFR0_EL1_DebugVer_V8P9) {
>> +		dfr1 = read_sanitised_ftr_reg(SYS_ID_AA64DFR1_EL1);
>> +		wrps = cpuid_feature_extract_unsigned_field_width(dfr1,
>> +								  ID_AA64DFR1_EL1_WRPs_SHIFT, 8);
>> +	} else {
>> +		wrps = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_WRPs_SHIFT);
>> +	}
>> +	return 1 + wrps;
>>  }
>>  
>>  #ifdef CONFIG_CPU_PM
>> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
>> index 024a7b245056..af643c935f2e 100644
>> --- a/arch/arm64/kernel/debug-monitors.c
>> +++ b/arch/arm64/kernel/debug-monitors.c
>> @@ -23,6 +23,7 @@
>>  #include <asm/debug-monitors.h>
>>  #include <asm/system_misc.h>
>>  #include <asm/traps.h>
>> +#include <asm/hw_breakpoint.h>
> 
> Nit: these are ordered alphabetically, please keep them that way.

Sure, will change.

> 
>>  
>>  /* Determine debug architecture. */
>>  u8 debug_monitors_arch(void)
>> @@ -34,7 +35,7 @@ u8 debug_monitors_arch(void)
>>  /*
>>   * MDSCR access routines.
>>   */
>> -static void mdscr_write(u32 mdscr)
>> +static void mdscr_write(u64 mdscr)
>>  {
>>  	unsigned long flags;
>>  	flags = local_daif_save();
>> @@ -43,7 +44,7 @@ static void mdscr_write(u32 mdscr)
>>  }
>>  NOKPROBE_SYMBOL(mdscr_write);
>>  
>> -static u32 mdscr_read(void)
>> +static u64 mdscr_read(void)
>>  {
>>  	return read_sysreg(mdscr_el1);
>>  }
>> @@ -76,10 +77,11 @@ early_param("nodebugmon", early_debug_disable);
>>   */
>>  static DEFINE_PER_CPU(int, mde_ref_count);
>>  static DEFINE_PER_CPU(int, kde_ref_count);
>> +static DEFINE_PER_CPU(int, embwe_ref_count);
> 
> We have refcounting for MDE and KDE because they enable debug exceptions
> to be taken (and e.g. require a hypervisor to do more work when they're
> enabled), but AFAICT that's not true for EMBWE.
Thought something similar would be required for EMBWE.

> 
> Do we need to refcount EMBWE?

TBH, not sure. Don't understand this component well enough. You are probably
right, this refcount mechanism for EMBWE might not be required here.

> 
>>  void enable_debug_monitors(enum dbg_active_el el)
>>  {
>> -	u32 mdscr, enable = 0;
>> +	u64 mdscr, enable = 0;
>>  
>>  	WARN_ON(preemptible());
>>  
>> @@ -90,6 +92,9 @@ void enable_debug_monitors(enum dbg_active_el el)
>>  	    this_cpu_inc_return(kde_ref_count) == 1)
>>  		enable |= DBG_MDSCR_KDE;
>>  
>> +	if (is_debug_v8p9_enabled() && this_cpu_inc_return(embwe_ref_count) == 1)
>> +		enable |= DBG_MDSCR_EMBWE;
> 
> ... which suggests that this could simplified to be:
> 
> 	if (is_debug_v8p9_enabled())
> 		enable != DBG_MDSCR_EMBWE;

Sure, will change.

> 
> ... and likewise below, unless I'm missing some reason why we must
> refcount this?

Okay, will drop the refcount mechanism completely and instead just
enable-disable DBG_MDSCR_EMBWE based on is_debug_v8p9_enabled()
feature test.

> 
>> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
>> index 722ac45f9f7b..30156d732284 100644
>> --- a/arch/arm64/kernel/hw_breakpoint.c
>> +++ b/arch/arm64/kernel/hw_breakpoint.c
>> @@ -103,10 +103,40 @@ int hw_breakpoint_slots(int type)
>>  	WRITE_WB_REG_CASE(OFF, 14, REG, VAL);	\
>>  	WRITE_WB_REG_CASE(OFF, 15, REG, VAL)
>>  
>> +static int set_bank_index(int n)
>> +{
>> +	int mdsel_bank;
>> +	int bank = n / 16, index = n % 16;
>> +
>> +	switch (bank) {
>> +	case 0:
>> +		mdsel_bank = MDSELR_EL1_BANK_BANK_0;
>> +		break;
>> +	case 1:
>> +		mdsel_bank = MDSELR_EL1_BANK_BANK_1;
>> +		break;
>> +	case 2:
>> +		mdsel_bank = MDSELR_EL1_BANK_BANK_2;
>> +		break;
>> +	case 3:
>> +		mdsel_bank = MDSELR_EL1_BANK_BANK_3;
>> +		break;
> 
> Since this is a trivial mapping, do we actually need the switch?

MDSELR_EL1_BANK_BANK_<N> = N (0..3) So mdsel_bank could
directly be used without going through the 'bank' based
switch case above. Will drop them as suggested.

> 
>> +	default:
>> +		pr_warn("Unknown register bank %d\n", bank);
>> +	}
>> +	preempt_disable();
>> +	write_sysreg_s(mdsel_bank << MDSELR_EL1_BANK_SHIFT, SYS_MDSELR_EL1);
>> +	isb();
>> +	return index;
>> +}
>> +
>>  static u64 read_wb_reg(int reg, int n)
>>  {
>>  	u64 val = 0;
>>  
>> +	if (is_debug_v8p9_enabled())
>> +		n = set_bank_index(n);
>> +
>>  	switch (reg + n) {
>>  	GEN_READ_WB_REG_CASES(AARCH64_DBG_REG_BVR, AARCH64_DBG_REG_NAME_BVR, val);
>>  	GEN_READ_WB_REG_CASES(AARCH64_DBG_REG_BCR, AARCH64_DBG_REG_NAME_BCR, val);
> 
> As above, this would be better as something like:
> 
> 	// rename the existing read_wb_reg(), unchanged
> 	static u64 __read_wb_reg(int reg, int n);
> 
> 	static u64 read_wb_reg(int reg, int n)
> 	{
> 		u64 val;
> 
> 		if (!is_debug_v8p9_enabled())
> 			return __read_wb_reg(reg, n);
> 
> 		/*
> 		 * TODO: explain here
> 		 */
> 		preempt_disable();
> 		write_sysreg_s(...); // MDSELR
> 		isb();
> 		val = __read_wb_reg(reg, idx_within_bank);
> 		preempt_enable();
> 
> 		return val;
> 	}
> 
> ... or:
> 
> 	static u64 read_wb_reg(int reg, int n)
> 	{
> 		u64 val;
> 
> 		if (is_debug_v8p9_enabled()) {
> 			/*
> 			 * TODO: explain here
> 			 */
> 			preempt_disable();
> 			write_sysreg_s(...); // MDSELR
> 			isb();
> 			val = __read_wb_reg(reg, idx_within_bank);
> 			preempt_enable();
> 		} else {
> 			val = __read_wb_reg(reg, n);
> 		}
> 
> 		return val;
> 	}
> 
> ... which is more lines but *vastly* clearer. 
> 
> Likewise for the write case.

Agreed, they are indeed much clearer, will change as suggested and respin.

Thanks for your detailed review.

Re: [PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Mark Rutland]

On Wed, Oct 23, 2024 at 11:18:12AM +0530, Anshuman Khandual wrote:
> 
> 
> On 10/22/24 21:26, Mark Rutland wrote:
> > On Tue, Oct 01, 2024 at 10:06:00AM +0530, Anshuman Khandual wrote:
> >> This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
> >> ftr_raz[] array which is now redundant. These register fields will be used
> >> to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
> >> later.

> >> +static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_EBEP_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ITE_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABLE_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_PMICNTR_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SPMU_SHIFT, 4, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_BRPs_SHIFT, 8, 0),
> >> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SYSPMUID_SHIFT, 8, 0),
> >> +	ARM64_FTR_END,
> >> +};
> >> +
> > 
> > Is there some general principle that has been applied here? e.g. is this
> > STRICT unless we know of variation in practice?
> 
> Yes, that's correct. STRICT unless there is a known variation in practice.

Please mention that somewhere, e.g. in the commit message.

> > e.g. it seems a bit odd that ABLE cannot vary while the number of
> > breakpoints can...
> But all these (ABL_CMPs, CTX_CMPs, WRPs, BRPs) are marked as FTR_NONSTRICT.
> Would not that allow ABL_CMPs to vary as well ?

I asked about ABLE, not ABL_CMPs.

ABL_CMPs is marked as FTR_NONSTRICT, but ABLE is marked as FTR_STRICT.

> Although the existing break-point numbers are currently marked FTR_STRICT,
> should they be changed first ?
> 
> static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> 	...................
> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_WRPs_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRPs_SHIFT, 4, 0),
> 	...................
> }

My point was that the above didn't seem to be logically consistent; I
think you didn't handle ABLE as you should have.

Mark.

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Mark Rutland]

On Wed, Oct 23, 2024 at 11:42:37AM +0530, Anshuman Khandual wrote:
> 
> 
> On 10/22/24 21:40, Mark Rutland wrote:
> > On Tue, Oct 01, 2024 at 10:06:01AM +0530, Anshuman Khandual wrote:
> >> Fine grained trap control for MDSELR_EL1 register needs to be configured in
> >> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2
> >> is also present. This adds a new helper __init_el2_fgt2() initializing this
> >> new FEAT_FGT2 based fine grained registers.
> >>
> >> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and
> >> watchpoint exceptions when kernel enters at EL1, but EL2 is also present.
> >> This updates __init_el2_debug() as required for FEAT_Debugv8p9.
> >>
> >> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements.
> > 
> > [...]
> > 
> >> +  For CPUs with FEAT_Debugv8p9 extension present:
> >> +
> >> +  - If the kernel is entered at EL1 and EL2 is present:
> >> +
> >> +    - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
> >> +    - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
> >> +    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
> >> +
> >> +  - If EL3 is present:
> >> +
> >> +    - MDCR_EL3.TDA (bit 9) must be initialized to 0b0
> > 
> > AFAICT we need TDA==0 this regardless of FEAT_Debugv8p9 (and e.g. we need
> 
> That's because MDCR_EL3.TDA=0, enables access to many other debug registers
> beside FEAT_Debugv8p9, which are currently used and hence this MDCR_EL3.TDA
> =0 requirement is a not a new one but rather a missing one instead ?

Yes, that's why I said we need it regardless; it's an existing
requirement that wasn't documented.

> 
> > MDCR_EL3.TPM==0 where FEAT_PMUv3 is implemented), so we should probably
> > check if there's anything else we haven't yet documented in MDCR_EL3.
> 
> Will scan through MDCR_EL3 register and match it with existing documentation
> i.e Documentation/arch/arm64/booting.rst. If there are some missing MDCR_EL3
> fields which should be mentioned, will add them via a separate pre-requisite
> patch ?

Yes please.

Mark.

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Mark Rutland]

On Wed, Oct 23, 2024 at 01:01:52PM +0530, Anshuman Khandual wrote:
> 
> 
> On 10/22/24 21:04, Mark Rutland wrote:
> > On Tue, Oct 01, 2024 at 10:06:02AM +0530, Anshuman Khandual wrote:

[...]

> > Wherever this lives it needs a comment explaining what it is doing and
> > why. I assume this is intended to protect the bank in sequences like:
> > 
> > 	MSR	MDSELR, <...>
> > 	ISB
> > 	MRS	<..._, BANKED_REGISTER
> 
> Correct, it is protecting the above sequence.
> 
> > 
> > ... but is theat suffucient for mutual exclusion against
> > exception handlers, or does that come from somewhere else?
> 
> Looking at all existing use cases for breakpoint/watchpoints, it should
> be sufficient to protect against mutual exclusion. But thinking, do you
> have a particular exception handler scenario in mind where this might
> still be problematic ? Will keep looking into it.

Where does the mutual exclusion come from for the existing sequences?
We should be able to descrive should be able to describe that in the
commit message or in a comment somewhere (or better, with some
assertions that get tested).

For example, what prevents watchpoint_handler() from firing in the
middle of arch_install_hw_breakpoint() or
arch_uninstall_hw_breakpoint()?

Is the existing code correct?

Mark.

Re: [PATCH 1/3] arm64/cpufeature: Add field details for ID_AA64DFR1_EL1 register

[Anshuman Khandual]

On 10/28/24 18:03, Mark Rutland wrote:
> On Wed, Oct 23, 2024 at 11:18:12AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 10/22/24 21:26, Mark Rutland wrote:
>>> On Tue, Oct 01, 2024 at 10:06:00AM +0530, Anshuman Khandual wrote:
>>>> This adds required field details for ID_AA64DFR1_EL1, and also drops dummy
>>>> ftr_raz[] array which is now redundant. These register fields will be used
>>>> to enable increased breakpoint and watchpoint registers via FEAT_Debugv8p9
>>>> later.
> 
>>>> +static const struct arm64_ftr_bits ftr_id_aa64dfr1[] = {
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_DPFZS_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_EBEP_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ITE_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABLE_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_PMICNTR_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SPMU_SHIFT, 4, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_BRPs_SHIFT, 8, 0),
>>>> +	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_SYSPMUID_SHIFT, 8, 0),
>>>> +	ARM64_FTR_END,
>>>> +};
>>>> +
>>>
>>> Is there some general principle that has been applied here? e.g. is this
>>> STRICT unless we know of variation in practice?
>>
>> Yes, that's correct. STRICT unless there is a known variation in practice.
> 
> Please mention that somewhere, e.g. in the commit message.

Sure, will add that.

> 
>>> e.g. it seems a bit odd that ABLE cannot vary while the number of
>>> breakpoints can...
>> But all these (ABL_CMPs, CTX_CMPs, WRPs, BRPs) are marked as FTR_NONSTRICT.
>> Would not that allow ABL_CMPs to vary as well ?
> 
> I asked about ABLE, not ABL_CMPs.
> 
> ABL_CMPs is marked as FTR_NONSTRICT, but ABLE is marked as FTR_STRICT.

Ahh, that was my bad, completely missed.

> 
>> Although the existing break-point numbers are currently marked FTR_STRICT,
>> should they be changed first ?
>>
>> static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>> 	...................
>> 	ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_WRPs_SHIFT, 4, 0),
>>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRPs_SHIFT, 4, 0),
>> 	...................
>> }
> 
> My point was that the above didn't seem to be logically consistent; I
> think you didn't handle ABLE as you should have.

Agreed, will change ABLE as FTR_NONSTRICT instead.

But what about the ID_AA64DFR0_EL1_WRPs_SHIFT and ID_AA64DFR0_EL1_BRPs_SHIFT
which could have variations in different cpus on the same system ? So should
those be fixed as FTR_NONSTRICT first ?

I have posted V2 for this series earlier today, hence will accommodate all the
new comments here in V3 going forward.

https://lore.kernel.org/all/20241028053426.2486633-1-anshuman.khandual@arm.com/

Re: [PATCH 2/3] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Anshuman Khandual]

On 10/28/24 18:05, Mark Rutland wrote:
> On Wed, Oct 23, 2024 at 11:42:37AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 10/22/24 21:40, Mark Rutland wrote:
>>> On Tue, Oct 01, 2024 at 10:06:01AM +0530, Anshuman Khandual wrote:
>>>> Fine grained trap control for MDSELR_EL1 register needs to be configured in
>>>> HDFGRTR2_EL2, and HDFGWTR2_EL2 registers when kernel enters at EL1, but EL2
>>>> is also present. This adds a new helper __init_el2_fgt2() initializing this
>>>> new FEAT_FGT2 based fine grained registers.
>>>>
>>>> MDCR_EL2.EBWE needs to be enabled for additional (beyond 16) breakpoint and
>>>> watchpoint exceptions when kernel enters at EL1, but EL2 is also present.
>>>> This updates __init_el2_debug() as required for FEAT_Debugv8p9.
>>>>
>>>> While here, also update booting.rst with MDCR_EL3 and SCR_EL3 requirements.
>>>
>>> [...]
>>>
>>>> +  For CPUs with FEAT_Debugv8p9 extension present:
>>>> +
>>>> +  - If the kernel is entered at EL1 and EL2 is present:
>>>> +
>>>> +    - HDFGRTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
>>>> +    - HDFGWTR2_EL2.nMDSELR_EL1 (bit 5) must be initialized to 0b1
>>>> +    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
>>>> +
>>>> +  - If EL3 is present:
>>>> +
>>>> +    - MDCR_EL3.TDA (bit 9) must be initialized to 0b0
>>>
>>> AFAICT we need TDA==0 this regardless of FEAT_Debugv8p9 (and e.g. we need
>>
>> That's because MDCR_EL3.TDA=0, enables access to many other debug registers
>> beside FEAT_Debugv8p9, which are currently used and hence this MDCR_EL3.TDA
>> =0 requirement is a not a new one but rather a missing one instead ?
> 
> Yes, that's why I said we need it regardless; it's an existing
> requirement that wasn't documented.

Alright, got it.

> 
>>
>>> MDCR_EL3.TPM==0 where FEAT_PMUv3 is implemented), so we should probably
>>> check if there's anything else we haven't yet documented in MDCR_EL3.
>>
>> Will scan through MDCR_EL3 register and match it with existing documentation
>> i.e Documentation/arch/arm64/booting.rst. If there are some missing MDCR_EL3
>> fields which should be mentioned, will add them via a separate pre-requisite
>> patch ?
> 
> Yes please.
> 
> Mark.

Sure, will separate those changes in a pre-requisite patch as suggested.

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Anshuman Khandual]

On 10/28/24 18:17, Mark Rutland wrote:
> On Wed, Oct 23, 2024 at 01:01:52PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 10/22/24 21:04, Mark Rutland wrote:
>>> On Tue, Oct 01, 2024 at 10:06:02AM +0530, Anshuman Khandual wrote:
> 
> [...]
> 
>>> Wherever this lives it needs a comment explaining what it is doing and
>>> why. I assume this is intended to protect the bank in sequences like:
>>>
>>> 	MSR	MDSELR, <...>
>>> 	ISB
>>> 	MRS	<..._, BANKED_REGISTER
>>
>> Correct, it is protecting the above sequence.
>>
>>>
>>> ... but is theat suffucient for mutual exclusion against
>>> exception handlers, or does that come from somewhere else?
>>
>> Looking at all existing use cases for breakpoint/watchpoints, it should
>> be sufficient to protect against mutual exclusion. But thinking, do you
>> have a particular exception handler scenario in mind where this might
>> still be problematic ? Will keep looking into it.
> 
> Where does the mutual exclusion come from for the existing sequences?

Bank selection followed by indexed read/write, inherently requires mutual
exclusion (ensuring that both these steps executed together) in order to
prevent read/write into wrong registers. That being said, HW breakpoints
get used in multiple different places such as perf, ptrace, debug monitor
based single stepping etc calling platform functions which operate on the
HW breakpoint registers here.

preempt_disable()/enable() sequence in the very last leaf level helpers
such as [read|write]_wb_reg(), will ensure required mutual exclusion.

> We should be able to descrive should be able to describe that in the
> commit message or in a comment somewhere (or better, with some
> assertions that get tested).

Planning to add a comment - something like this both for read and write
helpers.
       /*
        * Bank selection in MDSELR_EL1, followed by indexed read from
        * [break|watch]point registers cannot be interrupted, as that
        * might cause misread from wrong targets. Hence this requires
        * mutual exclusion via preventing any preemption.
        */

But regarding adding assertions, could you give some more details and
it will be great to have some relevant examples as well.

> 
> For example, what prevents watchpoint_handler() from firing in the
> middle of arch_install_hw_breakpoint() or
> arch_uninstall_hw_breakpoint()?

If perf is the only user, watchpoint_handler() will not get triggered
without watchpoints being installed via arch_install_hw_breakpoint().
Similarly once they get uninstalled via arch_uninstall_hw_breakpoint()
there will not be active watchpoints to trigger the handler. Although
there are other users (ptrace, debug monitor etc) besides perf which
could also be active simultaneously and race with each other ? TBH, I
am not sure.

> 
> Is the existing code correct?

I have not tested the concurrency aspects of the HW breakpoints enough
to be able to answer that question. But if there is a particular concern
here, happy to look into that.

But wondering how does this new bank indexed read/write mechanism (after
taking care of the mutual exclusion in the leaf level helpers such as
[read| write]_wb_reg()) still makes the existing concurrency situation
worse off than earlier ?

Re: [PATCH 3/3] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Mark Rutland]

On Tue, Oct 29, 2024 at 01:06:38PM +0530, Anshuman Khandual wrote:
> On 10/28/24 18:17, Mark Rutland wrote:
> > On Wed, Oct 23, 2024 at 01:01:52PM +0530, Anshuman Khandual wrote:
> >> On 10/22/24 21:04, Mark Rutland wrote:

> >>> I assume this is intended to protect the bank in sequences like:
> >>>
> >>> 	MSR	MDSELR, <...>
> >>> 	ISB
> >>> 	MRS	<..._, BANKED_REGISTER
> >>
> >> Correct, it is protecting the above sequence.
> >>
> >>> ... but is theat suffucient for mutual exclusion against
> >>> exception handlers, or does that come from somewhere else?
> >>
> >> Looking at all existing use cases for breakpoint/watchpoints, it should
> >> be sufficient to protect against mutual exclusion. But thinking, do you
> >> have a particular exception handler scenario in mind where this might
> >> still be problematic ? Will keep looking into it.
> > 
> > Where does the mutual exclusion come from for the existing sequences?
> 
> Bank selection followed by indexed read/write, inherently requires mutual
> exclusion (ensuring that both these steps executed together) in order to
> prevent read/write into wrong registers. That being said, HW breakpoints
> get used in multiple different places such as perf, ptrace, debug monitor
> based single stepping etc calling platform functions which operate on the
> HW breakpoint registers here.

Yes; that's *why* I'm asking. 

> preempt_disable()/enable() sequence in the very last leaf level helpers
> such as [read|write]_wb_reg(), will ensure required mutual exclusion.

I do not believe that this assertion is correct.

I specifically gave the example of mutual exclusion against exception
handlers, and preempt_disable() ... preempt_disable() does not prevent
exceptions being taken, so disabling preemption *cannot* be sufficient
to provide mutual exclusion against exception handlers.

What prevents a race with an exception handler? e.g. 

* Does the structure of the code prevent that somehow?

* What context(s) does this code execute in?
  - Are debug exceptions always masked?
  - Do we disable breakpoints/watchpoints around (some) manipulation of
    the relevant registers?

> > We should be able to descrive should be able to describe that in the
> > commit message or in a comment somewhere (or better, with some
> > assertions that get tested).
> 
> Planning to add a comment - something like this both for read and write
> helpers.
>        /*
>         * Bank selection in MDSELR_EL1, followed by indexed read from
>         * [break|watch]point registers cannot be interrupted, as that
>         * might cause misread from wrong targets. Hence this requires
>         * mutual exclusion via preventing any preemption.
>         */

As above, I do not believe this is correct. At minimum, disabling
preemption is not the full story here.

> But regarding adding assertions, could you give some more details and
> it will be great to have some relevant examples as well.

I've given some suggestions above. Please go and read the code and
figure this out.

> > For example, what prevents watchpoint_handler() from firing in the
> > middle of arch_install_hw_breakpoint() or
> > arch_uninstall_hw_breakpoint()?
> 
> If perf is the only user, watchpoint_handler() will not get triggered
> without watchpoints being installed via arch_install_hw_breakpoint().
> Similarly once they get uninstalled via arch_uninstall_hw_breakpoint()
> there will not be active watchpoints to trigger the handler. Although
> there are other users (ptrace, debug monitor etc) besides perf which
> could also be active simultaneously and race with each other ? TBH, I
> am not sure.

Please go and read the code and figure this out.

> > Is the existing code correct?
> 
> I have not tested the concurrency aspects of the HW breakpoints enough
> to be able to answer that question. But if there is a particular concern
> here, happy to look into that.
> 
> But wondering how does this new bank indexed read/write mechanism (after
> taking care of the mutual exclusion in the leaf level helpers such as
> [read| write]_wb_reg()) still makes the existing concurrency situation
> worse off than earlier ?

Please go and read the code and figure this out.

Mark.