1. Patchew
  2. QEMU
  3. [RFC PATCH v5 0/4] RISC-V Smstateen support
  4. View patch

[RFC PATCH v5 3/4] target/riscv: smstateen check for fcsr

Mayuresh Chitale posted 4 patches 3 years, 7 months ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>
There is a newer version of this series
[RFC PATCH v5 3/4] target/riscv: smstateen check for fcsr
Posted by Mayuresh Chitale 3 years, 7 months ago 
If smstateen is implemented and sstateen0.fcsr is clear
then the floating point operations must return illegal
instruction exception.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/csr.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ae91ae1f7e..8bbbed38ff 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -77,6 +77,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
         !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
+
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
+        return smstateen_acc_ok(env, PRV_U, SMSTATEEN0_FCSR);
+    }
 #endif
     return RISCV_EXCP_NONE;
 }
@@ -1700,6 +1704,10 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
                        (1UL << SMSTATEEN0_HSENVCFG);
 
     reg = &env->mstateen[csrno - CSR_MSTATEEN0];
+    if (riscv_has_ext(env, RVF)) {
+        wr_mask |= 1UL << SMSTATEEN0_FCSR;
+    }
+
     write_smstateen(env, reg, wr_mask, new_val);
 
     return RISCV_EXCP_NONE;
@@ -1724,6 +1732,10 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
     reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
     val = (uint64_t)new_val << 32;
     val |= *reg & 0xFFFFFFFF;
+    if (riscv_has_ext(env, RVF)) {
+        wr_mask |= 1UL << SMSTATEEN0_FCSR;
+    }
+
     write_smstateen(env, reg, wr_mask, val);
 
     return RISCV_EXCP_NONE;
@@ -1745,6 +1757,10 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
                        (1UL << SMSTATEEN0_HSENVCFG);
     int index = csrno - CSR_HSTATEEN0;
 
+    if (riscv_has_ext(env, RVF)) {
+        wr_mask |= 1UL << SMSTATEEN0_FCSR;
+    }
+
     reg = &env->hstateen[index];
     wr_mask &= env->mstateen[index];
     write_smstateen(env, reg, wr_mask, new_val);
@@ -1769,6 +1785,10 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
     uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
                        (1UL << SMSTATEEN0_HSENVCFG);
 
+    if (riscv_has_ext(env, RVF)) {
+        wr_mask |= 1UL << SMSTATEEN0_FCSR;
+    }
+
     reg = &env->hstateen[index];
     val = (uint64_t)new_val << 32;
     val |= *reg & 0xFFFFFFFF;
@@ -1794,6 +1814,10 @@ static RISCVException write_sstateen(CPURISCVState *env, int csrno,
     int index = csrno - CSR_SSTATEEN0;
     bool virt = riscv_cpu_virt_enabled(env);
 
+    if (riscv_has_ext(env, RVF)) {
+        wr_mask |= 1UL << SMSTATEEN0_FCSR;
+    }
+
     reg = &env->sstateen[index];
     if (virt) {
         wr_mask &= env->mstateen[index];
-- 
2.25.1
Re: [RFC PATCH v5 3/4] target/riscv: smstateen check for fcsr
Posted by Alistair Francis 3 years, 7 months ago 
On Sat, Jun 4, 2022 at 2:08 AM Mayuresh Chitale
<mchitale@ventanamicro.com> wrote:
>
> If smstateen is implemented and sstateen0.fcsr is clear
> then the floating point operations must return illegal
> instruction exception.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>  target/riscv/csr.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index ae91ae1f7e..8bbbed38ff 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -77,6 +77,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
>          !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
> +
> +    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> +        return smstateen_acc_ok(env, PRV_U, SMSTATEEN0_FCSR);
> +    }

This only checks access to the CSRs. Shouldn't we also be throwing
errors if any instruction operates on an x register?

>  #endif
>      return RISCV_EXCP_NONE;
>  }
> @@ -1700,6 +1704,10 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
>                         (1UL << SMSTATEEN0_HSENVCFG);
>
>      reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> +    if (riscv_has_ext(env, RVF)) {
> +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> +    }

This doesn't look right.

"Whenever misa.F = 1, bit 1 of mstateen0 is read-only zero". Shouldn't
that mean we don't allow writes if we have the RVF extension?

Alistair

