Re: [PATCH v2 2/2] target/riscv: Simpily support versioning of debug trigger module

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 11/26/25 1:43 PM, Alvin Chang wrote:
> To support multiple versions of debug specification, we have added
> "debug-1.0" CPU property. Now the debug trigger module inspects this
> property to determine the supported trigger types by the CPU. In this
> commit we validate written trigger type with CPU debug version. For
> example, the debug specification v0.13 does not support mcontrol6, and
> the indended tdata_csr_write() on tdata1 with type=mcontrol6 will be

I suppose you meant 'intended'.


With the typo fixed:

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

> ignored.
> 
> If debug v1.0 is selected, the default trigger type is mcontrol6
> instead of legacy mcontrol.
> 
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> Reviewed-by: Yu-Ming Chang <yumin686@andestech.com>
> ---
>   target/riscv/debug.c | 56 +++++++++++++++++++++++++++++++++++++++++---
>   target/riscv/debug.h |  1 +
>   2 files changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/debug.c b/target/riscv/debug.c
> index 5664466..5163193 100644
> --- a/target/riscv/debug.c
> +++ b/target/riscv/debug.c
> @@ -64,6 +64,26 @@ static tdata_avail tdata_mapping[TRIGGER_TYPE_NUM] = {
>       [TRIGGER_TYPE_UNAVAIL] = { true, true, true }
>   };
>   
> +/* Valid trigger types supported by debug specification v0.13 */
> +static bool valid_trigger_type_v013[TRIGGER_TYPE_NUM] = {
> +    [TRIGGER_TYPE_AD_MATCH] = true,
> +    [TRIGGER_TYPE_INST_CNT] = true,
> +    [TRIGGER_TYPE_INT] = true,
> +    [TRIGGER_TYPE_EXCP] = true,
> +    [TRIGGER_TYPE_UNAVAIL] = true
> +};
> +
> +/* Valid trigger types supported by debug specification v1.0 */
> +static bool valid_trigger_type_v100[TRIGGER_TYPE_NUM] = {
> +    [TRIGGER_TYPE_AD_MATCH] = true,
> +    [TRIGGER_TYPE_INST_CNT] = true,
> +    [TRIGGER_TYPE_INT] = true,
> +    [TRIGGER_TYPE_EXCP] = true,
> +    [TRIGGER_TYPE_AD_MATCH6] = true,
> +    [TRIGGER_TYPE_EXT_SRC] = true,
> +    [TRIGGER_TYPE_DISABLED] = true
> +};
> +
>   /* only breakpoint size 1/2/4/8 supported */
>   static int access_size[SIZE_NUM] = {
>       [SIZE_ANY] = 0,
> @@ -95,6 +115,20 @@ static inline target_ulong get_trigger_type(CPURISCVState *env,
>       return extract_trigger_type(env, env->tdata1[trigger_index]);
>   }
>   
> +static inline bool validate_trigger_type(CPURISCVState *env,
> +                                         target_ulong trigger_type)
> +{
> +    if (trigger_type >= TRIGGER_TYPE_NUM) {
> +        return false;
> +    }
> +
> +    if (riscv_cpu_cfg(env)->debug_1_00) {
> +        return valid_trigger_type_v100[trigger_type];
> +    }
> +
> +    return valid_trigger_type_v013[trigger_type];
> +}
> +
>   static trigger_action_t get_trigger_action(CPURISCVState *env,
>                                              target_ulong trigger_index)
>   {
> @@ -889,6 +923,13 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>           trigger_type = get_trigger_type(env, env->trigger_cur);
>       }
>   
> +    if (!validate_trigger_type(env, trigger_type)) {
> +        /* Since the tdada1.type is WARL, we simpily ignore write here. */
> +        qemu_log_mask(LOG_UNIMP, "trigger type: %d is not supported\n",
> +                      trigger_type);
> +        return;
> +    }
> +
>       switch (trigger_type) {
>       case TRIGGER_TYPE_AD_MATCH:
>           type2_reg_write(env, env->trigger_cur, tdata_index, val);
> @@ -918,8 +959,11 @@ void tdata_csr_write(CPURISCVState *env, int tdata_index, target_ulong val)
>   target_ulong tinfo_csr_read(CPURISCVState *env)
>   {
>       /* assume all triggers support the same types of triggers */
> -    return BIT(TRIGGER_TYPE_AD_MATCH) |
> -           BIT(TRIGGER_TYPE_AD_MATCH6);
> +    if (riscv_cpu_cfg(env)->debug_1_00) {
> +        return BIT(TRIGGER_TYPE_AD_MATCH) | BIT(TRIGGER_TYPE_AD_MATCH6);
> +    }
> +
> +    return BIT(TRIGGER_TYPE_AD_MATCH);
>   }
>   
>   void riscv_cpu_debug_excp_handler(CPUState *cs)
> @@ -1056,9 +1100,15 @@ void riscv_trigger_realize(CPURISCVState *env)
>   
>   void riscv_trigger_reset_hold(CPURISCVState *env)
>   {
> -    target_ulong tdata1 = build_tdata1(env, TRIGGER_TYPE_AD_MATCH, 0, 0);
> +    target_ulong tdata1;
>       int i;
>   
> +    if (riscv_cpu_cfg(env)->debug_1_00) {
> +        tdata1 = build_tdata1(env, TRIGGER_TYPE_AD_MATCH6, 0, 0);
> +    } else {
> +        tdata1 = build_tdata1(env, TRIGGER_TYPE_AD_MATCH, 0, 0);
> +    }
> +
>       /* init to type 2 triggers */
>       for (i = 0; i < RV_MAX_TRIGGERS; i++) {
>           /*
> diff --git a/target/riscv/debug.h b/target/riscv/debug.h
> index f76b8f9..0127cb9 100644
> --- a/target/riscv/debug.h
> +++ b/target/riscv/debug.h
> @@ -43,6 +43,7 @@ typedef enum {
>       TRIGGER_TYPE_AD_MATCH6 = 6,     /* new address/data match trigger */
>       TRIGGER_TYPE_EXT_SRC = 7,       /* external source trigger */
>       TRIGGER_TYPE_UNAVAIL = 15,      /* trigger exists, but unavailable */
> +    TRIGGER_TYPE_DISABLED = 15,     /* trigger exists, but disabled */
>       TRIGGER_TYPE_NUM
>   } trigger_type_t;
>