[PATCH v4 0/3] Smstateen FCSR

[PATCH v4 0/3] Smstateen FCSR

[Mayuresh Chitale]

Patch 4 and 5 of the smstateen series need to be re-submitted with
changes described in the email below.
https://lists.nongnu.org/archive/html/qemu-riscv/2022-11/msg00155.html
Hence splitting the patch 4 of the original series into three and
re-submitting along with the original patch 5.

Changes in v4:
- Drop patch 3 
- Add reviewed-by tag

Changes in v3:
- Reuse TB_FLAGS.FS (instead of TB_FLAGS.HS_FS) for smstateen as HS_FS bits been removed.
- Remove fcsr check for zfh and zfhmin

Changes in v2:
 - Improve patch 1 description
 - Reuse TB_FLAGS.HS_FS for smstateen
 - Convert smstateen_fcsr_check to function
 - Add fcsr check for zdinx

Mayuresh Chitale (3):
  target/riscv: smstateen check for fcsr
  target/riscv: Reuse tb->flags.FS
  target/riscv: smstateen knobs

 target/riscv/cpu.c                      |  3 ++-
 target/riscv/cpu_helper.c               |  6 ++++++
 target/riscv/csr.c                      | 15 +++++++++++++++
 target/riscv/insn_trans/trans_rvf.c.inc |  7 ++++---
 4 files changed, 27 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

If smstateen is implemented and smtateen0.fcsr is clear and misa.F
is off then the floating point operations must return illegal
instruction exception or virtual instruction trap, if relevant.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
---
 target/riscv/csr.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4451bd1263..3f6b824bd2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -82,6 +82,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
         !riscv_cpu_cfg(env)->ext_zfinx) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
+
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
+        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
+    }
 #endif
     return RISCV_EXCP_NONE;
 }
@@ -2100,6 +2104,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
     return write_mstateen(env, csrno, wr_mask, new_val);
 }
@@ -2173,6 +2180,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_hstateen(env, csrno, wr_mask, new_val);
 }
 
@@ -2259,6 +2270,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_sstateen(env, csrno, wr_mask, new_val);
 }
 
-- 
2.34.1

Re: [PATCH v4 1/3] target/riscv: smstateen check for fcsr

[Alistair Francis]

On Tue, May 2, 2023 at 12:00 AM Mayuresh Chitale
<mchitale@ventanamicro.com> wrote:
>
> If smstateen is implemented and smtateen0.fcsr is clear and misa.F
> is off then the floating point operations must return illegal
> instruction exception or virtual instruction trap, if relevant.

Do you mind re-wording this commit message? I can't get my head around
it. You talk about returning an illegal instruction exception, but
most of this patch is just adding SMSTATEEN0_FCSR to the write mask if
floating point is disabled.

It looks to me like you are returning an exception trying to access a
floating pointer register if FP is off and SMSTATEEN0_FCSR is not set
(which you describe) but also then only allow changing SMSTATEEN0_FCSR
if the RVF is not enabled, which is where I'm confused.

Your patch seems to be correct, I think the commit message and title
just needs a small tweak. Maybe something like this:

```
target/riscv: smstateen add support for fcsr bit

If smstateen is implemented and SMSTATEEN0.FCSR is zero floating point
CSR access should raise an illegal instruction exception or virtual
equivalent as required.

We also allow the guest to set/unset the FCSR bit, but only if misa.F
== 0, as defined in the spec.
```

Alistair

>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> ---
>  target/riscv/csr.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 4451bd1263..3f6b824bd2 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -82,6 +82,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
>          !riscv_cpu_cfg(env)->ext_zfinx) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
> +
> +    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> +        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
> +    }
>  #endif
>      return RISCV_EXCP_NONE;
>  }
> @@ -2100,6 +2104,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
>                                        target_ulong new_val)
>  {
>      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
>
>      return write_mstateen(env, csrno, wr_mask, new_val);
>  }
> @@ -2173,6 +2180,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
>  {
>      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>      return write_hstateen(env, csrno, wr_mask, new_val);
>  }
>
> @@ -2259,6 +2270,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
>  {
>      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
>
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>      return write_sstateen(env, csrno, wr_mask, new_val);
>  }
>
> --
> 2.34.1
>

Re: [PATCH v4 1/3] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

