[PATCH V3 0/7] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[PATCH V3 0/7] 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.13-rc3 although this depends on
FEAT_FGT2 FGU series posted earlier, for MDSELR_EL1 handling in various KVM
guest configurations.

https://lore.kernel.org/all/20241210055311.780688-3-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 V3:

- Marked ID_AA64DFR1_EL1.ABLE as FTR_NONSTRICT in ftr_id_aa64dfr1[]
- Dropped MDCR_EL3.TDA boot requirement from documentation (separate series)
- Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg
- Used SYS_FIELD_PREP() in read_wb_reg() and write_wb_reg()
- Added MAX_PER_BANK based BUILD_BUG_ON() tests in arch_hw_breakpoint_init()
- Dropped local variables i.e mdsel_bank and index
- Derived bank and index from MAX_PER_BANK as required

Changes in V2:

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

Following changes have been made per review comments from Mark Rutland

- Orr MDCR_EL2_EBWE directly without an intermittent register
- Alphabetically order header files in debug-monitors.c
- Dropped embwe_ref_count mechanism
- Dropped preempt_enable() from AARCH64_DBG_READ
- Dropped preempt_disable() from AARCH64_DBG_WRITE
- Dropped set_bank_index()
- Renamed read/write_wb_reg() as __read/__write_wb_reg()
- Modified read/write_wb_reg() to have MDSELR_E1 based banked read/write
- Added required sysreg tools patches from KVM FEAT_FGT2 series for build

Changes in V1:

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

- 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 (7):
  arm64/sysreg: Update register fields for ID_AA64MMFR0_EL1
  arm64/sysreg: Add register fields for MDSELR_EL1
  arm64/sysreg: Add register fields for HDFGRTR2_EL2
  arm64/sysreg: Add register fields for HDFGWTR2_EL2
  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    | 18 +++++++
 arch/arm64/include/asm/debug-monitors.h |  1 +
 arch/arm64/include/asm/el2_setup.h      | 26 +++++++++
 arch/arm64/include/asm/hw_breakpoint.h  | 47 +++++++++++++----
 arch/arm64/kernel/cpufeature.c          | 21 ++++++--
 arch/arm64/kernel/debug-monitors.c      | 15 ++++--
 arch/arm64/kernel/hw_breakpoint.c       | 48 ++++++++++++++++-
 arch/arm64/tools/sysreg                 | 70 +++++++++++++++++++++++++
 8 files changed, 225 insertions(+), 21 deletions(-)

-- 
2.25.1

[PATCH V3 1/7] arm64/sysreg: Update register fields for ID_AA64MMFR0_EL1

[Anshuman Khandual]

This updates ID_AA64MMFR0_EL1 register fields as per the definitions based
on DDI0601 2024-09.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/tools/sysreg | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index b081b54d6d22..a6cbe0dcd63b 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -1591,6 +1591,7 @@ EndEnum
 UnsignedEnum	59:56	FGT
 	0b0000	NI
 	0b0001	IMP
+	0b0010	FGT2
 EndEnum
 Res0	55:48
 UnsignedEnum	47:44	EXS
@@ -1652,6 +1653,7 @@ Enum	3:0	PARANGE
 	0b0100	44
 	0b0101	48
 	0b0110	52
+	0b0111	56
 EndEnum
 EndSysreg
 
-- 
2.25.1

[PATCH V3 2/7] arm64/sysreg: Add register fields for MDSELR_EL1

[Anshuman Khandual]

This adds register fields for MDSELR_EL1 as per the definitions based
on DDI0601 2024-09.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/tools/sysreg | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index a6cbe0dcd63b..fe878eb194a0 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -93,6 +93,17 @@ Res0	63:32
 Field	31:0	DTRTX
 EndSysreg
 
+Sysreg	MDSELR_EL1	2	0	0	4	2
+Res0	63:6
+Enum	5:4	BANK
+	0b00	BANK_0
+	0b01	BANK_1
+	0b10	BANK_2
+	0b11	BANK_3
+EndEnum
+Res0	3:0
+EndSysreg
+
 Sysreg	OSECCR_EL1	2	0	0	6	2
 Res0	63:32
 Field	31:0	EDECCR
-- 
2.25.1

[PATCH V3 3/7] arm64/sysreg: Add register fields for HDFGRTR2_EL2

[Anshuman Khandual]

This adds register fields for HDFGRTR2_EL2 as per the definitions based
on DDI0601 2024-09.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/tools/sysreg | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index fe878eb194a0..a9dc5e4f9d97 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -2557,6 +2557,35 @@ Field	1	ICIALLU
 Field	0	ICIALLUIS
 EndSysreg
 
