[v3] target/riscv: preserve vl on element-zero fault

[PATCH v3] target/riscv: preserve vl on element-zero fault

Posted by Zephyr Li 1 month, 2 weeks ago

A unit-stride fault-only-first load must leave vl unchanged when element
zero raises a synchronous exception. Keep the shortened value in a local
bound until all loads complete, and only then update the architectural vl.

Add a bare-metal RVV regression test which faults vle32ff.v on element zero
and checks that vl remains unchanged in the trap handler.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3544
Signed-off-by: Zephyr Li <fritchleybohrer@gmail.com>
---
Changes in v3:
- Rebase onto current master.
- Regenerate the patch to fix the malformed v2 submission.

Changes in v2:
- Fix commit message formatting.
- Enable vector state in the bare-metal test.
- Use an aligned unmapped address and compare against the vsetvli result.
- Express the mstatus.VS setting as a field shift.

 target/riscv/tcg/vector_helper.c          | 23 +++++----
 tests/tcg/riscv64/Makefile.softmmu-target |  5 ++
 tests/tcg/riscv64/test-vle32ff.S          | 57 +++++++++++++++++++++++
 3 files changed, 76 insertions(+), 9 deletions(-)
 create mode 100644 tests/tcg/riscv64/test-vle32ff.S

diff --git a/target/riscv/tcg/vector_helper.c b/target/riscv/tcg/vector_helper.c
index e321ca2616..5a310822b7 100644
--- a/target/riscv/tcg/vector_helper.c
+++ b/target/riscv/tcg/vector_helper.c
@@ -686,7 +686,7 @@ vext_ldff(void *vd, void *v0, target_ulong base, CPURISCVState *env,
           uint32_t desc, vext_ldst_elem_fn_tlb *ldst_tlb,
           vext_ldst_elem_fn_host *ldst_host, uint32_t log2_esz, uintptr_t ra)
 {
-    uint32_t i, k, vl = 0;
+    uint32_t i, k, vl = 0, load_vl;
     uint32_t nf = vext_nf(desc);
     uint32_t vm = vext_vm(desc);
     uint32_t max_elems = vext_max_elems(desc, log2_esz);
@@ -752,22 +752,24 @@ vext_ldff(void *vd, void *v0, target_ulong base, CPURISCVState *env,
         }
     }
 ProbeSuccess:
-    /* load bytes from guest memory */
-    if (vl != 0) {
-        env->vl = vl;
-    }
+    /*
+     * Keep a shortened vl local until all loads complete. In particular,
+     * an exception from element zero must leave the architectural vl alone.
+     */
+    load_vl = vl ? vl : env->vl;
 
