[PATCH v4 0/4] tls: add macros for coroutine-safe TLS variables

[PATCH v4 0/4] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

v4:
- Dropped '[RFC]'.
- Dropped inline asm for now. -fPIC versions of the code are missing and I
  hit several issues including a clang LTO bug where thread local variables are
  incorrectly discarded because inline asm is not analyzed to find symbol
  dependencies (Serge Guelton is aware).
- Fixed CI failures.
v3:
- Added __attribute__((weak)) to get_ptr_*() [Florian]
- Replace rdfsbase with mov %%fs:0,%0 [Florian]

This patch series solves the coroutines TLS problem. Coroutines re-entered from
another thread sometimes see stale TLS values. This happens because compilers
may cache values across yield points, so a value from the previous thread will
be used when the coroutine is re-entered in another thread.

Serge Guelton developed a portable technique, see the first patch for details.

I have audited all __thread variables in QEMU and converted those that can be
used from coroutines. Most actually look safe to me.

Stefan Hajnoczi (4):
  tls: add macros for coroutine-safe TLS variables
  util/async: replace __thread with QEMU TLS macros
  rcu: use coroutine TLS macros
  cpus: use coroutine TLS macros for iothread_locked

 include/qemu/coroutine-tls.h | 159 +++++++++++++++++++++++++++++++++++
 include/qemu/rcu.h           |   7 +-
 softmmu/cpus.c               |   8 +-
 tests/unit/rcutorture.c      |  10 +--
 tests/unit/test-rcu-list.c   |   4 +-
 util/async.c                 |  12 +--
 util/rcu.c                   |  10 +--
 7 files changed, 186 insertions(+), 24 deletions(-)
 create mode 100644 include/qemu/coroutine-tls.h

-- 
2.34.1

[PATCH v4 1/4] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

Compiler optimizations can cache TLS values across coroutine yield
points, resulting in stale values from the previous thread when a
coroutine is re-entered by a new thread.

Serge Guelton developed an __attribute__((noinline)) wrapper and tested
it with clang and gcc. I formatted his idea according to QEMU's coding
style and wrote documentation.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1952483
Suggested-by: Serge Guelton <sguelton@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/coroutine-tls.h | 159 +++++++++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)
 create mode 100644 include/qemu/coroutine-tls.h