+Sysreg HDFGRTR2_EL2	3	4	3	1	0
+Res0	63:25
+Field	24	nPMBMAR_EL1
+Field	23	nMDSTEPOP_EL1
+Field	22	nTRBMPAM_EL1
+Res0	21
+Field	20	nTRCITECR_EL1
+Field	19	nPMSDSFR_EL1
+Field	18	nSPMDEVAFF_EL1
+Field	17	nSPMID
+Field	16	nSPMSCR_EL1
+Field	15	nSPMACCESSR_EL1
+Field	14	nSPMCR_EL0
+Field	13	nSPMOVS
+Field	12	nSPMINTEN
+Field	11	nSPMCNTEN
+Field	10	nSPMSELR_EL0
+Field	9	nSPMEVTYPERn_EL0
+Field	8	nSPMEVCNTRn_EL0
+Field	7	nPMSSCR_EL1
+Field	6	nPMSSDATA
+Field	5	nMDSELR_EL1
+Field	4	nPMUACR_EL1
+Field	3	nPMICFILTR_EL0
+Field	2	nPMICNTR_EL0
+Field	1	nPMIAR_EL1
+Field	0	nPMECR_EL1
+EndSysreg
+
 Sysreg HDFGRTR_EL2	3	4	3	1	4
 Field	63	PMBIDR_EL1
 Field	62	nPMSNEVFR_EL1
-- 
2.25.1

[PATCH V3 4/7] arm64/sysreg: Add register fields for HDFGWTR2_EL2

[Anshuman Khandual]

This adds register fields for HDFGWTR2_EL2 as per the definitions based
on DDI0601 2024-09.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/tools/sysreg | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index a9dc5e4f9d97..8bf22f3904bd 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -2586,6 +2586,34 @@ Field	1	nPMIAR_EL1
 Field	0	nPMECR_EL1
 EndSysreg
 
+Sysreg HDFGWTR2_EL2	3	4	3	1	1
+Res0	63:25
+Field	24	nPMBMAR_EL1
+Field	23	nMDSTEPOP_EL1
+Field	22	nTRBMPAM_EL1
+Field	21	nPMZR_EL0
+Field	20	nTRCITECR_EL1
+Field	19	nPMSDSFR_EL1
+Res0	18:17
+Field	16	nSPMSCR_EL1
+Field	15	nSPMACCESSR_EL1
+Field	14	nSPMCR_EL0
+Field	13	nSPMOVS
+Field	12	nSPMINTEN
+Field	11	nSPMCNTEN
+Field	10	nSPMSELR_EL0
+Field	9	nSPMEVTYPERn_EL0
+Field	8	nSPMEVCNTRn_EL0
+Field	7	nPMSSCR_EL1
+Res0	6
+Field	5	nMDSELR_EL1
+Field	4	nPMUACR_EL1
+Field	3	nPMICFILTR_EL0
+Field	2	nPMICNTR_EL0
+Field	1	nPMIAR_EL1
+Field	0	nPMECR_EL1
+EndSysreg
+
 Sysreg HDFGRTR_EL2	3	4	3	1	4
 Field	63	PMBIDR_EL1
 Field	62	nPMSNEVFR_EL1
-- 
2.25.1

[PATCH V3 5/7] 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. The register fields have been marked as FTR_STRICT, unless there is
a known variation in practice.

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>
---
Changes in V3:

- Updated the commit message
- Marked ID_AA64DFR1_EL1.ABLE as FTR_NONSTRICT in ftr_id_aa64dfr1[]

 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 6ce71f444ed8..0dc22fde104e 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -534,6 +534,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_NONSTRICT, 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),
@@ -720,10 +735,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){		\
@@ -796,7 +807,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 V3 6/7] 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>
---
Changes in V3:

- Dropped MDCR_EL3.TDA boot requirement from documentation (separate series)
- Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg

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

 Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++
 arch/arm64/include/asm/el2_setup.h   | 26 ++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst
index 3278fb4bf219..054cfe1cad18 100644
--- a/Documentation/arch/arm64/booting.rst
+++ b/Documentation/arch/arm64/booting.rst
@@ -288,6 +288,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:
@@ -322,6 +328,18 @@ 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.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 4ef52d7245bb..2fbfe27d38b5 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -105,6 +105,13 @@
 						// 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_\@
+
+	orr	x2, x2, #MDCR_EL2_EBWE
+.Lskip_dbg_v8p9_\@:
 	msr	mdcr_el2, x2			// Configure debug traps
 .endm
 
@@ -244,6 +251,24 @@
 .Lskip_gcs_\@:
 .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
@@ -283,6 +308,7 @@
 	__init_el2_nvhe_idregs
 	__init_el2_cptr
 	__init_el2_fgt
+	__init_el2_fgt2
         __init_el2_gcs
 .endm
 
-- 
2.25.1

Re: [PATCH V3 6/7] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Rob Herring]

On Mon, Dec 16, 2024 at 09:38:30AM +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.

Shouldn't this be "when kernel enters at EL2, and EL3 is also present"? 
Though it is really "If EL3 set FGTEn2 and the kernel is entered in 
EL2, then FGT2 must be initialized for MDSELR_EL1."

If it was me, I'd just plagarize what was written for prior FGT 
commits for this code. :)

> This adds a new helper __init_el2_fgt2() initializing this
> new FEAT_FGT2 based fine grained registers.

"This adds" is the same as saying "This patch/commit adds" which is well 
documented to avoid. Use imperative "Add a new helper...". Though it is 
clear from the diff that is what you are doing...


