Re: [RFC 8/8] arm64/hw_breakpoint: Enable FEAT_Debugv8p9

From: Anshuman Khandual
Date: Mon Apr 15 2024 - 23:14:06 EST

Next message: Yunhui Cui: "[PATCH v3 1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init()"
Previous message: Huang, Ying: "Re: [PATCH v2 1/5] mm: swap: introduce swap_free_nr() for batched swap_free()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 4/5/24 15:56, Marc Zyngier wrote:
> On Fri, 05 Apr 2024 09:00:08 +0100,
> Anshuman Khandual <anshuman.khandual@xxxxxxx> wrote:
>>
>> Currently there can be maximum 16 breakpoints, and 16 watchpoints available
>> on a given platform - as detected from ID_AA64DFR0_EL1.[BRPs|WRPs] register
>> fields. But these breakpoint, and watchpoints can be extended further up to
>> 64 via a new arch feature FEAT_Debugv8p9.
>>
>> This first enables banked access for the breakpoint and watchpoint register
>> set via MDSELR_EL1, extended exceptions via MDSCR_EL1.EMBWE and determining
>> available breakpoints and watchpoints in the platform from ID_AA64DFR1_EL1,
>> when FEAT_Debugv8p9 is enabled.
>>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> 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.
>

Next message: Yunhui Cui: "[PATCH v3 1/3] riscv: cacheinfo: remove the useless parameter (node) of ci_leaf_init()"
Previous message: Huang, Ying: "Re: [PATCH v2 1/5] mm: swap: introduce swap_free_nr() for batched swap_free()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]