Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

2022-10-10 Thread weiwei 


On 2022/10/7 01:06, mchit...@ventanamicro.com wrote:

On Tue, 2022-10-04 at 21:23 +0800, weiwei wrote:



On 2022/10/4 14:51, mchit...@ventanamicro.com wrote:

On Mon, 2022-10-03 at 21:02 +0800, weiwei wrote:

On 2022/10/3 19:47, Mayuresh Chitale wrote:

If smstateen is implemented and sstateen0.fcsr is clear then the
floating point
operations must return illegal instruction exception or virtual
instruction
trap, if relevant.
Signed-off-by: Mayuresh Chitale

---
   target/riscv/csr.c| 23 
   target/riscv/insn_trans/trans_rvf.c.inc   | 43
+--
   target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++
   3 files changed, 75 insertions(+), 3 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 71236f2b5d..8b25f885ec 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -84,6 +84,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, 0, SMSTATEEN0_FCSR);
+}
   #endif
   return RISCV_EXCP_NONE;
   }
@@ -2023,6 +2027,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);

   }
@@ -2059,6 +2066,10 @@ static RISCVException
write_mstateen0h(CPURISCVState *env, int csrno,
   {
   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
   
+if (!riscv_has_ext(env, RVF)) {

+wr_mask |= SMSTATEEN0_FCSR;
+}
+
   return write_mstateenh(env, csrno, wr_mask, new_val);
   }
   
@@ -2096,6 +2107,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);
   }
   
@@ -2135,6 +2150,10 @@ static RISCVException

write_hstateen0h(CPURISCVState *env, int csrno,
   {
   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
   
+if (!riscv_has_ext(env, RVF)) {

+wr_mask |= SMSTATEEN0_FCSR;
+}
+
   return write_hstateenh(env, csrno, wr_mask, new_val);
   }
   
@@ -2182,6 +2201,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);
   }
   
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc

b/target/riscv/insn_trans/trans_rvf.c.inc
index a1d3eb52ad..93657680c6 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,9 +24,46 @@
   return false; \
   } while (0)
   
-#define REQUIRE_ZFINX_OR_F(ctx) do {\

-if (!ctx->cfg_ptr->ext_zfinx) { \
-REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int
index)
+{
+CPUState *cpu = ctx->cs;
+CPURISCVState *env = cpu->env_ptr;
+uint64_t stateen = env->mstateen[index];
+
+if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+return true;
+}
+
+if (ctx->virt_enabled) {
+stateen &= env->hstateen[index];
+}
+
+if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+stateen &= env->sstateen[index];
+}
+
+if (!(stateen & SMSTATEEN0_FCSR)) {
+if (ctx->virt_enabled) {
+ctx->virt_inst_excp = true;
+}

Are there any considerations for adding  virt_inst_excp instead of
directly
"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?
Regards,
Weiwei Li

I had considered it but I think this is a simpler approach as the rest
of the code path stays the same as generating an illegal instruction
exception, for e.g setting the bins value (tval).


OK,  we did need to set bins value for virt instruction exception. 
However I prefer directly call a


new gen_exception_virt function(similar togen_exception_illegal) here.


  Also we need to
return true from all the caller trans_* functions even if the smstateen
check has failed.


False is returned when smstateen check fails currently.

Yes, however if we make this change then should return true if the 
check fails so that the decode_opc doesnt fall through and generate 
another exception. It can be done but it would be contrary to the 
general convention.


OK.  Acceptable to me.

Reviewed-by: Weiwei Li 

Regards,

Weiwei Li


Regards,

Weiwei Li


+return false;
+}
+
+return true;
+}
+#else
+#define

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

