Re: Re: [PATCH v4] target/riscv: fix H extension TVM trap

[CHEN Yi]

> -----Original Messages-----
> From: "Alistair Francis" <alistair23@gmail.com>
> Sent Time: 2023-04-06 09:56:58 (Thursday)
> To: "Yi Chen" <chenyi2000@zju.edu.cn>
> Cc: qemu-devel@nongnu.org, "Weiwei Li" <liweiwei@iscas.ac.cn>, "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 v4] target/riscv: fix H extension TVM trap
> 
> On Sun, Mar 12, 2023 at 10:07 PM Yi Chen <chenyi2000@zju.edu.cn> wrote:
> >
> > - Trap satp/hgatp accesses from HS-mode when MSTATUS.TVM is enabled.
> > - Trap satp accesses from VS-mode when HSTATUS.VTVM is enabled.
> > - Raise RISCV_EXCP_ILLEGAL_INST when U-mode executes SFENCE.VMA/SINVAL.VMA.
> > - Raise RISCV_EXCP_VIRT_INSTRUCTION_FAULT when VU-mode executes
> >   SFENCE.VMA/SINVAL.VMA or VS-mode executes SFENCE.VMA/SINVAL.VMA with
> >   HSTATUS.VTVM enabled.
> > - Raise RISCV_EXCP_VIRT_INSTRUCTION_FAULT when VU-mode executes
> >   HFENCE.GVMA/HFENCE.VVMA/HINVAL.GVMA/HINVAL.VVMA.
> 
> Thanks for the patch!
> 
> It looks like this patch needs to be rebased. Do you mind rebasing it
> on https://github.com/alistair23/qemu/tree/riscv-to-apply.next and
> then re-sending?

Sure. I sent it just now.

> Also, when you are fixing a range of issues it's best to split the
> fixes into patches that fix each individual issue (where that is
> possible). This makes it easier to review but also makes it easier to
> track changes and regressions if any problems arise.
> 
> In this case you don't need to split them up for a v5, but in future
> it's something to keep in mind

I see. I will keep that in mind in the future. Thanks for your kind note.

Best,
Yi

