> -----Original Message----- > From: Alistair Francis <alistai...@gmail.com> > Sent: Monday, December 18, 2023 12:46 PM > To: Alvin Che-Chia Chang(張哲嘉) <alvi...@andestech.com> > Cc: qemu-ri...@nongnu.org; qemu-devel@nongnu.org; > alistair.fran...@wdc.com; bin.m...@windriver.com; liwei1...@gmail.com; > dbarb...@ventanamicro.com; zhiwei_...@linux.alibaba.com > Subject: Re: [PATCH] target/riscv: Implement optional CSR mcontext of debug > Sdtrig extension > > On Sun, Dec 17, 2023 at 5:17 PM Alvin Chang via <qemu-devel@nongnu.org> > wrote: > > > > The debug Sdtrig extension defines an optional CSR "mcontext". Since > > it is optional, this commit adds new CPU configuration > > "ext_sdtrig_mcontext" and uses property "sdtrig_mcontext" to control > > whether it is implemented or not. Its predicate and read/write > > operations are also implemented into CSR table. Its value is reset as > > 0 when the trigger module is reset. > > We don't support the Sdtrig extension though. I'm guessing it's all packaged > up > as part of the "debug" extension but should we expose Sdtrig before we expose > options for it?
Yes, Sdtrig extension is part of the "debug" extension. There have been several trigger implementations in target/riscv/debug.{c|h}. I can rename it to cfg.debug_mcontext instead. > > Also, why can't we just always implement mcontext if Sdtrig exists? Is there a > reason to gate it behind a config? > > Alistair I gate it behind a config because spec says that mcontext is optional CSR. Some CPUs might implement it (so they can say cfg->ext_sdtrig_mcontext = true; in their CPU init), while others don't. Or do you think we should always implement it? Alvin > > > > > Signed-off-by: Alvin Chang <alvi...@andestech.com> > > --- > > target/riscv/cpu.c | 4 ++++ > > target/riscv/cpu.h | 1 + > > target/riscv/cpu_bits.h | 7 +++++++ > > target/riscv/cpu_cfg.h | 1 + > > target/riscv/csr.c | 36 ++++++++++++++++++++++++++++++++++++ > > target/riscv/debug.c | 2 ++ > > 6 files changed, 51 insertions(+) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index > > 83c7c0c..dff757f 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -1479,6 +1479,10 @@ Property riscv_cpu_options[] = { > > DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, > cfg.cbom_blocksize, 64), > > DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, > > cfg.cboz_blocksize, 64), > > > > + /* Optional CSR of debug Sdtrig extension */ > > + DEFINE_PROP_BOOL("sdtrig_mcontext", RISCVCPU, > cfg.ext_sdtrig_mcontext, > > + false), > > + > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > 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/cpu_cfg.h b/target/riscv/cpu_cfg.h index > > f4605fb..4f1cb04 100644 > > --- a/target/riscv/cpu_cfg.h > > +++ b/target/riscv/cpu_cfg.h > > @@ -147,6 +147,7 @@ struct RISCVCPUConfig { > > bool pmp; > > bool debug; > > bool misa_w; > > + bool ext_sdtrig_mcontext; > > > > bool short_isa_string; > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c index > > fde7ce1..0b68787 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -549,6 +549,15 @@ static RISCVException debug(CPURISCVState *env, > > int csrno) > > > > return RISCV_EXCP_ILLEGAL_INST; > > } > > + > > +static RISCVException sdtrig_mcontext(CPURISCVState *env, int csrno) > > +{ > > + if (riscv_cpu_cfg(env)->debug && > riscv_cpu_cfg(env)->ext_sdtrig_mcontext) { > > + return RISCV_EXCP_NONE; > > + } > > + > > + return RISCV_EXCP_ILLEGAL_INST; > > +} > > #endif > > > > static RISCVException seed(CPURISCVState *env, int csrno) @@ -3900,6 > > +3909,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 @@ -4799,6 > > +4833,8 @@ 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_MCONTEXT] = { "mcontext", sdtrig_mcontext, > 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; > > } > > -- > > 2.34.1 > > > >