2022-10-06 Thread mchitale 
On Tue, 2022-10-04 at 21:23 +0800, weiwei wrote:
> 
> 
> 
> 
> On 2022/10/4 14:51,
>   mchit...@ventanamicro.com wrote:
> 
> 
> 
> 
> >   On Mon, 2022-10-03 at 21:02 +0800, weiwei wrote:
> >   
> > > On 2022/10/3 19:47, Mayuresh Chitale wrote:
> > > 
> > > >   If smstateen is implemented and sstateen0.fcsr is
> > > > clear then thefloating pointoperations must return illegal
> > > > instruction exception or virtualinstructiontrap, if relevant.
> > > > Signed-off-by: Mayuresh Chitale 
> > > > ---  target/riscv/csr.c| 23
> > > >   target/riscv/insn_trans/trans_rvf.c.inc   |
> > > > 43+
> > > > --  target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++  3
> > > > files changed, 75 insertions(+), 3 deletions(-)
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.cindex
> > > > 71236f2b5d..8b25f885ec 100644--- a/target/riscv/csr.c+++
> > > > b/target/riscv/csr.c@@ -84,6 +84,10 @@ static RISCVException
> > > > fs(CPURISCVState *env,
> > > > intcsrno)  !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, 0,
> > > > SMSTATEEN0_FCSR);+}  #endif  return
> > > > RISCV_EXCP_NONE;  }@@ -2023,6 +2027,9 @@ static
> > > > RISCVExceptionwrite_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);  }@@ -2059,6
> > > > +2066,10 @@ static RISCVExceptionwrite_mstateen0h(CPURISCVState
> > > > *env, int csrno,  {  uint64_t wr_mask = SMSTATEEN_STATEEN |
> > > > SMSTATEEN0_HSENVCFG;  +if (!riscv_has_ext(env, RVF))
> > > > {+wr_mask |= SMSTATEEN0_FCSR;+}+  return
> > > > write_mstateenh(env, csrno, wr_mask, new_val);  }  @@ -2096,6
> > > > +2107,10 @@ static RISCVExceptionwrite_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);  }  @@ -2135,6
> > > > +2150,10 @@ static RISCVExceptionwrite_hstateen0h(CPURISCVState
> > > > *env, int csrno,  {  uint64_t wr_mask = SMSTATEEN_STATEEN |
> > > > SMSTATEEN0_HSENVCFG;  +if (!riscv_has_ext(env, RVF))
> > > > {+wr_mask |= SMSTATEEN0_FCSR;+}+  return
> > > > write_hstateenh(env, csrno, wr_mask, new_val);  }  @@ -2182,6
> > > > +2201,10 @@ static RISCVExceptionwrite_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);  }  diff --git
> > > > a/target/riscv/insn_trans/trans_rvf.c.incb/target/riscv/insn_tr
> > > > ans/trans_rvf.c.incindex a1d3eb52ad..93657680c6 100644---
> > > > a/target/riscv/insn_trans/trans_rvf.c.inc+++
> > > > b/target/riscv/insn_trans/trans_rvf.c.inc@@ -24,9 +24,46
> > > > @@  return false; \  } while (0)  -#define
> > > > REQUIRE_ZFINX_OR_F(ctx) do {\-if (!ctx->cfg_ptr->ext_zfinx) 
> > > > { \-REQUIRE_EXT(ctx, RVF); \+#ifndef
> > > > CONFIG_USER_ONLY+static inline bool
> > > > smstateen_fcsr_check(DisasContext *ctx,
> > > > intindex)+{+CPUState *cpu = ctx->cs;+CPURISCVState *env
> > > > = cpu->env_ptr;+uint64_t stateen = env-
> > > > >mstateen[index];++if (!ctx->cfg_ptr->ext_smstateen || env-
> > > > >priv == PRV_M) {+return true;+}++if (ctx-
> > > > >virt_enabled) {+stateen &= env-
> > > > >hstateen[index];+}++if (env->priv == PRV_U &&
> > > > has_ext(ctx, RVS)) {+stateen &= env-
> > > > >sstateen[index];+}++if (!(stateen & SMSTATEEN0_FCSR))
> > > > {+if (ctx->virt_enabled) {+ctx-
> > > > >virt_inst_excp = true;+}
> > > > 
> > > 
> > > Are there any considerations for adding  virt_inst_excp
> > > instead ofdirectly
> > > "generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)"
> > > here?
> > > Regards,
> > > Weiwei Li
> > >   
> > 
> >   I had considered it but I think this is a simpler approach as
> > the restof the code path stays the same as generating an illegal
> > instructionexception, for e.g setting the bins value (tval).
> > 
> 
> OK,  we did need to set bins value for virt instruction
>   exception. However I prefer directly call a 
> 
> 
> 
> new gen_exception_virt function(similar to gen_exception_illegal)
> here.
> 
> 
> >

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

2022-10-04 Thread weiwei 


On 2022/10/4 14:51, mchit...@ventanamicro.com wrote:

On Mon, 2022-10-03 at 21:02 +0800, weiwei wrote:

On 2022/10/3 19:47, Mayuresh Chitale wrote:

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

Signed-off-by: Mayuresh Chitale
---
   target/riscv/csr.c| 23 
   target/riscv/insn_trans/trans_rvf.c.inc   | 43
+--
   target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++
   3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 71236f2b5d..8b25f885ec 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -84,6 +84,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, 0, SMSTATEEN0_FCSR);
+}
   #endif
   return RISCV_EXCP_NONE;
   }
@@ -2023,6 +2027,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);

   }