> 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>
> ---
> Changes in V3:
> 
> - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series)
> - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg
> 
> https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/
> 
>  Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++
>  arch/arm64/include/asm/el2_setup.h   | 26 ++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst
> index 3278fb4bf219..054cfe1cad18 100644
> --- a/Documentation/arch/arm64/booting.rst
> +++ b/Documentation/arch/arm64/booting.rst
> @@ -288,6 +288,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:
> @@ -322,6 +328,18 @@ 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.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 4ef52d7245bb..2fbfe27d38b5 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -105,6 +105,13 @@
>  						// 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_\@
> +
> +	orr	x2, x2, #MDCR_EL2_EBWE
> +.Lskip_dbg_v8p9_\@:
>  	msr	mdcr_el2, x2			// Configure debug traps
>  .endm
>  
> @@ -244,6 +251,24 @@
>  .Lskip_gcs_\@:
>  .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

We already read this field in __init_el2_fgt, shouldn't we leverage that 
and move all this there rather than read the feature reg twice.

> +	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

If Debug v8.9 is not present, but FGT2 is, shouldn't we write 0 to these 
registers? That is what we do for FGT.


I just realized I forgot to add FGT2 setup for the PMUv3.9 features I 
already added in 6.12 and 6.13. So this really needs to land sooner 
rather than later to add that. 

Rob

Re: [PATCH V3 6/7] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Anshuman Khandual]

On 12/17/24 05:12, Rob Herring wrote:
> On Mon, Dec 16, 2024 at 09:38:30AM +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.
> 
> Shouldn't this be "when kernel enters at EL2, and EL3 is also present"?

AFAICT - HDFGRTR2_EL2 and HDFGWTR2_EL2 registers configure traps into EL2 when
accessed from EL1/EL0, provided all required EL3 trap controls are in place as
well. These EL2 based trap configs need to be set before the kernel finally
enters EL1. Although there is an assumption about EL3 based trap configs being
in place, do we need to mention that in commit message as well. Is not updating 
booting.rst takes care of all EL3 requirements.

> Though it is really "If EL3 set FGTEn2 and the kernel is entered in 
> EL2, then FGT2 must be initialized for MDSELR_EL1."
> 
> If it was me, I'd just plagarize what was written for prior FGT 
> commits for this code. :)

There are many commits that changed __init_el2_fgt() with different description
formats. Do you have particular one in mind which can be followed here ? :)

> 
>> This adds a new helper __init_el2_fgt2() initializing this
>> new FEAT_FGT2 based fine grained registers.
> 
> "This adds" is the same as saying "This patch/commit adds" which is well 
> documented to avoid. Use imperative "Add a new helper...". Though it is 
> clear from the diff that is what you are doing...

Sure, will fix it.

> 
> 
>> 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>
>> ---
>> Changes in V3:
>>
>> - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series)
>> - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg
>>
>> https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/
>>
>>  Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++
>>  arch/arm64/include/asm/el2_setup.h   | 26 ++++++++++++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst
>> index 3278fb4bf219..054cfe1cad18 100644
>> --- a/Documentation/arch/arm64/booting.rst
>> +++ b/Documentation/arch/arm64/booting.rst
>> @@ -288,6 +288,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:
>> @@ -322,6 +328,18 @@ 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.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 4ef52d7245bb..2fbfe27d38b5 100644
>> --- a/arch/arm64/include/asm/el2_setup.h
>> +++ b/arch/arm64/include/asm/el2_setup.h
>> @@ -105,6 +105,13 @@
>>  						// 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_\@
>> +
>> +	orr	x2, x2, #MDCR_EL2_EBWE
>> +.Lskip_dbg_v8p9_\@:
>>  	msr	mdcr_el2, x2			// Configure debug traps
>>  .endm
>>  
>> @@ -244,6 +251,24 @@
>>  .Lskip_gcs_\@:
>>  .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
> 
> We already read this field in __init_el2_fgt, shouldn't we leverage that 
> and move all this there rather than read the feature reg twice.

Should not __init_el2_fgt2() remain separate to contain all future FEAT_FGT2
related trap enabled/disable checks ? OTOH reading id_aa64mmfr0_el1 register
should improve some performance as well.

> 
>> +	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
> 
> If Debug v8.9 is not present, but FGT2 is, shouldn't we write 0 to these 
> registers? That is what we do for FGT.

Just to summerize what's being done in __init_el2_fgt()

.macro __init_el2_fgt
	...............
	mov     x0, xzr
	...............
.Lskip_spe_fgt_\@:
        msr_s   SYS_HDFGRTR_EL2, x0
        msr_s   SYS_HDFGWTR_EL2, x0
	...............
.Lset_fgt_\@:
        msr_s   SYS_HFGRTR_EL2, x0
        msr_s   SYS_HFGWTR_EL2, x0
        msr_s   SYS_HFGITR_EL2, xzr
	...............

If the relevant features are not present and their traps need not be handled,
all FEAT_FGT based trap control registers (SYS_HDFGRTR_EL2, SYS_HDFGWTR_EL2,
SYS_HFGRTR_EL2, SYS_HFGWTR_EL2 and SYS_HFGITR_EL2) get cleared. Hence same
needs to be done for all FEAT_FGT2 register as well. Fair enough, something
like the following makes sense ?

