Re: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap

[CHEN Yi]

> -----Original Messages-----
> From: "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>
> Sent Time: 2023-03-09 03:44:03 (Thursday)
> To: chenyi2000@zju.edu.cn, qemu-devel@nongnu.org
> Cc: "Palmer Dabbelt" <palmer@dabbelt.com>, "Alistair Francis" 
> <alistair.francis@wdc.com>, "Bin Meng" <bin.meng@windriver.com>, "Weiwei Li" 
> <liweiwei@iscas.ac.cn>, "Liu Zhiwei" <zhiwei_liu@linux.alibaba.com>, "open 
> list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org>
> Subject: Re: [PATCH] target/riscv/csr.c: fix H extension TVM trap
> 
> 
> 
> On 3/8/23 09:34, chenyi2000@zju.edu.cn wrote:
> > From: Yi Chen <chenyi2000@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 <chenyi2000@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