@@ -2059,6 +2066,10 @@ static RISCVException
write_mstateen0h(CPURISCVState *env, int csrno,
   {
   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
   
+if (!riscv_has_ext(env, RVF)) {

+wr_mask |= SMSTATEEN0_FCSR;
+}
+
   return write_mstateenh(env, csrno, wr_mask, new_val);
   }
   
@@ -2096,6 +2107,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);
   }
   
@@ -2135,6 +2150,10 @@ static RISCVException

write_hstateen0h(CPURISCVState *env, int csrno,
   {
   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
   
+if (!riscv_has_ext(env, RVF)) {

+wr_mask |= SMSTATEEN0_FCSR;
+}
+
   return write_hstateenh(env, csrno, wr_mask, new_val);
   }
   
@@ -2182,6 +2201,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);
   }
   
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc

b/target/riscv/insn_trans/trans_rvf.c.inc
index a1d3eb52ad..93657680c6 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,9 +24,46 @@
   return false; \
   } while (0)
   
-#define REQUIRE_ZFINX_OR_F(ctx) do {\

-if (!ctx->cfg_ptr->ext_zfinx) { \
-REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int
index)
+{
+CPUState *cpu = ctx->cs;
+CPURISCVState *env = cpu->env_ptr;
+uint64_t stateen = env->mstateen[index];
+
+if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+return true;
+}
+
+if (ctx->virt_enabled) {
+stateen &= env->hstateen[index];
+}
+
+if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+stateen &= env->sstateen[index];
+}
+
+if (!(stateen & SMSTATEEN0_FCSR)) {
+if (ctx->virt_enabled) {
+ctx->virt_inst_excp = true;
+}

Are there any considerations for adding  virt_inst_excp instead of
directly

"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?

Regards,

Weiwei Li

I had considered it but I think this is a simpler approach as the rest
of the code path stays the same as generating an illegal instruction
exception, for e.g setting the bins value (tval).


OK,  we did need to set bins value for virt instruction exception. 
However I prefer directly call a


new gen_exception_virt function(similar togen_exception_illegal) here.


  Also we need to
return true from all the caller trans_* functions even if the smstateen
check has failed.


False is returned when smstateen check fails currently.

Regards,

Weiwei Li


+return false;
+}
+
+return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+if (!has_ext(ctx, RVF)) { \
+if (!ctx->cfg_ptr->ext_zfinx) { \
+return false; \
+} \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
   } \
   } while (0)
   
diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

