[PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[Richard Henderson]

While looking into Zoltan's attempt to speed up ppc64 DCBZ
(data cache block set to zero), I wondered what AArch64 was
doing differently.  It turned out that Arm is the only user
of tlb_vaddr_to_host.

None of the code sequences in use between AArch64, Power64 and S390X
are 100% safe, with race conditions vs mmap et al, however, AArch64
is the only one that will fail this single threaded test case.  Use
of these new functions fixes the race condition as well, though I
have not yet touched the other guests.

I thought about exposing accel/tcg/user-retaddr.h for direct use
from the targets, but perhaps these wrappers are cleaner.  RFC?


r~


Richard Henderson (2):
  accel/tcg: Introduce memset_ra, memmove_ra
  target/arm: Use memset_ra, memmove_ra in helper-a64.c

 include/exec/cpu_ldst.h            | 40 ++++++++++++++++
 accel/tcg/user-exec.c              | 22 +++++++++
 target/arm/tcg/helper-a64.c        | 10 ++--
 tests/tcg/multiarch/memset-fault.c | 77 ++++++++++++++++++++++++++++++
 4 files changed, 144 insertions(+), 5 deletions(-)
 create mode 100644 tests/tcg/multiarch/memset-fault.c

-- 
2.34.1

[PATCH 1/2] accel/tcg: Introduce memset_ra, memmove_ra

[Richard Henderson]

Add wrappers that set and clear helper_retaddr around the
host memory operation.  This cannot fail for system mode,
but might raise SIGSEGV for user mode.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu_ldst.h | 40 ++++++++++++++++++++++++++++++++++++++++
 accel/tcg/user-exec.c   | 22 ++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 71009f84f5..baf4f9367d 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -379,4 +379,44 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
                         MMUAccessType access_type, int mmu_idx);
 #endif
 
+/**
+ * memset_ra:
+ * @p: host pointer
+ * @c: data
+ * @n: length
+ * @ra: unwind return address
+ *
+ * Like system memset(p,c,n), except manages @ra for the calling
+ * helper in the event of a signal.  To be used with the result
+ * of tlb_vaddr_to_host to resolve the host pointer.
+ */
+#ifdef CONFIG_USER_ONLY
+void *memset_ra(void *p, int c, size_t n, uintptr_t ra);
+#else
+static inline void *memset_ra(void *p, int c, size_t n, uintptr_t ra)
+{
+    return memset(p, c, n);
+}
+#endif
+
+/**
+ * memmove_ra:
+ * @d: host destination pointer
+ * @s: host source pointer
+ * @n: length
+ * @ra: unwind return address
+ *
+ * Like system memmove(d,s,n), except manages @ra for the calling
+ * helper in the event of a signal.  To be used with the result of
+ * tlb_vaddr_to_host to resolve the host pointer.
+ */
+#ifdef CONFIG_USER_ONLY
+void *memmove_ra(void *d, const void *s, size_t n, uintptr_t ra);
+#else
+static inline void *memmove_ra(void *d, const void *s, size_t n, uintptr_t ra)
+{
+    return memmove(d, s, n);
+}
+#endif
+
 #endif /* CPU_LDST_H */
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 80d24540ed..a73da42e3a 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -1289,3 +1289,25 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
 #define DATA_SIZE 16
 #include "atomic_template.h"
 #endif
+
+void *memset_ra(void *p, int c, size_t n, uintptr_t ra)
+{
+    void *r;
+
+    set_helper_retaddr(ra);
+    r = memset(p, c, n);
+    clear_helper_retaddr();
+
+    return r;
+}
+
+void *memmove_ra(void *d, const void *s, size_t n, uintptr_t ra)
+{
+    void *r;
+
+    set_helper_retaddr(ra);
+    r = memmove(d, s, n);
+    clear_helper_retaddr();
+
+    return r;
+}
-- 
2.34.1

Re: [PATCH 1/2] accel/tcg: Introduce memset_ra, memmove_ra

[Peter Maydell]

