| Message ID | 20230410141316.3317474-4-mchitale@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Smstateen FCSR implementation | expand |
On 2023/4/10 22:13, Mayuresh Chitale wrote: > If misa.F and smstateen_fcsr_ok flag are clear then all the floating > point instructions must generate an appropriate exception. > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > --- > target/riscv/insn_trans/trans_rvf.c.inc | 24 ++++++++++++++++++++--- > target/riscv/insn_trans/trans_rvzfh.c.inc | 4 ++++ > 2 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc > index 052408f45c..6173dace46 100644 > --- a/target/riscv/insn_trans/trans_rvf.c.inc > +++ b/target/riscv/insn_trans/trans_rvf.c.inc > @@ -24,9 +24,27 @@ > 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 > +#define smstateen_fcsr_check(ctx) do { \ > + if (!ctx->smstateen_fcsr_ok) { \ > + if (ctx->virt_enabled) { \ > + generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \ > + } else { \ > + generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \ > + } \ We can setctx->virt_inst_excp = ctx->virt_enabledand return false here. Or we need store current opcode to bins before generate_exception. > > + return true; \ > + } \ > +} while (0) > +#else > +#define smstateen_fcsr_check(ctx) > +#endif > + > +#define REQUIRE_ZFINX_OR_F(ctx) do { \ > + if (!has_ext(ctx, RVF)) { \ > + if (!ctx->cfg_ptr->ext_zfinx) { \ > + return false; \ > + } \ > + smstateen_fcsr_check(ctx); \ > } \ > } while (0) > > diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc > index 74dde37ff7..304bee1002 100644 > --- a/target/riscv/insn_trans/trans_rvzfh.c.inc > +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc > @@ -20,24 +20,28 @@ > if (!ctx->cfg_ptr->ext_zfh) { \ > return false; \ > } \ > + smstateen_fcsr_check(ctx); \ > } while (0) > > #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \ > if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \ > return false; \ > } \ > + smstateen_fcsr_check(ctx); \ It's better to remain "\" alignment here. Similar to following cases. Regards, Weiwei Li > } while (0) > > #define REQUIRE_ZFHMIN(ctx) do { \ > if (!ctx->cfg_ptr->ext_zfhmin) { \ > return false; \ > } \ > + smstateen_fcsr_check(ctx); \ > } while (0) > > #define REQUIRE_ZFHMIN_OR_ZHINXMIN(ctx) do { \ > if (!(ctx->cfg_ptr->ext_zfhmin || ctx->cfg_ptr->ext_zhinxmin)) { \ > return false; \ > } \ > + smstateen_fcsr_check(ctx); \ > } while (0) > > static bool trans_flh(DisasContext *ctx, arg_flh *a)
On 4/10/23 07:13, Mayuresh Chitale wrote: > +#ifndef CONFIG_USER_ONLY > +#define smstateen_fcsr_check(ctx) do { \ > + if (!ctx->smstateen_fcsr_ok) { \ > + if (ctx->virt_enabled) { \ > + generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \ > + } else { \ > + generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \ > + } \ > + return true; \ > + } \ > +} while (0) > +#else > +#define smstateen_fcsr_check(ctx) > +#endif > + > +#define REQUIRE_ZFINX_OR_F(ctx) do { \ > + if (!has_ext(ctx, RVF)) { \ > + if (!ctx->cfg_ptr->ext_zfinx) { \ > + return false; \ > + } \ > + smstateen_fcsr_check(ctx); \ > } \ > } while (0) As a matter of style, I strongly object to a *nested* macro returning from the calling function. These should all be changed to normal functions of the form if (!require_xyz(ctx) || !require_abc(ctx)) { return something; } etc. insn_trans/trans_rvv.c.inc is much much cleaner in this respect. r~
On Mon, Apr 10, 2023 at 8:00 PM liweiwei <liweiwei@iscas.ac.cn> wrote: > > > On 2023/4/10 22:13, Mayuresh Chitale wrote: > > If misa.F and smstateen_fcsr_ok flag are clear then all the floating > > point instructions must generate an appropriate exception. > > > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> > > --- > > target/riscv/insn_trans/trans_rvf.c.inc | 24 ++++++++++++++++++++--- > > target/riscv/insn_trans/trans_rvzfh.c.inc | 4 ++++ > > 2 files changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc > > index 052408f45c..6173dace46 100644 > > --- a/target/riscv/insn_trans/trans_rvf.c.inc > > +++ b/target/riscv/insn_trans/trans_rvf.c.inc > > @@ -24,9 +24,27 @@ > > 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 > > +#define smstateen_fcsr_check(ctx) do { \ > > + if (!ctx->smstateen_fcsr_ok) { \ > > + if (ctx->virt_enabled) { \ > > + generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \ > > + } else { \ > > + generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \ > > + } \ > > We can setctx->virt_inst_excp = ctx->virt_enabledand return false here. Ok. > > Or we need store current opcode to bins before generate_exception. > > > > > + return true; \ > > + } \ > > +} while (0) > > +#else > > +#define smstateen_fcsr_check(ctx) > > +#endif > > + > > +#define REQUIRE_ZFINX_OR_F(ctx) do { \ > > + if (!has_ext(ctx, RVF)) { \ > > + if (!ctx->cfg_ptr->ext_zfinx) { \ > > + return false; \ > > + } \ > > + smstateen_fcsr_check(ctx); \ > > } \ > > } while (0) > > > > diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc > > index 74dde37ff7..304bee1002 100644 > > --- a/target/riscv/insn_trans/trans_rvzfh.c.inc > > +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc > > @@ -20,24 +20,28 @@ > > if (!ctx->cfg_ptr->ext_zfh) { \ > > return false; \ > > } \ > > + smstateen_fcsr_check(ctx); \ > > } while (0) > > > > #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \ > > if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \ > > return false; \ > > } \ > > + smstateen_fcsr_check(ctx); \ > > It's better to remain "\" alignment here. Ok. > > Similar to following cases. > > Regards, > > Weiwei Li > > > } while (0) > > > > #define REQUIRE_ZFHMIN(ctx) do { \ > > if (!ctx->cfg_ptr->ext_zfhmin) { \ > > return false; \ > > } \ > > + smstateen_fcsr_check(ctx); \ > > } while (0) > > > > #define REQUIRE_ZFHMIN_OR_ZHINXMIN(ctx) do { \ > > if (!(ctx->cfg_ptr->ext_zfhmin || ctx->cfg_ptr->ext_zhinxmin)) { \ > > return false; \ > > } \ > > + smstateen_fcsr_check(ctx); \ > > } while (0) > > > > static bool trans_flh(DisasContext *ctx, arg_flh *a) >
On Tue, Apr 11, 2023 at 7:23 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/10/23 07:13, Mayuresh Chitale wrote: > > +#ifndef CONFIG_USER_ONLY > > +#define smstateen_fcsr_check(ctx) do { \ > > + if (!ctx->smstateen_fcsr_ok) { \ > > + if (ctx->virt_enabled) { \ > > + generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \ > > + } else { \ > > + generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \ > > + } \ > > + return true; \ > > + } \ > > +} while (0) > > +#else > > +#define smstateen_fcsr_check(ctx) > > +#endif > > + > > +#define REQUIRE_ZFINX_OR_F(ctx) do { \ > > + if (!has_ext(ctx, RVF)) { \ > > + if (!ctx->cfg_ptr->ext_zfinx) { \ > > + return false; \ > > + } \ > > + smstateen_fcsr_check(ctx); \ > > } \ > > } while (0) > > As a matter of style, I strongly object to a *nested* macro returning from the calling > function. These should all be changed to normal functions of the form > > if (!require_xyz(ctx) || !require_abc(ctx)) { > return something; > } > > etc. insn_trans/trans_rvv.c.inc is much much cleaner in this respect. Ok. I will change smstateen_fcsr_check to a function. > > > r~
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc index 052408f45c..6173dace46 100644 --- a/target/riscv/insn_trans/trans_rvf.c.inc +++ b/target/riscv/insn_trans/trans_rvf.c.inc @@ -24,9 +24,27 @@ 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 +#define smstateen_fcsr_check(ctx) do { \ + if (!ctx->smstateen_fcsr_ok) { \ + if (ctx->virt_enabled) { \ + generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT); \ + } else { \ + generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST); \ + } \ + return true; \ + } \ +} while (0) +#else +#define smstateen_fcsr_check(ctx) +#endif + +#define REQUIRE_ZFINX_OR_F(ctx) do { \ + if (!has_ext(ctx, RVF)) { \ + if (!ctx->cfg_ptr->ext_zfinx) { \ + return false; \ + } \ + smstateen_fcsr_check(ctx); \ } \ } while (0) diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc index 74dde37ff7..304bee1002 100644 --- a/target/riscv/insn_trans/trans_rvzfh.c.inc +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc @@ -20,24 +20,28 @@ if (!ctx->cfg_ptr->ext_zfh) { \ return false; \ } \ + smstateen_fcsr_check(ctx); \ } while (0) #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \ if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \ return false; \ } \ + smstateen_fcsr_check(ctx); \ } while (0) #define REQUIRE_ZFHMIN(ctx) do { \ if (!ctx->cfg_ptr->ext_zfhmin) { \ return false; \ } \ + smstateen_fcsr_check(ctx); \ } while (0) #define REQUIRE_ZFHMIN_OR_ZHINXMIN(ctx) do { \ if (!(ctx->cfg_ptr->ext_zfhmin || ctx->cfg_ptr->ext_zhinxmin)) { \ return false; \ } \ + smstateen_fcsr_check(ctx); \ } while (0) static bool trans_flh(DisasContext *ctx, arg_flh *a)
If misa.F and smstateen_fcsr_ok flag are clear then all the floating point instructions must generate an appropriate exception. Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com> --- target/riscv/insn_trans/trans_rvf.c.inc | 24 ++++++++++++++++++++--- target/riscv/insn_trans/trans_rvzfh.c.inc | 4 ++++ 2 files changed, 25 insertions(+), 3 deletions(-)