-    if (env->vstart < env->vl) {
+    if (env->vstart < load_vl) {
         if (vm) {
             /* Load/store elements in the first page */
             if (likely(elems)) {
+                elems = MIN(elems, load_vl - env->vstart);
                 vext_page_ldst_us(env, vd, addr, elems, nf, max_elems,
                                   log2_esz, true, mmu_index, ldst_tlb,
                                   ldst_host, ra);
             }
 
             /* Load/store elements in the second page */
-            if (unlikely(env->vstart < env->vl)) {
+            if (unlikely(env->vstart < load_vl)) {
                 /* Cross page element */
                 if (unlikely(page_split % msize)) {
                     for (k = 0; k < nf; k++) {
@@ -780,7 +782,7 @@ ProbeSuccess:
 
                 addr = base + ((env->vstart * nf) << log2_esz);
                 /* Get number of elements of second page */
-                elems = env->vl - env->vstart;
+                elems = load_vl - env->vstart;
 
                 /* Load/store elements in the second page */
                 vext_page_ldst_us(env, vd, addr, elems, nf, max_elems,
@@ -788,7 +790,7 @@ ProbeSuccess:
                                   ldst_host, ra);
             }
         } else {
-            for (i = env->vstart; i < env->vl; i++) {
+            for (i = env->vstart; i < load_vl; i++) {
                 k = 0;
                 while (k < nf) {
                     if (!vext_elem_mask(v0, i)) {
@@ -806,6 +808,9 @@ ProbeSuccess:
             }
         }
     }
+    if (vl != 0) {
+        env->vl = vl;
+    }
     env->vstart = 0;
 
     vext_set_tail_elems_1s(env->vl, vd, desc, nf, esz, max_elems);
diff --git a/tests/tcg/riscv64/Makefile.softmmu-target b/tests/tcg/riscv64/Makefile.softmmu-target
index 82be8a2c91..7ae225bd73 100644
--- a/tests/tcg/riscv64/Makefile.softmmu-target
+++ b/tests/tcg/riscv64/Makefile.softmmu-target
@@ -41,5 +41,10 @@ comma:= ,
 run-test-crc32: test-crc32
 	$(call run-test, $<, $(QEMU) -cpu rv64$(comma)xlrbr=true $(QEMU_OPTS)$<)
 
+EXTRA_RUNS += run-test-vle32ff
+run-test-vle32ff: test-vle32ff
+	$(call run-test, $<, $(QEMU) -cpu rv64$(comma)v=true $(QEMU_OPTS)$<)
+test-vle32ff: CFLAGS += -march=rv64gcv
+
 # We don't currently support the multiarch system tests
 undefine MULTIARCH_TESTS
diff --git a/tests/tcg/riscv64/test-vle32ff.S b/tests/tcg/riscv64/test-vle32ff.S
new file mode 100644
index 0000000000..f510960c05
--- /dev/null
+++ b/tests/tcg/riscv64/test-vle32ff.S
@@ -0,0 +1,57 @@
+/*
+ * Verify that a fault-only-first load keeps vl unchanged when element zero
+ * raises a synchronous exception.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+	.option norvc
+
+	.text
+	.globl _start
+_start:
+	lla	t0, trap
+	csrw	mtvec, t0
+
+	li	t0, (1 << 9)	/* Set mstatus.VS to Initial. */
+	csrs	mstatus, t0
+	li	t1, 4
+	vsetvli	t2, t1, e32, m1, ta, ma
+	li	t0, 0x18000000	/* Unmapped gap in the virt memory map. */
+	vle32ff.v	v1, (t0)
+
+	/* Element zero did not trap. */
+	li	a0, 1
+	j	_exit
+
+trap:
+	csrr	t0, mcause
+	li	t1, 5		/* Load access fault. */
+	bne	t0, t1, trap_fail
+
+	csrr	t0, vl
+	bne	t0, t2, trap_fail
+
+	li	a0, 0
+	j	_exit
+
+trap_fail:
+	li	a0, 1
+
+_exit:
+	lla	a1, semiargs
+	li	t0, 0x20026	/* ADP_Stopped_ApplicationExit */
+	sd	t0, 0(a1)
+	sd	a0, 8(a1)
+	li	a0, 0x20	/* TARGET_SYS_EXIT_EXTENDED */
+
+	.balign	16
+	slli	zero, zero, 0x1f
+	ebreak
+	srai	zero, zero, 0x7
+	j	.
+
+	.data
+	.balign	8
+semiargs:
+	.space	16
-- 
2.43.0

Re: [PATCH v3] target/riscv: preserve vl on element-zero fault

Posted by Max Chou 1 month, 2 weeks ago

On 2026-08-11 10:55, Zephyr Li wrote:
> A unit-stride fault-only-first load must leave vl unchanged when element
> zero raises a synchronous exception. Keep the shortened value in a local
> bound until all loads complete, and only then update the architectural vl.
> 

The failing case is a corner case. When the element-0 faulting probe in
vext_ldff() faults, it longjmps out of the helper, ProbeSuccess is never
reached and env->vl is never touched -- that path was already correct.

The broken path is element 0 in a mapped MMIO page: probe_access()
succeeds there, but QEMU cannot know a device transaction will fail
without issuing it that broken the assumption of the pre probe_access()
checking.

> --- /dev/null
> +++ b/tests/tcg/riscv64/test-vle32ff.S
...
> +     vsetvli t2, t1, e32, m1, ta, ma
> +     li      t0, 0x18000000  /* Unmapped gap in the virt memory map.
> */

Actually it is not unmapped -- it is mapped as unassigned MMIO, and that
is exactly why the bug reproduces: an unmapped address would make the
element-0 probe fault and the test would pass even with the bug present.

I think the fixed vext_ldff implementation is valid according to the
RISC-V specification, considering the VL reduction rule and the
non-idempotent constraint. Only the test case wording and the root
cause explanation may need to be updated.

As for the patch:
Reviewed-by: Max Chou <max.chou@sifive.com>

Re: [PATCH v3] target/riscv: preserve vl on element-zero fault

Posted by Zephyr Li 1 month, 2 weeks ago

Thanks Max, that makes sense.

I'll update the root-cause description and the test comment accordingly.
The implementation itself will remain unchanged.

Thanks,
Zephyr

On Wed, Aug 12, 2026 at 11:13 PM Max Chou <max.chou@sifive.com> wrote:

> On 2026-08-11 10:55, Zephyr Li wrote:
> > A unit-stride fault-only-first load must leave vl unchanged when element
> > zero raises a synchronous exception. Keep the shortened value in a local
> > bound until all loads complete, and only then update the architectural
> vl.
> >
>
> The failing case is a corner case. When the element-0 faulting probe in
> vext_ldff() faults, it longjmps out of the helper, ProbeSuccess is never
> reached and env->vl is never touched -- that path was already correct.
>
> The broken path is element 0 in a mapped MMIO page: probe_access()
> succeeds there, but QEMU cannot know a device transaction will fail
> without issuing it that broken the assumption of the pre probe_access()
> checking.
>
> > --- /dev/null
> > +++ b/tests/tcg/riscv64/test-vle32ff.S
> ...
> > +     vsetvli t2, t1, e32, m1, ta, ma
> > +     li      t0, 0x18000000  /* Unmapped gap in the virt memory map.
> > */
>
> Actually it is not unmapped -- it is mapped as unassigned MMIO, and that
> is exactly why the bug reproduces: an unmapped address would make the
> element-0 probe fault and the test would pass even with the bug present.
>
>
> I think the fixed vext_ldff implementation is valid according to the
> RISC-V specification, considering the VL reduction rule and the
> non-idempotent constraint. Only the test case wording and the root
> cause explanation may need to be updated.
>
> As for the patch:
> Reviewed-by: Max Chou <max.chou@sifive.com>
>