Re: [PATCH 1/4] target/riscv: Add CSR tcontrol of debug trigger module

[Daniel Henrique Barboza]

On 2/16/24 03:13, Alvin Chang wrote:
> The RISC-V debug specification defines an optional CSR "tcontrol" within
> the trigger module. This commit adds its read/write operations and
> related bit-field definitions.
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
>   target/riscv/cpu.h      |  1 +
>   target/riscv/cpu_bits.h |  3 +++
>   target/riscv/csr.c      | 15 +++++++++++++++
>   3 files changed, 19 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f52dce78ba..f9ae3e3025 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -364,6 +364,7 @@ struct CPUArchState {
>       target_ulong tdata1[RV_MAX_TRIGGERS];
>       target_ulong tdata2[RV_MAX_TRIGGERS];
>       target_ulong tdata3[RV_MAX_TRIGGERS];
> +    target_ulong tcontrol;
>       target_ulong mcontext;
>       struct CPUBreakpoint *cpu_breakpoint[RV_MAX_TRIGGERS];
>       struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index fc2068ee4d..3b3a7a0fa4 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -353,6 +353,7 @@
>   #define CSR_TDATA2          0x7a2
>   #define CSR_TDATA3          0x7a3
>   #define CSR_TINFO           0x7a4
> +#define CSR_TCONTROL        0x7a5
>   #define CSR_MCONTEXT        0x7a8
>   
>   /* Debug Mode Registers */
> @@ -900,6 +901,8 @@ typedef enum RISCVException {
>   #define JVT_BASE                           (~0x3F)
>   
>   /* Debug Sdtrig CSR masks */
> +#define TCONTROL_MTE                       BIT(3)
> +#define TCONTROL_MPTE                      BIT(7)
>   #define MCONTEXT32                         0x0000003F
>   #define MCONTEXT64                         0x0000000000001FFFULL
>   #define MCONTEXT32_HCONTEXT                0x0000007F
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d4e8ac13b9..816c530481 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3937,6 +3937,20 @@ static RISCVException read_tinfo(CPURISCVState *env, int 
> csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> +static RISCVException read_tcontrol(CPURISCVState *env, int csrno,
> +                                    target_ulong *val)
> +{
> +    *val = env->tcontrol;
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_tcontrol(CPURISCVState *env, int csrno,
> +                                     target_ulong val)
> +{
> +    env->tcontrol = val & (TCONTROL_MPTE | TCONTROL_MTE);
> +    return RISCV_EXCP_NONE;
> +}
> +
>   static RISCVException read_mcontext(CPURISCVState *env, int csrno,
>                                       target_ulong *val)
>   {
> @@ -4861,6 +4875,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>       [CSR_TDATA2]    =  { "tdata2",   debug, read_tdata,    write_tdata    },
>       [CSR_TDATA3]    =  { "tdata3",   debug, read_tdata,    write_tdata    },
>       [CSR_TINFO]     =  { "tinfo",    debug, read_tinfo,    write_ignore   },
> +    [CSR_TCONTROL]  =  { "tcontrol", debug, read_tcontrol, write_tcontrol },

The spec reads:

"This optional register is only accessible in M-mode and Debug Mode and provides
 various control bits related to triggers."

"debug()" is checking only if we have the 'debug' cpu option enabled:


static RISCVException debug(CPURISCVState *env, int csrno)
{
    if (riscv_cpu_cfg(env)->debug) {
        return RISCV_EXCP_NONE;
    }

    return RISCV_EXCP_ILLEGAL_INST;
}


It looks like we don't have a "Debug Mode" model.

Section 4.1 of the spec mentions the following about "Debug Mode":

"1. All implemented instructions operate just as they do in M-mode, unless an
 exception is mentioned in this list.
 2. All operations are executed with machine mode privilege, except that 
additional
 Debug Mode CSRs are accessible and MPRV in mstatus may be ignored according to
 mprven. Full permission checks, or a relaxed set of permission checks, will 
apply
 according to relaxedpriv (...)"


So, if the operations are "executed with machine mode privilege" then can we 
expect
env->priv == PRV_M ? As it is now tcontrol will execute in any mode, so checking
for PRV_M seems reasonable.


Thanks,


Daniel





>       [CSR_MCONTEXT]  =  { "mcontext", debug, read_mcontext, write_mcontext },
>   
>       /* User Pointer Masking */