Re: [RFC PATCH 08/18] accel/tcg: Replace size with first/last in probe_access_flags

On 15/8/26 22:37, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <[email protected]>
> ---
>   include/accel/tcg/probe.h        | 24 +++++++++++++++++-------
>   accel/tcg/cputlb.c               | 15 ++++++++-------
>   accel/tcg/user-exec.c            |  7 +++++--
>   semihosting/uaccess.c            | 10 ++++------
>   target/arm/tcg/sve_helper.c      |  5 ++++-
>   target/riscv/tcg/op_helper.c     |  4 ++--
>   target/riscv/tcg/vector_helper.c | 21 ++++++++++++---------
>   target/s390x/tcg/mem_helper.c    |  4 ++--
>   8 files changed, 54 insertions(+), 36 deletions(-)
> 
> diff --git a/include/accel/tcg/probe.h b/include/accel/tcg/probe.h
> index e3068a79de..06faf1269d 100644
> --- a/include/accel/tcg/probe.h
> +++ b/include/accel/tcg/probe.h
> @@ -49,23 +49,33 @@ static inline void *probe_read(CPUArchState *env, vaddr addr, int size,
>   /**
>    * probe_access_flags:
>    * @env: CPUArchState
> - * @addr: guest virtual address to look up
> - * @size: size of the access
> + * @addr: virtual address
> + * @first: beginning of virtual address range
> + * @last: end of virtual address range
>    * @access_type: read, write or execute permission
>    * @mmu_idx: MMU index to use for lookup
>    * @nonfault: suppress the fault
>    * @phost: return value for host address
>    * @retaddr: return address for unwinding
>    *
> - * Similar to probe_access, loosely returning the TLB_FLAGS_MASK for
> - * the page, and storing the host address for RAM in @phost.
> + * Probe an access for [@first, @last], where @addr is somewhere
> + * in that range.  Normally @addr == @first, but some targets have
> + * accesses which are forcibly aligned after an initial fault for an
> + * inaccessible page (e.g. Arm DC_ZVA, where [@first, @last] will be
> + * the bounds of the cacheline containing @addr).
> + *
> + * If the access does not satisfy @access_type:
> + *   - if @nonfault is false, raise an exception at @addr
> + *   - otherwise return TLB_INVALID_MASK.
> + *
> + * Otherwise, return the TLB_FLAGS_MASK for the page, and set @phost:
> + *   - host address for @addr, if direct host accesses are allowed,
> + *   - otherwise NULL.

This If/Otherwise block sounds like implementation detail leaking into
documentation API contract, but I don't mind.

>    *
> - * If @nonfault is set, do not raise an exception but return TLB_INVALID_MASK.
>    * Do not handle watchpoints, but include TLB_WATCHPOINT in the returned flags.
>    * Do handle clean pages, so exclude TLB_NOTDIRY from the returned flags.
> - * For simplicity, all "mmio-like" flags are folded to TLB_MMIO.
>    */
> -int probe_access_flags(CPUArchState *env, vaddr addr, int size,
> +int probe_access_flags(CPUArchState *env, vaddr addr, vaddr first, vaddr last,
>                          MMUAccessType access_type, int mmu_idx,
>                          bool nonfault, void **phost, uintptr_t retaddr);


> diff --git a/target/riscv/tcg/vector_helper.c b/target/riscv/tcg/vector_helper.c
> index b7b3805a6d..87f2196bc7 100644
> --- a/target/riscv/tcg/vector_helper.c
> +++ b/target/riscv/tcg/vector_helper.c
> @@ -704,7 +704,7 @@ vext_ldff(void *vd, target_ulong base, CPURISCVState *env,
>       uint32_t esz = 1 << log2_esz;
>       uint32_t msize = nf * esz;
>       uint32_t vma = vext_vma(desc);
> -    target_ulong addr, last, last_in_page, page_split, elems;
> +    target_ulong addr, last, last_in_page, page_split, elems, adj, adj_last;
>       MemOpIdx oi = vext_oi(desc, log2_esz);
>       int mmu_index = get_mmuidx(oi);
>       bool first_active;
> @@ -759,10 +759,10 @@ vext_ldff(void *vd, target_ulong base, CPURISCVState *env,
>        * 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);
> +    adj = adjust_addr(env, addr);
> +    adj_last = MIN(last, last_in_page) - (addr - adj);
> +    flags = probe_access_flags(env, adj, adj, adj_last, MMU_DATA_LOAD,
> +                               mmu_index, !first_active, &host, ra);

Something in this function (on the pre-existing code) bugged me,
confirmed by your cover letter (I haven't looked at the RISCV
preliminary series).

>       /* Get number of complete elements in the first page. */
>       elems = MIN(page_split / msize, vl - i);
> @@ -853,8 +853,10 @@ vext_ldff(void *vd, target_ulong base, CPURISCVState *env,
>            * 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,
> +
> +        adj = adjust_addr(env, next_page);
> +        adj_last = last - (next_page - adj);
> +        flags |= probe_access_flags(env, adj, adj, adj_last, MMU_DATA_LOAD,
>                                       mmu_index, true, &host, ra);
>   
>           /* Stop if invalid (unmapped) or mmio (transaction may fail). */
> @@ -873,8 +875,9 @@ vext_ldff(void *vd, target_ulong base, CPURISCVState *env,
>               host += addr - next_page;
>           }
>       } else {
> -        flags = probe_access_flags(env, adjust_addr(env, addr),
> -                                   last - addr + 1, MMU_DATA_LOAD,
> +        adj = adjust_addr(env, addr);
> +        adj_last = last - (addr - adj);
> +        flags = probe_access_flags(env, adj, adj, adj_last, MMU_DATA_LOAD,
>                                      mmu_index, true, &host, ra);

Reviewed-by: Philippe Mathieu-Daudé <[email protected]>