target/riscv/insn_trans/trans_rvv.c.inc | 1 + 1 file changed, 1 insertion(+)
GEN_LDST_WHOLE_TRANS checks for require_rvv but it's always true, even with
e.g. -cpu=rv32,v=false. Instead the illegal signal for all vector instructions
(except vset{i}vl{i}) is generated by vext_check_isa_ill, which checks the
context-dependent DisasContext->vill that conditionally makes vector
instructions illegal depending on the state of vl.
Currently, vs*r.v runs even when RVV is disabled or when vl has not been set.
Adding the missing vext_check_isa_ill brings it in line with all other vector
instructions, raising SIGILL in these situations.
Signed-off-by: Sébastien Michelland <sebastien.michelland@inria.fr>
---
v[zs]ext_vf[248] is also suspiciously missing the check, but doesn't fail in
such an obvious manner. If it's also a bug then it might be worth fixing at
the same time.
target/riscv/insn_trans/trans_rvv.c.inc | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
index 4df9a40b44..1e99c35988 100644
--- a/target/riscv/insn_trans/trans_rvv.c.inc
+++ b/target/riscv/insn_trans/trans_rvv.c.inc
@@ -1235,6 +1235,7 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t rs1, uint32_t nf,
static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \
{ \
if (require_rvv(s) && \
+ vext_check_isa_ill(s) && \
QEMU_IS_ALIGNED(a->rd, ARG_NF)) { \
return ldst_whole_trans(a->rd, a->rs1, ARG_NF, ctzl(sizeof(ETYPE)), \
gen_helper_##NAME, s, IS_LOAD); \
--
2.52.0
On 1/26/2026 2:21 PM, Sébastien Michelland wrote:
> GEN_LDST_WHOLE_TRANS checks for require_rvv but it's always true, even with
> e.g. -cpu=rv32,v=false.
hmmm that really shouldn't happen hehe I wonder if there's more bugs
lurking in the shadows somewhere (spoiler: always) ...
> Instead the illegal signal for all vector instructions
> (except vset{i}vl{i}) is generated by vext_check_isa_ill, which checks the
> context-dependent DisasContext->vill that conditionally makes vector
> instructions illegal depending on the state of vl.
>
> Currently, vs*r.v runs even when RVV is disabled or when vl has not been set.
> Adding the missing vext_check_isa_ill brings it in line with all other vector
> instructions, raising SIGILL in these situations.
>
> Signed-off-by: Sébastien Michelland <sebastien.michelland@inria.fr>
> ---
Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
> v[zs]ext_vf[248] is also suspiciously missing the check, but doesn't fail in
> such an obvious manner. If it's also a bug then it might be worth fixing at
> the same time.
>
> target/riscv/insn_trans/trans_rvv.c.inc | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc b/target/riscv/insn_trans/trans_rvv.c.inc
> index 4df9a40b44..1e99c35988 100644
> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> @@ -1235,6 +1235,7 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t rs1, uint32_t nf,
> static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \
> { \
> if (require_rvv(s) && \
> + vext_check_isa_ill(s) && \
> QEMU_IS_ALIGNED(a->rd, ARG_NF)) { \
> return ldst_whole_trans(a->rd, a->rs1, ARG_NF, ctzl(sizeof(ETYPE)), \
> gen_helper_##NAME, s, IS_LOAD); \
>> GEN_LDST_WHOLE_TRANS checks for require_rvv but it's always true, even with
>> e.g. -cpu=rv32,v=false.
>
> hmmm that really shouldn't happen hehe I wonder if there's more bugs
> lurking in the shadows somewhere (spoiler: always) ...
I did wonder about the semantics of require_rvv. I assumed originally that
mstatus_vs would be equal to EXT_STATUS_DISABLED (thus require_rvv() = 0) with
v=false in -cpu.
The comment for vext_check_isa_ill says VILL is indeed intended to work as an
"RVV OK to use" check:
/*
* In cpu_get_tb_cpu_state(), set VILL if RVV was not present.
* So RVV is also be checked in this function.
*/
static bool vext_check_isa_ill(DisasContext *s)
{
return !s->vill;
}
But then I'm unclear why instructions do require_rvv(s) && vext_check_isa_ill(s)
compared to just vext_check_isa_ill(s).
riscv_get_tb_cpu_state sets VILL based on whether Zve32x (the minimal level of
RVV support) is enabled. Maybe the different levels of support are involved and
-cpu=rv32,v=false doesn't disable everything?
AFAICT riscv_get_tb_cpu_state is also the one setting VS to EXT_STATUS_DIRTY
instead of EXT_STATUS_DISABLED, in my case because I run into a CONFIG_USER_ONLY
check (target/riscv/tcg-cpu.c:154):
#ifdef CONFIG_USER_ONLY
fs = EXT_STATUS_DIRTY;
vs = EXT_STATUS_DIRTY;
#else
// non-trivial logic
#endif
That's about as much as I can decipher from a quick dive. If anyone knows how
these pieces are intended to work, that'd be helpful.
(In the meantime, patch below should still be relevant.)
Sébastien
>> Instead the illegal signal for all vector instructions
>> (except vset{i}vl{i}) is generated by vext_check_isa_ill, which checks the
>> context-dependent DisasContext->vill that conditionally makes vector
>> instructions illegal depending on the state of vl.
>>
>> Currently, vs*r.v runs even when RVV is disabled or when vl has not been set.
>> Adding the missing vext_check_isa_ill brings it in line with all other vector
>> instructions, raising SIGILL in these situations.
>>
>> Signed-off-by: Sébastien Michelland <sebastien.michelland@inria.fr>
>> ---
>
> Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>
>
>
>> v[zs]ext_vf[248] is also suspiciously missing the check, but doesn't fail in
>> such an obvious manner. If it's also a bug then it might be worth fixing at
>> the same time.
>>
>> target/riscv/insn_trans/trans_rvv.c.inc | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/target/riscv/insn_trans/trans_rvv.c.inc
>> b/target/riscv/insn_trans/trans_rvv.c.inc
>> index 4df9a40b44..1e99c35988 100644
>> --- a/target/riscv/insn_trans/trans_rvv.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
>> @@ -1235,6 +1235,7 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t rs1,
>> uint32_t nf,
>> static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \
>> { \
>> if (require_rvv(s) && \
>> + vext_check_isa_ill(s) && \
>> QEMU_IS_ALIGNED(a->rd, ARG_NF)) { \
>> return ldst_whole_trans(a->rd, a->rs1, ARG_NF, ctzl(sizeof(ETYPE)), \
> > gen_helper_##NAME, s, IS_LOAD); \
On 2026-01-28 14:26, Sebastien Michelland wrote:
>
> >> GEN_LDST_WHOLE_TRANS checks for require_rvv but it's always true, even with
> >> e.g. -cpu=rv32,v=false.
> >> --- a/target/riscv/insn_trans/trans_rvv.c.inc
> >> +++ b/target/riscv/insn_trans/trans_rvv.c.inc
> >> @@ -1235,6 +1235,7 @@ static bool ldst_whole_trans(uint32_t vd, uint32_t rs1,
> >> uint32_t nf,
> >> static bool trans_##NAME(DisasContext *s, arg_##NAME * a) \
> >> { \
> >> if (require_rvv(s) && \
> >> + vext_check_isa_ill(s) && \
> >> QEMU_IS_ALIGNED(a->rd, ARG_NF)) { \
> >> return ldst_whole_trans(a->rd, a->rs1, ARG_NF, ctzl(sizeof(ETYPE)), \
> > > gen_helper_##NAME, s, IS_LOAD); \
>
Hi Sebastien,
Thank you for this patch and for identifying this bug.
But I think that the proposed fix of adding vext_check_isa_ill to whole
register load/store instructions will violate the RISC-V vector specification.
There is an existing comment in trans_rvv.c.inc:
/*
* load and store whole register instructions ignore vtype and vl setting.
* Thus, we don't need to check vill bit. (Section 7.9)
*/
The RISC-V unpriviledge specification (31.3.4.4. Vector Type Illegal (vill))
states that whole register load/store instructions do not depend on vtype:
"If the vill bit is set, then any attempt to execute a vector instruction
that depends upon vtype will raise an illegal-instruction exception.
NOTE: vset{i}vl{i} and whole register loads and stores do not depend
upon vtype."
- https://github.com/riscv/riscv-isa-manual
This means whole register instructions (vl*r.v, vs*r.v) should not check
the vill bit.
The Spike reference implementation confirms this interpretation. Whole
register instructions use require_vector_novtype which does not check vill,
while regular vector instructions use require_vector which does:
// riscv/v_ext_macros.h - used by vl*r.v, vs*r.v
#define VI_ST_WHOLE \
require_vector_novtype(true); \
...
// riscv/decode_macros.h
#define require_vector_novtype(is_log) \
do { \
require_vector_vs; // Only checks VS status
...
} while (0); // NO vill check!
#define require_vector(alu) \
do { \
require_vector_vs;
require(!P.VU.vill); // Checks vill
...
} while (0);
- https://github.com/riscv-software-src/riscv-isa-sim
With this patch, whole register load/store instructions would incorrectly
raise illegal instruction exceptions when vill=1, even though the V extension
is enabled.
I would recommend that we should fix this bug at riscv_get_tb_cpu_state by
checking the CPU configuration in CONFIG_USER_ONLY block (user mode) to
ensures require_rvv returns false when the vector is disabled.
Thanks,
rnax
Hi Max, > But I think that the proposed fix of adding vext_check_isa_ill to whole > register load/store instructions will violate the RISC-V vector specification. > > With this patch, whole register load/store instructions would incorrectly > raise illegal instruction exceptions when vill=1, even though the V extension > is enabled. You're entirely correct! I should probably have known or noticed that, oops. So to sum up: * The bug is that vs*r currently don't raise SIGILL when using a non-RVV CPU; * but vs*r.v not having a vill check is intended and not the cause; * instead the bug affects all instructions but the vill check just masks it for everyone except vs*r.v. > I would recommend that we should fix this bug at riscv_get_tb_cpu_state by > checking the CPU configuration in CONFIG_USER_ONLY block (user mode) to > ensures require_rvv returns false when the vector is disabled. I'll start from here and come back with a v2 that (hopefully) does the right thing. Sébastien
© 2016 - 2026 Red Hat, Inc.