[v1] target/riscv: Add a property to set vill bit on reserved usage of vsetvli instruction

[PATCH] target/riscv: Add a property to set vill bit on reserved usage of vsetvli instruction

Posted by Vasilis Liaskovitis 5 months ago

Usage of vsetvli instruction is reserved if VLMAX is changed when vsetvli rs1
and rd arguments are x0.

In this case, if the new property is true, only the vill bit will be set.

See https://github.com/riscv/riscv-isa-manual/blob/main/src/v-st-ext.adoc#avl-encoding
According to the spec, the above use cases are reserved, and
"Implementations may set vill in either case."

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2422
Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com>
---
 target/riscv/cpu.c                      |  1 +
 target/riscv/cpu_cfg_fields.h.inc       |  1 +
 target/riscv/helper.h                   |  2 +-
 target/riscv/insn_trans/trans_rvv.c.inc |  4 ++--
 target/riscv/vector_helper.c            | 22 ++++++++++++----------
 5 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 629ac37501..1c29ed3b2b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -2595,6 +2595,7 @@ static const Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU, cfg.rvv_ta_all_1s, false),
     DEFINE_PROP_BOOL("rvv_ma_all_1s", RISCVCPU, cfg.rvv_ma_all_1s, false),
     DEFINE_PROP_BOOL("rvv_vl_half_avl", RISCVCPU, cfg.rvv_vl_half_avl, false),
+    DEFINE_PROP_BOOL("rvv_vsetvl_x0_vill", RISCVCPU, cfg.rvv_vsetvl_x0_vill, false),
 
     /*
      * write_misa() is marked as experimental for now so mark
diff --git a/target/riscv/cpu_cfg_fields.h.inc b/target/riscv/cpu_cfg_fields.h.inc
index 59f134a419..9c78a797cf 100644
--- a/target/riscv/cpu_cfg_fields.h.inc
+++ b/target/riscv/cpu_cfg_fields.h.inc
@@ -114,6 +114,7 @@ BOOL_FIELD(ext_supm)
 BOOL_FIELD(rvv_ta_all_1s)
 BOOL_FIELD(rvv_ma_all_1s)
 BOOL_FIELD(rvv_vl_half_avl)
+BOOL_FIELD(rvv_vsetvl_x0_vill)
 /* Named features  */
 BOOL_FIELD(ext_svade)
 BOOL_FIELD(ext_zic64b)
diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index 85d73e492d..f712b1c368 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -159,7 +159,7 @@ DEF_HELPER_FLAGS_3(hyp_hsv_d, TCG_CALL_NO_WG, void, env, tl, tl)
 #endif
 
 /* Vector functions */
-DEF_HELPER_3(vsetvl, tl, env, tl, tl)
+DEF_HELPER_4(vsetvl, tl, env, tl, tl, tl)
 DEF_HELPER_5(vle8_v, void, ptr, ptr, tl, env, i32)
 DEF_HELPER_5(vle16_v, void, ptr, ptr, tl, env, i32)
 DEF_HELPER_5(vle32_v, void, ptr, ptr, tl, env, i32)
diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
index 2b6077ac06..87071c5d62 100644
--- a/target/riscv/insn_trans/trans_rvv.c.inc
+++ b/target/riscv/insn_trans/trans_rvv.c.inc
@@ -202,7 +202,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1, TCGv s2)
         s1 = get_gpr(s, rs1, EXT_ZERO);
     }
 
-    gen_helper_vsetvl(dst, tcg_env, s1, s2);
+    gen_helper_vsetvl(dst, tcg_env, s1, s2, tcg_constant_tl((int) (rd == 0 && rs1 == 0)));
     gen_set_gpr(s, rd, dst);
     finalize_rvv_inst(s);
 
@@ -222,7 +222,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv s1, TCGv s2)
 
     dst = dest_gpr(s, rd);
 
-    gen_helper_vsetvl(dst, tcg_env, s1, s2);
+    gen_helper_vsetvl(dst, tcg_env, s1, s2, tcg_constant_tl(0));
     gen_set_gpr(s, rd, dst);
     finalize_rvv_inst(s);
     gen_update_pc(s, s->cur_insn_len);
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index 5dc1c10012..2545d73cc1 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -35,7 +35,7 @@
 #include <math.h>
 
 target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
-                            target_ulong s2)
+                            target_ulong s2, target_ulong x0)
 {
     int vlmax, vl;
     RISCVCPU *cpu = env_archcpu(env);
@@ -64,15 +64,6 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
         }
     }
 
