> -----Original Messages-----
> From: "Daniel Henrique Barboza" <dbarb...@ventanamicro.com>
> Sent Time: 2023-03-09 03:44:03 (Thursday)
> To: chenyi2...@zju.edu.cn, qemu-devel@nongnu.org
> Cc: "Palmer Dabbelt" <pal...@dabbelt.com>, "Alistair Francis"
> <alistair.fran...@wdc.com>, "Bin Meng" <bin.m...@windriver.com>, "Weiwei Li"
> <liwei...@iscas.ac.cn>, "Liu Zhiwei" <zhiwei_...@linux.alibaba.com>, "open
> list:RISC-V TCG CPUs" <qemu-ri...@nongnu.org>
> Subject: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
>
>
>
> On 3/8/23 09:34, chenyi2...@zju.edu.cn wrote:
> > From: Yi Chen <chenyi2...@zju.edu.cn>
> >
> > Trap accesses to hgatp if MSTATUS_TVM is enabled.
> > Don't trap accesses to vsatp even if MSTATUS_TVM is enabled.
> >
> > Signed-off-by: Yi Chen <chenyi2...@zju.edu.cn>
> > ---
> > target/riscv/csr.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index ab56663..09bc780 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -2655,7 +2655,7 @@ static RISCVException read_satp(CPURISCVState *env,
> > int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) &&
> > get_field(env->mstatus, MSTATUS_TVM)) {
>
> The commit message mentions 'vsatp' but this patch is changing satp callbacks.
>
> Any reason to not change read_vsatp() and write_vsatp() instead?
read_vsatp() and write_vsatp() have correctly implemented the behavior of
MSTATUS.TVM.
Meanwhile, if an HS-mode hart tries to access 'satp', what it actually accesses
is 'vsatp' according to the ISA. In Qemu's implementation, the 'satp' callbacks
are called at first, and riscv_cpu_swap_hypervisor_regs() will be called
afterward. So we also need to modify read_satp() and write_satp().
>
> > return RISCV_EXCP_ILLEGAL_INST;
> > } else {
> > *val = env->satp;
> > @@ -2683,7 +2683,7 @@ static RISCVException write_satp(CPURISCVState *env,
> > int csrno,
> > }
> >
> > if (vm && mask) {
> > - if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > + if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) &&
> > get_field(env->mstatus, MSTATUS_TVM)) {
> > return RISCV_EXCP_ILLEGAL_INST;
> > } else {
> > /*
> > @@ -3047,14 +3047,24 @@ static RISCVException read_hgeip(CPURISCVState
> > *env, int csrno,
> > static RISCVException read_hgatp(CPURISCVState *env, int csrno,
> > target_ulong *val)
> > {
> > - *val = env->hgatp;
> > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
>
> The end of the first paragraph of ISA 8.2.10 goes as follows:
>
> ====
> When mstatus.TVM=1, attempts to read or write hgatp while executing
> in HS-mode will raise an illegal instruction exception.
> ====
>
> I believe you need to check for HS-mode, not just PRV_S. riscv_csrrw_check()
> in
> target/riscv/csr.c checks for HS-mode as follows:
>
> if (riscv_has_ext(env, RVH) && env->priv == PRV_S &&
> !riscv_cpu_virt_enabled(env)) {
>
> Same goes for write_hgatp() below.
>
> > + } else {
> > + *val = env->hgatp;
> > + }
> > +
>
I think VS-mode can't access HS-mode CSR registers, which has been ensured in
riscv_csrrw_check(). You can see other callbacks of HS-mode CSR registers
(e.g., read_hgeip()) assume that it's M-mode or HS-mode, too.
> You can discard the 'else' since you're doing a return in the if:
>
> if (...) {
> return RISCV_EXCP_ILLEGAL_INST;
> }
>
> *val = env->hgatp;
>
>
> > return RISCV_EXCP_NONE;
> > }
> >
> > static RISCVException write_hgatp(CPURISCVState *env, int csrno,
> > target_ulong val)
> > {
> > - env->hgatp = val;
> > + if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > + return RISCV_EXCP_ILLEGAL_INST;
> > + } else {
> > + env->hgatp = val;
> > + }
>
> No need for else here either:
>
> if (...) {
> return RISCV_EXCP_ILLEGAL_INST;
> }
>
> env->hgatp = val;
>
>
I see. I will fix that in the next version of this patch.
>
> Thanks,
>
>
> Daniel
>
> > +
> > return RISCV_EXCP_NONE;
> > }
> >
Thanks!
Yi