.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_\@

        mov     x0, xzr
        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_\@

        orr     x0, x0, #HDFGWTR2_EL2_nMDSELR_EL1
.Lskip_dbg_v8p9_\@:
        msr_s   SYS_HDFGWTR2_EL2, x0
        msr_s   SYS_HDFGRTR2_EL2, x0
        msr_s   SYS_HFGRTR2_EL2, xzr
        msr_s   SYS_HFGWTR2_EL2, xzr
        msr_s   SYS_HFGITR2_EL2, xzr
.Lskip_fgt2_\@:
.endm

Just wondering - following the same principle if clearing these FEAT_FGT2 trap
registers would make sense in itself, even without FEAT_Debugv8p9 enablement ?

Something like the commit

commit 31c00d2aeaa2da89361f5b64a64ca831433be5fc
Author: Mark Brown <broonie@kernel.org>
Date:   Thu Apr 1 19:09:40 2021 +0100

    arm64: Disable fine grained traps on boot
    
    The arm64 FEAT_FGT extension introduces a set of traps to EL2 for accesses
    to small sets of registers and instructions from EL1 and EL0.  Currently
    Linux makes no use of this feature, ensure that it is not active at boot by
    disabling the traps during EL2 setup.


> 
> 
> I just realized I forgot to add FGT2 setup for the PMUv3.9 features I 
> already added in 6.12 and 6.13. So this really needs to land sooner 
> rather than later to add that. 
Not sure if I got this correctly. Are you suggesting to carve out __init_el2_fgt2()
from the series and post separately with PMUv3.9 requirements and fallback clearing
for all FEAT_FGT2 trap config registers as mentioned above ?

Re: [PATCH V3 6/7] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Rob Herring]

On Tue, Dec 17, 2024 at 2:48 AM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> On 12/17/24 05:12, Rob Herring wrote:
> > On Mon, Dec 16, 2024 at 09:38:30AM +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.
> >
> > Shouldn't this be "when kernel enters at EL2, and EL3 is also present"?
>
> AFAICT - HDFGRTR2_EL2 and HDFGWTR2_EL2 registers configure traps into EL2 when
> accessed from EL1/EL0, provided all required EL3 trap controls are in place as
> well. These EL2 based trap configs need to be set before the kernel finally
> enters EL1. Although there is an assumption about EL3 based trap configs being
> in place, do we need to mention that in commit message as well. Is not updating
> booting.rst takes care of all EL3 requirements.

My point is just I read 'kernel enters at EL1' as meaning the kernel
booted at EL1 and EL2 is not accessible. Maybe should reworded as
'before/if the kernel drops to EL1'

> > Though it is really "If EL3 set FGTEn2 and the kernel is entered in
> > EL2, then FGT2 must be initialized for MDSELR_EL1."
> >
> > If it was me, I'd just plagarize what was written for prior FGT
> > commits for this code. :)
>
> There are many commits that changed __init_el2_fgt() with different description
> formats. Do you have particular one in mind which can be followed here ? :)
>
> >
> >> This adds a new helper __init_el2_fgt2() initializing this
> >> new FEAT_FGT2 based fine grained registers.
> >
> > "This adds" is the same as saying "This patch/commit adds" which is well
> > documented to avoid. Use imperative "Add a new helper...". Though it is
> > clear from the diff that is what you are doing...
>
> Sure, will fix it.
>
> >
> >
> >> 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>
> >> ---
> >> Changes in V3:
> >>
> >> - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series)
> >> - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg
> >>
> >> https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/
> >>
> >>  Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++
> >>  arch/arm64/include/asm/el2_setup.h   | 26 ++++++++++++++++++++++++++
> >>  2 files changed, 44 insertions(+)
> >>
> >> diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst
> >> index 3278fb4bf219..054cfe1cad18 100644
> >> --- a/Documentation/arch/arm64/booting.rst
> >> +++ b/Documentation/arch/arm64/booting.rst
> >> @@ -288,6 +288,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:
> >> @@ -322,6 +328,18 @@ 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.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 4ef52d7245bb..2fbfe27d38b5 100644
> >> --- a/arch/arm64/include/asm/el2_setup.h
> >> +++ b/arch/arm64/include/asm/el2_setup.h
> >> @@ -105,6 +105,13 @@
> >>                                              // 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_\@
> >> +
> >> +    orr     x2, x2, #MDCR_EL2_EBWE
> >> +.Lskip_dbg_v8p9_\@:
> >>      msr     mdcr_el2, x2                    // Configure debug traps
> >>  .endm
> >>
> >> @@ -244,6 +251,24 @@
> >>  .Lskip_gcs_\@:
> >>  .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
> >
> > We already read this field in __init_el2_fgt, shouldn't we leverage that
> > and move all this there rather than read the feature reg twice.
>
> Should not __init_el2_fgt2() remain separate to contain all future FEAT_FGT2
> related trap enabled/disable checks ? OTOH reading id_aa64mmfr0_el1 register
> should improve some performance as well.