2022-10-04 Thread mchitale 
On Mon, 2022-10-03 at 21:02 +0800, weiwei wrote:
> On 2022/10/3 19:47, Mayuresh Chitale wrote:
> > If smstateen is implemented and sstateen0.fcsr is clear then the
> > floating point
> > operations must return illegal instruction exception or virtual
> > instruction
> > trap, if relevant.
> > 
> > Signed-off-by: Mayuresh Chitale 
> > ---
> >   target/riscv/csr.c| 23 
> >   target/riscv/insn_trans/trans_rvf.c.inc   | 43
> > +--
> >   target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++
> >   3 files changed, 75 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 71236f2b5d..8b25f885ec 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -84,6 +84,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, 0, SMSTATEEN0_FCSR);
> > +}
> >   #endif
> >   return RISCV_EXCP_NONE;
> >   }
> > @@ -2023,6 +2027,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);
> >   }
> > @@ -2059,6 +2066,10 @@ static RISCVException
> > write_mstateen0h(CPURISCVState *env, int csrno,
> >   {
> >   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >   
> > +if (!riscv_has_ext(env, RVF)) {
> > +wr_mask |= SMSTATEEN0_FCSR;
> > +}
> > +
> >   return write_mstateenh(env, csrno, wr_mask, new_val);
> >   }
> >   
> > @@ -2096,6 +2107,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);
> >   }
> >   
> > @@ -2135,6 +2150,10 @@ static RISCVException
> > write_hstateen0h(CPURISCVState *env, int csrno,
> >   {
> >   uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >   
> > +if (!riscv_has_ext(env, RVF)) {
> > +wr_mask |= SMSTATEEN0_FCSR;
> > +}
> > +
> >   return write_hstateenh(env, csrno, wr_mask, new_val);
> >   }
> >   
> > @@ -2182,6 +2201,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);
> >   }
> >   
> > diff --git a/target/riscv/insn_trans/trans_rvf.c.inc
> > b/target/riscv/insn_trans/trans_rvf.c.inc
> > index a1d3eb52ad..93657680c6 100644
> > --- a/target/riscv/insn_trans/trans_rvf.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> > @@ -24,9 +24,46 @@
> >   return false; \
> >   } while (0)
> >   
> > -#define REQUIRE_ZFINX_OR_F(ctx) do {\
> > -if (!ctx->cfg_ptr->ext_zfinx) { \
> > -REQUIRE_EXT(ctx, RVF); \
> > +#ifndef CONFIG_USER_ONLY
> > +static inline bool smstateen_fcsr_check(DisasContext *ctx, int
> > index)
> > +{
> > +CPUState *cpu = ctx->cs;
> > +CPURISCVState *env = cpu->env_ptr;
> > +uint64_t stateen = env->mstateen[index];
> > +
> > +if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
> > +return true;
> > +}
> > +
> > +if (ctx->virt_enabled) {
> > +stateen &= env->hstateen[index];
> > +}
> > +
> > +if (env->priv == PRV_U && has_ext(ctx, RVS)) {
> > +stateen &= env->sstateen[index];
> > +}
> > +
> > +if (!(stateen & SMSTATEEN0_FCSR)) {
> > +if (ctx->virt_enabled) {
> > +ctx->virt_inst_excp = true;
> > +}
> 
> Are there any considerations for adding  virt_inst_excp instead of
> directly
> 
> "generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?
> 
> Regards,
> 
> Weiwei Li

I had considered it but I think this is a simpler approach as the rest
of the code path stays the same as generating an illegal instruction
exception, for e.g setting the bins value (tval). Also we need to
return true from all the caller trans_* functions even if the smstateen
check has failed.
> 
> > +return false;
> > +}
> > +
> > +return true;
> > +}
> > +#else
> > +#define smstateen_fcsr_check(ctx, index) (true)
> > +#endif
> > +
> > +#define REQUIRE_ZFINX_OR_F(ctx) do { \
> > +if (!has_ext(ctx, RVF)) { \
> > +if (!ctx->cfg_ptr->ext_zfinx) { \
> > +return false; \
> > +

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

2022-10-03 Thread weiwei 



On 2022/10/3 19:47, Mayuresh Chitale wrote:

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

Signed-off-by: Mayuresh Chitale 
---
  target/riscv/csr.c| 23 
  target/riscv/insn_trans/trans_rvf.c.inc   | 43 +--
  target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++
  3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 71236f2b5d..8b25f885ec 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -84,6 +84,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, 0, SMSTATEEN0_FCSR);
+}
  #endif
  return RISCV_EXCP_NONE;
  }
@@ -2023,6 +2027,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);

  }
@@ -2059,6 +2066,10 @@ static RISCVException write_mstateen0h(CPURISCVState 
*env, int csrno,
  {
  uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
  
+if (!riscv_has_ext(env, RVF)) {

+wr_mask |= SMSTATEEN0_FCSR;
+}
+
  return write_mstateenh(env, csrno, wr_mask, new_val);
  }
  
@@ -2096,6 +2107,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);
  }
  
@@ -2135,6 +2150,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,

  {
  uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
  
+if (!riscv_has_ext(env, RVF)) {

+wr_mask |= SMSTATEEN0_FCSR;
+}
+
  return write_hstateenh(env, csrno, wr_mask, new_val);
  }
  
@@ -2182,6 +2201,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);
  }
  
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc

index a1d3eb52ad..93657680c6 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,9 +24,46 @@
  return false; \
  } while (0)
  
-#define REQUIRE_ZFINX_OR_F(ctx) do {\

