RE: [PATCH v2] target/riscv: Implement optional CSR mcontext of debug Sdtrig extension

[張哲嘉]

> -----Original Message-----
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Sent: Wednesday, January 10, 2024 6:04 AM
> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>;
> qemu-riscv@nongnu.org; qemu-devel@nongnu.org
> Cc: alistair.francis@wdc.com; bin.meng@windriver.com;
> liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
> Subject: Re: [PATCH v2] target/riscv: Implement optional CSR mcontext of
> debug Sdtrig extension
> 
> 
> On 12/19/23 09:32, Alvin Chang wrote:
> > The debug Sdtrig extension defines an CSR "mcontext". This commit
> > implements its predicate and read/write operations into CSR table.
> > Its value is reset as 0 when the trigger module is reset.
> >
> > Signed-off-by: Alvin Chang <alvinga@andestech.com>
> > ---
> 
> The patch per se LGTM:
> 
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Thank you!!

> 
> 
> But I have a question: shouldn't we just go ahead and add the 'sdtrig'
> extension?
> We have a handful of its CSRs already. Adding the extension would also add
> 'sdtrig'
> in riscv,isa, allowing software to be aware of its existence in QEMU.

I agree with you. I can prepare another PR for adding 'sdtrig' extension.
BTW, currently we have "riscv_cpu_cfg(env)->debug" to control those trigger 
module CSRs.
Maybe we can just remove "debug" and use "sdtrig" instead ? 

Alvin

> 
> 
> Thanks,
> 
> Daniel
> 
> 
> 
> > Changes from v1: Remove dedicated cfg, always implement mcontext.
> >
> >   target/riscv/cpu.h      |  1 +
> >   target/riscv/cpu_bits.h |  7 +++++++
> >   target/riscv/csr.c      | 36 +++++++++++++++++++++++++++++++-----
> >   target/riscv/debug.c    |  2 ++
> >   4 files changed, 41 insertions(+), 5 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index
> > d74b361..e117641 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -345,6 +345,7 @@ struct CPUArchState {
> >       target_ulong tdata1[RV_MAX_TRIGGERS];
> >       target_ulong tdata2[RV_MAX_TRIGGERS];
> >       target_ulong tdata3[RV_MAX_TRIGGERS];
> > +    target_ulong mcontext;
> >       struct CPUBreakpoint *cpu_breakpoint[RV_MAX_TRIGGERS];
> >       struct CPUWatchpoint *cpu_watchpoint[RV_MAX_TRIGGERS];
> >       QEMUTimer *itrigger_timer[RV_MAX_TRIGGERS]; diff --git
> > a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index
> > ebd7917..3296648 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -361,6 +361,7 @@
> >   #define CSR_TDATA2          0x7a2
> >   #define CSR_TDATA3          0x7a3
> >   #define CSR_TINFO           0x7a4
> > +#define CSR_MCONTEXT        0x7a8
> >
> >   /* Debug Mode Registers */
> >   #define CSR_DCSR            0x7b0
> > @@ -905,4 +906,10 @@ typedef enum RISCVException {
> >   /* JVT CSR bits */
> >   #define JVT_MODE                           0x3F
> >   #define JVT_BASE                           (~0x3F)
> > +
> > +/* Debug Sdtrig CSR masks */
> > +#define MCONTEXT32                         0x0000003F
> > +#define MCONTEXT64
> 0x0000000000001FFFULL
> > +#define MCONTEXT32_HCONTEXT                0x0000007F
> > +#define MCONTEXT64_HCONTEXT
> 0x0000000000003FFFULL
> >   #endif
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c index
> > fde7ce1..ff1e128 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -3900,6 +3900,31 @@ static RISCVException read_tinfo(CPURISCVState
> *env, int csrno,
> >       return RISCV_EXCP_NONE;
> >   }
> >
> > +static RISCVException read_mcontext(CPURISCVState *env, int csrno,
> > +                                    target_ulong *val) {
> > +    *val = env->mcontext;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_mcontext(CPURISCVState *env, int csrno,
> > +                                     target_ulong val) {
> > +    bool rv32 = riscv_cpu_mxl(env) == MXL_RV32 ? true : false;
> > +    int32_t mask;
> > +
> > +    if (riscv_has_ext(env, RVH)) {
> > +        /* Spec suggest 7-bit for RV32 and 14-bit for RV64 w/ H extension
> */
> > +        mask = rv32 ? MCONTEXT32_HCONTEXT :
> MCONTEXT64_HCONTEXT;
> > +    } else {
> > +        /* Spec suggest 6-bit for RV32 and 13-bit for RV64 w/o H
> extension */
> > +        mask = rv32 ? MCONTEXT32 : MCONTEXT64;
> > +    }
> > +
> > +    env->mcontext = val & mask;
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> >   /*
> >    * Functions to access Pointer Masking feature registers
> >    * We have to check if current priv lvl could modify @@ -4794,11
> > +4819,12 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >       [CSR_PMPADDR15] =  { "pmpaddr15", pmp, read_pmpaddr,
> > write_pmpaddr },
> >
> >       /* Debug CSRs */
> > -    [CSR_TSELECT]   =  { "tselect", debug, read_tselect, write_tselect },
> > -    [CSR_TDATA1]    =  { "tdata1",  debug, read_tdata,
> write_tdata   },
> > -    [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_TSELECT]   =  { "tselect",  debug, read_tselect,
> write_tselect  },
> > +    [CSR_TDATA1]    =  { "tdata1",   debug, read_tdata,
> write_tdata    },
> > +    [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_MCONTEXT]  =  { "mcontext", debug, read_mcontext,
> > + write_mcontext },
> >
> >       /* User Pointer Masking */
> >       [CSR_UMTE]    =    { "umte",    pointer_masking, read_umte,
> write_umte },
> > diff --git a/target/riscv/debug.c b/target/riscv/debug.c index
> > 4945d1a..e30d99c 100644
> > --- a/target/riscv/debug.c
> > +++ b/target/riscv/debug.c
> > @@ -940,4 +940,6 @@ void riscv_trigger_reset_hold(CPURISCVState *env)
> >           env->cpu_watchpoint[i] = NULL;
> >           timer_del(env->itrigger_timer[i]);
> >       }
> > +
> > +    env->mcontext = 0;
> >   }