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
> > >