Re: [Qemu-devel] [PATCH v2 27/32] s390x/tcg: Provide probe_write helper

[Richard Henderson <richard.henderson@linaro.org>]

On 3/1/19 3:54 AM, David Hildenbrand wrote:
> Instead of checking e.g. the first access on every touched page, we should
> check the actual access, otherwise we might get false positives when Low
> Address Protection (LAP) is active. As probe_write() can only deal with
> accesses to one page, we have to loop.
> 
> Use i64 for the length, although not needed - easier to reuse
> TCG temps we already have in the translation functions where this will
> be used. Also allow it to be used from other helpers.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/helper.h     |  1 +
>  target/s390x/internal.h   |  2 ++
>  target/s390x/mem_helper.c | 21 +++++++++++++++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 577edb384f..e2710f4fb3 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -123,6 +123,7 @@ DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>  DEF_HELPER_5(msa, i32, env, i32, i32, i32, i32)
>  DEF_HELPER_FLAGS_1(stpt, TCG_CALL_NO_RWG, i64, env)
>  DEF_HELPER_FLAGS_1(stck, TCG_CALL_NO_RWG_SE, i64, env)
> +DEF_HELPER_FLAGS_3(probe_write_access, TCG_CALL_NO_WG, void, env, i64, i64)
>  
>  /* === Vector Support Instructions === */
>  DEF_HELPER_FLAGS_4(vll, TCG_CALL_NO_WG, void, env, ptr, i64, i64)
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index 7baf0e2404..848d6c36d0 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -386,6 +386,8 @@ void ioinst_handle_sal(S390CPU *cpu, uint64_t reg1, uintptr_t ra);
>  
>  /* mem_helper.c */
>  target_ulong mmu_real2abs(CPUS390XState *env, target_ulong raddr);
> +void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
> +                        uintptr_t ra);
>  
>  
>  /* mmu_helper.c */
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index a506d9ef99..efd5256ebf 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -2623,3 +2623,24 @@ uint32_t HELPER(cu42)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t m3)
>      return convert_unicode(env, r1, r2, m3, GETPC(),
>                             decode_utf32, encode_utf16);
>  }
> +
> +void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
> +                        uintptr_t ra)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    /* test the actual access, not just any access to the page due to LAP */
> +    while (len) {
> +        uint64_t curlen = MIN(TARGET_PAGE_SIZE - (addr % TARGET_PAGE_SIZE),
> +                              len);

pagelen = -(addr | TARGET_PAGE_MASK);
curlen = MIN(pagelen, len);

> +
> +        probe_write(env, addr, curlen, cpu_mmu_index(env, false), ra);
> +        addr = wrap_address(env, addr + curlen);
> +        len -= curlen;
> +    }
> +#endif

That's annoying.  I thought we defined probe_write for user-only as well.

The user-only version would look something like

  uint64_t end = addr + len - 1;

  if (!h2g_valid(addr) ||
      !h2g_valid(end) ||
      page_check_range(addr, len, PAGE_WRITE) < 0) {
    // raise PGM_ADDRESSING
  }

Ah, I see why -- there's no user-only equivalent of tlb_fill with which to
produce the exception in the failure case.


r~