On Wed, May 17, 2023 at 8:42 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, May 2, 2023 at 12:00 AM Mayuresh Chitale
> <mchitale@ventanamicro.com> wrote:
> >
> > If smstateen is implemented and smtateen0.fcsr is clear and misa.F
> > is off then the floating point operations must return illegal
> > instruction exception or virtual instruction trap, if relevant.
>
> Do you mind re-wording this commit message? I can't get my head around
> it. You talk about returning an illegal instruction exception, but
> most of this patch is just adding SMSTATEEN0_FCSR to the write mask if
> floating point is disabled.
>
Sure. The generic code for access check was added in a previous patch.
> It looks to me like you are returning an exception trying to access a
> floating pointer register if FP is off and SMSTATEEN0_FCSR is not set
> (which you describe) but also then only allow changing SMSTATEEN0_FCSR
> if the RVF is not enabled, which is where I'm confused.
The smstateen0.fcsr bit is writable only when misa.F == 0.

>
> Your patch seems to be correct, I think the commit message and title
> just needs a small tweak. Maybe something like this:
>
> ```
> target/riscv: smstateen add support for fcsr bit
>
> If smstateen is implemented and SMSTATEEN0.FCSR is zero floating point
> CSR access should raise an illegal instruction exception or virtual
> equivalent as required.
>
> We also allow the guest to set/unset the FCSR bit, but only if misa.F
> == 0, as defined in the spec.

How about:

target/riscv: smstateen check for fcsr

Implement the s/h/mstateen.fcsr bit as as defined in the smstateen spec
and check for it when accessing the fcsr register and its fields.

> ```
>
> Alistair
>
> >
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > ---
> >  target/riscv/csr.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 4451bd1263..3f6b824bd2 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -82,6 +82,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
> >          !riscv_cpu_cfg(env)->ext_zfinx) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> > +
> > +    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> > +        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
> > +    }
> >  #endif
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -2100,6 +2104,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
> >                                        target_ulong new_val)
> >  {
> >      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> >
> >      return write_mstateen(env, csrno, wr_mask, new_val);
> >  }
> > @@ -2173,6 +2180,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
> >  {
> >      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >      return write_hstateen(env, csrno, wr_mask, new_val);
> >  }
> >
> > @@ -2259,6 +2270,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
> >  {
> >      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >      return write_sstateen(env, csrno, wr_mask, new_val);
> >  }
> >
> > --
> > 2.34.1
> >

Re: [PATCH v4 1/3] target/riscv: smstateen check for fcsr

[Alistair Francis]

On Thu, May 18, 2023 at 12:49 PM Mayuresh Chitale
<mchitale@ventanamicro.com> wrote:
>
> On Wed, May 17, 2023 at 8:42 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Tue, May 2, 2023 at 12:00 AM Mayuresh Chitale
> > <mchitale@ventanamicro.com> wrote:
> > >
> > > If smstateen is implemented and smtateen0.fcsr is clear and misa.F
> > > is off then the floating point operations must return illegal
> > > instruction exception or virtual instruction trap, if relevant.
> >
> > Do you mind re-wording this commit message? I can't get my head around
> > it. You talk about returning an illegal instruction exception, but
> > most of this patch is just adding SMSTATEEN0_FCSR to the write mask if
> > floating point is disabled.
> >
> Sure. The generic code for access check was added in a previous patch.
> > It looks to me like you are returning an exception trying to access a
> > floating pointer register if FP is off and SMSTATEEN0_FCSR is not set
> > (which you describe) but also then only allow changing SMSTATEEN0_FCSR
> > if the RVF is not enabled, which is where I'm confused.
> The smstateen0.fcsr bit is writable only when misa.F == 0.
>
> >
> > Your patch seems to be correct, I think the commit message and title
> > just needs a small tweak. Maybe something like this:
> >
> > ```
> > target/riscv: smstateen add support for fcsr bit
> >
> > If smstateen is implemented and SMSTATEEN0.FCSR is zero floating point
> > CSR access should raise an illegal instruction exception or virtual
> > equivalent as required.
> >
> > We also allow the guest to set/unset the FCSR bit, but only if misa.F
> > == 0, as defined in the spec.
>
> How about:
>
> target/riscv: smstateen check for fcsr
>
> Implement the s/h/mstateen.fcsr bit as as defined in the smstateen spec
> and check for it when accessing the fcsr register and its fields.

That works as well

Alistair

>
> > ```
> >
> > Alistair
> >
> > >
> > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > > ---
> > >  target/riscv/csr.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index 4451bd1263..3f6b824bd2 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -82,6 +82,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
> > >          !riscv_cpu_cfg(env)->ext_zfinx) {
> > >          return RISCV_EXCP_ILLEGAL_INST;
> > >      }
> > > +
> > > +    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> > > +        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
> > > +    }
> > >  #endif
> > >      return RISCV_EXCP_NONE;
> > >  }
> > > @@ -2100,6 +2104,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
> > >                                        target_ulong new_val)
> > >  {
> > >      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> > > +    if (!riscv_has_ext(env, RVF)) {
> > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > +    }
> > >
> > >      return write_mstateen(env, csrno, wr_mask, new_val);
> > >  }
> > > @@ -2173,6 +2180,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
> > >  {
> > >      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> > >
> > > +    if (!riscv_has_ext(env, RVF)) {
> > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > +    }
> > > +
> > >      return write_hstateen(env, csrno, wr_mask, new_val);
> > >  }
> > >
> > > @@ -2259,6 +2270,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
> > >  {
> > >      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> > >
> > > +    if (!riscv_has_ext(env, RVF)) {
> > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > +    }
> > > +
> > >      return write_sstateen(env, csrno, wr_mask, new_val);
> > >  }
> > >
> > > --
> > > 2.34.1
> > >