> The changes look good otherwise though :)
> 
> Alistair
> 
> >
> > Signed-off-by: Yi Chen <chenyi2000@zju.edu.cn>
> > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > ---
> > Add reviewed-by
> > Replace "env->priv <= PRV_S && riscv_cpu_virt_enabled(env)" with 
> > "riscv_cpu_virt_enabled(env)"
> >  target/riscv/csr.c       | 56 +++++++++++++++++++++++++---------------
> >  target/riscv/op_helper.c | 12 ++++-----
> >  2 files changed, 41 insertions(+), 27 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index d522efc0b6..26a02e57bd 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -443,6 +443,30 @@ static RISCVException sstc_32(CPURISCVState *env, int 
> > csrno)
> >      return sstc(env, csrno);
> >  }
> >
> > +static RISCVException satp(CPURISCVState *env, int csrno)
> > +{
> > +    if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) &&
> > +        get_field(env->mstatus, MSTATUS_TVM)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +    if (env->priv == PRV_S && riscv_cpu_virt_enabled(env) &&
> > +        get_field(env->hstatus, HSTATUS_VTVM)) {
> > +        return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +    }
> > +
> > +    return smode(env, csrno);
> > +}
> > +
> > +static RISCVException hgatp(CPURISCVState *env, int csrno)
> > +{
> > +    if (env->priv == PRV_S && !riscv_cpu_virt_enabled(env) &&
> > +        get_field(env->mstatus, MSTATUS_TVM)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return hmode(env, csrno);
> > +}
> > +
> >  /* Checks if PointerMasking registers could be accessed */
> >  static RISCVException pointer_masking(CPURISCVState *env, int csrno)
> >  {
> > @@ -2655,13 +2679,7 @@ static RISCVException read_satp(CPURISCVState *env, 
> > int csrno,
> >          *val = 0;
> >          return RISCV_EXCP_NONE;
> >      }
> > -
> > -    if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > -        return RISCV_EXCP_ILLEGAL_INST;
> > -    } else {
> > -        *val = env->satp;
> > -    }
> > -
> > +    *val = env->satp;
> >      return RISCV_EXCP_NONE;
> >  }
> >
> > @@ -2684,18 +2702,14 @@ static RISCVException write_satp(CPURISCVState 
> > *env, int csrno,
> >      }
> >
> >      if (vm && mask) {
> > -        if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> > -            return RISCV_EXCP_ILLEGAL_INST;
> > -        } else {
> > -            /*
> > -             * The ISA defines SATP.MODE=Bare as "no translation", but we 
> > still
> > -             * pass these through QEMU's TLB emulation as it improves
> > -             * performance.  Flushing the TLB on SATP writes with paging
> > -             * enabled avoids leaking those invalid cached mappings.
> > -             */
> > -            tlb_flush(env_cpu(env));
> > -            env->satp = val;
> > -        }
> > +        /*
> > +         * The ISA defines SATP.MODE=Bare as "no translation", but we still
> > +         * pass these through QEMU's TLB emulation as it improves
> > +         * performance.  Flushing the TLB on SATP writes with paging
> > +         * enabled avoids leaking those invalid cached mappings.
> > +         */
> > +        tlb_flush(env_cpu(env));
> > +        env->satp = val;
> >      }
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -4180,7 +4194,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >                           .min_priv_ver = PRIV_VERSION_1_12_0 },
> >
> >      /* Supervisor Protection and Translation */
> > -    [CSR_SATP]     = { "satp",     smode, read_satp,     write_satp     },
> > +    [CSR_SATP]     = { "satp",     satp, read_satp,     write_satp     },
> >
> >      /* Supervisor-Level Window to Indirectly Accessed Registers (AIA) */
> >      [CSR_SISELECT]   = { "siselect",   aia_smode, NULL, NULL, rmw_xiselect 
> > },
> > @@ -4217,7 +4231,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >                            .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> >      [CSR_HGEIP]       = { "hgeip",       hmode,   read_hgeip,
> >                            .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> > -    [CSR_HGATP]       = { "hgatp",       hmode,   read_hgatp,   
> > write_hgatp,
> > +    [CSR_HGATP]       = { "hgatp",       hgatp,   read_hgatp,   
> > write_hgatp,
> >                            .min_priv_ver = PRIV_VERSION_1_12_0              
> >   },
> >      [CSR_HTIMEDELTA]  = { "htimedelta",  hmode,   read_htimedelta,
> >                            write_htimedelta,
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index 84ee018f7d..8e16020f8d 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -381,12 +381,12 @@ void helper_wfi(CPURISCVState *env)
> >  void helper_tlb_flush(CPURISCVState *env)
> >  {
> >      CPUState *cs = env_cpu(env);
> > -    if (!(env->priv >= PRV_S) ||
> > -        (env->priv == PRV_S &&
> > -         get_field(env->mstatus, MSTATUS_TVM))) {
> > +    if (!riscv_cpu_virt_enabled(env) &&
> > +        (env->priv == PRV_U ||
> > +         (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)))) {
> >          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > -    } else if (riscv_has_ext(env, RVH) && riscv_cpu_virt_enabled(env) &&
> > -               get_field(env->hstatus, HSTATUS_VTVM)) {
> > +    } else if (riscv_cpu_virt_enabled(env) &&
> > +               (env->priv == PRV_U || get_field(env->hstatus, 
> > HSTATUS_VTVM))) {
> >          riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, 
> > GETPC());
> >      } else {
> >          tlb_flush(cs);
> > @@ -403,7 +403,7 @@ void helper_hyp_tlb_flush(CPURISCVState *env)
> >  {
> >      CPUState *cs = env_cpu(env);
> >
> > -    if (env->priv == PRV_S && riscv_cpu_virt_enabled(env)) {
> > +    if (riscv_cpu_virt_enabled(env)) {
> >          riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, 
> > GETPC());
> >      }
> >
> > --
> > 2.39.2
> >
> >