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

Yi Chen posted 1 patch 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230312120538.15286-1-chenyi2000@zju.edu.cn
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
There is a newer version of this series
target/riscv/csr.c       | 56 +++++++++++++++++++++++++---------------
target/riscv/op_helper.c | 12 ++++-----
2 files changed, 41 insertions(+), 27 deletions(-)
[PATCH v4] target/riscv: fix H extension TVM trap
Posted by Yi Chen 1 year, 1 month ago
- 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.

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
Re: [PATCH v4] target/riscv: fix H extension TVM trap
Posted by Alistair Francis 1 year ago
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?

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

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
>
>
Re: Re: [PATCH v4] target/riscv: fix H extension TVM trap
Posted by CHEN Yi 1 year ago


> -----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
> >
> >
Re: [PATCH v4] target/riscv: fix H extension TVM trap
Posted by LIU Zhiwei 1 year, 1 month ago
On 2023/3/12 20:05, Yi Chen 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.
>
> 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());
>       }

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>