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

[liweiwei]

On 2023/3/9 23:02, CHEN Yi wrote:



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


Yeah, You are right. Sorry that I mistook this check for check in the 
predicate.

By the way, I think this check is better to be done in the predicate.

Regards,

Weiwei Li

    +    } 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