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

[Alistair Francis <alistair23@gmail.com>]

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
>