-    if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0)) {
-        /* only set vill bit. */
-        env->vill = 1;
-        env->vtype = 0;
-        env->vl = 0;
-        env->vstart = 0;
-        return 0;
-    }
-
     /* lmul encoded as in DisasContext::lmul */
     lmul = sextract32(FIELD_EX64(s2, VTYPE, VLMUL), 0, 3);
     vlmax = vext_get_vlmax(cpu->cfg.vlenb, vsew, lmul);
@@ -83,6 +74,17 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
     } else {
         vl = vlmax;
     }
+
+    if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0) ||
+        (cpu->cfg.rvv_vsetvl_x0_vill && x0 && (env->vl != vl))) {
+        /* only set vill bit. */
+        env->vill = 1;
+        env->vtype = 0;
+        env->vl = 0;
+        env->vstart = 0;
+        return 0;
+    }
+
     env->vl = vl;
     env->vtype = s2;
     env->vstart = 0;
-- 
2.46.0

Re: [PATCH] target/riscv: Add a property to set vill bit on reserved usage of vsetvli instruction

Posted by Daniel Henrique Barboza 4 months, 4 weeks ago

Hi,

Thanks for putting this into a patch. A comment below:

On 6/18/25 4:25 AM, Vasilis Liaskovitis wrote:
> Usage of vsetvli instruction is reserved if VLMAX is changed when vsetvli rs1
> and rd arguments are x0.
> 
> In this case, if the new property is true, only the vill bit will be set.
> 
> See https://github.com/riscv/riscv-isa-manual/blob/main/src/v-st-ext.adoc#avl-encoding
> According to the spec, the above use cases are reserved, and
> "Implementations may set vill in either case."
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2422
> Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com>
> ---
>   target/riscv/cpu.c                      |  1 +
>   target/riscv/cpu_cfg_fields.h.inc       |  1 +
>   target/riscv/helper.h                   |  2 +-
>   target/riscv/insn_trans/trans_rvv.c.inc |  4 ++--
>   target/riscv/vector_helper.c            | 22 ++++++++++++----------
>   5 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 629ac37501..1c29ed3b2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2595,6 +2595,7 @@ static const Property riscv_cpu_properties[] = {
>       DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU, cfg.rvv_ta_all_1s, false),
>       DEFINE_PROP_BOOL("rvv_ma_all_1s", RISCVCPU, cfg.rvv_ma_all_1s, false),
>       DEFINE_PROP_BOOL("rvv_vl_half_avl", RISCVCPU, cfg.rvv_vl_half_avl, false),
> +    DEFINE_PROP_BOOL("rvv_vsetvl_x0_vill", RISCVCPU, cfg.rvv_vsetvl_x0_vill, false),
>   
>       /*
>        * write_misa() is marked as experimental for now so mark
> diff --git a/target/riscv/cpu_cfg_fields.h.inc b/target/riscv/cpu_cfg_fields.h.inc
> index 59f134a419..9c78a797cf 100644
> --- a/target/riscv/cpu_cfg_fields.h.inc
> +++ b/target/riscv/cpu_cfg_fields.h.inc
> @@ -114,6 +114,7 @@ BOOL_FIELD(ext_supm)
>   BOOL_FIELD(rvv_ta_all_1s)
>   BOOL_FIELD(rvv_ma_all_1s)
>   BOOL_FIELD(rvv_vl_half_avl)
> +BOOL_FIELD(rvv_vsetvl_x0_vill)
>   /* Named features  */
>   BOOL_FIELD(ext_svade)
>   BOOL_FIELD(ext_zic64b)
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 85d73e492d..f712b1c368 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -159,7 +159,7 @@ DEF_HELPER_FLAGS_3(hyp_hsv_d, TCG_CALL_NO_WG, void, env, tl, tl)
>   #endif
>   
>   /* Vector functions */
> -DEF_HELPER_3(vsetvl, tl, env, tl, tl)
> +DEF_HELPER_4(vsetvl, tl, env, tl, tl, tl)
>   DEF_HELPER_5(vle8_v, void, ptr, ptr, tl, env, i32)
>   DEF_HELPER_5(vle16_v, void, ptr, ptr, tl, env, i32)
>   DEF_HELPER_5(vle32_v, void, ptr, ptr, tl, env, i32)
> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
> index 2b6077ac06..87071c5d62 100644
> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> @@ -202,7 +202,7 @@ static bool do_vsetvl(DisasContext *s, int rd, int rs1, TCGv s2)
>           s1 = get_gpr(s, rs1, EXT_ZERO);
>       }
>   
> -    gen_helper_vsetvl(dst, tcg_env, s1, s2);
> +    gen_helper_vsetvl(dst, tcg_env, s1, s2, tcg_constant_tl((int) (rd == 0 && rs1 == 0)));
>       gen_set_gpr(s, rd, dst);
>       finalize_rvv_inst(s);
>   
> @@ -222,7 +222,7 @@ static bool do_vsetivli(DisasContext *s, int rd, TCGv s1, TCGv s2)
>   
>       dst = dest_gpr(s, rd);
>   
> -    gen_helper_vsetvl(dst, tcg_env, s1, s2);
> +    gen_helper_vsetvl(dst, tcg_env, s1, s2, tcg_constant_tl(0));
>       gen_set_gpr(s, rd, dst);
>       finalize_rvv_inst(s);
>       gen_update_pc(s, s->cur_insn_len);
> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> index 5dc1c10012..2545d73cc1 100644
> --- a/target/riscv/vector_helper.c
> +++ b/target/riscv/vector_helper.c
> @@ -35,7 +35,7 @@
>   #include <math.h>
>   
>   target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
> -                            target_ulong s2)
> +                            target_ulong s2, target_ulong x0)
>   {
>       int vlmax, vl;
>       RISCVCPU *cpu = env_archcpu(env);
> @@ -64,15 +64,6 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
>           }
>       }
>   
> -    if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0)) {
> -        /* only set vill bit. */
> -        env->vill = 1;
> -        env->vtype = 0;
> -        env->vl = 0;
> -        env->vstart = 0;
> -        return 0;
> -    }
> -
>       /* lmul encoded as in DisasContext::lmul */
>       lmul = sextract32(FIELD_EX64(s2, VTYPE, VLMUL), 0, 3);
>       vlmax = vext_get_vlmax(cpu->cfg.vlenb, vsew, lmul);
> @@ -83,6 +74,17 @@ target_ulong HELPER(vsetvl)(CPURISCVState *env, target_ulong s1,
>       } else {
>           vl = vlmax;
>       }
> +
> +    if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0) ||
> +        (cpu->cfg.rvv_vsetvl_x0_vill && x0 && (env->vl != vl))) {

In this particular case I don't mind replicating code. The first existing check can be
left untouched, exiting early before spending time calculating vlmax and vl.

We can then add the new check that depends on vl here, even if we end up copying those
few "env->something =" lines again. If you really want to avoid code duplication I
wouldn't oppose adding a 'vill' label and using 'goto' as an alternative of duplicating
those 6 lines.


Patch LGTM otherwise. Thanks,


Daniel

> +        /* only set vill bit. */
> +        env->vill = 1;
> +        env->vtype = 0;
> +        env->vl = 0;
> +        env->vstart = 0;
> +        return 0;
> +    }
> +
>       env->vl = vl;
>       env->vtype = s2;
>       env->vstart = 0;

