Re: [RFC PATCH v2 3/4] target/riscv: check smstateen fcsr flag

[Mayuresh Chitale <mchitale@ventanamicro.com>]

On Sat, Apr 15, 2023 at 6:55 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> On 2023/4/15 00:02, 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_rvd.c.inc   | 13 ++++++++----
> >   target/riscv/insn_trans/trans_rvf.c.inc   | 24 +++++++++++++++++++----
> >   target/riscv/insn_trans/trans_rvzfh.c.inc | 18 ++++++++++++++---
> >   3 files changed, 44 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
> > index 2c51e01c40..d9e0cf116f 100644
> > --- a/target/riscv/insn_trans/trans_rvd.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvd.c.inc
> > @@ -18,10 +18,15 @@
> >    * this program.  If not, see <http://www.gnu.org/licenses/>.
> >    */
> >
> > -#define REQUIRE_ZDINX_OR_D(ctx) do { \
> > -    if (!ctx->cfg_ptr->ext_zdinx) { \
> > -        REQUIRE_EXT(ctx, RVD); \
> > -    } \
> > +#define REQUIRE_ZDINX_OR_D(ctx) do {           \
> > +    if (!has_ext(ctx, RVD)) {                  \
> > +        if (!ctx->cfg_ptr->ext_zdinx) {        \
> > +            return false;                      \
> > +        }                                      \
> > +        if (!smstateen_fcsr_check(ctx)) {      \
> > +            return false;                      \
> > +        }                                      \
> > +    }                                          \
> >   } while (0)
> >
> >   #define REQUIRE_EVEN(ctx, reg) do { \
> > diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
> > index 9e9fa2087a..6bf6fe16be 100644
> > --- a/target/riscv/insn_trans/trans_rvf.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> > @@ -24,10 +24,26 @@
> >               return false; \
> >   } while (0)
> >
> > -#define REQUIRE_ZFINX_OR_F(ctx) do {\
> > -    if (!ctx->cfg_ptr->ext_zfinx) { \
> > -        REQUIRE_EXT(ctx, RVF); \
> > -    } \
> > +static inline bool smstateen_fcsr_check(DisasContext *ctx)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +    if (!has_ext(ctx, RVF) && !ctx->smstateen_fcsr_ok) {
>
> We needn't check RVF here. Two reasons:
>
> 1. This check only be done when RVF is diabled.
>
> 2. ctx->smstateen_fcsr_ok is always be true (set in last patch) when RVF
> is enabled

Ok.
>
> > +        ctx->virt_inst_excp = ctx->virt_enabled;
> > +        return false;
> > +    }
> > +#endif
> > +    return true;
> > +}
> > +
> > +#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)) {      \
> > +            return false;                      \
> > +        }                                      \
> > +    }                                          \
> >   } while (0)
> >
> >   #define REQUIRE_ZCF(ctx) do {                  \
> > diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > index 74dde37ff7..74a125e4c0 100644
> > --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > @@ -16,28 +16,40 @@
> >    * this program.  If not, see <http://www.gnu.org/licenses/>.
> >    */
> >
> > -#define REQUIRE_ZFH(ctx) do { \
> > +#define REQUIRE_ZFH(ctx) do {          \
> >       if (!ctx->cfg_ptr->ext_zfh) {      \
> > -        return false;         \
> > -    }                         \
> > +        return false;                  \
> > +    }                                  \
> > +    if (!smstateen_fcsr_check(ctx)) {  \
> > +        return false;                  \
> > +    }                                  \
>
> This is unnecessary here. This check is only for Zhinx.
>
> Similar to REQUIRE_ZFHMIN.
>
> >   } 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)) {  \
> > +        return false;                  \
> > +    }                                  \
>
> This check is only for Zhinx here. So it's better to take the similar
> way as REQUIRE_ZFINX_OR_F.
>
> Similar to REQUIRE_ZFHMIN_OR_ZHINXMIN.

I am not sure I follow the comments above.
The FCSR check is applicable to all the extensions ie zfinx, zdinx,
zhinx and zhinxmin.
>
> Regards,
>
> Weiwei Li
>
> >   } while (0)
> >
> >   #define REQUIRE_ZFHMIN(ctx) do {              \
> >       if (!ctx->cfg_ptr->ext_zfhmin) {          \
> >           return false;                         \
> >       }                                         \
> > +    if (!smstateen_fcsr_check(ctx)) {         \
> > +        return false;                         \
> > +    }                                         \
> >   } while (0)
> >
> >   #define REQUIRE_ZFHMIN_OR_ZHINXMIN(ctx) do {                 \
> >       if (!(ctx->cfg_ptr->ext_zfhmin || ctx->cfg_ptr->ext_zhinxmin)) { \
> >           return false;                                        \
> >       }                                                        \
> > +    if (!smstateen_fcsr_check(ctx)) {                        \
> > +        return false;                                        \
> > +    }                                                        \
> >   } while (0)
> >
> >   static bool trans_flh(DisasContext *ctx, arg_flh *a)
>