That's the tradeoff. I'll defer to others whether a single id register
read here is preferred.

> >> +    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
> >
> > If Debug v8.9 is not present, but FGT2 is, shouldn't we write 0 to these
> > registers? That is what we do for FGT.
>
> Just to summerize what's being done in __init_el2_fgt()
>
> .macro __init_el2_fgt
>         ...............
>         mov     x0, xzr
>         ...............
> .Lskip_spe_fgt_\@:
>         msr_s   SYS_HDFGRTR_EL2, x0
>         msr_s   SYS_HDFGWTR_EL2, x0
>         ...............
> .Lset_fgt_\@:
>         msr_s   SYS_HFGRTR_EL2, x0
>         msr_s   SYS_HFGWTR_EL2, x0
>         msr_s   SYS_HFGITR_EL2, xzr
>         ...............
>
> If the relevant features are not present and their traps need not be handled,
> all FEAT_FGT based trap control registers (SYS_HDFGRTR_EL2, SYS_HDFGWTR_EL2,
> SYS_HFGRTR_EL2, SYS_HFGWTR_EL2 and SYS_HFGITR_EL2) get cleared. Hence same
> needs to be done for all FEAT_FGT2 register as well. Fair enough, something
> like the following makes sense ?
>
> .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_\@
>
>         mov     x0, xzr
>         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_\@
>
>         orr     x0, x0, #HDFGWTR2_EL2_nMDSELR_EL1
> .Lskip_dbg_v8p9_\@:
>         msr_s   SYS_HDFGWTR2_EL2, x0
>         msr_s   SYS_HDFGRTR2_EL2, x0
>         msr_s   SYS_HFGRTR2_EL2, xzr
>         msr_s   SYS_HFGWTR2_EL2, xzr
>         msr_s   SYS_HFGITR2_EL2, xzr
> .Lskip_fgt2_\@:
> .endm

Looks good.

>
> Just wondering - following the same principle if clearing these FEAT_FGT2 trap
> registers would make sense in itself, even without FEAT_Debugv8p9 enablement ?

Probably and there's a Jira for it. Though there was some debate about
whether anything was needed before any users. But that's kind of a
mute point anyway because I need it for PMU now, too.

> Something like the commit
>
> commit 31c00d2aeaa2da89361f5b64a64ca831433be5fc
> Author: Mark Brown <broonie@kernel.org>
> Date:   Thu Apr 1 19:09:40 2021 +0100
>
>     arm64: Disable fine grained traps on boot
>
>     The arm64 FEAT_FGT extension introduces a set of traps to EL2 for accesses
>     to small sets of registers and instructions from EL1 and EL0.  Currently
>     Linux makes no use of this feature, ensure that it is not active at boot by
>     disabling the traps during EL2 setup.
>
>
> >
> >
> > I just realized I forgot to add FGT2 setup for the PMUv3.9 features I
> > already added in 6.12 and 6.13. So this really needs to land sooner
> > rather than later to add that.
> Not sure if I got this correctly. Are you suggesting to carve out __init_el2_fgt2()
> from the series and post separately with PMUv3.9 requirements and fallback clearing
> for all FEAT_FGT2 trap config registers as mentioned above ?

Yes, as it needs to not be held up by any of the debug issues Mark
raised. Also, it may need to be back ported to 6.12. And for that we'd
want the PMU parts, but not the Debug. I still have to figure out what
needs to be done on the KVM side.

Rob

Re: [PATCH V3 6/7] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Anshuman Khandual]

[....]

On 12/17/24 23:23, Rob Herring wrote:
>> Something like the commit
>>
>> commit 31c00d2aeaa2da89361f5b64a64ca831433be5fc
>> Author: Mark Brown <broonie@kernel.org>
>> Date:   Thu Apr 1 19:09:40 2021 +0100
>>
>>     arm64: Disable fine grained traps on boot
>>
>>     The arm64 FEAT_FGT extension introduces a set of traps to EL2 for accesses
>>     to small sets of registers and instructions from EL1 and EL0.  Currently
>>     Linux makes no use of this feature, ensure that it is not active at boot by
>>     disabling the traps during EL2 setup.
>>
>>
>>>
>>> I just realized I forgot to add FGT2 setup for the PMUv3.9 features I
>>> already added in 6.12 and 6.13. So this really needs to land sooner
>>> rather than later to add that.
>> Not sure if I got this correctly. Are you suggesting to carve out __init_el2_fgt2()
>> from the series and post separately with PMUv3.9 requirements and fallback clearing
>> for all FEAT_FGT2 trap config registers as mentioned above ?
> Yes, as it needs to not be held up by any of the debug issues Mark
> raised. Also, it may need to be back ported to 6.12. And for that we'd
> want the PMU parts, but not the Debug. I still have to figure out what
> needs to be done on the KVM side.

Hi Rob,

I did go through all the five FEAT_FGT2 trap control registers and it
seems like the following are the controls available for FEAT_PMUv3p9
based registers. Although PMZR_EL0 does not get used in kernel right
now but still might be a good idea to include anyway. Please let me
know, if I might have missed something else related to FEAT_PMUv3p9.