[PATCH v4 2/3] target/riscv: Reuse tb->flags.FS

[Mayuresh Chitale]

When misa.F is 0 tb->flags.FS field is unused and can be used to save
the current state of smstateen0.FCSR check which is needed by the
floating point translation routines.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
---
 target/riscv/cpu_helper.c               | 6 ++++++
 target/riscv/insn_trans/trans_rvf.c.inc | 7 ++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index b68dcfe7b6..695c189f96 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -119,6 +119,12 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
         vs = MIN(vs, get_field(env->mstatus_hs, MSTATUS_VS));
     }
 
+    /* With Zfinx, floating point is enabled/disabled by Smstateen. */
+    if (!riscv_has_ext(env, RVF)) {
+        fs = (smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR) == RISCV_EXCP_NONE)
+             ? EXT_STATUS_DIRTY : EXT_STATUS_DISABLED;
+    }
+
     if (cpu->cfg.debug && !icount_enabled()) {
         flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
     }
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index b2de4fcf3f..509a6acffe 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -19,9 +19,10 @@
  */
 
 #define REQUIRE_FPU do {\
-    if (ctx->mstatus_fs == EXT_STATUS_DISABLED) \
-        if (!ctx->cfg_ptr->ext_zfinx) \
-            return false; \
+    if (ctx->mstatus_fs == EXT_STATUS_DISABLED) {                           \
+        ctx->virt_inst_excp = ctx->virt_enabled && ctx->cfg_ptr->ext_zfinx; \
+        return false;                                                       \
+    }                                                                       \
 } while (0)
 
 #define REQUIRE_ZFINX_OR_F(ctx) do {\
-- 
2.34.1

Re: [PATCH v4 2/3] target/riscv: Reuse tb->flags.FS

[Richard Henderson]

On 5/1/23 15:00, Mayuresh Chitale wrote:
> When misa.F is 0 tb->flags.FS field is unused and can be used to save
> the current state of smstateen0.FCSR check which is needed by the
> floating point translation routines.
> 
> Signed-off-by: Mayuresh Chitale<mchitale@ventanamicro.com>
> Reviewed-by: Weiwei Li<liweiwei@iscas.ac.cn>
> ---
>   target/riscv/cpu_helper.c               | 6 ++++++
>   target/riscv/insn_trans/trans_rvf.c.inc | 7 ++++---
>   2 files changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v4 3/3] target/riscv: smstateen knobs

[Mayuresh Chitale]

Add knobs to allow users to enable smstateen and also export it via the
ISA extension string.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Reviewed-by: Weiwei Li<liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index befa64528f..9420cd670e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -119,6 +119,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zhinx, PRIV_VERSION_1_12_0, ext_zhinx),
     ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
     ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
+    ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
     ISA_EXT_DATA_ENTRY(ssaia, PRIV_VERSION_1_12_0, ext_ssaia),
     ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
     ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
@@ -1498,8 +1499,8 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
     DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
 
+    DEFINE_PROP_BOOL("smstateen", RISCVCPU, cfg.ext_smstateen, false),
     DEFINE_PROP_BOOL("svadu", RISCVCPU, cfg.ext_svadu, true),
-
     DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
     DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
     DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
-- 
2.34.1