diff --git a/include/qemu/coroutine-tls.h b/include/qemu/coroutine-tls.h
new file mode 100644
index 0000000000..e1f8e78d25
--- /dev/null
+++ b/include/qemu/coroutine-tls.h
@@ -0,0 +1,159 @@
+/*
+ * QEMU Thread Local Storage for coroutines
+ *
+ * Copyright Red Hat
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ * It is forbidden to access Thread Local Storage in coroutines because
+ * compiler optimizations may cause values to be cached across coroutine
+ * re-entry. Coroutines can run in more than one thread through the course of
+ * their life, leading bugs when stale TLS values from the wrong thread are
+ * used as a result of compiler optimization.
+ *
+ * An example is:
+ *
+ * ..code-block:: c
+ *   :caption: A coroutine that may see the wrong TLS value
+ *
+ *   static __thread AioContext *current_aio_context;
+ *   ...
+ *   static void coroutine_fn foo(void)
+ *   {
+ *       aio_notify(current_aio_context);
+ *       qemu_coroutine_yield();
+ *       aio_notify(current_aio_context); // <-- may be stale after yielding!
+ *   }
+ *
+ * This header provides macros for safely defining variables in Thread Local
+ * Storage:
+ *
+ * ..code-block:: c
+ *   :caption: A coroutine that safely uses TLS
+ *
+ *   QEMU_DEFINE_STATIC_CO_TLS(AioContext *, current_aio_context)
+ *   ...
+ *   static void coroutine_fn foo(void)
+ *   {
+ *       aio_notify(get_current_aio_context());
+ *       qemu_coroutine_yield();
+ *       aio_notify(get_current_aio_context()); // <-- safe
+ *   }
+ */
+
+#ifndef QEMU_COROUTINE_TLS_H
+#define QEMU_COROUTINE_TLS_H
+
+/*
+ * To stop the compiler from caching TLS values we define accessor functions
+ * with __attribute__((noinline)) plus asm volatile("") to prevent
+ * optimizations that override noinline. This is fragile and ultimately needs
+ * to be solved by a mechanism that is guaranteed to work by the compiler (e.g.
+ * stackless coroutines), but for now we use this approach to prevent issues.
+ */
+
+/**
+ * QEMU_DECLARE_CO_TLS:
+ * @type: the variable's C type
+ * @var: the variable name
+ *
+ * Declare an extern variable in Thread Local Storage from a header file:
+ *
+ * .. code-block:: c
+ *   :caption: Declaring an extern variable in Thread Local Storage
+ *
+ *   QEMU_DECLARE_CO_TLS(int, my_count)
+ *   ...
+ *   int c = get_my_count();
+ *   set_my_count(c + 1);
+ *   *get_ptr_my_count() = 0;
+ *
+ * This is a coroutine-safe replacement for the __thread keyword and is
+ * equivalent to the following code:
+ *
+ * .. code-block:: c
+ *   :caption: Declaring a TLS variable using __thread
+ *
+ *   extern __thread int my_count;
+ *   ...
+ *   int c = my_count;
+ *   my_count = c + 1;
+ *   *(&my_count) = 0;
+ */
+#define QEMU_DECLARE_CO_TLS(type, var)                                       \
+    __attribute__((noinline)) type get_##var(void);                          \
+    __attribute__((noinline)) void set_##var(type v);                        \
+    __attribute__((noinline)) type *get_ptr_##var(void);
+
+/**
+ * QEMU_DEFINE_CO_TLS:
+ * @type: the variable's C type
+ * @var: the variable name
+ *
+ * Define a variable in Thread Local Storage that was previously declared from
+ * a header file with QEMU_DECLARE_CO_TLS():
+ *
+ * .. code-block:: c
+ *   :caption: Defining a variable in Thread Local Storage
+ *
+ *   QEMU_DEFINE_CO_TLS(int, my_count)
+ *
+ * This is a coroutine-safe replacement for the __thread keyword and is
+ * equivalent to the following code:
+ *
+ * .. code-block:: c
+ *   :caption: Defining a TLS variable using __thread
+ *
+ *   __thread int my_count;
+ */
+#define QEMU_DEFINE_CO_TLS(type, var)                                        \
+    static __thread type co_tls_##var;                                       \
+    type get_##var(void) { asm volatile(""); return co_tls_##var; }          \
+    void set_##var(type v) { asm volatile(""); co_tls_##var = v; }           \
+    type *get_ptr_##var(void)                                                \
+    { type *ptr = &co_tls_##var; asm volatile("" : "+rm" (ptr)); return ptr; }
+
+/**
+ * QEMU_DEFINE_STATIC_CO_TLS:
+ * @type: the variable's C type
+ * @var: the variable name
+ *
+ * Define a static variable in Thread Local Storage:
+ *
+ * .. code-block:: c
+ *   :caption: Defining a static variable in Thread Local Storage
+ *
+ *   QEMU_DEFINE_STATIC_CO_TLS(int, my_count)
+ *   ...
+ *   int c = get_my_count();
+ *   set_my_count(c + 1);
+ *   *get_ptr_my_count() = 0;
+ *
+ * This is a coroutine-safe replacement for the __thread keyword and is
+ * equivalent to the following code:
+ *
+ * .. code-block:: c
+ *   :caption: Defining a static TLS variable using __thread
+ *
+ *   static __thread int my_count;
+ *   ...
+ *   int c = my_count;
+ *   my_count = c + 1;
+ *   *(&my_count) = 0;
+ */
+#define QEMU_DEFINE_STATIC_CO_TLS(type, var)                                 \
+    static __thread type co_tls_##var;                                       \
+    static __attribute__((noinline, unused))                                 \
+    type get_##var(void)                                                     \
+    { asm volatile(""); return co_tls_##var; }                               \
+    static __attribute__((noinline, unused))                                 \
+    void set_##var(type v)                                                   \
+    { asm volatile(""); co_tls_##var = v; }                                  \
+    static __attribute__((noinline, unused))                                 \
+    type *get_ptr_##var(void)                                                \
+    { type *ptr = &co_tls_##var; asm volatile("" : "+rm" (ptr)); return ptr; }
+
+#endif /* QEMU_COROUTINE_TLS_H */
-- 
2.34.1

[PATCH v4 2/4] util/async: replace __thread with QEMU TLS macros

[Stefan Hajnoczi]

QEMU TLS macros must be used to make TLS variables safe with coroutines.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/async.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/util/async.c b/util/async.c
index 08d25feef5..2ea1172f3e 100644
--- a/util/async.c
+++ b/util/async.c
@@ -32,6 +32,7 @@
 #include "qemu/rcu_queue.h"
 #include "block/raw-aio.h"
 #include "qemu/coroutine_int.h"
+#include "qemu/coroutine-tls.h"
 #include "trace.h"
 
 /***********************************************************/
@@ -675,12 +676,13 @@ void aio_context_release(AioContext *ctx)
     qemu_rec_mutex_unlock(&ctx->lock);
 }
 
-static __thread AioContext *my_aiocontext;
+QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
 
 AioContext *qemu_get_current_aio_context(void)
 {
-    if (my_aiocontext) {
-        return my_aiocontext;
+    AioContext *ctx = get_my_aiocontext();
+    if (ctx) {
+        return ctx;
     }
     if (qemu_mutex_iothread_locked()) {
         /* Possibly in a vCPU thread.  */
@@ -691,6 +693,6 @@ AioContext *qemu_get_current_aio_context(void)
 
 void qemu_set_current_aio_context(AioContext *ctx)
 {
-    assert(!my_aiocontext);
-    my_aiocontext = ctx;
+    assert(!get_my_aiocontext());
+    set_my_aiocontext(ctx);
 }
-- 
2.34.1

[PATCH v4 3/4] rcu: use coroutine TLS macros

[Stefan Hajnoczi]

RCU may be used from coroutines. Standard __thread variables cannot be
used by coroutines. Use the coroutine TLS macros instead.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/rcu.h         |  7 ++++---
 tests/unit/rcutorture.c    | 10 +++++-----
 tests/unit/test-rcu-list.c |  4 ++--
 util/rcu.c                 | 10 +++++-----
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index e69efbd47f..b063c6fde8 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -29,6 +29,7 @@
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
 #include "qemu/sys_membarrier.h"
+#include "qemu/coroutine-tls.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -76,11 +77,11 @@ struct rcu_reader_data {
     NotifierList force_rcu;
 };
 
-extern __thread struct rcu_reader_data rcu_reader;
+QEMU_DECLARE_CO_TLS(struct rcu_reader_data, rcu_reader)
 
 static inline void rcu_read_lock(void)
 {
-    struct rcu_reader_data *p_rcu_reader = &rcu_reader;
+    struct rcu_reader_data *p_rcu_reader = get_ptr_rcu_reader();
     unsigned ctr;
 
     if (p_rcu_reader->depth++ > 0) {
@@ -96,7 +97,7 @@ static inline void rcu_read_lock(void)
 
 static inline void rcu_read_unlock(void)
 {
-    struct rcu_reader_data *p_rcu_reader = &rcu_reader;
+    struct rcu_reader_data *p_rcu_reader = get_ptr_rcu_reader();
 
     assert(p_rcu_reader->depth != 0);
     if (--p_rcu_reader->depth > 0) {
diff --git a/tests/unit/rcutorture.c b/tests/unit/rcutorture.c
index de6f649058..495a4e6f42 100644
--- a/tests/unit/rcutorture.c
+++ b/tests/unit/rcutorture.c
@@ -122,7 +122,7 @@ static void *rcu_read_perf_test(void *arg)
 
     rcu_register_thread();
 
-    *(struct rcu_reader_data **)arg = &rcu_reader;
+    *(struct rcu_reader_data **)arg = get_ptr_rcu_reader();
     qatomic_inc(&nthreadsrunning);
     while (goflag == GOFLAG_INIT) {
         g_usleep(1000);
@@ -148,7 +148,7 @@ static void *rcu_update_perf_test(void *arg)
 
     rcu_register_thread();
 
-    *(struct rcu_reader_data **)arg = &rcu_reader;
+    *(struct rcu_reader_data **)arg = get_ptr_rcu_reader();
     qatomic_inc(&nthreadsrunning);
     while (goflag == GOFLAG_INIT) {
         g_usleep(1000);
@@ -253,7 +253,7 @@ static void *rcu_read_stress_test(void *arg)
 
     rcu_register_thread();
 
-    *(struct rcu_reader_data **)arg = &rcu_reader;
+    *(struct rcu_reader_data **)arg = get_ptr_rcu_reader();
     while (goflag == GOFLAG_INIT) {
         g_usleep(1000);
     }
@@ -304,7 +304,7 @@ static void *rcu_update_stress_test(void *arg)
     struct rcu_stress *cp = qatomic_read(&rcu_stress_current);
 
     rcu_register_thread();
-    *(struct rcu_reader_data **)arg = &rcu_reader;
+    *(struct rcu_reader_data **)arg = get_ptr_rcu_reader();
 
     while (goflag == GOFLAG_INIT) {
         g_usleep(1000);
@@ -347,7 +347,7 @@ static void *rcu_fake_update_stress_test(void *arg)
 {
     rcu_register_thread();
 
-    *(struct rcu_reader_data **)arg = &rcu_reader;
+    *(struct rcu_reader_data **)arg = get_ptr_rcu_reader();
     while (goflag == GOFLAG_INIT) {
         g_usleep(1000);
     }
diff --git a/tests/unit/test-rcu-list.c b/tests/unit/test-rcu-list.c
index 49641e1936..64b81ae058 100644
--- a/tests/unit/test-rcu-list.c
+++ b/tests/unit/test-rcu-list.c
@@ -171,7 +171,7 @@ static void *rcu_q_reader(void *arg)
 
     rcu_register_thread();
 
-    *(struct rcu_reader_data **)arg = &rcu_reader;
+    *(struct rcu_reader_data **)arg = get_ptr_rcu_reader();
     qatomic_inc(&nthreadsrunning);
     while (qatomic_read(&goflag) == GOFLAG_INIT) {
         g_usleep(1000);
@@ -206,7 +206,7 @@ static void *rcu_q_updater(void *arg)
     long long n_removed_local = 0;
     struct list_element *el, *prev_el;
 
-    *(struct rcu_reader_data **)arg = &rcu_reader;
+    *(struct rcu_reader_data **)arg = get_ptr_rcu_reader();
     qatomic_inc(&nthreadsrunning);
     while (qatomic_read(&goflag) == GOFLAG_INIT) {
         g_usleep(1000);
diff --git a/util/rcu.c b/util/rcu.c
index c91da9f137..b6d6c71cff 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -65,7 +65,7 @@ static inline int rcu_gp_ongoing(unsigned long *ctr)
 /* Written to only by each individual reader. Read by both the reader and the
  * writers.
  */
-__thread struct rcu_reader_data rcu_reader;
+QEMU_DEFINE_CO_TLS(struct rcu_reader_data, rcu_reader)
 
 /* Protected by rcu_registry_lock.  */
 typedef QLIST_HEAD(, rcu_reader_data) ThreadList;
@@ -355,23 +355,23 @@ void drain_call_rcu(void)
 
 void rcu_register_thread(void)
 {
-    assert(rcu_reader.ctr == 0);
+    assert(get_ptr_rcu_reader()->ctr == 0);
     qemu_mutex_lock(&rcu_registry_lock);
-    QLIST_INSERT_HEAD(&registry, &rcu_reader, node);
+    QLIST_INSERT_HEAD(&registry, get_ptr_rcu_reader(), node);
     qemu_mutex_unlock(&rcu_registry_lock);
 }
 
 void rcu_unregister_thread(void)
 {
     qemu_mutex_lock(&rcu_registry_lock);
-    QLIST_REMOVE(&rcu_reader, node);
+    QLIST_REMOVE(get_ptr_rcu_reader(), node);
     qemu_mutex_unlock(&rcu_registry_lock);
 }
 
 void rcu_add_force_rcu_notifier(Notifier *n)
 {
     qemu_mutex_lock(&rcu_registry_lock);
-    notifier_list_add(&rcu_reader.force_rcu, n);
+    notifier_list_add(&get_ptr_rcu_reader()->force_rcu, n);
     qemu_mutex_unlock(&rcu_registry_lock);
 }
 
-- 
2.34.1

[PATCH v4 4/4] cpus: use coroutine TLS macros for iothread_locked

[Stefan Hajnoczi]

qemu_mutex_iothread_locked() may be used from coroutines. Standard
__thread variables cannot be used by coroutines. Use the coroutine TLS
macros instead.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 softmmu/cpus.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 035395ae13..005a5c31ef 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -473,11 +473,11 @@ bool qemu_in_vcpu_thread(void)
     return current_cpu && qemu_cpu_is_self(current_cpu);
 }
 
-static __thread bool iothread_locked = false;
+QEMU_DEFINE_STATIC_CO_TLS(bool, iothread_locked)
 
 bool qemu_mutex_iothread_locked(void)
 {
-    return iothread_locked;
+    return get_iothread_locked();
 }
 
 /*
@@ -490,13 +490,13 @@ void qemu_mutex_lock_iothread_impl(const char *file, int line)
 
     g_assert(!qemu_mutex_iothread_locked());
     bql_lock(&qemu_global_mutex, file, line);
-    iothread_locked = true;
+    set_iothread_locked(true);
 }
 
 void qemu_mutex_unlock_iothread(void)
 {
     g_assert(qemu_mutex_iothread_locked());
-    iothread_locked = false;
+    set_iothread_locked(false);
     qemu_mutex_unlock(&qemu_global_mutex);
 }
 
-- 
2.34.1

Re: [PATCH v4 4/4] cpus: use coroutine TLS macros for iothread_locked

[Philippe Mathieu-Daudé]

On 21/2/22 15:29, Stefan Hajnoczi wrote:
> qemu_mutex_iothread_locked() may be used from coroutines. Standard
> __thread variables cannot be used by coroutines. Use the coroutine TLS
> macros instead.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   softmmu/cpus.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 4/4] cpus: use coroutine TLS macros for iothread_locked

[Philippe Mathieu-Daudé]

On 21/2/22 15:29, Stefan Hajnoczi wrote:
> qemu_mutex_iothread_locked() may be used from coroutines. Standard
> __thread variables cannot be used by coroutines. Use the coroutine TLS
> macros instead.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   softmmu/cpus.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 035395ae13..005a5c31ef 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -473,11 +473,11 @@ bool qemu_in_vcpu_thread(void)
>       return current_cpu && qemu_cpu_is_self(current_cpu);
>   }
>   
> -static __thread bool iothread_locked = false;
> +QEMU_DEFINE_STATIC_CO_TLS(bool, iothread_locked)

While "qemu/coroutine-tls.h" is indirectly included by "rcu.h",
please include it explicitly.

Re: [PATCH v4 2/4] util/async: replace __thread with QEMU TLS macros

[Philippe Mathieu-Daudé]

On 21/2/22 15:29, Stefan Hajnoczi wrote:
> QEMU TLS macros must be used to make TLS variables safe with coroutines.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   util/async.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 1/4] tls: add macros for coroutine-safe TLS variables

[Peter Maydell]

On Mon, 21 Feb 2022 at 14:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Compiler optimizations can cache TLS values across coroutine yield
> points, resulting in stale values from the previous thread when a
> coroutine is re-entered by a new thread.
>
> Serge Guelton developed an __attribute__((noinline)) wrapper and tested
> it with clang and gcc. I formatted his idea according to QEMU's coding
> style and wrote documentation.

The commit message says "attribute noinline" but the code
opts for "attribute noinline plus asm-volatile barrier":

> +/*
> + * To stop the compiler from caching TLS values we define accessor functions
> + * with __attribute__((noinline)) plus asm volatile("") to prevent
> + * optimizations that override noinline. This is fragile and ultimately needs
> + * to be solved by a mechanism that is guaranteed to work by the compiler (e.g.
> + * stackless coroutines), but for now we use this approach to prevent issues.
> + */

I thought we'd determined previously that noinline + asm-volatile wasn't
sufficient?

https://lore.kernel.org/qemu-devel/YbdUDkTkt5srNdW+@stefanha-x1.localdomain/

This version of the patchset does seem to include the asm input operand
you describe there (in one of the three wrappers, anyway), but if that's
necessary then we should document it in the comment here.

thanks
-- PMM

Re: [PATCH v4 4/4] cpus: use coroutine TLS macros for iothread_locked

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

On Mon, Feb 21, 2022 at 04:09:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 21/2/22 15:29, Stefan Hajnoczi wrote:
> > qemu_mutex_iothread_locked() may be used from coroutines. Standard
> > __thread variables cannot be used by coroutines. Use the coroutine TLS
> > macros instead.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >   softmmu/cpus.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> > index 035395ae13..005a5c31ef 100644
> > --- a/softmmu/cpus.c
> > +++ b/softmmu/cpus.c
> > @@ -473,11 +473,11 @@ bool qemu_in_vcpu_thread(void)
> >       return current_cpu && qemu_cpu_is_self(current_cpu);
> >   }
> > -static __thread bool iothread_locked = false;
> > +QEMU_DEFINE_STATIC_CO_TLS(bool, iothread_locked)
> 
> While "qemu/coroutine-tls.h" is indirectly included by "rcu.h",
> please include it explicitly.

Thanks, will fix.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 1/4] tls: add macros for coroutine-safe TLS variables

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On Mon, Feb 21, 2022 at 03:16:22PM +0000, Peter Maydell wrote:
> On Mon, 21 Feb 2022 at 14:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > Compiler optimizations can cache TLS values across coroutine yield
> > points, resulting in stale values from the previous thread when a
> > coroutine is re-entered by a new thread.
> >
> > Serge Guelton developed an __attribute__((noinline)) wrapper and tested
> > it with clang and gcc. I formatted his idea according to QEMU's coding
> > style and wrote documentation.
> 
> The commit message says "attribute noinline" but the code
> opts for "attribute noinline plus asm-volatile barrier":
> 
> > +/*
> > + * To stop the compiler from caching TLS values we define accessor functions
> > + * with __attribute__((noinline)) plus asm volatile("") to prevent
> > + * optimizations that override noinline. This is fragile and ultimately needs
> > + * to be solved by a mechanism that is guaranteed to work by the compiler (e.g.
> > + * stackless coroutines), but for now we use this approach to prevent issues.
> > + */
> 
> I thought we'd determined previously that noinline + asm-volatile wasn't
> sufficient?
> 
> https://lore.kernel.org/qemu-devel/YbdUDkTkt5srNdW+@stefanha-x1.localdomain/
> 
> This version of the patchset does seem to include the asm input operand
> you describe there (in one of the three wrappers, anyway), but if that's
> necessary then we should document it in the comment here.

Sorry, the message/comments are outdated. I'll change them to reflect
the current approach.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 4/4] cpus: use coroutine TLS macros for iothread_locked

[Paolo Bonzini]

On 2/21/22 15:29, Stefan Hajnoczi wrote:
> -static __thread bool iothread_locked = false;
> +QEMU_DEFINE_STATIC_CO_TLS(bool, iothread_locked)
>   
>   bool qemu_mutex_iothread_locked(void)
>   {
> -    return iothread_locked;
> +    return get_iothread_locked();
>   }
>   

Can we rename either the variable or the function, and avoid the wrapper 
altogether?

Paolo

Re: [PATCH v4 1/4] tls: add macros for coroutine-safe TLS variables

[Paolo Bonzini]

On 2/21/22 15:29, Stefan Hajnoczi wrote:
> +#define QEMU_DEFINE_CO_TLS(type, var)                                        \
> +    static __thread type co_tls_##var;                                       \
> +    type get_##var(void) { asm volatile(""); return co_tls_##var; }          \
> +    void set_##var(type v) { asm volatile(""); co_tls_##var = v; }           \
> +    type *get_ptr_##var(void)                                                \
> +    { type *ptr = &co_tls_##var; asm volatile("" : "+rm" (ptr)); return ptr; }
> +

Is noinline needed here too in case LTO is used?

Paolo

Re: [PATCH v4 4/4] cpus: use coroutine TLS macros for iothread_locked

[Peter Maydell]

On Wed, 23 Feb 2022 at 09:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 2/21/22 15:29, Stefan Hajnoczi wrote:
> > -static __thread bool iothread_locked = false;
> > +QEMU_DEFINE_STATIC_CO_TLS(bool, iothread_locked)
> >
> >   bool qemu_mutex_iothread_locked(void)
> >   {
> > -    return iothread_locked;
> > +    return get_iothread_locked();
> >   }
> >
>
> Can we rename either the variable or the function, and avoid the wrapper
> altogether?

I think it's useful to distinguish the API for the rest of QEMU
(a function) from the implementation used internally (previously
a thread-local static, now a similar thing with wrappers.)

-- PMM