On 1/29/25 11:44 AM, Paolo Savini wrote:
Fault-only-first loads in the RISC-V vector extension need to update
the vl with the element index that causes an exception.
In order to ensure this the emulation of this instruction used to probe the
memory covered by the load operation with a loop that iterated over each element
so that when a flag was raised it was possible to set the vl to the
corresponding element index.
This loop was executed every time whether an exception happened or not.
This commit removes the per element memory probing from the main execution path
and adds a braod memory probing first. If this probing raises any flag that is
s/braod/broad ?
not a watchpoint flag (that per standard is allowed by this instruction) we
proceed with the per element probing to find the index of the element causing
the exception and set vl to such index.
Signed-off-by: Paolo Savini <paolo.sav...@embecosm.com>
---
target/riscv/vector_helper.c | 91 ++++++++++++++++++++++--------------
1 file changed, 56 insertions(+), 35 deletions(-)
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index 5386e3b97c..d3ac99c72d 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -635,45 +635,66 @@ vext_ldff(void *vd, void *v0, target_ulong base,
CPURISCVState *env,
uint32_t vma = vext_vma(desc);
target_ulong addr, offset, remain, page_split, elems;
int mmu_index = riscv_env_mmu_index(env, false);
+ int flags;
+ void *host;
VSTART_CHECK_EARLY_EXIT(env);
- /* probe every access */
- for (i = env->vstart; i < env->vl; i++) {
- if (!vm && !vext_elem_mask(v0, i)) {
- continue;
- }
- addr = adjust_addr(env, base + i * (nf << log2_esz));
- if (i == 0) {
- /* Allow fault on first element. */
- probe_pages(env, addr, nf << log2_esz, ra, MMU_DATA_LOAD);
- } else {
- remain = nf << log2_esz;
- while (remain > 0) {
- void *host;
- int flags;
-
- offset = -(addr | TARGET_PAGE_MASK);
-
- /* Probe nonfault on subsequent elements. */
- flags = probe_access_flags(env, addr, offset, MMU_DATA_LOAD,
- mmu_index, true, &host, 0);
-
- /*
- * Stop if invalid (unmapped) or mmio (transaction may fail).
- * Do not stop if watchpoint, as the spec says that
- * first-fault should continue to access the same
- * elements regardless of any watchpoint.
- */
- if (flags & ~TLB_WATCHPOINT) {
- vl = i;
- goto ProbeSuccess;
- }
- if (remain <= offset) {
- break;
+ uint32_t probe_size = env->vl << log2_esz;
I believe this var can be declared and initialized at the start before
VSTART_CHECK_EARLY_EXIT(env).
+ addr = base + ((env->vstart * nf) << log2_esz);
+ page_split = -(addr | TARGET_PAGE_MASK);
+ /* Get number of elements */
+ probe_size = page_split / msize;
+ if (unlikely(env->vstart + probe_size >= env->vl)) {
+ probe_size = env->vl - env->vstart;
+ }
Isn't this the same code, with 'probe_size' instead of 'elems', than this code
after "ProbeSuccess"?
ProbeSuccess:
(...)
/* Calculate the page range of first page */
addr = base + ((env->vstart * nf) << log2_esz);
page_split = -(addr | TARGET_PAGE_MASK);
/* Get number of elements */
elems = page_split / msize;
if (unlikely(env->vstart + elems >= env->vl)) {
elems = env->vl - env->vstart;
}
If that's the case I advise creating a 'vext_get_elems_number()' helper (feel
free to use
another name) that contains this common code and use it in both locations.
+
+ /* Check page permission/pmp/watchpoint/etc. */
+ flags = probe_access_flags(env, adjust_addr(env, addr), probe_size,
+ MMU_DATA_LOAD, mmu_index, true, &host, ra);
+
+ /* If we are crossing a page check also the second page. */
+ if (env->vl > probe_size) {
+ addr = addr + (probe_size << log2_esz);
+ flags |= probe_access_flags(env, adjust_addr(env, addr), probe_size,
+ MMU_DATA_LOAD, mmu_index, true, &host, ra);
+ }
I think it's a good idea to take a look at the 'probe_pages' helper in the same
file.
It seems to be doing a similar job you're doing in these lines, with the
exception
that probe_pages() is using probe_access() instead of probe_access_flags(). A
small
change in the helper could make it work for this case and for the other existing
caller.
This is optional - if it's feasible it would be nice since we'll re-use code,
but
if it makes things too complex to make it happen then just leave it as is.
Thanks,
Daniel
+
+ if (flags & ~TLB_WATCHPOINT) {
+ /* probe every access */
+ for (i = env->vstart; i < env->vl; i++) {
+ if (!vm && !vext_elem_mask(v0, i)) {
+ continue;
+ }
+ addr = adjust_addr(env, base + i * (nf << log2_esz));
+ if (i == 0) {
+ /* Allow fault on first element. */
+ probe_pages(env, addr, nf << log2_esz, ra, MMU_DATA_LOAD);
+ } else {
+ remain = nf << log2_esz;
+ while (remain > 0) {
+ offset = -(addr | TARGET_PAGE_MASK);
+
+ /* Probe nonfault on subsequent elements. */
+ flags = probe_access_flags(env, addr, offset,
MMU_DATA_LOAD,
+ mmu_index, true, &host, 0);
+
+ /*
+ * Stop if invalid (unmapped) or mmio (transaction may
+ * fail). Do not stop if watchpoint, as the spec says that
+ * first-fault should continue to access the same
+ * elements regardless of any watchpoint.
+ */
+ if (flags & ~TLB_WATCHPOINT) {
+ vl = i;
+ goto ProbeSuccess;
+ }
+ if (remain <= offset) {
+ break;
+ }
+ remain -= offset;
+ addr = adjust_addr(env, addr + offset);
}
- remain -= offset;
- addr = adjust_addr(env, addr + offset);
}
}
}
