[PATCH v2 0/2] linux user patches

[PATCH v2 0/2] linux user patches

[Helge Deller]

From: Helge Deller <deller@gmx.de>

Two patches from Peter Maydell for linux-user:
One fixes the signal context on loongarch64 and the other replaces an old
pthread implementation by an QemuSemaphore implementation which better mimics
the wanted semantics.

Peter Maydell (2):
  linux-user/loongarch64: In extframe_alloc, align before subtracting
    sctx_info size
  linux-user: Use QemuSemaphore to wait for clone child thread

 linux-user/loongarch64/signal.c |  3 ++-
 linux-user/syscall.c            | 17 +++++------------
 2 files changed, 7 insertions(+), 13 deletions(-)

-- 
2.54.0

[PATCH v2 1/2] linux-user/loongarch64: In extframe_alloc, align before subtracting sctx_info size

[Helge Deller]

From: Peter Maydell <peter.maydell@linaro.org>

In extframe_alloc() we allocate space on the guest stack for a
context struct, which consists of a target_sctx_info plus a more
specific struct such as target_lasx_context.  The specific struct may
have a higher alignment requirement than the basic target_sctx_info.

The Linux kernel code for this is in arch/loongarch/kernel/signal.c
in extframe_alloc():

       unsigned long new_base = base - size;

       new_base = round_down(new_base, (align < 16 ? 16 : align));
       new_base -= sizeof(struct sctx_info);

and it first adjusts the base (SP) address down by the size of the
specific struct, then rounds that down to provide the required
alignment, and finally subtracts the size of the sctx_info.

QEMU's version does this in the wrong order, subtracting both the
specific struct size and the sctx_info size before rounding down for
alignment.  This means that for the target_lasx_context which needs
32-alignment, we instead give the start of sctx_info 32-alignment,
which (since that is 16 bytes long) means that the
target_lasx_context is only 16-aligned.

This manifests as a failure in the TCG tests such as
loongarch64-linux-user-signals.test if you build with the clang
sanitizers enabled:

../../linux-user/loongarch64/signal.c:182:13: runtime error: member access within misaligned address 0x7dc793950b90 for type 'struct target_lasx_context', which requires 32 byte alignment
0x7dc793950b90: note: pointer points here
 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^
    #0 0x57216113d881 in setup_sigframe /home/pm215/qemu/build/clang/../../linux-user/loongarch64/signal.c:182:13
    #1 0x57216113d881 in setup_rt_frame /home/pm215/qemu/build/clang/../../linux-user/loongarch64/signal.c:389:5
    #2 0x5721612a5041 in handle_pending_signal /home/pm215/qemu/build/clang/../../linux-user/signal.c:1349:9
    #3 0x5721612a46ef in process_pending_signals /home/pm215/qemu/build/clang/../../linux-user/signal.c:1401:17
    #4 0x57216113baa3 in cpu_loop /home/pm215/qemu/build/clang/../../linux-user/loongarch64/cpu_loop.c:121:9

Correct setup_extcontext() to match the order the kernel does it.

Cc: qemu-stable@nongnu.org
Fixes: 9d5cd6587a70d613 ("linux-user: Add LoongArch signal support")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Helge Deller <deller@gmx.de>
---
 linux-user/loongarch64/signal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/loongarch64/signal.c b/linux-user/loongarch64/signal.c
index eff75bcdc8..04015e06fd 100644
--- a/linux-user/loongarch64/signal.c
+++ b/linux-user/loongarch64/signal.c
@@ -111,9 +111,10 @@ static abi_ptr extframe_alloc(struct extctx_layout *extctx,
 {
     abi_ptr sp = orig_sp;
 
-    sp -= sizeof(struct target_sctx_info) + size;
+    sp -= size;
     align = MAX(align, CONTEXT_INFO_ALIGN);
     sp = ROUND_DOWN(sp, align);
+    sp -= sizeof(struct target_sctx_info);
     sctx->gaddr = sp;
 
     size = orig_sp - sp;
-- 
2.54.0

[PATCH v2 2/2] linux-user: Use QemuSemaphore to wait for clone child thread

[Helge Deller]

From: Peter Maydell <peter.maydell@linaro.org>

In do_fork() and clone_func() we currently use a pthread_mutex_t and
a pthread_cond_t to allow the parent thread to wait until the child
thread has started and e.g.  set fields like info->tid, before the
parent reads those fields and otherwise proceeds.

Coverity points out that this use of a pthread condition variable
isn't reliable: pthread_cond_wait() may wake up spuriously, so the
idiom for "wait on a condition" should be "loop around calling
pthread_cond_wait() until a predicate that was updated by the caller
of pthread_cond_broadcast() is true".

This is quite old code; rather than fixing up its use of raw pthread
condition variables, we can use a QemuSemaphore.  This gives us the
"thread A waits until thread B signals it" semantics we want.  (Under
the hood it is implemented with a mutex and condition variable.)

Cc: qemu-stable@nongnu.org
Resolves: CID 1527417
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
Signed-off-by: Helge Deller <deller@gmx.de>
---
 linux-user/syscall.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0380691a8b..9d85a193ef 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -6932,8 +6932,7 @@ static abi_long do_map_shadow_stack(CPUArchState *env, abi_ulong addr,
 static pthread_mutex_t clone_lock = PTHREAD_MUTEX_INITIALIZER;
 typedef struct {
     CPUArchState *env;
-    pthread_mutex_t mutex;
-    pthread_cond_t cond;
+    QemuSemaphore sem;
     pthread_t thread;
     uint32_t tid;
     abi_ulong child_tidptr;
@@ -6964,9 +6963,7 @@ static void *clone_func(void *arg)
     /* Enable signals.  */
     sigprocmask(SIG_SETMASK, &info->sigmask, NULL);
     /* Signal to the parent that we're ready.  */
-    pthread_mutex_lock(&info->mutex);
-    pthread_cond_broadcast(&info->cond);
-    pthread_mutex_unlock(&info->mutex);
+    qemu_sem_post(&info->sem);
     /* Wait until the parent has finished initializing the tls state.  */
     pthread_mutex_lock(&clone_lock);
     pthread_mutex_unlock(&clone_lock);
@@ -7071,9 +7068,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         }
 
         memset(&info, 0, sizeof(info));
-        pthread_mutex_init(&info.mutex, NULL);
-        pthread_mutex_lock(&info.mutex);
-        pthread_cond_init(&info.cond, NULL);
+        qemu_sem_init(&info.sem, 0);
         info.env = new_env;
         if (flags & CLONE_CHILD_SETTID) {
             info.child_tidptr = child_tidptr;
@@ -7097,7 +7092,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         pthread_attr_destroy(&attr);
         if (ret == 0) {
             /* Wait for the child to initialize.  */
-            pthread_cond_wait(&info.cond, &info.mutex);
+            qemu_sem_wait(&info.sem);
             ret = info.tid;
         } else {
             errno = ret;
@@ -7111,9 +7106,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
 #endif
             g_free(ts);
         }
-        pthread_mutex_unlock(&info.mutex);
-        pthread_cond_destroy(&info.cond);
-        pthread_mutex_destroy(&info.mutex);
+        qemu_sem_destroy(&info.sem);
         pthread_mutex_unlock(&clone_lock);
     } else {
         /* if no CLONE_VM, we consider it is a fork */
-- 
2.54.0