-if (!ctx->cfg_ptr->ext_zfinx) { \
-REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int index)
+{
+CPUState *cpu = ctx->cs;
+CPURISCVState *env = cpu->env_ptr;
+uint64_t stateen = env->mstateen[index];
+
+if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+return true;
+}
+
+if (ctx->virt_enabled) {
+stateen &= env->hstateen[index];
+}
+
+if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+stateen &= env->sstateen[index];
+}
+
+if (!(stateen & SMSTATEEN0_FCSR)) {
+if (ctx->virt_enabled) {
+ctx->virt_inst_excp = true;
+}


Are there any considerations for adding  virt_inst_excp instead of directly

"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?

Regards,

Weiwei Li


+return false;
+}
+
+return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+if (!has_ext(ctx, RVF)) { \
+if (!ctx->cfg_ptr->ext_zfinx) { \
+return false; \
+} \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
  } \
  } while (0)
  
diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc

index 5d07150cd0..6c2e338c0a 100644
--- a/target/riscv/insn_trans/trans_rvzfh.c.inc
+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
@@ -20,18 +20,27 @@
  if (!ctx->cfg_ptr->ext_zfh) {  \
  return false; \
  } \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
  } while (0)
  
  #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \

  if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
  return false;  \
  }  \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
  } while (0)
  
  #define

[PATCH v10 4/5] target/riscv: smstateen check for fcsr

2022-10-03 Thread Mayuresh Chitale 
If smstateen is implemented and sstateen0.fcsr is clear then the floating point
operations must return illegal instruction exception or virtual instruction
trap, if relevant.

Signed-off-by: Mayuresh Chitale 
---
 target/riscv/csr.c| 23 
 target/riscv/insn_trans/trans_rvf.c.inc   | 43 +--
 target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++
 3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 71236f2b5d..8b25f885ec 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -84,6 +84,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, 0, SMSTATEEN0_FCSR);
+}
 #endif
 return RISCV_EXCP_NONE;
 }
@@ -2023,6 +2027,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);
 }
@@ -2059,6 +2066,10 @@ static RISCVException write_mstateen0h(CPURISCVState 
*env, int csrno,
 {
 uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+if (!riscv_has_ext(env, RVF)) {
+wr_mask |= SMSTATEEN0_FCSR;
+}
+
 return write_mstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -2096,6 +2107,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);
 }
 
@@ -2135,6 +2150,10 @@ static RISCVException write_hstateen0h(CPURISCVState 
*env, int csrno,
 {
 uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+if (!riscv_has_ext(env, RVF)) {
+wr_mask |= SMSTATEEN0_FCSR;
+}
+
 return write_hstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -2182,6 +2201,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);
 }
 
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc 
b/target/riscv/insn_trans/trans_rvf.c.inc
index a1d3eb52ad..93657680c6 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,9 +24,46 @@
 return false; \
 } while (0)
 
-#define REQUIRE_ZFINX_OR_F(ctx) do {\
-if (!ctx->cfg_ptr->ext_zfinx) { \
-REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int index)
+{
+CPUState *cpu = ctx->cs;
+CPURISCVState *env = cpu->env_ptr;
+uint64_t stateen = env->mstateen[index];
+
+if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+return true;
+}
+
+if (ctx->virt_enabled) {
+stateen &= env->hstateen[index];
+}
+
+if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+stateen &= env->sstateen[index];
+}
+
+if (!(stateen & SMSTATEEN0_FCSR)) {
+if (ctx->virt_enabled) {
+ctx->virt_inst_excp = true;
+}
+return false;
+}
+
+return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+if (!has_ext(ctx, RVF)) { \
+if (!ctx->cfg_ptr->ext_zfinx) { \
+return false; \
+} \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
 } \
 } while (0)
 
diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc 
b/target/riscv/insn_trans/trans_rvzfh.c.inc
index 5d07150cd0..6c2e338c0a 100644
--- a/target/riscv/insn_trans/trans_rvzfh.c.inc
+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
@@ -20,18 +20,27 @@
 if (!ctx->cfg_ptr->ext_zfh) {  \
 return false; \
 } \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
 } while (0)
 
 #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
 if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
 return false;  \
 }  \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
 } while (0)
 
 #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {   \
 if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
 return false; \
 } \
+if (!smstateen_fcsr_check(ctx, 0)) { \
+return false; \
+} \
 } while (0)