> +
>      write_smstateen(env, reg, wr_mask, new_val);
>
>      return RISCV_EXCP_NONE;
> @@ -1724,6 +1732,10 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
>      reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
>      val = (uint64_t)new_val << 32;
>      val |= *reg & 0xFFFFFFFF;
> +    if (riscv_has_ext(env, RVF)) {
> +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> +    }
> +
>      write_smstateen(env, reg, wr_mask, val);
>
>      return RISCV_EXCP_NONE;
> @@ -1745,6 +1757,10 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
>                         (1UL << SMSTATEEN0_HSENVCFG);
>      int index = csrno - CSR_HSTATEEN0;
>
> +    if (riscv_has_ext(env, RVF)) {
> +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> +    }
> +
>      reg = &env->hstateen[index];
>      wr_mask &= env->mstateen[index];
>      write_smstateen(env, reg, wr_mask, new_val);
> @@ -1769,6 +1785,10 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
>      uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
>                         (1UL << SMSTATEEN0_HSENVCFG);
>
> +    if (riscv_has_ext(env, RVF)) {
> +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> +    }
> +
>      reg = &env->hstateen[index];
>      val = (uint64_t)new_val << 32;
>      val |= *reg & 0xFFFFFFFF;
> @@ -1794,6 +1814,10 @@ static RISCVException write_sstateen(CPURISCVState *env, int csrno,
>      int index = csrno - CSR_SSTATEEN0;
>      bool virt = riscv_cpu_virt_enabled(env);
>
> +    if (riscv_has_ext(env, RVF)) {
> +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> +    }
> +
>      reg = &env->sstateen[index];
>      if (virt) {
>          wr_mask &= env->mstateen[index];
> --
> 2.25.1
>
>
Re: [RFC PATCH v5 3/4] target/riscv: smstateen check for fcsr
Posted by Mayuresh Chitale 3 years, 6 months ago 
On Thu, 2022-06-16 at 17:17 +1000, Alistair Francis wrote:
> On Sat, Jun 4, 2022 at 2:08 AM Mayuresh Chitale
> <mchitale@ventanamicro.com> wrote:
> > If smstateen is implemented and sstateen0.fcsr is clear
> > then the floating point operations must return illegal
> > instruction exception.
> > 
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >  target/riscv/csr.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index ae91ae1f7e..8bbbed38ff 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -77,6 +77,10 @@ static RISCVException fs(CPURISCVState *env, int
> > csrno)
> >          !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
> >          return RISCV_EXCP_ILLEGAL_INST;
> >      }
> > +
> > +    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
> > +        return smstateen_acc_ok(env, PRV_U, SMSTATEEN0_FCSR);
> > +    }
> 
> This only checks access to the CSRs. Shouldn't we also be throwing
> errors if any instruction operates on an x register?
Yes.
> 
> >  #endif
> >      return RISCV_EXCP_NONE;
> >  }
> > @@ -1700,6 +1704,10 @@ static RISCVException
> > write_mstateen(CPURISCVState *env, int csrno,
> >                         (1UL << SMSTATEEN0_HSENVCFG);
> > 
> >      reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> > +    if (riscv_has_ext(env, RVF)) {
> > +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> > +    }
> 
> This doesn't look right.
> 
> "Whenever misa.F = 1, bit 1 of mstateen0 is read-only zero".
> Shouldn't
> that mean we don't allow writes if we have the RVF extension?

I will fix it in the next version.
> 
> Alistair
> 
> > +
> >      write_smstateen(env, reg, wr_mask, new_val);
> > 
> >      return RISCV_EXCP_NONE;
> > @@ -1724,6 +1732,10 @@ static RISCVException
> > write_mstateenh(CPURISCVState *env, int csrno,
> >      reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
> >      val = (uint64_t)new_val << 32;
> >      val |= *reg & 0xFFFFFFFF;
> > +    if (riscv_has_ext(env, RVF)) {
> > +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> > +    }
> > +
> >      write_smstateen(env, reg, wr_mask, val);
> > 
> >      return RISCV_EXCP_NONE;
> > @@ -1745,6 +1757,10 @@ static RISCVException
> > write_hstateen(CPURISCVState *env, int csrno,
> >                         (1UL << SMSTATEEN0_HSENVCFG);
> >      int index = csrno - CSR_HSTATEEN0;
> > 
> > +    if (riscv_has_ext(env, RVF)) {
> > +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> > +    }
> > +
> >      reg = &env->hstateen[index];
> >      wr_mask &= env->mstateen[index];
> >      write_smstateen(env, reg, wr_mask, new_val);
> > @@ -1769,6 +1785,10 @@ static RISCVException
> > write_hstateenh(CPURISCVState *env, int csrno,
> >      uint64_t wr_mask = (1UL << SMSTATEEN_STATEN) |
> >                         (1UL << SMSTATEEN0_HSENVCFG);
> > 
> > +    if (riscv_has_ext(env, RVF)) {
> > +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> > +    }
> > +
> >      reg = &env->hstateen[index];
> >      val = (uint64_t)new_val << 32;
> >      val |= *reg & 0xFFFFFFFF;
> > @@ -1794,6 +1814,10 @@ static RISCVException
> > write_sstateen(CPURISCVState *env, int csrno,
> >      int index = csrno - CSR_SSTATEEN0;
> >      bool virt = riscv_cpu_virt_enabled(env);
> > 
> > +    if (riscv_has_ext(env, RVF)) {
> > +        wr_mask |= 1UL << SMSTATEEN0_FCSR;
> > +    }
> > +
> >      reg = &env->sstateen[index];
> >      if (virt) {
> >          wr_mask &= env->mstateen[index];
> > --
> > 2.25.1
> > 
> >

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.