Re: [PATCH 11/23] target/riscv: Rewrite vext_ldff

On 2026-08-15 12:45, Richard Henderson wrote:
> Do not call probe_pages for every active element.
> We can make do with no more than 2 such calls for
> the two pages the insn might reference.
> 
> Signed-off-by: Richard Henderson <[email protected]>
> +    /*
> +     * Test whether the first page is accessible.
> +     * If the first element is active, it must succeed.
> +     */
> +    flags = probe_access_flags(env, adjust_addr(env, addr),
> +                               MIN(last, last_in_page) - addr + 1,
> +                               MMU_DATA_LOAD, mmu_index, !first_active,
> +                               &host, ra);
>  

Hi Richard,

This probe traverses every byte from the initial active element to the
end of the page, not just the bytes of active elements. In the masked
case, the range may encompass a masked-off element, and masked-off body
elements do not perform memory accesses.
I think it may causes unexpected vl.

> +    /* Get number of complete elements in the first page. */
> +    elems = MIN(page_split / msize, vl - i);
> +
> +    /* Load complete elements from the first page. */
> +    if (likely(elems)) {
> +        uint32_t page_evl = i + elems;
> +
> +        if (flags == 0) {
...
> +        } else {
> +            /*
> +             * If the first element is active, it must succeed.
> +             * This will load from MMIO or fault from INVALID.
> +             */
> +            if (first_active) {
> +                vext_ldst_nf_tlb(env, vd, addr, 0, nf, esz,
> +                                 max_elems, ldst_tlb, ra);
> +                i = 1;
> +                addr += msize;
> +            }
> +
> +            /* Stop if invalid (unmapped) or mmio (transaction may fail). */
> +            if (flags & (TLB_INVALID_MASK | TLB_MMIO)) {
> +                env->vl = i;
> +                goto tail;
> +            }
> +

For an example, assume
- vl = 3
- vstart = 0
- the mask be [1, 0, 1]
- assume element 0 and element 2 be readable, but deny the byte at element 1
- all three elements are in the same target page.

In theory, the element 1 is masked off and doesn’t perform any memory
access, so the value of vl remains 3.

But the previous probe covers element 1 to 2 and the flags will be non
zero due to the denied masked-off element 2. Then the vl will set to 1
here.

Maybe we could switch to per element prob when vm is 0 and flags is not 0?

rnax

> +            /* None of these ldst_tlb calls may fault. */
> +            if (vm) {
> +                vext_page_ldst_us_tlb(env, vd, addr, i, page_evl, nf,
> +                                      log2_esz, max_elems,
> +                                      ldst_tlb, mmu_index, ra);
> +            } else {
> +                do {
> +                    if (vext_elem_mask(v0, i)) {
> +                        vext_ldst_nf_tlb(env, vd, base + i * msize, i, nf,
> +                                         esz, max_elems, ldst_tlb, ra);
> +                    } else if (vma) {
> +                        vext_set_nf_elems_1s(vd, i, nf, esz, max_elems);
>                      }
> -                    remain -= offset;
> -                    addr_i = adjust_addr(env, addr_i + offset);
> -                }
> +                } while (++i < page_evl);
> +            }
> +        }
> +
> +        /* Usually the first page contains the entire vector. */
> +        if (likely(page_evl == vl)) {
> +            goto tail;
> +        }
> +        i = page_evl;
> +    }
> +
> +    /* Skip forward to the next active element. */
> +    if (!vm) {
> +        while (1) {
> +            if (vext_elem_mask(v0, i)) {
> +                break;
> +            }
> +            if (vma) {
> +                vext_set_nf_elems_1s(vd, i, nf, esz, max_elems);
> +            }
> +            if (++i == vl) {
> +                goto tail;
>              }
>          }
>      }
> -ProbeSuccess:
> -    /* load bytes from guest memory */
> -    if (vl != 0) {
> -        env->vl = vl;
> +
> +    addr = base + i * msize;
> +    page_split = -(addr | TARGET_PAGE_MASK);
> +
> +    /* Validate the second page is accessible. */
> +    if (unlikely(page_split < msize)) {
> +        /*
> +         * Cross page element which isn't first.
> +         * We have not yet advanced addr to the next page.
> +         */
> +        target_ulong next_page = addr + page_split;
> +        flags |= probe_access_flags(env, adjust_addr(env, next_page),
> +                                    last - next_page + 1, MMU_DATA_LOAD,
> +                                    mmu_index, true, &host, ra);
> +
> +        /* Stop if invalid (unmapped) or mmio (transaction may fail). */
> +        if (flags & (TLB_INVALID_MASK | TLB_MMIO)) {
> +            env->vl = i;
> +            goto tail;
> +        }
> +
> +        vext_ldst_nf_tlb(env, vd, addr, i, nf, esz, max_elems, ldst_tlb, ra);
> +        if (++i == vl) {
> +            goto tail;
> +        }
> +        addr += msize;
> +        if (host) {
> +            host += addr - next_page;
> +        }
> +    } else {
> +        flags = probe_access_flags(env, adjust_addr(env, addr),
> +                                   last - addr + 1, MMU_DATA_LOAD,
> +                                   mmu_index, true, &host, ra);
> +
> +        /* Stop if invalid (unmapped) or mmio (transaction may fail). */
> +        if (flags & (TLB_INVALID_MASK | TLB_MMIO)) {
> +            env->vl = i;
> +            goto tail;
> +        }
>      }
>  
> -    if (env->vstart < env->vl) {
> +    /* Load complete elements from the second page. */
> +    if (flags == 0) {
>          if (vm) {
> -            /* Load/store elements in the first page */
> -            if (likely(elems)) {
> -                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)) {
> -                addr = base + env->vstart * msize;
> -
> -                /* Cross page element */
> -                if (unlikely(page_split % msize)) {
> -                    vext_ldst_nf_tlb(env, vd, addr, env->vstart, nf,
> -                                     esz, max_elems, ldst_tlb, ra);
> -                    env->vstart++;
> -                    addr += msize;
> -                }
> -
> -                /* Get number of elements of second page */
> -                elems = env->vl - env->vstart;
> -
> -                /* Load/store elements in the second page */
> -                vext_page_ldst_us(env, vd, addr, elems, nf, max_elems,
> -                                  log2_esz, true, mmu_index, ldst_tlb,
> -                                  ldst_host, ra);
> -            }
> +            vext_page_ldst_us_host(vd, host, i, vl, nf,
> +                                   log2_esz, max_elems, ldst_host);
>          } else {
> -            for (i = env->vstart; i < env->vl; i++) {
> +            host -= addr - base;
> +            do {
> +                if (vext_elem_mask(v0, i)) {
> +                    vext_ldst_nf_host(vd, host + i * msize, i, nf, esz,
> +                                      max_elems, ldst_host);
> +                } else if (vma) {
> +                    vext_set_nf_elems_1s(vd, i, nf, esz, max_elems);
> +                }
> +            } while (++i < vl);
> +        }
> +    } else {
> +        /* None of these ldst_tlb calls may fault. */
> +        if (vm) {
> +            vext_page_ldst_us_tlb(env, vd, addr, i, vl, nf,
> +                                  log2_esz, max_elems,
> +                                  ldst_tlb, mmu_index, ra);
> +        } else {
> +            do {
>                  if (vext_elem_mask(v0, i)) {
>                      vext_ldst_nf_tlb(env, vd, base + i * msize, i, nf,
>                                       esz, max_elems, ldst_tlb, ra);
>                  } else if (vma) {
>                      vext_set_nf_elems_1s(vd, i, nf, esz, max_elems);
>                  }
> -            }
> +            } while (++i < vl);
>          }
>      }
>  
> -- 
> 2.43.0
> 
>