Re: [PATCH] target/riscv: Add a property to set vill bit on reserved usage of vsetvli instruction

Posted by Vasilis Liaskovitis 4 months, 4 weeks ago

Hi,

On Wed, Jun 18, 2025 at 4:22 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> > +    if ((sew > cpu->cfg.elen) || vill || (ediv != 0) || (reserved != 0) ||
> > +        (cpu->cfg.rvv_vsetvl_x0_vill && x0 && (env->vl != vl))) {
>
> In this particular case I don't mind replicating code. The first existing check can be
> left untouched, exiting early before spending time calculating vlmax and vl.
>
> We can then add the new check that depends on vl here, even if we end up copying those
> few "env->something =" lines again. If you really want to avoid code duplication I
> wouldn't oppose adding a 'vill' label and using 'goto' as an alternative of duplicating
> those 6 lines.

thank you for the review. I sent a v2 simply duplicating the code.

thanks,

- Vasilis

>
>
> Patch LGTM otherwise. Thanks,
>
>
> Daniel
>
> > +        /* only set vill bit. */
> > +        env->vill = 1;
> > +        env->vtype = 0;
> > +        env->vl = 0;
> > +        env->vstart = 0;
> > +        return 0;
> > +    }
> > +
> >       env->vl = vl;
> >       env->vtype = s2;
> >       env->vstart = 0;
>