This series enables FEAT_Debugv8p9 thus extending breakpoint and watchpoint
support upto 64. This has been lightly tested and still work is in progress
but would like to get some early feedback on the approach.
Possible impact of context switches while tracing kernel addresses needs to
be evaluated regarding MDSELR_EL1 access. This series is based on v6.9-rc2.
Cc: Jonathan Corbet <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Oliver Upton <[email protected]>
Cc: James Morse <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Anshuman Khandual (8):
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/sysreg: Update ID_AA64MMFR0_EL1 register
KVM: arm64: Explicitly handle MDSELR_EL1 traps as UNDEFINED
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 | 2 +-
arch/arm64/include/asm/el2_setup.h | 27 ++++++++++
arch/arm64/include/asm/hw_breakpoint.h | 46 +++++++++++++----
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 ++++++++++++
arch/arm64/kvm/sys_regs.c | 1 +
arch/arm64/tools/sysreg | 68 +++++++++++++++++++++++++
10 files changed, 214 insertions(+), 20 deletions(-)
--
2.25.1
This adds register fields for MDSELR_EL1 as per the definitions based
on DDI0601 2023-12.
Cc: Will Deacon <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
arch/arm64/tools/sysreg | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index a4c1dd4741a4..4c58fd7a70e6 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
This adds register fields for HDFGRTR2_EL2 as per the definitions based
on DDI0601 2023-12.
Cc: Will Deacon <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
arch/arm64/tools/sysreg | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 4c58fd7a70e6..9bcd8a0d55c4 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -2444,6 +2444,34 @@ Field 1 ICIALLU
Field 0 ICIALLUIS
EndSysreg
+Sysreg HDFGRTR2_EL2 3 4 3 1 0
+Res0 63:24
+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
This updates ID_AA64MMFR0_EL1.FGT and ID_AA64MMFR0_EL1.PARANGE register
fields as per the definitions based on DDI0601 2023-12.
Cc: Will Deacon <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
arch/arm64/tools/sysreg | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index c38414352dd8..7ba4fa99c160 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -1567,6 +1567,7 @@ EndEnum
UnsignedEnum 59:56 FGT
0b0000 NI
0b0001 IMP
+ 0b0010 FGT2
EndEnum
Res0 55:48
UnsignedEnum 47:44 EXS
@@ -1628,6 +1629,7 @@ Enum 3:0 PARANGE
0b0100 44
0b0101 48
0b0110 52
+ 0b0111 56
EndEnum
EndSysreg
--
2.25.1
Currently read_sanitised_id_aa64dfr0_el1() caps the ID_AA64DFR0.DebugVer to
ID_AA64DFR0_DebugVer_V8P8, resulting in FEAT_Debugv8p9 not being exposed to
the guest. MDSELR_EL1 register access in the guest, is currently trapped by
the existing configuration of the fine-grained traps.
As the register is not described in sys_reg_descs[] table emulate_sys_reg()
will warn that this is unknown access before injecting an UNDEFINED
exception into the guest. Any well-behaved guests shouldn't try to use this
register, but any badly-behaved guests could, thus resulting in unnecessary
warnings. To avoid such warnings, access to MDSELR_EL1 should be explicitly
handled as UNDEFINED via updating sys_reg_desc[] as required.
Cc: Marc Zyngier <[email protected]>
Cc: Oliver Upton <[email protected]>
Cc: James Morse <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
arch/arm64/kvm/sys_regs.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c9f4f387155f..2956bdcd358e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2203,6 +2203,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_MDSCR_EL1), trap_debug_regs, reset_val, MDSCR_EL1, 0 },
DBG_BCR_BVR_WCR_WVR_EL1(2),
DBG_BCR_BVR_WCR_WVR_EL1(3),
+ { SYS_DESC(SYS_MDSELR_EL1), undef_access },
DBG_BCR_BVR_WCR_WVR_EL1(4),
DBG_BCR_BVR_WCR_WVR_EL1(5),
DBG_BCR_BVR_WCR_WVR_EL1(6),
--
2.25.1
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 <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Oliver Upton <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
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 b7afaa026842..0425067a93d9 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -96,6 +96,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
@@ -203,6 +211,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
@@ -228,6 +254,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 e01bb5ca13b7..9d77dfc43e08 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -306,6 +306,7 @@
BIT(11))
/* 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
This adds register fields for HDFGWTR2_EL2 as per the definitions based
on DDI0601 2023-12.
Cc: Will Deacon <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
arch/arm64/tools/sysreg | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/arch/arm64/tools/sysreg b/arch/arm64/tools/sysreg
index 9bcd8a0d55c4..c38414352dd8 100644
--- a/arch/arm64/tools/sysreg
+++ b/arch/arm64/tools/sysreg
@@ -2472,6 +2472,33 @@ Field 1 nPMIAR_EL1
Field 0 nPMECR_EL1
EndSysreg
+Sysreg HDFGWTR2_EL2 3 4 3 1 1
+Res0 63:24
+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
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 <[email protected]>
Cc: Will Deacon <[email protected]>
cc: Mark Brown <[email protected]
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
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 56583677c1f2..128f2836fc1e 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -527,6 +527,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_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_ABL_CMPs_SHIFT, 8, 0),
+ ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, 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_STRICT, 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_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_CTX_CMPs_SHIFT, 8, 0),
+ ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR1_EL1_WRPs_SHIFT, 8, 0),
+ ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, 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),
@@ -705,10 +720,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){ \
@@ -781,7 +792,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
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 <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
arch/arm64/include/asm/debug-monitors.h | 2 +-
arch/arm64/include/asm/hw_breakpoint.h | 46 +++++++++++++++++++------
arch/arm64/kernel/debug-monitors.c | 16 ++++++---
arch/arm64/kernel/hw_breakpoint.c | 33 ++++++++++++++++++
4 files changed, 82 insertions(+), 15 deletions(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 13d437bcbf58..75eedba2abbe 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -19,7 +19,7 @@
/* MDSCR_EL1 enabling bits */
#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..6b9822140f71 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
@@ -135,22 +135,48 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
}
#endif
+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;
+}
+
/* 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 64f2ecbdfe5c..3299d1e8dc61 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 2f5755192c2b..7b9169535b76 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);
+ return -EINVAL;
+ }
+ 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
On Fri, 05 Apr 2024 09:00:05 +0100,
Anshuman Khandual <[email protected]> wrote:
>
> Currently read_sanitised_id_aa64dfr0_el1() caps the ID_AA64DFR0.DebugVer to
> ID_AA64DFR0_DebugVer_V8P8, resulting in FEAT_Debugv8p9 not being exposed to
> the guest. MDSELR_EL1 register access in the guest, is currently trapped by
> the existing configuration of the fine-grained traps.
Please add support for the HDFGxTR2_EL2 registers in the trap routing
arrays, add support for the corresponding FGUs in the corresponding
structure, and condition the UNDEF on the lack of *guest* support for
the feature.
In short, implement the architecture as described in the pseudocode,
and not a cheap shortcut.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Fri, 05 Apr 2024 09:00:08 +0100,
Anshuman Khandual <[email protected]> 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 <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> arch/arm64/include/asm/debug-monitors.h | 2 +-
> arch/arm64/include/asm/hw_breakpoint.h | 46 +++++++++++++++++++------
> arch/arm64/kernel/debug-monitors.c | 16 ++++++---
> arch/arm64/kernel/hw_breakpoint.c | 33 ++++++++++++++++++
> 4 files changed, 82 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 13d437bcbf58..75eedba2abbe 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -19,7 +19,7 @@
> /* MDSCR_EL1 enabling bits */
> #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..6b9822140f71 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
> @@ -135,22 +135,48 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
> }
> #endif
>
> +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;
> +}
> +
> /* 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 64f2ecbdfe5c..3299d1e8dc61 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>
include order.
>
> /* 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 2f5755192c2b..7b9169535b76 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);
> + return -EINVAL;
> + }
> + 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);
Are these things guaranteed to be only used in contexts where there
can be no interleaving of such operations? If any interleaving can
occur, this is broken.
M.
--
Without deviation from the norm, progress is not possible.
On Fri, Apr 05, 2024 at 01:30:01PM +0530, Anshuman Khandual wrote:
> This adds register fields for MDSELR_EL1 as per the definitions based
> on DDI0601 2023-12.
Reviewed-by: Mark Brown <[email protected]>
against DDI0601 2024-03.
> +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
I think this is a reasonable translation of the values for BANK.
On Fri, Apr 05, 2024 at 01:30:02PM +0530, Anshuman Khandual wrote:
> This adds register fields for HDFGRTR2_EL2 as per the definitions based
> on DDI0601 2023-12.
Reviewed-by: Mark Brown <[email protected]>
against DDI0601 2024-03.
On Fri, Apr 05, 2024 at 01:30:03PM +0530, Anshuman Khandual wrote:
> This adds register fields for HDFGWTR2_EL2 as per the definitions based
> on DDI0601 2023-12.
Reviewed-by: Mark Brown <[email protected]>
aginst DDT0601 2024-03.
On Fri, Apr 05, 2024 at 01:30:04PM +0530, Anshuman Khandual wrote:
> This updates ID_AA64MMFR0_EL1.FGT and ID_AA64MMFR0_EL1.PARANGE register
> fields as per the definitions based on DDI0601 2023-12.
Reviewed-by: Mark Brown <[email protected]>
against DDI0601 2024-03
On 4/5/24 15:45, Marc Zyngier wrote:
> On Fri, 05 Apr 2024 09:00:05 +0100,
> Anshuman Khandual <[email protected]> wrote:
>>
>> Currently read_sanitised_id_aa64dfr0_el1() caps the ID_AA64DFR0.DebugVer to
>> ID_AA64DFR0_DebugVer_V8P8, resulting in FEAT_Debugv8p9 not being exposed to
>> the guest. MDSELR_EL1 register access in the guest, is currently trapped by
>> the existing configuration of the fine-grained traps.
>
> Please add support for the HDFGxTR2_EL2 registers in the trap routing
> arrays, add support for the corresponding FGUs in the corresponding
Afraid that I might not have enough background here to sufficiently understand
your suggestion above, but nonetheless here is an attempt in this regard.
- Add HDFGRTR2_EL2/HDFGWTR2_EL2 to enum vcpu_sysreg
enum vcpu_sysreg {
..........
VNCR(HDFGRTR2_EL2),
VNCR(HDFGWTR2_EL2),
..........
}
- Add their VNCR mappings addresses
#define VNCR_HDFGRTR2_EL2 0x1A0
#define VNCR_HDFGWTR2_EL2 0x1B0
- Add HDFGRTR2_EL2/HDFGWTR2_EL2 to sys_reg_descs[]
static const struct sys_reg_desc sys_reg_descs[] = {
..........
EL2_REG_VNCR(HDFGRTR2_EL2, reset_val, 0),
EL2_REG_VNCR(HDFGWTR2_EL2, reset_val, 0),
..........
}
- Add HDFGRTR2_GROUP to enum fgt_group_id
- Add HDFGRTR2_GROUP to reg_to_fgt_group_id()
- Update triage_sysreg_trap() for HDFGRTR2_GROUP
- Update __activate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2
- Updated __deactivate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2
> structure, and condition the UNDEF on the lack of *guest* support for
> the feature.
Does something like the following looks OK for preventing guest access into
MDSELR_EL1 instead ?
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1711,6 +1711,19 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
return val;
}
+static bool trap_mdselr_el1(struct kvm_vcpu *vcpu,
+ struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ u64 dfr0 = read_sanitised_id_aa64dfr0_el1(vcpu, r);
+ int dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
+
+ if (dver != ID_AA64DFR0_EL1_DebugVer_V8P9)
+ return undef_access(vcpu, p, r);
+
+ return true;
+}
+
static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *rd,
u64 val)
@@ -2203,7 +2216,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ SYS_DESC(SYS_MDSCR_EL1), trap_debug_regs, reset_val, MDSCR_EL1, 0 },
DBG_BCR_BVR_WCR_WVR_EL1(2),
DBG_BCR_BVR_WCR_WVR_EL1(3),
- { SYS_DESC(SYS_MDSELR_EL1), undef_access },
+ { SYS_DESC(SYS_MDSELR_EL1), trap_mdselr_el1 },
DBG_BCR_BVR_WCR_WVR_EL1(4),
DBG_BCR_BVR_WCR_WVR_EL1(5),
DBG_BCR_BVR_WCR_WVR_EL1(6),
I am sure this is rather incomplete, but will really appreciate if you could
provide some details and pointers.
>
> In short, implement the architecture as described in the pseudocode,
> and not a cheap shortcut.
>
> Thanks,
>
> M.
>
On Fri, 12 Apr 2024 03:41:23 +0100,
Anshuman Khandual <[email protected]> wrote:
>
>
>
> On 4/5/24 15:45, Marc Zyngier wrote:
> > On Fri, 05 Apr 2024 09:00:05 +0100,
> > Anshuman Khandual <[email protected]> wrote:
> >>
> >> Currently read_sanitised_id_aa64dfr0_el1() caps the ID_AA64DFR0.DebugVer to
> >> ID_AA64DFR0_DebugVer_V8P8, resulting in FEAT_Debugv8p9 not being exposed to
> >> the guest. MDSELR_EL1 register access in the guest, is currently trapped by
> >> the existing configuration of the fine-grained traps.
> >
> > Please add support for the HDFGxTR2_EL2 registers in the trap routing
> > arrays, add support for the corresponding FGUs in the corresponding
>
> Afraid that I might not have enough background here to sufficiently understand
> your suggestion above, but nonetheless here is an attempt in this regard.
Thanks for at least giving it a try, this is *MUCH* appreciated.
>
> - Add HDFGRTR2_EL2/HDFGWTR2_EL2 to enum vcpu_sysreg
> enum vcpu_sysreg {
> ..........
> VNCR(HDFGRTR2_EL2),
> VNCR(HDFGWTR2_EL2),
> ..........
> }
Yes.
>
> - Add their VNCR mappings addresses
>
> #define VNCR_HDFGRTR2_EL2 0x1A0
> #define VNCR_HDFGWTR2_EL2 0x1B0
Yes.
>
> - Add HDFGRTR2_EL2/HDFGWTR2_EL2 to sys_reg_descs[]
>
> static const struct sys_reg_desc sys_reg_descs[] = {
> ..........
> EL2_REG_VNCR(HDFGRTR2_EL2, reset_val, 0),
> EL2_REG_VNCR(HDFGWTR2_EL2, reset_val, 0),
> ..........
> }
Yes
>
> - Add HDFGRTR2_GROUP to enum fgt_group_id
> - Add HDFGRTR2_GROUP to reg_to_fgt_group_id()
> - Update triage_sysreg_trap() for HDFGRTR2_GROUP
> - Update __activate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2
> - Updated __deactivate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2
Yes. Don't miss check_fgt_bit() though. You also need to update
kvm_init_nv_sysregs() to ensure that these new registers have the
correct RES0/RES1 behaviour depending on the supported feature set for
the guest.
>
> > structure, and condition the UNDEF on the lack of *guest* support for
> > the feature.
>
> Does something like the following looks OK for preventing guest access into
> MDSELR_EL1 instead ?
>
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1711,6 +1711,19 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> return val;
> }
>
> +static bool trap_mdselr_el1(struct kvm_vcpu *vcpu,
> + struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + u64 dfr0 = read_sanitised_id_aa64dfr0_el1(vcpu, r);
> + int dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
> +
> + if (dver != ID_AA64DFR0_EL1_DebugVer_V8P9)
> + return undef_access(vcpu, p, r);
This is very cumbersome, and we now have a much better infrastructure
for the stuff that is handled with FGTs, see below.
> +
> + return true;
> +}
> +
> static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *rd,
> u64 val)
> @@ -2203,7 +2216,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> { SYS_DESC(SYS_MDSCR_EL1), trap_debug_regs, reset_val, MDSCR_EL1, 0 },
> DBG_BCR_BVR_WCR_WVR_EL1(2),
> DBG_BCR_BVR_WCR_WVR_EL1(3),
> - { SYS_DESC(SYS_MDSELR_EL1), undef_access },
> + { SYS_DESC(SYS_MDSELR_EL1), trap_mdselr_el1 },
> DBG_BCR_BVR_WCR_WVR_EL1(4),
> DBG_BCR_BVR_WCR_WVR_EL1(5),
> DBG_BCR_BVR_WCR_WVR_EL1(6),
>
> I am sure this is rather incomplete, but will really appreciate if you could
> provide some details and pointers.
What is missing is the Fine-Grained-Undef part. You need to update
kvm_init_sysreg() so that kvm->arch.fgu[HDFGRTR2_GROUP] has all the
correct bits set for anything that needs to UNDEF depending on the
guest configuration.
For example, in your case, I'd expect to see something like:
if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9))
kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSELR_EL1 | [...]);
Then allowing the feature becomes conditioned on the bit being clear,
and the trap handler only needs to deal with the actual emulation, and
not the feature checking.
I appreciate that this is a lot to swallow, but I'd be very happy to
review patches implementing this and provide guidance. It is all
pretty simple, just that there is a lot of parts all over the place.
In the end, this is only about following the architecture.
Thanks again,
M.
--
Without deviation from the norm, progress is not possible.
On 4/5/24 15:56, Marc Zyngier wrote:
> On Fri, 05 Apr 2024 09:00:08 +0100,
> Anshuman Khandual <[email protected]> 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 <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> arch/arm64/include/asm/debug-monitors.h | 2 +-
>> arch/arm64/include/asm/hw_breakpoint.h | 46 +++++++++++++++++++------
>> arch/arm64/kernel/debug-monitors.c | 16 ++++++---
>> arch/arm64/kernel/hw_breakpoint.c | 33 ++++++++++++++++++
>> 4 files changed, 82 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
>> index 13d437bcbf58..75eedba2abbe 100644
>> --- a/arch/arm64/include/asm/debug-monitors.h
>> +++ b/arch/arm64/include/asm/debug-monitors.h
>> @@ -19,7 +19,7 @@
>> /* MDSCR_EL1 enabling bits */
>> #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..6b9822140f71 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
>> @@ -135,22 +135,48 @@ static inline void ptrace_hw_copy_thread(struct task_struct *task)
>> }
>> #endif
>>
>> +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;
>> +}
>> +
>> /* 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 64f2ecbdfe5c..3299d1e8dc61 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>
>
> include order.
Sure, will fix the order here.
>
>>
>> /* 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 2f5755192c2b..7b9169535b76 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);
>> + return -EINVAL;
>> + }
>> + 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);
>
> Are these things guaranteed to be only used in contexts where there
> can be no interleaving of such operations? If any interleaving can
> occur, this is broken.
That is a valid concern, breakpoints and watchpoints get used in multiple
code paths such perf, ptrace etc, where preemption might cause wrong bank
to be selected before the breakpoint-watchpoint registers being read thus
impacting the state. I guess preemption should be disabled between those
two operations i.e selection of the bank and reading its registers
>
> M.
>
On 4/5/24 13:30, Anshuman Khandual wrote:
> This series enables FEAT_Debugv8p9 thus extending breakpoint and watchpoint
> support upto 64. This has been lightly tested and still work is in progress
> but would like to get some early feedback on the approach.
>
> Possible impact of context switches while tracing kernel addresses needs to
> be evaluated regarding MDSELR_EL1 access. This series is based on v6.9-rc2.
>
> Cc: Jonathan Corbet <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Oliver Upton <[email protected]>
> Cc: James Morse <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Mark Brown <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Anshuman Khandual (8):
> 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/sysreg: Update ID_AA64MMFR0_EL1 register
Since the above patches add and update register definitions related to
HW breakpoints, and have already been reviewed by Mark, will send them
independently.
On 4/12/24 16:35, Marc Zyngier wrote:
> On Fri, 12 Apr 2024 03:41:23 +0100,
> Anshuman Khandual <[email protected]> wrote:
>>
>>
>>
>> On 4/5/24 15:45, Marc Zyngier wrote:
>>> On Fri, 05 Apr 2024 09:00:05 +0100,
>>> Anshuman Khandual <[email protected]> wrote:
>>>>
>>>> Currently read_sanitised_id_aa64dfr0_el1() caps the ID_AA64DFR0.DebugVer to
>>>> ID_AA64DFR0_DebugVer_V8P8, resulting in FEAT_Debugv8p9 not being exposed to
>>>> the guest. MDSELR_EL1 register access in the guest, is currently trapped by
>>>> the existing configuration of the fine-grained traps.
>>>
>>> Please add support for the HDFGxTR2_EL2 registers in the trap routing
>>> arrays, add support for the corresponding FGUs in the corresponding
>>
>> Afraid that I might not have enough background here to sufficiently understand
>> your suggestion above, but nonetheless here is an attempt in this regard.
>
> Thanks for at least giving it a try, this is *MUCH* appreciated.
>
>>
>> - Add HDFGRTR2_EL2/HDFGWTR2_EL2 to enum vcpu_sysreg
>> enum vcpu_sysreg {
>> ..........
>> VNCR(HDFGRTR2_EL2),
>> VNCR(HDFGWTR2_EL2),
>> ..........
>> }
>
> Yes.
>
>>
>> - Add their VNCR mappings addresses
>>
>> #define VNCR_HDFGRTR2_EL2 0x1A0
>> #define VNCR_HDFGWTR2_EL2 0x1B0
>
> Yes.
>
>>
>> - Add HDFGRTR2_EL2/HDFGWTR2_EL2 to sys_reg_descs[]
>>
>> static const struct sys_reg_desc sys_reg_descs[] = {
>> ..........
>> EL2_REG_VNCR(HDFGRTR2_EL2, reset_val, 0),
>> EL2_REG_VNCR(HDFGWTR2_EL2, reset_val, 0),
>> ..........
>> }
>
> Yes
>
>>
>> - Add HDFGRTR2_GROUP to enum fgt_group_id
>> - Add HDFGRTR2_GROUP to reg_to_fgt_group_id()
>> - Update triage_sysreg_trap() for HDFGRTR2_GROUP
>> - Update __activate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2
>> - Updated __deactivate_traps_hfgxtr() both for HDFGRTR2_EL2 and HDFGWTR2_EL2
>
> Yes. Don't miss check_fgt_bit() though. You also need to update
Right, added the following in there.
case HDFGRTR2_GROUP:
sr = is_read ? HDFGRTR2_EL2 : HDFGWTR2_EL2;
break;
> kvm_init_nv_sysregs() to ensure that these new registers have the
> correct RES0/RES1 behaviour depending on the supported feature set for
> the guest.
Following might be sufficient for MDSELR_EL1, but wondering if these fine
grained control registers (HDFG[RW]TR2_EL2) need to be completely defined
for the entire guest feature set, probably required.
/* HDFG[RW]TR2_EL2 */
res0 = res1 = 0;
if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9))
res0 |= HDFGRTR2_EL2_nMDSELR_EL1;
set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1);
set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1);
>
>>
>>> structure, and condition the UNDEF on the lack of *guest* support for
>>> the feature.
>>
>> Does something like the following looks OK for preventing guest access into
>> MDSELR_EL1 instead ?
>>
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1711,6 +1711,19 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>> return val;
>> }
>>
>> +static bool trap_mdselr_el1(struct kvm_vcpu *vcpu,
>> + struct sys_reg_params *p,
>> + const struct sys_reg_desc *r)
>> +{
>> + u64 dfr0 = read_sanitised_id_aa64dfr0_el1(vcpu, r);
>> + int dver = cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_DebugVer_SHIFT);
>> +
>> + if (dver != ID_AA64DFR0_EL1_DebugVer_V8P9)
>> + return undef_access(vcpu, p, r);
>
> This is very cumbersome, and we now have a much better infrastructure
> for the stuff that is handled with FGTs, see below.
Okay
>
>> +
>> + return true;
>> +}
>> +
>> static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>> const struct sys_reg_desc *rd,
>> u64 val)
>> @@ -2203,7 +2216,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>> { SYS_DESC(SYS_MDSCR_EL1), trap_debug_regs, reset_val, MDSCR_EL1, 0 },
>> DBG_BCR_BVR_WCR_WVR_EL1(2),
>> DBG_BCR_BVR_WCR_WVR_EL1(3),
>> - { SYS_DESC(SYS_MDSELR_EL1), undef_access },
>> + { SYS_DESC(SYS_MDSELR_EL1), trap_mdselr_el1 },
>> DBG_BCR_BVR_WCR_WVR_EL1(4),
>> DBG_BCR_BVR_WCR_WVR_EL1(5),
>> DBG_BCR_BVR_WCR_WVR_EL1(6),
>>
>> I am sure this is rather incomplete, but will really appreciate if you could
>> provide some details and pointers.
>
> What is missing is the Fine-Grained-Undef part. You need to update
> kvm_init_sysreg() so that kvm->arch.fgu[HDFGRTR2_GROUP] has all the
> correct bits set for anything that needs to UNDEF depending on the
> guest configuration.
>
> For example, in your case, I'd expect to see something like:
>
> if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9))
> kvm->arch.fgu[HDFGRTR2_GROUP] |= ~(HDFGRTR2_EL2_nMDSELR_EL1 | [...]);
Understood.
>
> Then allowing the feature becomes conditioned on the bit being clear,
> and the trap handler only needs to deal with the actual emulation, and
> not the feature checking.
Got it.
>
> I appreciate that this is a lot to swallow, but I'd be very happy to
> review patches implementing this and provide guidance. It is all
> pretty simple, just that there is a lot of parts all over the place.
> In the end, this is only about following the architecture.
Sure, will read through all these pointers you have mentioned here,
and be back with an implementation.
>
> Thanks again,
Thanks for the detailed explanation.
>
> M.
>
On Tue, 16 Apr 2024 06:46:13 +0100,
Anshuman Khandual <[email protected]> wrote:
>
> On 4/12/24 16:35, Marc Zyngier wrote:
> > kvm_init_nv_sysregs() to ensure that these new registers have the
> > correct RES0/RES1 behaviour depending on the supported feature set for
> > the guest.
>
> Following might be sufficient for MDSELR_EL1, but wondering if these fine
> grained control registers (HDFG[RW]TR2_EL2) need to be completely defined
> for the entire guest feature set, probably required.
Yes, you should check for all features defining a valid bit in these
registers, and apply the correct mask if the feature isn't advertised
to the guest, even if KVM doesn't currently support the feature at
all. This is a bit cumbersome at first, but we don't have to revisit
it when the feature gets enabled, which is a massive maintainability
improvement.
It also means that we just have to read the documentation and match it
against the code, which should be pretty trivial.
>
> /* HDFG[RW]TR2_EL2 */
> res0 = res1 = 0;
> if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, DebugVer, V8P9))
> res0 |= HDFGRTR2_EL2_nMDSELR_EL1;
> set_sysreg_masks(kvm, HDFGRTR2_EL2, res0 | HDFGRTR2_EL2_RES0, res1);
> set_sysreg_masks(kvm, HDFGWTR2_EL2, res0 | HDFGWTR2_EL2_RES0, res1);
Yup, this looks sensible for that particular bit. A few more to
go... ;-)
Thanks,
M.
--
Without deviation from the norm, progress is not possible.