On Wed, 3 Jul 2024 at 00:43, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Add wrappers that set and clear helper_retaddr around the
> host memory operation.  This cannot fail for system mode,
> but might raise SIGSEGV for user mode.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/cpu_ldst.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  accel/tcg/user-exec.c   | 22 ++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index 71009f84f5..baf4f9367d 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -379,4 +379,44 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
>                          MMUAccessType access_type, int mmu_idx);
>  #endif
>
> +/**
> + * memset_ra:
> + * @p: host pointer
> + * @c: data
> + * @n: length
> + * @ra: unwind return address
> + *
> + * Like system memset(p,c,n), except manages @ra for the calling
> + * helper in the event of a signal.  To be used with the result
> + * of tlb_vaddr_to_host to resolve the host pointer.
> + */
> +#ifdef CONFIG_USER_ONLY
> +void *memset_ra(void *p, int c, size_t n, uintptr_t ra);
> +#else
> +static inline void *memset_ra(void *p, int c, size_t n, uintptr_t ra)
> +{
> +    return memset(p, c, n);
> +}
> +#endif
> +
> +/**
> + * memmove_ra:
> + * @d: host destination pointer
> + * @s: host source pointer
> + * @n: length
> + * @ra: unwind return address
> + *
> + * Like system memmove(d,s,n), except manages @ra for the calling
> + * helper in the event of a signal.  To be used with the result of
> + * tlb_vaddr_to_host to resolve the host pointer.
> + */
> +#ifdef CONFIG_USER_ONLY
> +void *memmove_ra(void *d, const void *s, size_t n, uintptr_t ra);
> +#else
> +static inline void *memmove_ra(void *d, const void *s, size_t n, uintptr_t ra)
> +{
> +    return memmove(d, s, n);
> +}
> +#endif

I guess these make sense. I feel like they're a function where
the caller needs to be quite careful about what they're doing
(e.g. not to use them in a way that the memmove or memset
would cross a page boundary if other guest register state needs
to be kept in sync with the reported fault address), but I
can't think of a useful non-architecture-specific warning that
would be worth putting in the doc comments.

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

thanks
-- PMM

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

[Richard Henderson]

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;
 }
 
@@ -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>
+
+#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();
+}
-- 
2.34.1

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

[Peter Maydell]

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

