[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH 1/4] target/riscv: Add CSR tcontrol of debug trigger module
|
From: |
張哲嘉 |
|
Subject: |
RE: [PATCH 1/4] target/riscv: Add CSR tcontrol of debug trigger module |
|
Date: |
Fri, 16 Feb 2024 13:42:28 +0000 |
Hi Daniel,
> -----Original Message-----
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Sent: Friday, February 16, 2024 8:51 PM
> 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 1/4] target/riscv: Add CSR tcontrol of debug trigger
> module
>
>
>
> 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.
Yes, currently RISC-V QEMU doesn't have "Debug Mode", so I just ignore it and
only consider M-mode here.
>
> 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
> env->checking
> for PRV_M seems reasonable.
The riscv_csrrw_check() function has checked the privilege level by the
accessed CSR address.
Please see https://github.com/qemu/qemu/blob/master/target/riscv/csr.c#L4360.
If the current privilege level is lower than the accessed CSR, the illegal
instruction exception is raised.
According to privilege spec, each CSR address has two bits to encode the lowest
privilege level to access that CSR.
Sdtrig CSRs (tselect, tdata{1~3}, tinfo, mcontext, and tcontrol) are encoded
within 0x7A0~0x7AF,
Therefore, their lowest required privilege level is M-mode.
If env->priv < PRV_M, the checking on those CSR fails in riscv_csrrw_check().
Is this enough, or do I miss something?
Sincerely,
Alvin Chang
>
>
> Thanks,
>
>
> Daniel
>
>
>
>
>
> > [CSR_MCONTEXT] = { "mcontext", debug, read_mcontext,
> > write_mcontext },
> >
> > /* User Pointer Masking */