HDFGRTR2_EL2_nPMUACR_EL1	(mrs PMUACR_EL1)
HDFGWTR2_EL2_nPMUACR_EL1	(msr PMUACR_EL1)
HDFGWTR2_EL2_nPMZR_EL0		(msr PMZR_EL0)

Following will be the change required for __init_el2_fgt2() along with
all the tools sysreg updates required for the mentioned registers here.

--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -233,6 +233,31 @@
 .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_\@
+
+       mov     x0, xzr
+       mov     x2, xzr
+       mrs     x1, id_aa64dfr0_el1
+       ubfx    x1, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
+       cmp     x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
+       b.lt    .Lskip_pmuv3p9_\@
+
+       orr     x0, x0, #HDFGRTR2_EL2_nPMUACR_EL1
+       orr     x2, x2, #HDFGWTR2_EL2_nPMUACR_EL1
+       orr     x2, x2, #HDFGWTR2_EL2_nPMZR_EL0
+.Lskip_pmuv3p9_\@:
+       msr_s   SYS_HDFGRTR2_EL2, x0
+       msr_s   SYS_HDFGWTR2_EL2, x2
+       msr_s   SYS_HFGRTR2_EL2, xzr
+       msr_s   SYS_HFGWTR2_EL2, xzr
+       msr_s   SYS_HFGITR2_EL2, xzr
+.Lskip_fgt2_\@:
+.endm
+
 .macro __init_el2_gcs
        mrs_s   x1, SYS_ID_AA64PFR1_EL1
        ubfx    x1, x1, #ID_AA64PFR1_EL1_GCS_SHIFT, #4
@@ -283,6 +308,7 @@
        __init_el2_nvhe_idregs
        __init_el2_cptr
        __init_el2_fgt
+       __init_el2_fgt2
         __init_el2_gcs
 .endm

Re: [PATCH V3 6/7] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Marc Zyngier]

On Tue, 17 Dec 2024 17:53:41 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Tue, Dec 17, 2024 at 2:48 AM Anshuman Khandual
> <anshuman.khandual@arm.com> wrote:
> >
> > On 12/17/24 05:12, Rob Herring wrote:
> > > On Mon, Dec 16, 2024 at 09:38:30AM +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.
> > >
> > > Shouldn't this be "when kernel enters at EL2, and EL3 is also present"?
> >
> > AFAICT - HDFGRTR2_EL2 and HDFGWTR2_EL2 registers configure traps into EL2 when
> > accessed from EL1/EL0, provided all required EL3 trap controls are in place as
> > well. These EL2 based trap configs need to be set before the kernel finally
> > enters EL1. Although there is an assumption about EL3 based trap configs being
> > in place, do we need to mention that in commit message as well. Is not updating
> > booting.rst takes care of all EL3 requirements.
> 
> My point is just I read 'kernel enters at EL1' as meaning the kernel
> booted at EL1 and EL2 is not accessible. Maybe should reworded as
> 'before/if the kernel drops to EL1'

The EL2->EL1 downgrade is internal to the kernel. If we get it wrong,
that's our fault, and there isn't anything to document. This document
is about booting the kernel at any NS EL, so only the "external"
requirements matter.

But I don't think we should be prescriptive about the state of these
registers, as long as the potential traps are correctly handled.

For the above, I'd rather have something like:

"The MDSELR_EL1 register must be freely accessible when the kernel is
 entered at EL1 and that higher ELs are present in the system."

On its own, that should be enough. You can subsequently add:

"This includes HDFGRTR2_EL2, HDFGWTR2_EL2 and MDCR_EL2 when EL2 is
 present, as well as MDCR_EL3 when EL3 is present".

but that's paraphrasing the architecture, and is definitely incomplete
(see the MDSELR_EL1 pseudocode for reference).

> 
> > > Though it is really "If EL3 set FGTEn2 and the kernel is entered in
> > > EL2, then FGT2 must be initialized for MDSELR_EL1."
> > >
> > > If it was me, I'd just plagarize what was written for prior FGT
> > > commits for this code. :)
> >
> > There are many commits that changed __init_el2_fgt() with different description
> > formats. Do you have particular one in mind which can be followed here ? :)
> >
> > >
> > >> This adds a new helper __init_el2_fgt2() initializing this
> > >> new FEAT_FGT2 based fine grained registers.
> > >
> > > "This adds" is the same as saying "This patch/commit adds" which is well
> > > documented to avoid. Use imperative "Add a new helper...". Though it is
> > > clear from the diff that is what you are doing...
> >
> > Sure, will fix it.
> >
> > >
> > >
> > >> 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>
> > >> ---
> > >> Changes in V3:
> > >>
> > >> - Dropped MDCR_EL3.TDA boot requirement from documentation (separate series)
> > >> - Dropped MDCR_EL2_EBWE definition as MDCR_EL2 is now defined in tools sysreg
> > >>
> > >> https://lore.kernel.org/all/20241211065425.1106683-1-anshuman.khandual@arm.com/
> > >>
> > >>  Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++
> > >>  arch/arm64/include/asm/el2_setup.h   | 26 ++++++++++++++++++++++++++
> > >>  2 files changed, 44 insertions(+)
> > >>
> > >> diff --git a/Documentation/arch/arm64/booting.rst b/Documentation/arch/arm64/booting.rst
> > >> index 3278fb4bf219..054cfe1cad18 100644
> > >> --- a/Documentation/arch/arm64/booting.rst
> > >> +++ b/Documentation/arch/arm64/booting.rst
> > >> @@ -288,6 +288,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:
> > >> @@ -322,6 +328,18 @@ 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

