Re: [PATCH 2/2] target/arm: Use memset_ra, memmove_ra in helper-a64.c

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 3 Jul 2024 at 00:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Without this, qemu user will not unwind from the SIGSEGV
> properly and die with
>
>   qemu-aarch64: QEMU internal SIGSEGV {code=ACCERR, addr=0x7d1b36ec2000}
>   Segmentation fault
>
> Fill in the test case for ppc and s390x, which also use memset
> from within a helper (but don't currently crash fwiw).
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/tcg/helper-a64.c        | 10 ++--
>  tests/tcg/multiarch/memset-fault.c | 77 ++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 5 deletions(-)
>  create mode 100644 tests/tcg/multiarch/memset-fault.c
>
> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
> index 0ea8668ab4..666bdb7c1a 100644
> --- a/target/arm/tcg/helper-a64.c
> +++ b/target/arm/tcg/helper-a64.c
> @@ -971,7 +971,7 @@ void HELPER(dc_zva)(CPUARMState *env, uint64_t vaddr_in)
>      }
>  #endif
>
> -    memset(mem, 0, blocklen);
> +    memset_ra(mem, 0, blocklen, GETPC());
>  }
>
>  void HELPER(unaligned_access)(CPUARMState *env, uint64_t addr,
> @@ -1120,7 +1120,7 @@ static uint64_t set_step(CPUARMState *env, uint64_t toaddr,
>      }
>  #endif
>      /* Easy case: just memset the host memory */
> -    memset(mem, data, setsize);
> +    memset_ra(mem, data, setsize, ra);
>      return setsize;
>  }

I think strictly speaking since page_limit() and page_limit_rev()
only look at the target page size and not the host page size,
this will not quite behave correctly in the case where the
host page size is smaller than the target page size, but that
case doesn't work properly in any number of other situations
already, so I don't really care about it.

> @@ -1163,7 +1163,7 @@ static uint64_t set_step_tags(CPUARMState *env, uint64_t toaddr,
>      }
>  #endif
>      /* Easy case: just memset the host memory */
> -    memset(mem, data, setsize);
> +    memset_ra(mem, data, setsize, ra);
>      mte_mops_set_tags(env, toaddr, setsize, *mtedesc);
>      return setsize;
>  }
> @@ -1497,7 +1497,7 @@ static uint64_t copy_step(CPUARMState *env, uint64_t toaddr, uint64_t fromaddr,
>      }
>  #endif
>      /* Easy case: just memmove the host memory */
> -    memmove(wmem, rmem, copysize);
> +    memmove_ra(wmem, rmem, copysize, ra);
>      return copysize;
>  }
>
> @@ -1572,7 +1572,7 @@ static uint64_t copy_step_rev(CPUARMState *env, uint64_t toaddr,
>       * Easy case: just memmove the host memory. Note that wmem and
>       * rmem here point to the *last* byte to copy.
>       */
> -    memmove(wmem - (copysize - 1), rmem - (copysize - 1), copysize);
> +    memmove_ra(wmem - (copysize - 1), rmem - (copysize - 1), copysize, ra);
>      return copysize;
>  }
>
> diff --git a/tests/tcg/multiarch/memset-fault.c b/tests/tcg/multiarch/memset-fault.c
> new file mode 100644
> index 0000000000..0e8258a88f
> --- /dev/null
> +++ b/tests/tcg/multiarch/memset-fault.c
> @@ -0,0 +1,77 @@
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include <assert.h>

Can we have a copyright-and-license header comment for all new
files, please?

> +
> +#if defined(__powerpc64__)
> +/* Needed for PT_* constants */
> +#include <asm/ptrace.h>
> +#endif
> +
> +static void *ptr;
> +static void *pc;
> +
> +static void test(void)
> +{
> +#ifdef __aarch64__
> +    void *t;
> +    asm("adr %0,1f; str %0,%1; 1: dc zva,%2"
> +        : "=&r"(t), "=m"(pc) : "r"(ptr));
> +#elif defined(__powerpc64__)
> +    void *t;
> +    asm("bl 0f; 0: mflr %0; addi %0,%0,1f-0b; std %0,%1; 1: dcbz 0,%2"
> +        : "=&r"(t), "=m"(pc) : "r"(ptr) : "lr");
> +#elif defined(__s390x__)
> +    void *t;
> +    asm("larl %0,1f; stg %0,%1; 1: xc 0(256,%2),0(%2)"
> +        : "=&r"(t), "=m"(pc) : "r"(ptr));
> +#else
> +    *(int *)ptr = 0;
> +#endif
> +}
> +
> +static void *host_signal_pc(ucontext_t *uc)
> +{
> +#ifdef __aarch64__
> +    return (void *)uc->uc_mcontext.pc;
> +#elif defined(__powerpc64__)
> +    return (void *)uc->uc_mcontext.gp_regs[PT_NIP];
> +#elif defined(__s390x__)
> +    return (void *)uc->uc_mcontext.psw.addr;
> +#else
> +    return NULL;
> +#endif
> +}
> +
> +static void sigsegv(int sig, siginfo_t *info, void *uc)
> +{
> +    assert(info->si_addr == ptr);
> +    assert(host_signal_pc(uc) == pc);
> +    exit(0);
> +}
> +
> +int main(void)
> +{
> +    static const struct sigaction sa = {
> +        .sa_flags = SA_SIGINFO,
> +        .sa_sigaction = sigsegv
> +    };
> +    size_t size;
> +    int r;
> +
> +    size = getpagesize();
> +    ptr = mmap(NULL, size, PROT_READ | PROT_WRITE,
> +               MAP_ANON | MAP_PRIVATE, -1, 0);
> +    assert(ptr != MAP_FAILED);
> +
> +    test();
> +
> +    r = sigaction(SIGSEGV, &sa, NULL);
> +    assert(r == 0);
> +    r = mprotect(ptr, size, PROT_NONE);
> +    assert(r == 0);
> +
> +    test();
> +    abort();
> +}

A few comments in this test program to explain what it's
doing would be helpful.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM