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

[CHEN Yi]

-----Original Messages-----
From:liweiwei <liweiwei@iscas.ac.cn>
Sent Time:2023-03-09 15:48:17 (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>, "Daniel Henrique Barboza" <dbarboza@ventanamicro.com>, "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 2023/3/8 20: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)) {
         return RISCV_EXCP_ILLEGAL_INST;

This line seems too long (> 80).

And hstatus.VTVM should also be taken into consideration.

Similar to following write_satp.

     } 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 {
             /*

Thanks a lot. In the next version, I will fix the code style issue and consider hstatus.VTVM.

@@ -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;

This check will do before privilege check in riscv_csrrw_check. So it will make VS mode access trigger

ILLEGAL_INST exception, However, it should be VIRTUAL_INST exception in this case.

Regards,

Weiwei Li

In riscv_csrrw(), riscv_csrrw_check() is called before riscv_csrrw_do64(). So I think VIRTUAL_INST will be triggered. Could you please explain why this check will do before the privilege check in riscv_csrrw_check? I'm new to Qemu source code and am sorry I can't understand that.

> > +    } else {
> > +        *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;
> > +    }
> > +
> >      return RISCV_EXCP_NONE;
> >  }
> >  
> >

Thanks,

Yi