But then you don't say anything about SCR_EL3.FGTEn2. And what if the
hypervisor wants to trap things in order to emulate on lazy switch
things?

The more I look at those, the more I think these requirements are not
saying what we want to express, which is that some registers must be
accessible one way or another.

> > >> +    - MDCR_EL2.EBWE (bit 43) must be initialized to 0b1
> > >> +
> > >> +  - If EL3 is present:
> > >> +
> > >> +    - 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 4ef52d7245bb..2fbfe27d38b5 100644
> > >> --- a/arch/arm64/include/asm/el2_setup.h
> > >> +++ b/arch/arm64/include/asm/el2_setup.h
> > >> @@ -105,6 +105,13 @@
> > >>                                              // 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_\@
> > >> +
> > >> +    orr     x2, x2, #MDCR_EL2_EBWE
> > >> +.Lskip_dbg_v8p9_\@:
> > >>      msr     mdcr_el2, x2                    // Configure debug traps
> > >>  .endm
> > >>
> > >> @@ -244,6 +251,24 @@
> > >>  .Lskip_gcs_\@:
> > >>  .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
> > >
> > > We already read this field in __init_el2_fgt, shouldn't we leverage that
> > > and move all this there rather than read the feature reg twice.
> >
> > Should not __init_el2_fgt2() remain separate to contain all future FEAT_FGT2
> > related trap enabled/disable checks ? OTOH reading id_aa64mmfr0_el1 register
> > should improve some performance as well.
> 
> That's the tradeoff. I'll defer to others whether a single id register
> read here is preferred.

Are we *really* talking about *performance* here?  For something that
is executed once per CPU boot? Just keep the damn thing readable, if
at all possible...

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH V3 6/7] arm64/boot: Enable EL2 requirements for FEAT_Debugv8p9

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Wed, Dec 18, 2024 at 09:10:51AM +0000, Marc Zyngier wrote:

> But I don't think we should be prescriptive about the state of these
> registers, as long as the potential traps are correctly handled.

The rest of the document is written in terms of explicit register
values, if we're changing approach we should probably update all the
existing requirements to be written in the same way since having a mix
of approaches tends to be a big red flag that there should be different
handling.  Consistency will make the document clearer and easier for
people to work with.

FWIW there is an explict note in the document about the fact that it's
an as-if rule:

|                                                Where the values documented
| disable traps it is permissible for these traps to be enabled so long as
| those traps are handled transparently by higher exception levels as though
| the values documented were set.

from when I raised this before.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH V3 7/7] 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>
---
Changes in V3:

- Used SYS_FIELD_PREP() in read_wb_reg() and write_wb_reg()
- Added MAX_PER_BANK based BUILD_BUG_ON() tests in arch_hw_breakpoint_init()
- Dropped local variables i.e mdsel_bank and index
- Derived bank and index from MAX_PER_BANK as required

 arch/arm64/include/asm/debug-monitors.h |  1 +
 arch/arm64/include/asm/hw_breakpoint.h  | 47 ++++++++++++++++++------
 arch/arm64/kernel/debug-monitors.c      | 15 +++++---
 arch/arm64/kernel/hw_breakpoint.c       | 48 +++++++++++++++++++++++--
 4 files changed, 95 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 8f6ba31b8658..56b89a582a0d 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..e48273b64109 100644
--- a/arch/arm64/include/asm/hw_breakpoint.h
+++ b/arch/arm64/include/asm/hw_breakpoint.h
@@ -79,8 +79,9 @@ 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
+#define MAX_PER_BANK		16
 
 /* Virtual debug register bases. */
 #define AARCH64_DBG_REG_BVR	0
@@ -94,6 +95,14 @@ 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);\
@@ -138,19 +147,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 58f047de3e1c..50779c68f11e 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -21,6 +21,7 @@
 #include <asm/cputype.h>
 #include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
+#include <asm/hw_breakpoint.h>
 #include <asm/system_misc.h>
 #include <asm/traps.h>
 
@@ -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);
 }
@@ -79,7 +80,7 @@ static DEFINE_PER_CPU(int, kde_ref_count);
 
 void enable_debug_monitors(enum dbg_active_el el)
 {
-	u32 mdscr, enable = 0;
+	u64 mdscr, enable = 0;
 
 	WARN_ON(preemptible());
 
@@ -90,6 +91,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())
+		enable |= DBG_MDSCR_EMBWE;
+
 	if (enable && debug_enabled) {
 		mdscr = mdscr_read();
 		mdscr |= enable;
@@ -100,7 +104,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 +115,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())
+		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..e9c87fb0e772 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -103,7 +103,7 @@ int hw_breakpoint_slots(int type)
 	WRITE_WB_REG_CASE(OFF, 14, REG, VAL);	\
 	WRITE_WB_REG_CASE(OFF, 15, REG, VAL)
 