Re: [PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[Ilya Leoshkevich]

On Tue, 2024-07-02 at 16:41 -0700, Richard Henderson wrote:
> While looking into Zoltan's attempt to speed up ppc64 DCBZ
> (data cache block set to zero), I wondered what AArch64 was
> doing differently.  It turned out that Arm is the only user
> of tlb_vaddr_to_host.
> 
> None of the code sequences in use between AArch64, Power64 and S390X
> are 100% safe, with race conditions vs mmap et al, however, AArch64
> is the only one that will fail this single threaded test case.  Use
> of these new functions fixes the race condition as well, though I
> have not yet touched the other guests.
> 
> I thought about exposing accel/tcg/user-retaddr.h for direct use
> from the targets, but perhaps these wrappers are cleaner.  RFC?
> 
> 
> r~
> 
> 
> Richard Henderson (2):
>   accel/tcg: Introduce memset_ra, memmove_ra
>   target/arm: Use memset_ra, memmove_ra in helper-a64.c
> 
>  include/exec/cpu_ldst.h            | 40 ++++++++++++++++
>  accel/tcg/user-exec.c              | 22 +++++++++
>  target/arm/tcg/helper-a64.c        | 10 ++--
>  tests/tcg/multiarch/memset-fault.c | 77
> ++++++++++++++++++++++++++++++
>  4 files changed, 144 insertions(+), 5 deletions(-)
>  create mode 100644 tests/tcg/multiarch/memset-fault.c

This sounds good to me.

I haven't debugged it, but I wonder why doesn't s390x fail here.
For XC with src == dst, it does access_memset() -> do_access_memset()
-> memset() without setting the RA. And I don't think that anything
around it sets the RA either.

Re: [PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[Richard Henderson]

On 7/4/24 07:50, Ilya Leoshkevich wrote:
> On Tue, 2024-07-02 at 16:41 -0700, Richard Henderson wrote:
>> While looking into Zoltan's attempt to speed up ppc64 DCBZ
>> (data cache block set to zero), I wondered what AArch64 was
>> doing differently.  It turned out that Arm is the only user
>> of tlb_vaddr_to_host.
>>
>> None of the code sequences in use between AArch64, Power64 and S390X
>> are 100% safe, with race conditions vs mmap et al, however, AArch64
>> is the only one that will fail this single threaded test case.  Use
>> of these new functions fixes the race condition as well, though I
>> have not yet touched the other guests.
>>
>> I thought about exposing accel/tcg/user-retaddr.h for direct use
>> from the targets, but perhaps these wrappers are cleaner.  RFC?
>>
>>
>> r~
>>
>>
>> Richard Henderson (2):
>>    accel/tcg: Introduce memset_ra, memmove_ra
>>    target/arm: Use memset_ra, memmove_ra in helper-a64.c
>>
>>   include/exec/cpu_ldst.h            | 40 ++++++++++++++++
>>   accel/tcg/user-exec.c              | 22 +++++++++
>>   target/arm/tcg/helper-a64.c        | 10 ++--
>>   tests/tcg/multiarch/memset-fault.c | 77
>> ++++++++++++++++++++++++++++++
>>   4 files changed, 144 insertions(+), 5 deletions(-)
>>   create mode 100644 tests/tcg/multiarch/memset-fault.c
> 
> This sounds good to me.
> 
> I haven't debugged it, but I wonder why doesn't s390x fail here.
> For XC with src == dst, it does access_memset() -> do_access_memset()
> -> memset() without setting the RA. And I don't think that anything
> around it sets the RA either.

s390x uses probe_access_flags, which verifies the page is mapped and writable, and raises 
the exception when it isn't.  In contrast, for user-only, tlb_vaddr_to_host *only* 
performs the guest -> host address mapping, i.e. (addr + guest_base).


r~

Re: [PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[Richard Henderson]

On 7/4/24 08:18, Richard Henderson wrote:
> On 7/4/24 07:50, Ilya Leoshkevich wrote:
>> On Tue, 2024-07-02 at 16:41 -0700, Richard Henderson wrote:
>>> While looking into Zoltan's attempt to speed up ppc64 DCBZ
>>> (data cache block set to zero), I wondered what AArch64 was
>>> doing differently.  It turned out that Arm is the only user
>>> of tlb_vaddr_to_host.
>>>
>>> None of the code sequences in use between AArch64, Power64 and S390X
>>> are 100% safe, with race conditions vs mmap et al, however, AArch64
>>> is the only one that will fail this single threaded test case.  Use
>>> of these new functions fixes the race condition as well, though I
>>> have not yet touched the other guests.
>>>
>>> I thought about exposing accel/tcg/user-retaddr.h for direct use
>>> from the targets, but perhaps these wrappers are cleaner.  RFC?
>>>
>>>
>>> r~
>>>
>>>
>>> Richard Henderson (2):
>>>    accel/tcg: Introduce memset_ra, memmove_ra
>>>    target/arm: Use memset_ra, memmove_ra in helper-a64.c
>>>
>>>   include/exec/cpu_ldst.h            | 40 ++++++++++++++++
>>>   accel/tcg/user-exec.c              | 22 +++++++++
>>>   target/arm/tcg/helper-a64.c        | 10 ++--
>>>   tests/tcg/multiarch/memset-fault.c | 77
>>> ++++++++++++++++++++++++++++++
>>>   4 files changed, 144 insertions(+), 5 deletions(-)
>>>   create mode 100644 tests/tcg/multiarch/memset-fault.c
>>
>> This sounds good to me.
>>
>> I haven't debugged it, but I wonder why doesn't s390x fail here.
>> For XC with src == dst, it does access_memset() -> do_access_memset()
>> -> memset() without setting the RA. And I don't think that anything
>> around it sets the RA either.
> 
> s390x uses probe_access_flags, which verifies the page is mapped and writable, and raises 
> the exception when it isn't.  In contrast, for user-only, tlb_vaddr_to_host *only* 
> performs the guest -> host address mapping, i.e. (addr + guest_base).

I should clarify: probe_access_flags verifies that the page is mapped *at that moment*, 
but does not take the mmap_lock.  So the race is that the page can be unmapped by another 
thread after probe_access_flags and before the memset completes.


r~

Re: [PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[Ilya Leoshkevich]

On Thu, 2024-07-04 at 14:48 -0700, Richard Henderson wrote:
> On 7/4/24 08:18, Richard Henderson wrote:
> > On 7/4/24 07:50, Ilya Leoshkevich wrote:
> > > On Tue, 2024-07-02 at 16:41 -0700, Richard Henderson wrote:
> > > > While looking into Zoltan's attempt to speed up ppc64 DCBZ
> > > > (data cache block set to zero), I wondered what AArch64 was
> > > > doing differently.  It turned out that Arm is the only user
> > > > of tlb_vaddr_to_host.
> > > > 
> > > > None of the code sequences in use between AArch64, Power64 and
> > > > S390X
> > > > are 100% safe, with race conditions vs mmap et al, however,
> > > > AArch64
> > > > is the only one that will fail this single threaded test case. 
> > > > Use
> > > > of these new functions fixes the race condition as well, though
> > > > I
> > > > have not yet touched the other guests.
> > > > 
> > > > I thought about exposing accel/tcg/user-retaddr.h for direct
> > > > use
> > > > from the targets, but perhaps these wrappers are cleaner.  RFC?
> > > > 
> > > > 
> > > > r~
> > > > 
> > > > 
> > > > Richard Henderson (2):
> > > >    accel/tcg: Introduce memset_ra, memmove_ra
> > > >    target/arm: Use memset_ra, memmove_ra in helper-a64.c
> > > > 
> > > >   include/exec/cpu_ldst.h            | 40 ++++++++++++++++
> > > >   accel/tcg/user-exec.c              | 22 +++++++++
> > > >   target/arm/tcg/helper-a64.c        | 10 ++--
> > > >   tests/tcg/multiarch/memset-fault.c | 77
> > > > ++++++++++++++++++++++++++++++
> > > >   4 files changed, 144 insertions(+), 5 deletions(-)
> > > >   create mode 100644 tests/tcg/multiarch/memset-fault.c
> > > 
> > > This sounds good to me.
> > > 
> > > I haven't debugged it, but I wonder why doesn't s390x fail here.
> > > For XC with src == dst, it does access_memset() ->
> > > do_access_memset()
> > > -> memset() without setting the RA. And I don't think that
> > > anything
> > > around it sets the RA either.
> > 
> > s390x uses probe_access_flags, which verifies the page is mapped
> > and writable, and raises 
> > the exception when it isn't.  In contrast, for user-only,
> > tlb_vaddr_to_host *only* 
> > performs the guest -> host address mapping, i.e. (addr +
> > guest_base).
> 
> I should clarify: probe_access_flags verifies that the page is mapped
> *at that moment*, 
> but does not take the mmap_lock.  So the race is that the page can be
> unmapped by another 
> thread after probe_access_flags and before the memset completes.

I see, thanks. I completely overlooked the access_prepare() calls.

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

Re: [PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[Peter Maydell]

On Wed, 3 Jul 2024 at 00:43, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> While looking into Zoltan's attempt to speed up ppc64 DCBZ
> (data cache block set to zero), I wondered what AArch64 was
> doing differently.  It turned out that Arm is the only user
> of tlb_vaddr_to_host.

riscv also seems to use it in vext_ldff(), fwiw.

-- PMM

Re: [PATCH 0/2] target/arm: Fix unwind from dc zva and FEAT_MOPS

[Richard Henderson]

On 7/8/24 07:25, Peter Maydell wrote:
> On Wed, 3 Jul 2024 at 00:43, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> While looking into Zoltan's attempt to speed up ppc64 DCBZ
>> (data cache block set to zero), I wondered what AArch64 was
>> doing differently.  It turned out that Arm is the only user
>> of tlb_vaddr_to_host.
> 
> riscv also seems to use it in vext_ldff(), fwiw.

So it does, followed by a second probe for read.
That's quite wrong...

But you're right that it has a similar race condition.


r~