diff mbox series

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

Message ID 20230501140020.3667219-2-mchitale@ventanamicro.com
State New
Headers show
Series Smstateen FCSR | expand

Commit Message

Mayuresh Chitale May 1, 2023, 2 p.m. UTC 
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(+)

Comments

Alistair Francis May 17, 2023, 3:11 a.m. UTC | #1 
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
>
Mayuresh Chitale May 18, 2023, 2:48 a.m. UTC | #2 
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
> >
Alistair Francis May 18, 2023, 4:48 a.m. UTC | #3 
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
> > >
diff mbox series

Patch

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);
 }