-static u64 read_wb_reg(int reg, int n)
+static u64 __read_wb_reg(int reg, int n)
 {
 	u64 val = 0;
 
@@ -118,9 +118,31 @@ static u64 read_wb_reg(int reg, int n)
 
 	return val;
 }
+
+static u64 read_wb_reg(int reg, int n)
+{
+	unsigned long flags;
+	u64 val;
+
+	if (!is_debug_v8p9_enabled())
+		return __read_wb_reg(reg, n);
+
+	/*
+	 * Bank selection in MDSELR_EL1, followed by an indexed read from
+	 * breakpoint (or watchpoint) registers cannot be interrupted, as
+	 * that might cause misread from the wrong targets instead. Hence
+	 * this requires mutual exclusion.
+	 */
+	local_irq_save(flags);
+	write_sysreg_s(SYS_FIELD_PREP(MDSELR_EL1, BANK, n / MAX_PER_BANK), SYS_MDSELR_EL1);
+	isb();
+	val = __read_wb_reg(reg, n % MAX_PER_BANK);
+	local_irq_restore(flags);
+	return val;
+}
 NOKPROBE_SYMBOL(read_wb_reg);
 
-static void write_wb_reg(int reg, int n, u64 val)
+static void __write_wb_reg(int reg, int n, u64 val)
 {
 	switch (reg + n) {
 	GEN_WRITE_WB_REG_CASES(AARCH64_DBG_REG_BVR, AARCH64_DBG_REG_NAME_BVR, val);
@@ -132,6 +154,26 @@ static void write_wb_reg(int reg, int n, u64 val)
 	}
 	isb();
 }
+
+static void write_wb_reg(int reg, int n, u64 val)
+{
+	unsigned long flags;
+
+	if (!is_debug_v8p9_enabled())
+		return __write_wb_reg(reg, n, val);
+
+	/*
+	 * Bank selection in MDSELR_EL1, followed by an indexed read from
+	 * breakpoint (or watchpoint) registers cannot be interrupted, as
+	 * that might cause misread from the wrong targets instead. Hence
+	 * this requires mutual exclusion.
+	 */
+	local_irq_save(flags);
+	write_sysreg_s(SYS_FIELD_PREP(MDSELR_EL1, BANK, n / MAX_PER_BANK), SYS_MDSELR_EL1);
+	isb();
+	__write_wb_reg(reg, n % MAX_PER_BANK, val);
+	local_irq_restore(flags);
+}
 NOKPROBE_SYMBOL(write_wb_reg);
 
 /*
@@ -1005,6 +1047,8 @@ static int __init arch_hw_breakpoint_init(void)
 
 	/* Register cpu_suspend hw breakpoint restore hook */
 	cpu_suspend_set_dbg_restorer(hw_breakpoint_reset);
+	BUILD_BUG_ON((ARM_MAX_BRP % MAX_PER_BANK) != 0);
+	BUILD_BUG_ON((ARM_MAX_WRP % MAX_PER_BANK) != 0);
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH V3 7/7] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

[Mark Rutland]

On Mon, Dec 16, 2024 at 09:38:31AM +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.

[...]

> +static u64 read_wb_reg(int reg, int n)
> +{
> +	unsigned long flags;
> +	u64 val;
> +
> +	if (!is_debug_v8p9_enabled())
> +		return __read_wb_reg(reg, n);
> +
> +	/*
> +	 * Bank selection in MDSELR_EL1, followed by an indexed read from
> +	 * breakpoint (or watchpoint) registers cannot be interrupted, as
> +	 * that might cause misread from the wrong targets instead. Hence
> +	 * this requires mutual exclusion.
> +	 */
> +	local_irq_save(flags);
> +	write_sysreg_s(SYS_FIELD_PREP(MDSELR_EL1, BANK, n / MAX_PER_BANK), SYS_MDSELR_EL1);
> +	isb();
> +	val = __read_wb_reg(reg, n % MAX_PER_BANK);
> +	local_irq_restore(flags);
> +	return val;
> +}
>  NOKPROBE_SYMBOL(read_wb_reg);

I don't believe that disabling interrupts here is sufficient. On the
last version I asked about the case of racing with a watchpoint handler:

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

... and disabling interrupts cannot prevent that, because
local_irq_{save,restore}() do not affect the behaviour of watchpoints or
breakpoints.

Please can you try to answer the questions I asked last time, i.e.

| 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?

... and the question form the earlier comment, i.e.

| Is the existing code correct?

I suspect that the existing code might not have the necessary mutual
exclusion in all cases, but it's difficult to trigger an issue by
accident. Is there any way a handler could race with some other
manipulation of watchpoints/breakpoints such that some data structure
gets corrupted, or such that the kernel deadlocks?

Mark.