[Qemu-devel] [PATCH 00/10] translate-all.c thread-safety

[Qemu-devel] [PATCH 00/10] translate-all.c thread-safety

[Paolo Bonzini]

Hi, this is my attempt at 1) extracting upstreamable parts out of Fred's
MTTCG, and 2) documenting what's going on in user-mode MTTCG 3) fix
one bug in the process.  I couldn't find any other locking problem
from reading the code.

The final two patches are not really upstreamable because they
add some still unnecessary locks to system emulation, but I included
them to show what's going on.  With this locking logic I do not need
tb_lock to be recursive anymore.

Paolo

KONRAD Frederic (4):
  cpus: protect work list with work_mutex
  cpus: remove tcg_halt_cond global variable.
  replace spinlock by QemuMutex.
  tcg: protect TBContext with tb_lock.

Paolo Bonzini (8):
  exec-all: remove non-TCG stuff from exec-all.h header.
  cpu-exec: elide more icount code if CONFIG_USER_ONLY
  tcg: code_bitmap is not used by user-mode emulation
  tcg: comment on which functions have to be called with mmap_lock held
  tcg: add memory barriers in page_find_alloc accesses
  exec: make mmap_lock/mmap_unlock globally available
  cpu-exec: fix lock hierarchy for user-mode emulation
  tcg: comment on which functions have to be called with tb_lock held

 bsd-user/qemu.h          |   2 -
 cpu-exec.c               | 107 +++++++++++++++++++++----------
 cpus.c                   |  34 ++++++----
 exec.c                   |   4 ++
 hw/i386/kvmvapic.c       |   2 +
 include/exec/exec-all.h  |  19 +++---
 include/exec/ram_addr.h  |   1 +
 include/qom/cpu.h        |   9 ++-
 include/sysemu/sysemu.h  |   3 +
 linux-user/main.c        |   6 +-
 linux-user/qemu.h        |   2 -
 qom/cpu.c                |   1 +
 target-i386/cpu.h        |   3 +
 target-i386/mem_helper.c |  25 +++++++-
 target-i386/translate.c  |   2 +
 tcg/tcg.h                |   6 ++
 translate-all.c          | 161 +++++++++++++++++++++++++++++++++++++----------
 17 files changed, 290 insertions(+), 97 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/10] cpus: protect work list with work_mutex

[Paolo Bonzini]

From: KONRAD Frederic <fred.konrad@greensocs.com>

Protect the list of queued work items with something other than
the BQL, as a preparation for running the work items outside it.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c            | 22 ++++++++++++++++++----
 include/qom/cpu.h |  6 +++++-
 qom/cpu.c         |  1 +
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/cpus.c b/cpus.c
index c1e74d9..9224488 100644
--- a/cpus.c
+++ b/cpus.c
@@ -845,6 +845,8 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     wi.func = func;
     wi.data = data;
     wi.free = false;
+
+    qemu_mutex_lock(&cpu->work_mutex);
     if (cpu->queued_work_first == NULL) {
         cpu->queued_work_first = &wi;
     } else {
@@ -853,9 +855,10 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     cpu->queued_work_last = &wi;
     wi.next = NULL;
     wi.done = false;
+    qemu_mutex_unlock(&cpu->work_mutex);
 
     qemu_cpu_kick(cpu);
-    while (!wi.done) {
+    while (!atomic_mb_read(&wi.done)) {
         CPUState *self_cpu = current_cpu;
 
         qemu_cond_wait(&qemu_work_cond, &qemu_global_mutex);
@@ -876,6 +879,8 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     wi->func = func;
     wi->data = data;
     wi->free = true;
+
+    qemu_mutex_lock(&cpu->work_mutex);
     if (cpu->queued_work_first == NULL) {
         cpu->queued_work_first = wi;
     } else {
@@ -884,6 +889,7 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
     cpu->queued_work_last = wi;
     wi->next = NULL;
     wi->done = false;
+    qemu_mutex_unlock(&cpu->work_mutex);
 
     qemu_cpu_kick(cpu);
 }
@@ -896,15 +902,23 @@ static void flush_queued_work(CPUState *cpu)
         return;
     }
 
-    while ((wi = cpu->queued_work_first)) {
+    qemu_mutex_lock(&cpu->work_mutex);
+    while (cpu->queued_work_first != NULL) {
+        wi = cpu->queued_work_first;
         cpu->queued_work_first = wi->next;
+        if (!cpu->queued_work_first) {
+            cpu->queued_work_last = NULL;
+        }
+        qemu_mutex_unlock(&cpu->work_mutex);
         wi->func(wi->data);
-        wi->done = true;
+        qemu_mutex_lock(&cpu->work_mutex);
         if (wi->free) {
             g_free(wi);
+        } else {
+            atomic_mb_set(&wi->done, true);
         }
     }
-    cpu->queued_work_last = NULL;
+    qemu_mutex_unlock(&cpu->work_mutex);
     qemu_cond_broadcast(&qemu_work_cond);
 }
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39712ab..77bbff2 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -244,6 +244,8 @@ struct kvm_run;
  * @mem_io_pc: Host Program Counter at which the memory was accessed.
  * @mem_io_vaddr: Target virtual address at which the memory was accessed.
  * @kvm_fd: vCPU file descriptor for KVM.
+ * @work_mutex: Lock to prevent multiple access to queued_work_*.
+ * @queued_work_first: First asynchronous work pending.
  *
  * State of one CPU core or thread.
  */
@@ -264,7 +266,6 @@ struct CPUState {
     uint32_t host_tid;
     bool running;
     struct QemuCond *halt_cond;
-    struct qemu_work_item *queued_work_first, *queued_work_last;
     bool thread_kicked;
     bool created;
     bool stop;
@@ -275,6 +276,9 @@ struct CPUState {
     int64_t icount_extra;
     sigjmp_buf jmp_env;
 
+    QemuMutex work_mutex;
+    struct qemu_work_item *queued_work_first, *queued_work_last;
+
     AddressSpace *as;
     struct AddressSpaceDispatch *memory_dispatch;
     MemoryListener *tcg_as_listener;
diff --git a/qom/cpu.c b/qom/cpu.c
index 62f4b5d..3e93223 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -314,6 +314,7 @@ static void cpu_common_initfn(Object *obj)
 
     cpu->cpu_index = -1;
     cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
+    qemu_mutex_init(&cpu->work_mutex);
     QTAILQ_INIT(&cpu->breakpoints);
     QTAILQ_INIT(&cpu->watchpoints);
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/10] cpus: remove tcg_halt_cond global variable.

[Paolo Bonzini]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This removes tcg_halt_cond global variable.
We need one QemuCond per virtual cpu for multithread TCG.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Message-Id: <1439220437-23957-9-git-send-email-fred.konrad@greensocs.com>
[Keep tcg_halt_cond for bisectability, while making it static. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/cpus.c b/cpus.c
index 9224488..8884278 100644
--- a/cpus.c
+++ b/cpus.c
@@ -813,7 +813,6 @@ static unsigned iothread_requesting_mutex;
 static QemuThread io_thread;
 
 static QemuThread *tcg_cpu_thread;
-static QemuCond *tcg_halt_cond;
 
 /* cpu creation */
 static QemuCond qemu_cpu_cond;
@@ -933,15 +932,13 @@ static void qemu_wait_io_event_common(CPUState *cpu)
     cpu->thread_kicked = false;
 }
 
-static void qemu_tcg_wait_io_event(void)
+static void qemu_tcg_wait_io_event(CPUState *cpu)
 {
-    CPUState *cpu;
-
     while (all_cpu_threads_idle()) {
        /* Start accounting real time to the virtual clock if the CPUs
           are idle.  */
         qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
-        qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
+        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
     }
 
     while (iothread_requesting_mutex) {
@@ -1067,7 +1064,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     /* wait for initial kick-off after machine start */
     while (first_cpu->stopped) {
-        qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
+        qemu_cond_wait(first_cpu->halt_cond, &qemu_global_mutex);
 
         /* process any pending work */
         CPU_FOREACH(cpu) {
@@ -1088,7 +1085,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
                 qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
             }
         }
-        qemu_tcg_wait_io_event();
+        qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
     }
 
     return NULL;
@@ -1265,6 +1262,7 @@ void resume_all_vcpus(void)
 static void qemu_tcg_init_vcpu(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
+    static QemuCond *tcg_halt_cond;
 
     tcg_cpu_address_space_init(cpu, cpu->as);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/10] replace spinlock by QemuMutex.

[Paolo Bonzini]

From: KONRAD Frederic <fred.konrad@greensocs.com>

spinlock is only used in two cases:
  * cpu-exec.c: to protect TranslationBlock
  * mem_helper.c: for lock helper in target-i386 (which seems broken).

It's a pthread_mutex_t in user-mode so better using QemuMutex directly in this
case.
It allows as well to reuse tb_lock mutex of TBContext in case of multithread
TCG.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Message-Id: <1439220437-23957-5-git-send-email-fred.konrad@greensocs.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c               | 15 +++------------
 include/exec/exec-all.h  |  4 ++--
 linux-user/main.c        |  6 +++---
 target-i386/cpu.h        |  3 +++
 target-i386/mem_helper.c | 25 ++++++++++++++++++++++---
 target-i386/translate.c  |  2 ++
 tcg/tcg.h                |  4 ++++
 translate-all.c          | 34 ++++++++++++++++++++++++++++++++++
 8 files changed, 73 insertions(+), 20 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 713540f..c680cca 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -356,9 +356,6 @@ int cpu_exec(CPUState *cpu)
     uintptr_t next_tb;
     SyncClocks sc;
 
-    /* This must be volatile so it is not trashed by longjmp() */
-    volatile bool have_tb_lock = false;
-
     if (cpu->halted) {
         if (!cpu_has_work(cpu)) {
             return EXCP_HALTED;
@@ -475,8 +472,7 @@ int cpu_exec(CPUState *cpu)
                     cpu->exception_index = EXCP_INTERRUPT;
                     cpu_loop_exit(cpu);
                 }
-                spin_lock(&tcg_ctx.tb_ctx.tb_lock);
-                have_tb_lock = true;
+                tb_lock();
                 tb = tb_find_fast(cpu);
                 /* Note: we do it here to avoid a gcc bug on Mac OS X when
                    doing it in tb_find_slow */
@@ -498,9 +494,7 @@ int cpu_exec(CPUState *cpu)
                     tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
                                 next_tb & TB_EXIT_MASK, tb);
                 }
-                have_tb_lock = false;
-                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
-
+                tb_unlock();
                 /* cpu_interrupt might be called while translating the
                    TB, but before it is linked into a potentially
                    infinite loop and becomes env->current_tb. Avoid
@@ -567,10 +561,7 @@ int cpu_exec(CPUState *cpu)
             x86_cpu = X86_CPU(cpu);
             env = &x86_cpu->env;
 #endif
-            if (have_tb_lock) {
-                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
-                have_tb_lock = false;
-            }
+            tb_lock_reset();
         }
     } /* for(;;) */
 
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 29775c0..dafd177 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -178,7 +178,7 @@ struct TranslationBlock {
     struct TranslationBlock *jmp_first;
 };
 
-#include "exec/spinlock.h"
+#include "qemu/thread.h"
 
 typedef struct TBContext TBContext;
 
@@ -188,7 +188,7 @@ struct TBContext {
     TranslationBlock *tb_phys_hash[CODE_GEN_PHYS_HASH_SIZE];
     int nb_tbs;
     /* any access to the tbs or the page table must use this lock */
-    spinlock_t tb_lock;
+    QemuMutex tb_lock;
 
     /* statistics */
     int tb_flush_count;
diff --git a/linux-user/main.c b/linux-user/main.c
index fdee981..fd06ce9 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -107,7 +107,7 @@ static int pending_cpus;
 /* Make sure everything is in a consistent state for calling fork().  */
 void fork_start(void)
 {
-    pthread_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
+    qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
     pthread_mutex_lock(&exclusive_lock);
     mmap_fork_start();
 }
@@ -129,11 +129,11 @@ void fork_end(int child)
         pthread_mutex_init(&cpu_list_mutex, NULL);
         pthread_cond_init(&exclusive_cond, NULL);
         pthread_cond_init(&exclusive_resume, NULL);
-        pthread_mutex_init(&tcg_ctx.tb_ctx.tb_lock, NULL);
+        qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
         gdbserver_fork(thread_cpu);
     } else {
         pthread_mutex_unlock(&exclusive_lock);
-        pthread_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
+        qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
     }
 }
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index ead2832..3655ff3 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1318,6 +1318,9 @@ static inline MemTxAttrs cpu_get_mem_attrs(CPUX86State *env)
 void cpu_set_mxcsr(CPUX86State *env, uint32_t val);
 void cpu_set_fpuc(CPUX86State *env, uint16_t val);
 
+/* mem_helper.c */
+void helper_lock_init(void);
+
 /* svm_helper.c */
 void cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
                                    uint64_t param);
diff --git a/target-i386/mem_helper.c b/target-i386/mem_helper.c
index 1aec8a5..8bf0da2 100644
--- a/target-i386/mem_helper.c
+++ b/target-i386/mem_helper.c
@@ -23,18 +23,37 @@
 
 /* broken thread support */
 
-static spinlock_t global_cpu_lock = SPIN_LOCK_UNLOCKED;
+#if defined(CONFIG_USER_ONLY)
+QemuMutex global_cpu_lock;
 
 void helper_lock(void)
 {
-    spin_lock(&global_cpu_lock);
+    qemu_mutex_lock(&global_cpu_lock);
 }
 
 void helper_unlock(void)
 {
-    spin_unlock(&global_cpu_lock);
+    qemu_mutex_unlock(&global_cpu_lock);
 }
 
+void helper_lock_init(void)
+{
+    qemu_mutex_init(&global_cpu_lock);
+}
+#else
+void helper_lock(void)
+{
+}
+
+void helper_unlock(void)
+{
+}
+
+void helper_lock_init(void)
+{
+}
+#endif
+
 void helper_cmpxchg8b(CPUX86State *env, target_ulong a0)
 {
     uint64_t d;
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 82e2245..443bf60 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7899,6 +7899,8 @@ void optimize_flags_init(void)
                                          offsetof(CPUX86State, regs[i]),
                                          reg_names[i]);
     }
+
+    helper_lock_init();
 }
 
 /* generate intermediate code in gen_opc_buf and gen_opparam_buf for
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 231a781..0ae648f 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -594,6 +594,10 @@ void *tcg_malloc_internal(TCGContext *s, int size);
 void tcg_pool_reset(TCGContext *s);
 void tcg_pool_delete(TCGContext *s);
 
+void tb_lock(void);
+void tb_unlock(void);
+void tb_lock_reset(void);
+
 static inline void *tcg_malloc(int size)
 {
     TCGContext *s = &tcg_ctx;
diff --git a/translate-all.c b/translate-all.c
index 9c46ffa..a6bff72 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -129,6 +129,39 @@ static void *l1_map[V_L1_SIZE];
 /* code generation context */
 TCGContext tcg_ctx;
 
+/* translation block context */
+#ifdef CONFIG_USER_ONLY
+__thread volatile int have_tb_lock;
+#endif
+
+void tb_lock(void)
+{
+#ifdef CONFIG_USER_ONLY
+    assert(!have_tb_lock);
+    qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
+    have_tb_lock++;
+#endif
+}
+
+void tb_unlock(void)
+{
+#ifdef CONFIG_USER_ONLY
+    assert(have_tb_lock);
+    have_tb_lock--;
+    qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
+#endif
+}
+
+void tb_lock_reset(void)
+{
+#ifdef CONFIG_USER_ONLY
+    if (have_tb_lock) {
+        qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
+        have_tb_lock = 0;
+    }
+#endif
+}
+
 static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
                          tb_page_addr_t phys_page2);
 static TranslationBlock *tb_find_pc(uintptr_t tc_ptr);
@@ -676,6 +709,7 @@ static inline void code_gen_alloc(size_t tb_size)
             CODE_GEN_AVG_BLOCK_SIZE;
     tcg_ctx.tb_ctx.tbs =
             g_malloc(tcg_ctx.code_gen_max_blocks * sizeof(TranslationBlock));
+    qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
 }
 
 /* Must be called before using the QEMU cpus. 'tb_size' is the size
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/10] exec-all: remove non-TCG stuff from exec-all.h header.

[Paolo Bonzini]

The header is included from basically everywhere, thanks to cpu.h.
It should be moved to the (TCG only) files that actually need it.
As a start, remove non-TCG stuff.

While adding a #ifndef CONFIG_USER_ONLY include section to cpu-exec.c,
move memory API files under it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c              | 9 ++++++---
 include/exec/exec-all.h | 6 ------
 include/exec/ram_addr.h | 1 +
 include/sysemu/sysemu.h | 3 +++
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index c680cca..599e64d 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -22,12 +22,15 @@
 #include "disas/disas.h"
 #include "tcg.h"
 #include "qemu/atomic.h"
-#include "sysemu/qtest.h"
 #include "qemu/timer.h"
+#include "exec/tb-hash.h"
+#include "qemu/rcu.h"
+
+#if !defined(CONFIG_USER_ONLY)
 #include "exec/address-spaces.h"
 #include "exec/memory-internal.h"
-#include "qemu/rcu.h"
-#include "exec/tb-hash.h"
+#include "sysemu/sysemu.h"
+#endif
 
 /* -icount align implementation. */
 
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index dafd177..eb77373 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -92,7 +92,6 @@ void cpu_exec_init(CPUState *cpu, Error **errp);
 void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 
 #if !defined(CONFIG_USER_ONLY)
-bool qemu_in_vcpu_thread(void);
 void cpu_reload_memory_map(CPUState *cpu);
 void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as);
 /* cputlb.c */
@@ -320,8 +319,6 @@ extern uintptr_t tci_tb_ptr;
 
 #if !defined(CONFIG_USER_ONLY)
 
-void phys_mem_set_alloc(void *(*alloc)(size_t, uint64_t *align));
-
 struct MemoryRegion *iotlb_to_region(CPUState *cpu,
                                      hwaddr index);
 
@@ -346,7 +343,4 @@ extern int singlestep;
 /* cpu-exec.c */
 extern volatile sig_atomic_t exit_request;
 
-#if !defined(CONFIG_USER_ONLY)
-void migration_bitmap_extend(ram_addr_t old, ram_addr_t new);
-#endif
 #endif
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index c113f21..7e7566e 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -249,5 +249,6 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest,
     return num_dirty;
 }
 
+void migration_bitmap_extend(ram_addr_t old, ram_addr_t new);
 #endif
 #endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 44570d1..93a70e3 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -205,6 +205,9 @@ QemuOpts *qemu_get_machine_opts(void);
 bool defaults_enabled(void);
 bool usb_enabled(void);
 
+bool qemu_in_vcpu_thread(void);
+void phys_mem_set_alloc(void *(*alloc)(size_t, uint64_t *align));
+
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
 extern QemuOptsList qemu_drive_opts;
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/10] cpu-exec: elide more icount code if CONFIG_USER_ONLY

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/cpu-exec.c b/cpu-exec.c
index 599e64d..bde5fd1 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -228,6 +228,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, uint8_t *tb_ptr)
     return next_tb;
 }
 
+#if defined(CONFIG_SOFTMMU)
 /* Execute the code without caching the generated code. An interpreter
    could be used if available. */
 static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
@@ -251,6 +252,7 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
     tb_phys_invalidate(tb, -1);
     tb_free(tb);
 }
+#endif
 
 static TranslationBlock *tb_find_slow(CPUState *cpu,
                                       target_ulong pc,
@@ -523,6 +525,9 @@ int cpu_exec(CPUState *cpu)
                     case TB_EXIT_ICOUNT_EXPIRED:
                     {
                         /* Instruction counter expired.  */
+#ifdef CONFIG_USER_ONLY
+                        abort();
+#else
                         int insns_left = cpu->icount_decr.u32;
                         if (cpu->icount_extra && insns_left >= 0) {
                             /* Refill decrementer and continue execution.  */
@@ -542,6 +547,7 @@ int cpu_exec(CPUState *cpu)
                             cpu_loop_exit(cpu);
                         }
                         break;
+#endif
                     }
                     default:
                         break;
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/10] tcg: code_bitmap is not used by user-mode emulation

[Paolo Bonzini]

More #ifdefs are not nice, but this clarifies why its usage is not
protected by tb_lock.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 translate-all.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index a6bff72..7aa5664 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -765,11 +765,15 @@ void tb_free(TranslationBlock *tb)
 
 static inline void invalidate_page_bitmap(PageDesc *p)
 {
+#ifdef CONFIG_SOFTMMU
     if (p->code_bitmap) {
         g_free(p->code_bitmap);
         p->code_bitmap = NULL;
     }
     p->code_write_count = 0;
+#else
+    assert(p->code_bitmap == NULL);
+#endif
 }
 
 /* Set to NULL all the 'first_tb' fields in all PageDescs. */
@@ -1001,6 +1005,7 @@ void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
     tcg_ctx.tb_ctx.tb_phys_invalidate_count++;
 }
 
+#ifdef CONFIG_SOFTMMU
 static void build_page_bitmap(PageDesc *p)
 {
     int n, tb_start, tb_end;
@@ -1029,6 +1034,7 @@ static void build_page_bitmap(PageDesc *p)
         tb = tb->page_next[n];
     }
 }
+#endif
 
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base,
@@ -1202,6 +1208,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
 #endif
 }
 
+#ifdef CONFIG_SOFTMMU
 /* len must be <= 8 and start must be a multiple of len */
 void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
 {
@@ -1239,8 +1246,7 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
         tb_invalidate_phys_page_range(start, start + len, 1);
     }
 }
-
-#if !defined(CONFIG_SOFTMMU)
+#else
 static void tb_invalidate_phys_page(tb_page_addr_t addr,
                                     uintptr_t pc, void *puc,
                                     bool locked)
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/10] tcg: comment on which functions have to be called with mmap_lock held

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 translate-all.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 7aa5664..7727091 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -172,11 +172,13 @@ void cpu_gen_init(void)
 }
 
 /* return non zero if the very first instruction is invalid so that
-   the virtual CPU can trigger an exception.
-
-   '*gen_code_size_ptr' contains the size of the generated code (host
-   code).
-*/
+ * the virtual CPU can trigger an exception.
+ *
+ * '*gen_code_size_ptr' contains the size of the generated code (host
+ * code).
+ *
+ * Called with mmap_lock held for user-mode emulation.
+ */
 int cpu_gen_code(CPUArchState *env, TranslationBlock *tb, int *gen_code_size_ptr)
 {
     TCGContext *s = &tcg_ctx;
@@ -421,6 +423,9 @@ static void page_init(void)
 #endif
 }
 
+/* If alloc=1:
+ * Called with mmap_lock held for user-mode emulation.
+ */
 static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
 {
     PageDesc *pd;
@@ -1036,6 +1041,7 @@ static void build_page_bitmap(PageDesc *p)
 }
 #endif
 
+/* Called with mmap_lock held for user mode emulation.  */
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base,
                               int flags, int cflags)
@@ -1083,6 +1089,8 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
  * 'is_cpu_write_access' should be true if called from a real cpu write
  * access: the virtual CPU will exit the current TB if code is modified inside
  * this TB.
+ *
+ * Called with mmap_lock held for user-mode emulation
  */
 void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end)
 {
@@ -1099,6 +1107,8 @@ void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end)
  * 'is_cpu_write_access' should be true if called from a real cpu write
  * access: the virtual CPU will exit the current TB if code is modified inside
  * this TB.
+ *
+ * Called with mmap_lock held for user-mode emulation
  */
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access)
@@ -1247,6 +1257,7 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
     }
 }
 #else
+/* Called with mmap_lock held.  */
 static void tb_invalidate_phys_page(tb_page_addr_t addr,
                                     uintptr_t pc, void *puc,
                                     bool locked)
@@ -1316,7 +1327,10 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
 }
 #endif
 
-/* add the tb in the target page and protect it if necessary */
+/* add the tb in the target page and protect it if necessary
+ *
+ * Called with mmap_lock held for user-mode emulation.
+ */
 static inline void tb_alloc_page(TranslationBlock *tb,
                                  unsigned int n, tb_page_addr_t page_addr)
 {
@@ -1372,7 +1386,8 @@ static inline void tb_alloc_page(TranslationBlock *tb,
 }
 
 /* add a new TB and link it to the physical page tables. phys_page2 is
-   (-1) to indicate that only one page contains the TB. */
+ * (-1) to indicate that only one page contains the TB.
+ */
 static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
                          tb_page_addr_t phys_page2)
 {
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses

[Paolo Bonzini]

page_find is reading the radix tree outside all locks, so it has to
use the RCU primitives.  It does not need RCU critical sections
because the PageDescs are never removed, so there is never a need
to wait for the end of code sections that use a PageDesc.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 translate-all.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 7727091..78a787d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -437,14 +437,14 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
 
     /* Level 2..N-1.  */
     for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
-        void **p = *lp;
+        void **p = atomic_rcu_read(lp);
 
         if (p == NULL) {
             if (!alloc) {
                 return NULL;
             }
             p = g_new0(void *, V_L2_SIZE);
-            *lp = p;
+            atomic_rcu_set(lp, p);
         }
 
         lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));
@@ -456,7 +456,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
             return NULL;
         }
         pd = g_new0(PageDesc, V_L2_SIZE);
-        *lp = pd;
+        atomic_rcu_set(lp, pd);
     }
 
     return pd + (index & (V_L2_SIZE - 1));
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/10] exec: make mmap_lock/mmap_unlock globally available

[Paolo Bonzini]

There is some iffy lock hierarchy going on in translate-all.c.  To
fix it, we need to take the mmap_lock in cpu-exec.c.  Make the
functions globally available.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 bsd-user/qemu.h         | 2 --
 include/exec/exec-all.h | 7 ++++++-
 linux-user/qemu.h       | 2 --
 translate-all.c         | 5 -----
 4 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 5362297..5902614 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -213,8 +213,6 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
                        abi_ulong new_addr);
 int target_msync(abi_ulong start, abi_ulong len, int flags);
 extern unsigned long last_brk;
-void mmap_lock(void);
-void mmap_unlock(void);
 void cpu_list_lock(void);
 void cpu_list_unlock(void);
 #if defined(CONFIG_USE_NPTL)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index eb77373..b3f900a 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -318,7 +318,6 @@ extern uintptr_t tci_tb_ptr;
 #define GETPC()  (GETRA() - GETPC_ADJ)
 
 #if !defined(CONFIG_USER_ONLY)
-
 struct MemoryRegion *iotlb_to_region(CPUState *cpu,
                                      hwaddr index);
 
@@ -328,11 +327,17 @@ void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
 #endif
 
 #if defined(CONFIG_USER_ONLY)
+void mmap_lock(void);
+void mmap_unlock(void);
+
 static inline tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
 {
     return addr;
 }
 #else
+static inline void mmap_lock(void) {}
+static inline void mmap_unlock(void) {}
+
 /* cputlb.c */
 tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr);
 #endif
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 8012cc2..e8606b2 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -261,8 +261,6 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
 int target_msync(abi_ulong start, abi_ulong len, int flags);
 extern unsigned long last_brk;
 extern abi_ulong mmap_next_start;
-void mmap_lock(void);
-void mmap_unlock(void);
 abi_ulong mmap_find_vma(abi_ulong, abi_ulong);
 void cpu_list_lock(void);
 void cpu_list_unlock(void);
diff --git a/translate-all.c b/translate-all.c
index 78a787d..61fa03d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -467,11 +467,6 @@ static inline PageDesc *page_find(tb_page_addr_t index)
     return page_find_alloc(index, 0);
 }
 
-#if !defined(CONFIG_USER_ONLY)
-#define mmap_lock() do { } while (0)
-#define mmap_unlock() do { } while (0)
-#endif
-
 #if defined(CONFIG_USER_ONLY)
 /* Currently it is not recommended to allocate big chunks of data in
    user mode. It will change when a dedicated libc will be used.  */
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/10] cpu-exec: fix lock hierarchy for user-mode emulation

[Paolo Bonzini]

tb_lock has to be taken inside the mmap_lock (example:
tb_invalidate_phys_range is called by target_mmap), but
tb_link_page is taking the mmap_lock and it is called
with the tb_lock held.

To fix this, take the mmap_lock in tb_find_slow, not
in tb_link_page.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c      | 71 ++++++++++++++++++++++++++++++++++++++++++---------------
 translate-all.c |  6 ++---
 2 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index bde5fd1..e712c6a 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -254,10 +254,10 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
 }
 #endif
 
-static TranslationBlock *tb_find_slow(CPUState *cpu,
-                                      target_ulong pc,
-                                      target_ulong cs_base,
-                                      uint64_t flags)
+static TranslationBlock *tb_find_physical(CPUState *cpu,
+                                          target_ulong pc,
+                                          target_ulong cs_base,
+                                          uint64_t flags)
 {
     CPUArchState *env = (CPUArchState *)cpu->env_ptr;
     TranslationBlock *tb, **ptb1;
@@ -274,8 +274,9 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
     ptb1 = &tcg_ctx.tb_ctx.tb_phys_hash[h];
     for(;;) {
         tb = *ptb1;
-        if (!tb)
-            goto not_found;
+        if (!tb) {
+            return NULL;
+        }
         if (tb->pc == pc &&
             tb->page_addr[0] == phys_page1 &&
             tb->cs_base == cs_base &&
@@ -287,25 +288,59 @@ static TranslationBlock *tb_find_slow(CPUState *cpu,
                 virt_page2 = (pc & TARGET_PAGE_MASK) +
                     TARGET_PAGE_SIZE;
                 phys_page2 = get_page_addr_code(env, virt_page2);
-                if (tb->page_addr[1] == phys_page2)
-                    goto found;
+                if (tb->page_addr[1] == phys_page2) {
+                    break;
+                }
             } else {
-                goto found;
+                break;
             }
         }
         ptb1 = &tb->phys_hash_next;
     }
- not_found:
-   /* if no translated code available, then translate it now */
-    tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
 
- found:
-    /* Move the last found TB to the head of the list */
-    if (likely(*ptb1)) {
-        *ptb1 = tb->phys_hash_next;
-        tb->phys_hash_next = tcg_ctx.tb_ctx.tb_phys_hash[h];
-        tcg_ctx.tb_ctx.tb_phys_hash[h] = tb;
+    /* Move the TB to the head of the list */
+    *ptb1 = tb->phys_hash_next;
+    tb->phys_hash_next = tcg_ctx.tb_ctx.tb_phys_hash[h];
+    tcg_ctx.tb_ctx.tb_phys_hash[h] = tb;
+    return tb;
+}
+
+static TranslationBlock *tb_find_slow(CPUState *cpu,
+                                      target_ulong pc,
+                                      target_ulong cs_base,
+                                      uint64_t flags)
+{
+    TranslationBlock *tb;
+
+    tb = tb_find_physical(cpu, pc, cs_base, flags);
+    if (tb) {
+        goto found;
+    }
+
+#ifdef CONFIG_USER_ONLY
+    /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
+     * taken outside tb_lock.  Since we're momentarily dropping
+     * tb_lock, there's a chance that our desired tb has been
+     * translated.
+     */
+    tb_unlock();
+    mmap_lock();
+    tb_lock();
+    tb = tb_find_physical(cpu, pc, cs_base, flags);
+    if (tb) {
+        mmap_unlock();
+        goto found;
     }
+#endif
+
+    /* if no translated code available, then translate it now */
+    tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
+
+#ifdef CONFIG_USER_ONLY
+    mmap_unlock();
+#endif
+
+found:
     /* we add the TB in the virtual pc hash table */
     cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)] = tb;
     return tb;
diff --git a/translate-all.c b/translate-all.c
index 61fa03d..edb9cb1 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1382,6 +1382,8 @@ static inline void tb_alloc_page(TranslationBlock *tb,
 
 /* add a new TB and link it to the physical page tables. phys_page2 is
  * (-1) to indicate that only one page contains the TB.
+ *
+ * Called with mmap_lock held for user-mode emulation.
  */
 static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
                          tb_page_addr_t phys_page2)
@@ -1389,9 +1391,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
     unsigned int h;
     TranslationBlock **ptb;
 
-    /* Grab the mmap lock to stop another thread invalidating this TB
-       before we are done.  */
-    mmap_lock();
     /* add in the physical hash table */
     h = tb_phys_hash_func(phys_pc);
     ptb = &tcg_ctx.tb_ctx.tb_phys_hash[h];
@@ -1421,7 +1420,6 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
 #ifdef DEBUG_TB_CHECK
     tb_page_check();
 #endif
-    mmap_unlock();
 }
 
 /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Paolo Bonzini]

softmmu requires more functions to be thread-safe, because translation
blocks can be invalidated from e.g. notdirty callbacks.  Probably the
same holds for user-mode emulation, it's just that no one has ever
tried to produce a coherent locking there.

This patch will guide the introduction of more tb_lock and tb_unlock
calls for system emulation.

Note that after this patch some (most) of the mentioned functions are
still called outside tb_lock/tb_unlock.  The next one will rectify this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                  |  1 +
 include/exec/exec-all.h |  2 ++
 include/qom/cpu.h       |  3 +++
 tcg/tcg.h               |  2 ++
 translate-all.c         | 35 ++++++++++++++++++++++++++++-------
 5 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/exec.c b/exec.c
index 54cd70a..856a859 100644
--- a/exec.c
+++ b/exec.c
@@ -748,6 +748,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
 {
     CPUBreakpoint *bp;
 
+    /* TODO: locking (RCU?) */
     bp = g_malloc(sizeof(*bp));
 
     bp->pc = pc;
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index b3f900a..943d97a 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -74,6 +74,7 @@ typedef struct TranslationBlock TranslationBlock;
 
 void gen_intermediate_code(CPUArchState *env, struct TranslationBlock *tb);
 void gen_intermediate_code_pc(CPUArchState *env, struct TranslationBlock *tb);
+/* Called with tb_lock held.  */
 void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb,
                           int pc_pos);
 
@@ -278,6 +279,7 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
 
 #endif
 
+/* Called with tb_lock held.  */
 static inline void tb_add_jump(TranslationBlock *tb, int n,
                                TranslationBlock *tb_next)
 {
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 77bbff2..56b1f4d 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -285,7 +285,10 @@ struct CPUState {
 
     void *env_ptr; /* CPUArchState */
     struct TranslationBlock *current_tb;
+
+    /* Protected by tb_lock.  */
     struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
+
     struct GDBRegisterState *gdb_regs;
     int gdb_num_regs;
     int gdb_num_g_regs;
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 0ae648f..a2cad31 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -590,6 +590,7 @@ static inline bool tcg_op_buf_full(void)
 
 /* pool based memory allocation */
 
+/* tb_lock must be held for tcg_malloc_internal. */
 void *tcg_malloc_internal(TCGContext *s, int size);
 void tcg_pool_reset(TCGContext *s);
 void tcg_pool_delete(TCGContext *s);
@@ -598,6 +599,7 @@ void tb_lock(void);
 void tb_unlock(void);
 void tb_lock_reset(void);
 
+/* Called with tb_lock held.  */
 static inline void *tcg_malloc(int size)
 {
     TCGContext *s = &tcg_ctx;
diff --git a/translate-all.c b/translate-all.c
index edb9cb1..17d3cd1 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -237,6 +237,7 @@ int cpu_gen_code(CPUArchState *env, TranslationBlock *tb, int *gen_code_size_ptr
 }
 
 /* The cpu state corresponding to 'searched_pc' is restored.
+ * Called with tb_lock held.
  */
 static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
                                      uintptr_t searched_pc)
@@ -424,6 +425,7 @@ static void page_init(void)
 }
 
 /* If alloc=1:
+ * Called with tb_lock held for system emulation.
  * Called with mmap_lock held for user-mode emulation.
  */
 static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
@@ -734,8 +736,12 @@ bool tcg_enabled(void)
     return tcg_ctx.code_gen_buffer != NULL;
 }
 
-/* Allocate a new translation block. Flush the translation buffer if
-   too many translation blocks or too much generated code. */
+/*
+ * Allocate a new translation block. Flush the translation buffer if
+ * too many translation blocks or too much generated code.
+ *
+ * Called with tb_lock held.
+ */
 static TranslationBlock *tb_alloc(target_ulong pc)
 {
     TranslationBlock *tb;
@@ -751,6 +757,7 @@ static TranslationBlock *tb_alloc(target_ulong pc)
     return tb;
 }
 
+/* Called with tb_lock held.  */
 void tb_free(TranslationBlock *tb)
 {
     /* In practice this is mostly used for single use temporary TB
@@ -859,7 +866,10 @@ static void tb_invalidate_check(target_ulong address)
     }
 }
 
-/* verify that all the pages have correct rights for code */
+/* verify that all the pages have correct rights for code
+ *
+ * Called with tb_lock held.
+ */
 static void tb_page_check(void)
 {
     TranslationBlock *tb;
@@ -947,7 +957,10 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
     tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr + tb->tb_next_offset[n]));
 }
 
-/* invalidate one TB */
+/* invalidate one TB
+ *
+ * Called with tb_lock held.
+ */
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
 {
     CPUState *cpu;
@@ -1036,7 +1049,7 @@ static void build_page_bitmap(PageDesc *p)
 }
 #endif
 
-/* Called with mmap_lock held for user mode emulation.  */
+/* Called with tb_lock held, and mmap_lock too for user mode emulation.  */
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base,
                               int flags, int cflags)
@@ -1234,7 +1247,9 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
     }
     if (!p->code_bitmap &&
         ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD) {
-        /* build code bitmap */
+        /* build code bitmap.  FIXME: writes should be protected by
+         * tb_lock, reads by tb_lock or RCU.
+         */
         build_page_bitmap(p);
     }
     if (p->code_bitmap) {
@@ -1324,6 +1339,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
 
 /* add the tb in the target page and protect it if necessary
  *
+ * Called with tb_lock held.
  * Called with mmap_lock held for user-mode emulation.
  */
 static inline void tb_alloc_page(TranslationBlock *tb,
@@ -1383,6 +1399,7 @@ static inline void tb_alloc_page(TranslationBlock *tb,
 /* add a new TB and link it to the physical page tables. phys_page2 is
  * (-1) to indicate that only one page contains the TB.
  *
+ * Called with tb_lock held.
  * Called with mmap_lock held for user-mode emulation.
  */
 static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
@@ -1423,7 +1440,10 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
 }
 
 /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
-   tb[1].tc_ptr. Return NULL if not found */
+ * tb[1].tc_ptr. Return NULL if not found
+ *
+ * Called with tb_lock held.
+ */
 static TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
 {
     int m_min, m_max, m;
@@ -1476,6 +1496,7 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr)
 }
 #endif /* !defined(CONFIG_USER_ONLY) */
 
+/* Called with tb_lock held.  */
 void tb_check_watchpoint(CPUState *cpu)
 {
     TranslationBlock *tb;
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/10] tcg: protect TBContext with tb_lock.

[Paolo Bonzini]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This protects TBContext with tb_lock to make tb_* thread safe.

We can still have issue with tb_flush in case of multithread TCG:
another CPU can be executing code during a flush.

This can be fixed later by making all other TCG thread exiting before calling
tb_flush().

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>

Changes:
V6 -> V7:
  * Drop a tb_lock in already locked restore_state_to_opc.
V5 -> V6:
  * Drop a tb_lock arround tb_find_fast in cpu-exec.c.
Message-Id: <1439220437-23957-8-git-send-email-fred.konrad@greensocs.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c         |  6 ++++++
 exec.c             |  3 +++
 hw/i386/kvmvapic.c |  2 ++
 translate-all.c    | 38 ++++++++++++++++++++++++++++++++------
 4 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index e712c6a..89b66f5 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -241,16 +241,22 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
     if (max_cycles > CF_COUNT_MASK)
         max_cycles = CF_COUNT_MASK;
 
+    tb_lock();
     tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
                      max_cycles | CF_NOCACHE);
     tb->orig_tb = tcg_ctx.tb_ctx.tb_invalidated_flag ? NULL : orig_tb;
     cpu->current_tb = tb;
+    tb_unlock();
+
     /* execute the generated code */
     trace_exec_tb_nocache(tb, tb->pc);
     cpu_tb_exec(cpu, tb->tc_ptr);
+
+    tb_lock();
     cpu->current_tb = NULL;
     tb_phys_invalidate(tb, -1);
     tb_free(tb);
+    tb_unlock();
 }
 #endif
 
diff --git a/exec.c b/exec.c
index 856a859..9083307 100644
--- a/exec.c
+++ b/exec.c
@@ -1948,6 +1948,9 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
             wp->hitattrs = attrs;
             if (!cpu->watchpoint_hit) {
                 cpu->watchpoint_hit = wp;
+
+                /* Unlocked by cpu_loop_exit or cpu_resume_from_signal.  */
+                tb_lock();
                 tb_check_watchpoint(cpu);
                 if (wp->flags & BP_STOP_BEFORE_ACCESS) {
                     cpu->exception_index = EXCP_DEBUG;
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index c6d34b2..d823e15 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -445,6 +445,8 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
     resume_all_vcpus();
 
     if (!kvm_enabled()) {
+        /* Unlocked by cpu_resume_from_signal.  */
+        tb_lock();
         cs->current_tb = NULL;
         tb_gen_code(cs, current_pc, current_cs_base, current_flags, 1);
         cpu_resume_from_signal(cs, NULL);
diff --git a/translate-all.c b/translate-all.c
index 17d3cd1..7a4f8f1 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -301,6 +301,8 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
 {
     TranslationBlock *tb;
 
+    tb_lock();
+
     tb = tb_find_pc(retaddr);
     if (tb) {
         cpu_restore_state_from_tb(cpu, tb, retaddr);
@@ -310,8 +312,12 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
             tb_phys_invalidate(tb, -1);
             tb_free(tb);
         }
+
+        tb_unlock();
         return true;
     }
+
+    tb_unlock();
     return false;
 }
 
@@ -820,6 +826,8 @@ static void page_flush_tb(void)
 /* XXX: tb_flush is currently not thread safe */
 void tb_flush(CPUState *cpu)
 {
+    tb_lock();
+
 #if defined(DEBUG_FLUSH)
     printf("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n",
            (unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer),
@@ -844,6 +852,8 @@ void tb_flush(CPUState *cpu)
     /* XXX: flush processor icache at this point if cache flush is
        expensive */
     tcg_ctx.tb_ctx.tb_flush_count++;
+
+    tb_unlock();
 }
 
 #ifdef DEBUG_TB_CHECK
@@ -1151,6 +1161,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
     /* we remove all the TBs in the range [start, end[ */
     /* XXX: see if in some cases it could be faster to invalidate all
        the code */
+    tb_lock();
     tb = p->first_tb;
     while (tb != NULL) {
         n = (uintptr_t)tb & 3;
@@ -1218,12 +1229,13 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
     if (current_tb_modified) {
         /* we generate a block containing just the instruction
            modifying the memory. It will ensure that it cannot modify
-           itself */
+           itself.  cpu_resume_from_signal unlocks tb_lock.  */
         cpu->current_tb = NULL;
         tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1);
         cpu_resume_from_signal(cpu, NULL);
     }
 #endif
+    tb_unlock();
 }
 
 #ifdef CONFIG_SOFTMMU
@@ -1290,6 +1302,8 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
     if (!p) {
         return;
     }
+
+    tb_lock();
     tb = p->first_tb;
 #ifdef TARGET_HAS_PRECISE_SMC
     if (tb && pc != 0) {
@@ -1331,9 +1345,12 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
         if (locked) {
             mmap_unlock();
         }
+
+        /* tb_lock released by cpu_resume_from_signal.  */
         cpu_resume_from_signal(cpu, puc);
     }
 #endif
+    tb_unlock();
 }
 #endif
 
@@ -1563,6 +1580,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
     target_ulong pc, cs_base;
     uint64_t flags;
 
+    tb_lock();
     tb = tb_find_pc(retaddr);
     if (!tb) {
         cpu_abort(cpu, "cpu_io_recompile: could not find TB for pc=%p",
@@ -1614,11 +1632,15 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
     /* FIXME: In theory this could raise an exception.  In practice
        we have already translated the block once so it's probably ok.  */
     tb_gen_code(cpu, pc, cs_base, flags, cflags);
-    /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
-       the first in the TB) then we end up generating a whole new TB and
-       repeating the fault, which is horribly inefficient.
-       Better would be to execute just this insn uncached, or generate a
-       second new TB.  */
+
+    /* This unlocks the tb_lock.
+     *
+     * TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
+     * the first in the TB) then we end up generating a whole new TB and
+     * repeating the fault, which is horribly inefficient.
+     * Better would be to execute just this insn uncached, or generate a
+     * second new TB.
+     */
     cpu_resume_from_signal(cpu, NULL);
 }
 
@@ -1643,6 +1665,8 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
     int direct_jmp_count, direct_jmp2_count, cross_page;
     TranslationBlock *tb;
 
+    tb_lock();
+
     target_code_size = 0;
     max_target_code_size = 0;
     cross_page = 0;
@@ -1698,6 +1722,8 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
             tcg_ctx.tb_ctx.tb_phys_invalidate_count);
     cpu_fprintf(f, "TLB flush count     %d\n", tlb_flush_count);
     tcg_dump_info(f, cpu_fprintf);
+
+    tb_unlock();
 }
 
 void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses

[Emilio G. Cota]

On Wed, Aug 12, 2015 at 18:41:00 +0200, Paolo Bonzini wrote:
> page_find is reading the radix tree outside all locks, so it has to
> use the RCU primitives.  It does not need RCU critical sections
> because the PageDescs are never removed, so there is never a need
> to wait for the end of code sections that use a PageDesc.

Note that rcu_find_alloc might end up writing to the tree, see below.

BTW the fact that there are no removals makes the use of RCU unnecessary.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  translate-all.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/translate-all.c b/translate-all.c
> index 7727091..78a787d 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -437,14 +437,14 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>  
>      /* Level 2..N-1.  */
>      for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
> -        void **p = *lp;
> +        void **p = atomic_rcu_read(lp);
>  
>          if (p == NULL) {
>              if (!alloc) {
>                  return NULL;
>              }
>              p = g_new0(void *, V_L2_SIZE);
> -            *lp = p;
> +            atomic_rcu_set(lp, p);
>          }
>  
>          lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));
> @@ -456,7 +456,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>              return NULL;
>          }
>          pd = g_new0(PageDesc, V_L2_SIZE);
> -        *lp = pd;
> +        atomic_rcu_set(lp, pd);

rcu_set is not enough; a cmpxchg with a fail path would be needed here, since
if the find_alloc is called without any locks held (as described in the commit message)
several threads could concurrently write to the same node, corrupting the tree.

I argue however that it is better to call page_find/_alloc with a mutex held,
since otherwise we'd have to add per-PageDesc locks (it's very common to
call page_find and then update the PageDesc). I have an RFC patchset for multithreaded tcg
that deals with this, will submit once I bring it up to date with upstream.

		Emilio

Re: [Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses

[Paolo Bonzini]

On 12/08/2015 22:37, Emilio G. Cota wrote:
> > page_find is reading the radix tree outside all locks, so it has to
> > use the RCU primitives.  It does not need RCU critical sections
> > because the PageDescs are never removed, so there is never a need
> > to wait for the end of code sections that use a PageDesc.
>
> Note that rcu_find_alloc might end up writing to the tree, see below.

Yes, but in that case it's always called with the mmap_lock held, see
patch 7.

page_find_alloc is only called by tb_alloc_page (called by tb_link_page
which takes mmap_lock), or by page_set_flags (called with mmap_lock held
by linux-user/mmap.c).

> BTW the fact that there are no removals makes the use of RCU unnecessary.

It only makes it not use the RCU synchronization primitives.  You still
need the memory barriers.

> I argue however that it is better to call page_find/_alloc with a mutex held,
> since otherwise we'd have to add per-PageDesc locks (it's very common to
> call page_find and then update the PageDesc). 

The fields are protected by either the mmap_lock (e.g. the flags, see
page_unprotect and tb_alloc_page) or the tb_lock (e.g. the tb lists).

The code is complicated and could definitely use more documentation,
especially for struct PageDesc, but it seems correct to me apart from
the lock inversion fixed in patch 10.

Paolo

Re: [Qemu-devel] [PATCH 03/10] replace spinlock by QemuMutex.

[Frederic Konrad]

On 12/08/2015 18:40, Paolo Bonzini wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> spinlock is only used in two cases:
>    * cpu-exec.c: to protect TranslationBlock
>    * mem_helper.c: for lock helper in target-i386 (which seems broken).
>
> It's a pthread_mutex_t in user-mode so better using QemuMutex directly in this
> case.
> It allows as well to reuse tb_lock mutex of TBContext in case of multithread
> TCG.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Message-Id: <1439220437-23957-5-git-send-email-fred.konrad@greensocs.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   cpu-exec.c               | 15 +++------------
>   include/exec/exec-all.h  |  4 ++--
>   linux-user/main.c        |  6 +++---
>   target-i386/cpu.h        |  3 +++
>   target-i386/mem_helper.c | 25 ++++++++++++++++++++++---
>   target-i386/translate.c  |  2 ++
>   tcg/tcg.h                |  4 ++++
>   translate-all.c          | 34 ++++++++++++++++++++++++++++++++++
>   8 files changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 713540f..c680cca 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -356,9 +356,6 @@ int cpu_exec(CPUState *cpu)
>       uintptr_t next_tb;
>       SyncClocks sc;
>   
> -    /* This must be volatile so it is not trashed by longjmp() */
> -    volatile bool have_tb_lock = false;
> -
>       if (cpu->halted) {
>           if (!cpu_has_work(cpu)) {
>               return EXCP_HALTED;
> @@ -475,8 +472,7 @@ int cpu_exec(CPUState *cpu)
>                       cpu->exception_index = EXCP_INTERRUPT;
>                       cpu_loop_exit(cpu);
>                   }
> -                spin_lock(&tcg_ctx.tb_ctx.tb_lock);
> -                have_tb_lock = true;
> +                tb_lock();
>                   tb = tb_find_fast(cpu);
>                   /* Note: we do it here to avoid a gcc bug on Mac OS X when
>                      doing it in tb_find_slow */
> @@ -498,9 +494,7 @@ int cpu_exec(CPUState *cpu)
>                       tb_add_jump((TranslationBlock *)(next_tb & ~TB_EXIT_MASK),
>                                   next_tb & TB_EXIT_MASK, tb);
>                   }
> -                have_tb_lock = false;
> -                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
> -
> +                tb_unlock();
>                   /* cpu_interrupt might be called while translating the
>                      TB, but before it is linked into a potentially
>                      infinite loop and becomes env->current_tb. Avoid
> @@ -567,10 +561,7 @@ int cpu_exec(CPUState *cpu)
>               x86_cpu = X86_CPU(cpu);
>               env = &x86_cpu->env;
>   #endif
> -            if (have_tb_lock) {
> -                spin_unlock(&tcg_ctx.tb_ctx.tb_lock);
> -                have_tb_lock = false;
> -            }
> +            tb_lock_reset();
>           }
>       } /* for(;;) */
>   
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 29775c0..dafd177 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -178,7 +178,7 @@ struct TranslationBlock {
>       struct TranslationBlock *jmp_first;
>   };
>   
> -#include "exec/spinlock.h"
> +#include "qemu/thread.h"
>   
>   typedef struct TBContext TBContext;
>   
> @@ -188,7 +188,7 @@ struct TBContext {
>       TranslationBlock *tb_phys_hash[CODE_GEN_PHYS_HASH_SIZE];
>       int nb_tbs;
>       /* any access to the tbs or the page table must use this lock */
> -    spinlock_t tb_lock;
> +    QemuMutex tb_lock;
>   
>       /* statistics */
>       int tb_flush_count;
> diff --git a/linux-user/main.c b/linux-user/main.c
> index fdee981..fd06ce9 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -107,7 +107,7 @@ static int pending_cpus;
>   /* Make sure everything is in a consistent state for calling fork().  */
>   void fork_start(void)
>   {
> -    pthread_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
> +    qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
>       pthread_mutex_lock(&exclusive_lock);
>       mmap_fork_start();
>   }
> @@ -129,11 +129,11 @@ void fork_end(int child)
>           pthread_mutex_init(&cpu_list_mutex, NULL);
>           pthread_cond_init(&exclusive_cond, NULL);
>           pthread_cond_init(&exclusive_resume, NULL);
> -        pthread_mutex_init(&tcg_ctx.tb_ctx.tb_lock, NULL);
> +        qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
>           gdbserver_fork(thread_cpu);
>       } else {
>           pthread_mutex_unlock(&exclusive_lock);
> -        pthread_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
> +        qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
We might want to use tb_lock/unlock in user code as well instead of 
calling directly
qemu_mutex_* ?

>       }
>   }
>   
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index ead2832..3655ff3 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1318,6 +1318,9 @@ static inline MemTxAttrs cpu_get_mem_attrs(CPUX86State *env)
>   void cpu_set_mxcsr(CPUX86State *env, uint32_t val);
>   void cpu_set_fpuc(CPUX86State *env, uint16_t val);
>   
> +/* mem_helper.c */
> +void helper_lock_init(void);
> +
>   /* svm_helper.c */
>   void cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
>                                      uint64_t param);
> diff --git a/target-i386/mem_helper.c b/target-i386/mem_helper.c
> index 1aec8a5..8bf0da2 100644
> --- a/target-i386/mem_helper.c
> +++ b/target-i386/mem_helper.c
> @@ -23,18 +23,37 @@
>   
>   /* broken thread support */
>   
> -static spinlock_t global_cpu_lock = SPIN_LOCK_UNLOCKED;
> +#if defined(CONFIG_USER_ONLY)
> +QemuMutex global_cpu_lock;
>   
>   void helper_lock(void)
>   {
> -    spin_lock(&global_cpu_lock);
> +    qemu_mutex_lock(&global_cpu_lock);
>   }
>   
>   void helper_unlock(void)
>   {
> -    spin_unlock(&global_cpu_lock);
> +    qemu_mutex_unlock(&global_cpu_lock);
>   }
>   
> +void helper_lock_init(void)
> +{
> +    qemu_mutex_init(&global_cpu_lock);
> +}
> +#else
> +void helper_lock(void)
> +{
> +}
> +
> +void helper_unlock(void)
> +{
> +}
> +
> +void helper_lock_init(void)
> +{
> +}
> +#endif
> +
>   void helper_cmpxchg8b(CPUX86State *env, target_ulong a0)
>   {
>       uint64_t d;
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 82e2245..443bf60 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7899,6 +7899,8 @@ void optimize_flags_init(void)
>                                            offsetof(CPUX86State, regs[i]),
>                                            reg_names[i]);
>       }
> +
> +    helper_lock_init();
>   }
>   
>   /* generate intermediate code in gen_opc_buf and gen_opparam_buf for
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 231a781..0ae648f 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -594,6 +594,10 @@ void *tcg_malloc_internal(TCGContext *s, int size);
>   void tcg_pool_reset(TCGContext *s);
>   void tcg_pool_delete(TCGContext *s);
>   
> +void tb_lock(void);
> +void tb_unlock(void);
> +void tb_lock_reset(void);
> +
>   static inline void *tcg_malloc(int size)
>   {
>       TCGContext *s = &tcg_ctx;
> diff --git a/translate-all.c b/translate-all.c
> index 9c46ffa..a6bff72 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -129,6 +129,39 @@ static void *l1_map[V_L1_SIZE];
>   /* code generation context */
>   TCGContext tcg_ctx;
>   
> +/* translation block context */
> +#ifdef CONFIG_USER_ONLY
> +__thread volatile int have_tb_lock;
> +#endif
> +
> +void tb_lock(void)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    assert(!have_tb_lock);
> +    qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
> +    have_tb_lock++;
> +#endif
> +}
> +
> +void tb_unlock(void)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    assert(have_tb_lock);
> +    have_tb_lock--;
> +    qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
> +#endif
> +}
> +
> +void tb_lock_reset(void)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    if (have_tb_lock) {
> +        qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
> +        have_tb_lock = 0;
> +    }
> +#endif
> +}
> +
>   static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>                            tb_page_addr_t phys_page2);
>   static TranslationBlock *tb_find_pc(uintptr_t tc_ptr);
> @@ -676,6 +709,7 @@ static inline void code_gen_alloc(size_t tb_size)
>               CODE_GEN_AVG_BLOCK_SIZE;
>       tcg_ctx.tb_ctx.tbs =
>               g_malloc(tcg_ctx.code_gen_max_blocks * sizeof(TranslationBlock));
> +    qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
Maybe we can initialize the mutex only for CONFIG_USER_ONLY?

Fred
>   }
>   
>   /* Must be called before using the QEMU cpus. 'tb_size' is the size

Re: [Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Frederic Konrad]

On 12/08/2015 18:41, Paolo Bonzini wrote:
> softmmu requires more functions to be thread-safe, because translation
> blocks can be invalidated from e.g. notdirty callbacks.  Probably the
> same holds for user-mode emulation, it's just that no one has ever
> tried to produce a coherent locking there.
>
> This patch will guide the introduction of more tb_lock and tb_unlock
> calls for system emulation.
>
> Note that after this patch some (most) of the mentioned functions are
> still called outside tb_lock/tb_unlock.  The next one will rectify this.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   exec.c                  |  1 +
>   include/exec/exec-all.h |  2 ++
>   include/qom/cpu.h       |  3 +++
>   tcg/tcg.h               |  2 ++
>   translate-all.c         | 35 ++++++++++++++++++++++++++++-------
>   5 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 54cd70a..856a859 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -748,6 +748,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>   {
>       CPUBreakpoint *bp;
>   
> +    /* TODO: locking (RCU?) */
>       bp = g_malloc(sizeof(*bp));
>   
>       bp->pc = pc;
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index b3f900a..943d97a 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -74,6 +74,7 @@ typedef struct TranslationBlock TranslationBlock;
>   
>   void gen_intermediate_code(CPUArchState *env, struct TranslationBlock *tb);
>   void gen_intermediate_code_pc(CPUArchState *env, struct TranslationBlock *tb);
> +/* Called with tb_lock held.  */
>   void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb,
>                             int pc_pos);
>   
> @@ -278,6 +279,7 @@ static inline void tb_set_jmp_target(TranslationBlock *tb,
>   
>   #endif
>   
> +/* Called with tb_lock held.  */
>   static inline void tb_add_jump(TranslationBlock *tb, int n,
>                                  TranslationBlock *tb_next)
>   {
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 77bbff2..56b1f4d 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -285,7 +285,10 @@ struct CPUState {
>   
>       void *env_ptr; /* CPUArchState */
>       struct TranslationBlock *current_tb;
> +
> +    /* Protected by tb_lock.  */
>       struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
This is temporary as a first step?

> +
>       struct GDBRegisterState *gdb_regs;
>       int gdb_num_regs;
>       int gdb_num_g_regs;
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 0ae648f..a2cad31 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -590,6 +590,7 @@ static inline bool tcg_op_buf_full(void)
>   
>   /* pool based memory allocation */
>   
> +/* tb_lock must be held for tcg_malloc_internal. */
>   void *tcg_malloc_internal(TCGContext *s, int size);
>   void tcg_pool_reset(TCGContext *s);
>   void tcg_pool_delete(TCGContext *s);
> @@ -598,6 +599,7 @@ void tb_lock(void);
>   void tb_unlock(void);
>   void tb_lock_reset(void);
>   
> +/* Called with tb_lock held.  */
>   static inline void *tcg_malloc(int size)
>   {
>       TCGContext *s = &tcg_ctx;
> diff --git a/translate-all.c b/translate-all.c
> index edb9cb1..17d3cd1 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -237,6 +237,7 @@ int cpu_gen_code(CPUArchState *env, TranslationBlock *tb, int *gen_code_size_ptr
>   }
>   
>   /* The cpu state corresponding to 'searched_pc' is restored.
> + * Called with tb_lock held.
>    */
>   static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
>                                        uintptr_t searched_pc)
> @@ -424,6 +425,7 @@ static void page_init(void)
>   }
>   
>   /* If alloc=1:
> + * Called with tb_lock held for system emulation.
>    * Called with mmap_lock held for user-mode emulation.
>    */
>   static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
> @@ -734,8 +736,12 @@ bool tcg_enabled(void)
>       return tcg_ctx.code_gen_buffer != NULL;
>   }
>   
> -/* Allocate a new translation block. Flush the translation buffer if
> -   too many translation blocks or too much generated code. */
> +/*
> + * Allocate a new translation block. Flush the translation buffer if
> + * too many translation blocks or too much generated code.
> + *
> + * Called with tb_lock held.
> + */
>   static TranslationBlock *tb_alloc(target_ulong pc)
>   {
There is the famous tb_flush which needs to be called with tb_lock held 
as well.
There are several place where it's called.

>       TranslationBlock *tb;
> @@ -751,6 +757,7 @@ static TranslationBlock *tb_alloc(target_ulong pc)
>       return tb;
>   }
>   
> +/* Called with tb_lock held.  */
>   void tb_free(TranslationBlock *tb)
>   {
>       /* In practice this is mostly used for single use temporary TB
> @@ -859,7 +866,10 @@ static void tb_invalidate_check(target_ulong address)
>       }
>   }
>   
> -/* verify that all the pages have correct rights for code */
> +/* verify that all the pages have correct rights for code
> + *
> + * Called with tb_lock held.
> + */
>   static void tb_page_check(void)
>   {
>       TranslationBlock *tb;
> @@ -947,7 +957,10 @@ static inline void tb_reset_jump(TranslationBlock *tb, int n)
>       tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr + tb->tb_next_offset[n]));
>   }
>   
> -/* invalidate one TB */
> +/* invalidate one TB
> + *
> + * Called with tb_lock held.
> + */
>   void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>   {
>       CPUState *cpu;
> @@ -1036,7 +1049,7 @@ static void build_page_bitmap(PageDesc *p)
>   }
>   #endif
>   
> -/* Called with mmap_lock held for user mode emulation.  */
> +/* Called with tb_lock held, and mmap_lock too for user mode emulation.  */
>   TranslationBlock *tb_gen_code(CPUState *cpu,
>                                 target_ulong pc, target_ulong cs_base,
>                                 int flags, int cflags)
> @@ -1234,7 +1247,9 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
>       }
>       if (!p->code_bitmap &&
>           ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD) {
> -        /* build code bitmap */
> +        /* build code bitmap.  FIXME: writes should be protected by
> +         * tb_lock, reads by tb_lock or RCU.
> +         */
>           build_page_bitmap(p);
>       }
>       if (p->code_bitmap) {
> @@ -1324,6 +1339,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>   
>   /* add the tb in the target page and protect it if necessary
>    *
> + * Called with tb_lock held.
>    * Called with mmap_lock held for user-mode emulation.
>    */
>   static inline void tb_alloc_page(TranslationBlock *tb,
> @@ -1383,6 +1399,7 @@ static inline void tb_alloc_page(TranslationBlock *tb,
>   /* add a new TB and link it to the physical page tables. phys_page2 is
>    * (-1) to indicate that only one page contains the TB.
>    *
> + * Called with tb_lock held.
>    * Called with mmap_lock held for user-mode emulation.
>    */
>   static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
> @@ -1423,7 +1440,10 @@ static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>   }
>   
>   /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
> -   tb[1].tc_ptr. Return NULL if not found */
> + * tb[1].tc_ptr. Return NULL if not found
> + *
> + * Called with tb_lock held.
> + */
>   static TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
>   {
>       int m_min, m_max, m;
> @@ -1476,6 +1496,7 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr)
>   }
>   #endif /* !defined(CONFIG_USER_ONLY) */
>   
> +/* Called with tb_lock held.  */
>   void tb_check_watchpoint(CPUState *cpu)
>   {
>       TranslationBlock *tb;

Re: [Qemu-devel] [PATCH 12/10] tcg: protect TBContext with tb_lock.

[Frederic Konrad]

On 12/08/2015 18:41, Paolo Bonzini wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This protects TBContext with tb_lock to make tb_* thread safe.
>
> We can still have issue with tb_flush in case of multithread TCG:
> another CPU can be executing code during a flush.
>
> This can be fixed later by making all other TCG thread exiting before calling
> tb_flush().
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Changes:
> V6 -> V7:
>    * Drop a tb_lock in already locked restore_state_to_opc.
> V5 -> V6:
>    * Drop a tb_lock arround tb_find_fast in cpu-exec.c.
> Message-Id: <1439220437-23957-8-git-send-email-fred.konrad@greensocs.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   cpu-exec.c         |  6 ++++++
>   exec.c             |  3 +++
>   hw/i386/kvmvapic.c |  2 ++
>   translate-all.c    | 38 ++++++++++++++++++++++++++++++++------
>   4 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index e712c6a..89b66f5 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -241,16 +241,22 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
>       if (max_cycles > CF_COUNT_MASK)
>           max_cycles = CF_COUNT_MASK;
>   
> +    tb_lock();
>       tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
>                        max_cycles | CF_NOCACHE);
tb_gen_code() calls tb_alloc() which calls tb_flush() we end in a double 
tb_lock here.
But that's probably not really important here as we want to either do a 
tb_flush
outside cpu_exec or realloc an other code buffer.

Fred
>       tb->orig_tb = tcg_ctx.tb_ctx.tb_invalidated_flag ? NULL : orig_tb;
>       cpu->current_tb = tb;
> +    tb_unlock();
> +
>       /* execute the generated code */
>       trace_exec_tb_nocache(tb, tb->pc);
>       cpu_tb_exec(cpu, tb->tc_ptr);
> +
> +    tb_lock();
>       cpu->current_tb = NULL;
>       tb_phys_invalidate(tb, -1);
>       tb_free(tb);
> +    tb_unlock();
>   }
>   #endif
>   
> diff --git a/exec.c b/exec.c
> index 856a859..9083307 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1948,6 +1948,9 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
>               wp->hitattrs = attrs;
>               if (!cpu->watchpoint_hit) {
>                   cpu->watchpoint_hit = wp;
> +
> +                /* Unlocked by cpu_loop_exit or cpu_resume_from_signal.  */
> +                tb_lock();
>                   tb_check_watchpoint(cpu);
>                   if (wp->flags & BP_STOP_BEFORE_ACCESS) {
>                       cpu->exception_index = EXCP_DEBUG;
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index c6d34b2..d823e15 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -445,6 +445,8 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
>       resume_all_vcpus();
>   
>       if (!kvm_enabled()) {
> +        /* Unlocked by cpu_resume_from_signal.  */
> +        tb_lock();
>           cs->current_tb = NULL;
>           tb_gen_code(cs, current_pc, current_cs_base, current_flags, 1);
>           cpu_resume_from_signal(cs, NULL);
> diff --git a/translate-all.c b/translate-all.c
> index 17d3cd1..7a4f8f1 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -301,6 +301,8 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
>   {
>       TranslationBlock *tb;
>   
> +    tb_lock();
> +
>       tb = tb_find_pc(retaddr);
>       if (tb) {
>           cpu_restore_state_from_tb(cpu, tb, retaddr);
> @@ -310,8 +312,12 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
>               tb_phys_invalidate(tb, -1);
>               tb_free(tb);
>           }
> +
> +        tb_unlock();
>           return true;
>       }
> +
> +    tb_unlock();
>       return false;
>   }
>   
> @@ -820,6 +826,8 @@ static void page_flush_tb(void)
>   /* XXX: tb_flush is currently not thread safe */
>   void tb_flush(CPUState *cpu)
>   {
> +    tb_lock();
> +
>   #if defined(DEBUG_FLUSH)
>       printf("qemu: flush code_size=%ld nb_tbs=%d avg_tb_size=%ld\n",
>              (unsigned long)(tcg_ctx.code_gen_ptr - tcg_ctx.code_gen_buffer),
> @@ -844,6 +852,8 @@ void tb_flush(CPUState *cpu)
>       /* XXX: flush processor icache at this point if cache flush is
>          expensive */
>       tcg_ctx.tb_ctx.tb_flush_count++;
> +
> +    tb_unlock();
>   }
>   
>   #ifdef DEBUG_TB_CHECK
> @@ -1151,6 +1161,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>       /* we remove all the TBs in the range [start, end[ */
>       /* XXX: see if in some cases it could be faster to invalidate all
>          the code */
> +    tb_lock();
>       tb = p->first_tb;
>       while (tb != NULL) {
>           n = (uintptr_t)tb & 3;
> @@ -1218,12 +1229,13 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>       if (current_tb_modified) {
>           /* we generate a block containing just the instruction
>              modifying the memory. It will ensure that it cannot modify
> -           itself */
> +           itself.  cpu_resume_from_signal unlocks tb_lock.  */
>           cpu->current_tb = NULL;
>           tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1);
>           cpu_resume_from_signal(cpu, NULL);
>       }
>   #endif
> +    tb_unlock();
>   }
>   
>   #ifdef CONFIG_SOFTMMU
> @@ -1290,6 +1302,8 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>       if (!p) {
>           return;
>       }
> +
> +    tb_lock();
>       tb = p->first_tb;
>   #ifdef TARGET_HAS_PRECISE_SMC
>       if (tb && pc != 0) {
> @@ -1331,9 +1345,12 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>           if (locked) {
>               mmap_unlock();
>           }
> +
> +        /* tb_lock released by cpu_resume_from_signal.  */
>           cpu_resume_from_signal(cpu, puc);
>       }
>   #endif
> +    tb_unlock();
>   }
>   #endif
>   
> @@ -1563,6 +1580,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>       target_ulong pc, cs_base;
>       uint64_t flags;
>   
> +    tb_lock();
>       tb = tb_find_pc(retaddr);
>       if (!tb) {
>           cpu_abort(cpu, "cpu_io_recompile: could not find TB for pc=%p",
> @@ -1614,11 +1632,15 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>       /* FIXME: In theory this could raise an exception.  In practice
>          we have already translated the block once so it's probably ok.  */
>       tb_gen_code(cpu, pc, cs_base, flags, cflags);
> -    /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
> -       the first in the TB) then we end up generating a whole new TB and
> -       repeating the fault, which is horribly inefficient.
> -       Better would be to execute just this insn uncached, or generate a
> -       second new TB.  */
> +
> +    /* This unlocks the tb_lock.
> +     *
> +     * TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
> +     * the first in the TB) then we end up generating a whole new TB and
> +     * repeating the fault, which is horribly inefficient.
> +     * Better would be to execute just this insn uncached, or generate a
> +     * second new TB.
> +     */
>       cpu_resume_from_signal(cpu, NULL);
>   }
>   
> @@ -1643,6 +1665,8 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
>       int direct_jmp_count, direct_jmp2_count, cross_page;
>       TranslationBlock *tb;
>   
> +    tb_lock();
> +
>       target_code_size = 0;
>       max_target_code_size = 0;
>       cross_page = 0;
> @@ -1698,6 +1722,8 @@ void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
>               tcg_ctx.tb_ctx.tb_phys_invalidate_count);
>       cpu_fprintf(f, "TLB flush count     %d\n", tlb_flush_count);
>       tcg_dump_info(f, cpu_fprintf);
> +
> +    tb_unlock();
>   }
>   
>   void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf)

Re: [Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Paolo Bonzini]

On 13/08/2015 14:51, Frederic Konrad wrote:
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 77bbff2..56b1f4d 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -285,7 +285,10 @@ struct CPUState {
>>         void *env_ptr; /* CPUArchState */
>>       struct TranslationBlock *current_tb;
>> +
>> +    /* Protected by tb_lock.  */
>>       struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
> This is temporary as a first step?

Yes, I now saw that tb_lock has a huge contention in tb_find_fast. :)

I've now extracted parts of your patch "tcg: protect TBContext with
tb_lock" into a separate "tcg: move tb_find_fast outside the tb_lock
critical section" that also applies to user-mode emulation.  That way I
get good scalability on Dhrystone, same as with your branch.

Do you agree with the first 10 patches as a first step towards
upstreaming the MTTCG work?

Paolo

>> +
>>       struct GDBRegisterState *gdb_regs;
>>       int gdb_num_regs;
>>       int gdb_num_g_regs;
>> diff --git a/tcg/tcg.h b/tcg/tcg.h
>> index 0ae648f..a2cad31 100644
>> --- a/tcg/tcg.h
>> +++ b/tcg/tcg.h
>> @@ -590,6 +590,7 @@ static inline bool tcg_op_buf_full(void)
>>     /* pool based memory allocation */
>>   +/* tb_lock must be held for tcg_malloc_internal. */
>>   void *tcg_malloc_internal(TCGContext *s, int size);
>>   void tcg_pool_reset(TCGContext *s);
>>   void tcg_pool_delete(TCGContext *s);
>> @@ -598,6 +599,7 @@ void tb_lock(void);
>>   void tb_unlock(void);
>>   void tb_lock_reset(void);
>>   +/* Called with tb_lock held.  */
>>   static inline void *tcg_malloc(int size)
>>   {
>>       TCGContext *s = &tcg_ctx;
>> diff --git a/translate-all.c b/translate-all.c
>> index edb9cb1..17d3cd1 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -237,6 +237,7 @@ int cpu_gen_code(CPUArchState *env,
>> TranslationBlock *tb, int *gen_code_size_ptr
>>   }
>>     /* The cpu state corresponding to 'searched_pc' is restored.
>> + * Called with tb_lock held.
>>    */
>>   static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock
>> *tb,
>>                                        uintptr_t searched_pc)
>> @@ -424,6 +425,7 @@ static void page_init(void)
>>   }
>>     /* If alloc=1:
>> + * Called with tb_lock held for system emulation.
>>    * Called with mmap_lock held for user-mode emulation.
>>    */
>>   static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>> @@ -734,8 +736,12 @@ bool tcg_enabled(void)
>>       return tcg_ctx.code_gen_buffer != NULL;
>>   }
>>   -/* Allocate a new translation block. Flush the translation buffer if
>> -   too many translation blocks or too much generated code. */
>> +/*
>> + * Allocate a new translation block. Flush the translation buffer if
>> + * too many translation blocks or too much generated code.
>> + *
>> + * Called with tb_lock held.
>> + */
>>   static TranslationBlock *tb_alloc(target_ulong pc)
>>   {
> There is the famous tb_flush which needs to be called with tb_lock held
> as well.
> There are several place where it's called.
> 
>>       TranslationBlock *tb;
>> @@ -751,6 +757,7 @@ static TranslationBlock *tb_alloc(target_ulong pc)
>>       return tb;
>>   }
>>   +/* Called with tb_lock held.  */
>>   void tb_free(TranslationBlock *tb)
>>   {
>>       /* In practice this is mostly used for single use temporary TB
>> @@ -859,7 +866,10 @@ static void tb_invalidate_check(target_ulong
>> address)
>>       }
>>   }
>>   -/* verify that all the pages have correct rights for code */
>> +/* verify that all the pages have correct rights for code
>> + *
>> + * Called with tb_lock held.
>> + */
>>   static void tb_page_check(void)
>>   {
>>       TranslationBlock *tb;
>> @@ -947,7 +957,10 @@ static inline void tb_reset_jump(TranslationBlock
>> *tb, int n)
>>       tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr +
>> tb->tb_next_offset[n]));
>>   }
>>   -/* invalidate one TB */
>> +/* invalidate one TB
>> + *
>> + * Called with tb_lock held.
>> + */
>>   void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>>   {
>>       CPUState *cpu;
>> @@ -1036,7 +1049,7 @@ static void build_page_bitmap(PageDesc *p)
>>   }
>>   #endif
>>   -/* Called with mmap_lock held for user mode emulation.  */
>> +/* Called with tb_lock held, and mmap_lock too for user mode
>> emulation.  */
>>   TranslationBlock *tb_gen_code(CPUState *cpu,
>>                                 target_ulong pc, target_ulong cs_base,
>>                                 int flags, int cflags)
>> @@ -1234,7 +1247,9 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t
>> start, int len)
>>       }
>>       if (!p->code_bitmap &&
>>           ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD) {
>> -        /* build code bitmap */
>> +        /* build code bitmap.  FIXME: writes should be protected by
>> +         * tb_lock, reads by tb_lock or RCU.
>> +         */
>>           build_page_bitmap(p);
>>       }
>>       if (p->code_bitmap) {
>> @@ -1324,6 +1339,7 @@ static void
>> tb_invalidate_phys_page(tb_page_addr_t addr,
>>     /* add the tb in the target page and protect it if necessary
>>    *
>> + * Called with tb_lock held.
>>    * Called with mmap_lock held for user-mode emulation.
>>    */
>>   static inline void tb_alloc_page(TranslationBlock *tb,
>> @@ -1383,6 +1399,7 @@ static inline void
>> tb_alloc_page(TranslationBlock *tb,
>>   /* add a new TB and link it to the physical page tables. phys_page2 is
>>    * (-1) to indicate that only one page contains the TB.
>>    *
>> + * Called with tb_lock held.
>>    * Called with mmap_lock held for user-mode emulation.
>>    */
>>   static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>> @@ -1423,7 +1440,10 @@ static void tb_link_page(TranslationBlock *tb,
>> tb_page_addr_t phys_pc,
>>   }
>>     /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
>> -   tb[1].tc_ptr. Return NULL if not found */
>> + * tb[1].tc_ptr. Return NULL if not found
>> + *
>> + * Called with tb_lock held.
>> + */
>>   static TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
>>   {
>>       int m_min, m_max, m;
>> @@ -1476,6 +1496,7 @@ void tb_invalidate_phys_addr(AddressSpace *as,
>> hwaddr addr)
>>   }
>>   #endif /* !defined(CONFIG_USER_ONLY) */
>>   +/* Called with tb_lock held.  */
>>   void tb_check_watchpoint(CPUState *cpu)
>>   {
>>       TranslationBlock *tb;
> 

Re: [Qemu-devel] [PATCH 12/10] tcg: protect TBContext with tb_lock.

[Paolo Bonzini]

> > +    tb_lock();
> >       tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
> >                        max_cycles | CF_NOCACHE);
> 
> tb_gen_code() calls tb_alloc() which calls tb_flush() we end in a double
> tb_lock here.
> But that's probably not really important here as we want to either do a
> tb_flush outside cpu_exec or realloc an other code buffer.

You're right!  Honestly I haven't tested tb_flush() at all with these
patches since it's documented as broken with multiple threads.

Luckily the bug is not in the first 10 patches. :)

Paolo

Re: [Qemu-devel] [PATCH 12/10] tcg: protect TBContext with tb_lock.

[Frederic Konrad]

On 13/08/2015 15:01, Paolo Bonzini wrote:
>>> +    tb_lock();
>>>        tb = tb_gen_code(cpu, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
>>>                         max_cycles | CF_NOCACHE);
>> tb_gen_code() calls tb_alloc() which calls tb_flush() we end in a double
>> tb_lock here.
>> But that's probably not really important here as we want to either do a
>> tb_flush outside cpu_exec or realloc an other code buffer.
> You're right!  Honestly I haven't tested tb_flush() at all with these
> patches since it's documented as broken with multiple threads.
>
> Luckily the bug is not in the first 10 patches. :)
Fortunately this revealed my yesterday bug with tb_alloc :).

Fred
>
> Paolo

Re: [Qemu-devel] [PATCH 02/10] cpus: remove tcg_halt_cond global variable.

[Frederic Konrad]

On 12/08/2015 18:40, Paolo Bonzini wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This removes tcg_halt_cond global variable.
> We need one QemuCond per virtual cpu for multithread TCG.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Message-Id: <1439220437-23957-9-git-send-email-fred.konrad@greensocs.com>
> [Keep tcg_halt_cond for bisectability, while making it static. - Paolo]
How does that help bisectability?

Fred
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   cpus.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 9224488..8884278 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -813,7 +813,6 @@ static unsigned iothread_requesting_mutex;
>   static QemuThread io_thread;
>   
>   static QemuThread *tcg_cpu_thread;
> -static QemuCond *tcg_halt_cond;
>   
>   /* cpu creation */
>   static QemuCond qemu_cpu_cond;
> @@ -933,15 +932,13 @@ static void qemu_wait_io_event_common(CPUState *cpu)
>       cpu->thread_kicked = false;
>   }
>   
> -static void qemu_tcg_wait_io_event(void)
> +static void qemu_tcg_wait_io_event(CPUState *cpu)
>   {
> -    CPUState *cpu;
> -
>       while (all_cpu_threads_idle()) {
>          /* Start accounting real time to the virtual clock if the CPUs
>             are idle.  */
>           qemu_clock_warp(QEMU_CLOCK_VIRTUAL);
> -        qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
> +        qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
>       }
>   
>       while (iothread_requesting_mutex) {
> @@ -1067,7 +1064,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>   
>       /* wait for initial kick-off after machine start */
>       while (first_cpu->stopped) {
> -        qemu_cond_wait(tcg_halt_cond, &qemu_global_mutex);
> +        qemu_cond_wait(first_cpu->halt_cond, &qemu_global_mutex);
>   
>           /* process any pending work */
>           CPU_FOREACH(cpu) {
> @@ -1088,7 +1085,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>                   qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
>               }
>           }
> -        qemu_tcg_wait_io_event();
> +        qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
>       }
>   
>       return NULL;
> @@ -1265,6 +1262,7 @@ void resume_all_vcpus(void)
>   static void qemu_tcg_init_vcpu(CPUState *cpu)
>   {
>       char thread_name[VCPU_THREAD_NAME_SIZE];
> +    static QemuCond *tcg_halt_cond;
>   
>       tcg_cpu_address_space_init(cpu, cpu->as);
>   

Re: [Qemu-devel] [PATCH 02/10] cpus: remove tcg_halt_cond global variable.

[Paolo Bonzini]

On 13/08/2015 15:05, Frederic Konrad wrote:
>>
>>
>> This removes tcg_halt_cond global variable.
>> We need one QemuCond per virtual cpu for multithread TCG.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> Message-Id: <1439220437-23957-9-git-send-email-fred.konrad@greensocs.com>
>> [Keep tcg_halt_cond for bisectability, while making it static. - Paolo]
> How does that help bisectability?

With your patch (08/19), QEMU will only wait on first_cpu->halt_cond but
will call broadcast on cpu->halt_cond.  Here I do the opposite: I wait
on cpu->halt_cond from some random CPU, but all of them point to the
same condvar tcg_halt_cond.

Paolo

Re: [Qemu-devel] [PATCH 03/10] replace spinlock by QemuMutex.

[Paolo Bonzini]

On 13/08/2015 14:17, Frederic Konrad wrote:
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index fdee981..fd06ce9 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -107,7 +107,7 @@ static int pending_cpus;
>>   /* Make sure everything is in a consistent state for calling
>> fork().  */
>>   void fork_start(void)
>>   {
>> -    pthread_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
>> +    qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
>>       pthread_mutex_lock(&exclusive_lock);
>>       mmap_fork_start();
>>   }
>> @@ -129,11 +129,11 @@ void fork_end(int child)
>>           pthread_mutex_init(&cpu_list_mutex, NULL);
>>           pthread_cond_init(&exclusive_cond, NULL);
>>           pthread_cond_init(&exclusive_resume, NULL);
>> -        pthread_mutex_init(&tcg_ctx.tb_ctx.tb_lock, NULL);
>> +        qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
>>           gdbserver_fork(thread_cpu);
>>       } else {
>>           pthread_mutex_unlock(&exclusive_lock);
>> -        pthread_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
>> +        qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
> We might want to use tb_lock/unlock in user code as well instead of
> calling directly qemu_mutex_* ?

You cannot do that because of the recursive locking assertions; the
child is not using qemu_mutex_unlock, it's using qemu_mutex_init.  So I
would have to add some kind of tb_lock_reset_after_fork() function which
is a bit ugly.

>> @@ -676,6 +709,7 @@ static inline void code_gen_alloc(size_t tb_size)
>>               CODE_GEN_AVG_BLOCK_SIZE;
>>       tcg_ctx.tb_ctx.tbs =
>>               g_malloc(tcg_ctx.code_gen_max_blocks *
>> sizeof(TranslationBlock));
>> +    qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
> Maybe we can initialize the mutex only for CONFIG_USER_ONLY?

It's okay, it doesn't consume system resources.

Paolo

Re: [Qemu-devel] [PATCH 05/10] cpu-exec: elide more icount code if CONFIG_USER_ONLY

[Frederic Konrad]

On 12/08/2015 18:40, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   cpu-exec.c | 6 ++++++
>   1 file changed, 6 insertions(+)
What about the icount part in CPUState and the tb_start/end ?
Can't this be removed as well?

Fred
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 599e64d..bde5fd1 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -228,6 +228,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, uint8_t *tb_ptr)
>       return next_tb;
>   }
>   
> +#if defined(CONFIG_SOFTMMU)
>   /* Execute the code without caching the generated code. An interpreter
>      could be used if available. */
>   static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
> @@ -251,6 +252,7 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
>       tb_phys_invalidate(tb, -1);
>       tb_free(tb);
>   }
> +#endif
>   
>   static TranslationBlock *tb_find_slow(CPUState *cpu,
>                                         target_ulong pc,
> @@ -523,6 +525,9 @@ int cpu_exec(CPUState *cpu)
>                       case TB_EXIT_ICOUNT_EXPIRED:
>                       {
>                           /* Instruction counter expired.  */
> +#ifdef CONFIG_USER_ONLY
> +                        abort();
> +#else
>                           int insns_left = cpu->icount_decr.u32;
>                           if (cpu->icount_extra && insns_left >= 0) {
>                               /* Refill decrementer and continue execution.  */
> @@ -542,6 +547,7 @@ int cpu_exec(CPUState *cpu)
>                               cpu_loop_exit(cpu);
>                           }
>                           break;
> +#endif
>                       }
>                       default:
>                           break;

Re: [Qemu-devel] [PATCH 02/10] cpus: remove tcg_halt_cond global variable.

[Frederic Konrad]

On 13/08/2015 15:08, Paolo Bonzini wrote:
>
> On 13/08/2015 15:05, Frederic Konrad wrote:
>>>
>>> This removes tcg_halt_cond global variable.
>>> We need one QemuCond per virtual cpu for multithread TCG.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> Message-Id: <1439220437-23957-9-git-send-email-fred.konrad@greensocs.com>
>>> [Keep tcg_halt_cond for bisectability, while making it static. - Paolo]
>> How does that help bisectability?
> With your patch (08/19), QEMU will only wait on first_cpu->halt_cond but
> will call broadcast on cpu->halt_cond.  Here I do the opposite: I wait
> on cpu->halt_cond from some random CPU, but all of them point to the
> same condvar tcg_halt_cond.
>
> Paolo
Ok got it.

Fred

Re: [Qemu-devel] [PATCH 03/10] replace spinlock by QemuMutex.

[Frederic Konrad]

On 13/08/2015 15:12, Paolo Bonzini wrote:
>
> On 13/08/2015 14:17, Frederic Konrad wrote:
>>> diff --git a/linux-user/main.c b/linux-user/main.c
>>> index fdee981..fd06ce9 100644
>>> --- a/linux-user/main.c
>>> +++ b/linux-user/main.c
>>> @@ -107,7 +107,7 @@ static int pending_cpus;
>>>    /* Make sure everything is in a consistent state for calling
>>> fork().  */
>>>    void fork_start(void)
>>>    {
>>> -    pthread_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
>>> +    qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
>>>        pthread_mutex_lock(&exclusive_lock);
>>>        mmap_fork_start();
>>>    }
>>> @@ -129,11 +129,11 @@ void fork_end(int child)
>>>            pthread_mutex_init(&cpu_list_mutex, NULL);
>>>            pthread_cond_init(&exclusive_cond, NULL);
>>>            pthread_cond_init(&exclusive_resume, NULL);
>>> -        pthread_mutex_init(&tcg_ctx.tb_ctx.tb_lock, NULL);
>>> +        qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
>>>            gdbserver_fork(thread_cpu);
>>>        } else {
>>>            pthread_mutex_unlock(&exclusive_lock);
>>> -        pthread_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
>>> +        qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
>> We might want to use tb_lock/unlock in user code as well instead of
>> calling directly qemu_mutex_* ?
> You cannot do that because of the recursive locking assertions; the
> child is not using qemu_mutex_unlock, it's using qemu_mutex_init.  So I
> would have to add some kind of tb_lock_reset_after_fork() function which
> is a bit ugly.

True.

Fred
>>> @@ -676,6 +709,7 @@ static inline void code_gen_alloc(size_t tb_size)
>>>                CODE_GEN_AVG_BLOCK_SIZE;
>>>        tcg_ctx.tb_ctx.tbs =
>>>                g_malloc(tcg_ctx.code_gen_max_blocks *
>>> sizeof(TranslationBlock));
>>> +    qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock);
>> Maybe we can initialize the mutex only for CONFIG_USER_ONLY?
> It's okay, it doesn't consume system resources.
>
> Paolo

Re: [Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Frederic Konrad]

On 13/08/2015 14:59, Paolo Bonzini wrote:
>
> On 13/08/2015 14:51, Frederic Konrad wrote:
>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>> index 77bbff2..56b1f4d 100644
>>> --- a/include/qom/cpu.h
>>> +++ b/include/qom/cpu.h
>>> @@ -285,7 +285,10 @@ struct CPUState {
>>>          void *env_ptr; /* CPUArchState */
>>>        struct TranslationBlock *current_tb;
>>> +
>>> +    /* Protected by tb_lock.  */
>>>        struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
>> This is temporary as a first step?
> Yes, I now saw that tb_lock has a huge contention in tb_find_fast. :)
Yes it is just enormous. Makes MTTCG 2x slower than upstream :).

> I've now extracted parts of your patch "tcg: protect TBContext with
> tb_lock" into a separate "tcg: move tb_find_fast outside the tb_lock
> critical section" that also applies to user-mode emulation.  That way I
> get good scalability on Dhrystone, same as with your branch.

I guess with the whole tlb/tb flush safe? Which is theorically protecting
tb_jmp_cache (or at least let only the right thread accessing it).
The drawback of all that is I'm not sure this is faster when we have a 
lot of
context switches. For tb_flush it's not really a problem as it happen 
approximately
never but the tb_invalidate, tlb_*_flush are more regular.

Fred
>
> Do you agree with the first 10 patches as a first step towards
> upstreaming the MTTCG work?
>
> Paolo
>
>>> +
>>>        struct GDBRegisterState *gdb_regs;
>>>        int gdb_num_regs;
>>>        int gdb_num_g_regs;
>>> diff --git a/tcg/tcg.h b/tcg/tcg.h
>>> index 0ae648f..a2cad31 100644
>>> --- a/tcg/tcg.h
>>> +++ b/tcg/tcg.h
>>> @@ -590,6 +590,7 @@ static inline bool tcg_op_buf_full(void)
>>>      /* pool based memory allocation */
>>>    +/* tb_lock must be held for tcg_malloc_internal. */
>>>    void *tcg_malloc_internal(TCGContext *s, int size);
>>>    void tcg_pool_reset(TCGContext *s);
>>>    void tcg_pool_delete(TCGContext *s);
>>> @@ -598,6 +599,7 @@ void tb_lock(void);
>>>    void tb_unlock(void);
>>>    void tb_lock_reset(void);
>>>    +/* Called with tb_lock held.  */
>>>    static inline void *tcg_malloc(int size)
>>>    {
>>>        TCGContext *s = &tcg_ctx;
>>> diff --git a/translate-all.c b/translate-all.c
>>> index edb9cb1..17d3cd1 100644
>>> --- a/translate-all.c
>>> +++ b/translate-all.c
>>> @@ -237,6 +237,7 @@ int cpu_gen_code(CPUArchState *env,
>>> TranslationBlock *tb, int *gen_code_size_ptr
>>>    }
>>>      /* The cpu state corresponding to 'searched_pc' is restored.
>>> + * Called with tb_lock held.
>>>     */
>>>    static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock
>>> *tb,
>>>                                         uintptr_t searched_pc)
>>> @@ -424,6 +425,7 @@ static void page_init(void)
>>>    }
>>>      /* If alloc=1:
>>> + * Called with tb_lock held for system emulation.
>>>     * Called with mmap_lock held for user-mode emulation.
>>>     */
>>>    static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>>> @@ -734,8 +736,12 @@ bool tcg_enabled(void)
>>>        return tcg_ctx.code_gen_buffer != NULL;
>>>    }
>>>    -/* Allocate a new translation block. Flush the translation buffer if
>>> -   too many translation blocks or too much generated code. */
>>> +/*
>>> + * Allocate a new translation block. Flush the translation buffer if
>>> + * too many translation blocks or too much generated code.
>>> + *
>>> + * Called with tb_lock held.
>>> + */
>>>    static TranslationBlock *tb_alloc(target_ulong pc)
>>>    {
>> There is the famous tb_flush which needs to be called with tb_lock held
>> as well.
>> There are several place where it's called.
>>
>>>        TranslationBlock *tb;
>>> @@ -751,6 +757,7 @@ static TranslationBlock *tb_alloc(target_ulong pc)
>>>        return tb;
>>>    }
>>>    +/* Called with tb_lock held.  */
>>>    void tb_free(TranslationBlock *tb)
>>>    {
>>>        /* In practice this is mostly used for single use temporary TB
>>> @@ -859,7 +866,10 @@ static void tb_invalidate_check(target_ulong
>>> address)
>>>        }
>>>    }
>>>    -/* verify that all the pages have correct rights for code */
>>> +/* verify that all the pages have correct rights for code
>>> + *
>>> + * Called with tb_lock held.
>>> + */
>>>    static void tb_page_check(void)
>>>    {
>>>        TranslationBlock *tb;
>>> @@ -947,7 +957,10 @@ static inline void tb_reset_jump(TranslationBlock
>>> *tb, int n)
>>>        tb_set_jmp_target(tb, n, (uintptr_t)(tb->tc_ptr +
>>> tb->tb_next_offset[n]));
>>>    }
>>>    -/* invalidate one TB */
>>> +/* invalidate one TB
>>> + *
>>> + * Called with tb_lock held.
>>> + */
>>>    void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr)
>>>    {
>>>        CPUState *cpu;
>>> @@ -1036,7 +1049,7 @@ static void build_page_bitmap(PageDesc *p)
>>>    }
>>>    #endif
>>>    -/* Called with mmap_lock held for user mode emulation.  */
>>> +/* Called with tb_lock held, and mmap_lock too for user mode
>>> emulation.  */
>>>    TranslationBlock *tb_gen_code(CPUState *cpu,
>>>                                  target_ulong pc, target_ulong cs_base,
>>>                                  int flags, int cflags)
>>> @@ -1234,7 +1247,9 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t
>>> start, int len)
>>>        }
>>>        if (!p->code_bitmap &&
>>>            ++p->code_write_count >= SMC_BITMAP_USE_THRESHOLD) {
>>> -        /* build code bitmap */
>>> +        /* build code bitmap.  FIXME: writes should be protected by
>>> +         * tb_lock, reads by tb_lock or RCU.
>>> +         */
>>>            build_page_bitmap(p);
>>>        }
>>>        if (p->code_bitmap) {
>>> @@ -1324,6 +1339,7 @@ static void
>>> tb_invalidate_phys_page(tb_page_addr_t addr,
>>>      /* add the tb in the target page and protect it if necessary
>>>     *
>>> + * Called with tb_lock held.
>>>     * Called with mmap_lock held for user-mode emulation.
>>>     */
>>>    static inline void tb_alloc_page(TranslationBlock *tb,
>>> @@ -1383,6 +1399,7 @@ static inline void
>>> tb_alloc_page(TranslationBlock *tb,
>>>    /* add a new TB and link it to the physical page tables. phys_page2 is
>>>     * (-1) to indicate that only one page contains the TB.
>>>     *
>>> + * Called with tb_lock held.
>>>     * Called with mmap_lock held for user-mode emulation.
>>>     */
>>>    static void tb_link_page(TranslationBlock *tb, tb_page_addr_t phys_pc,
>>> @@ -1423,7 +1440,10 @@ static void tb_link_page(TranslationBlock *tb,
>>> tb_page_addr_t phys_pc,
>>>    }
>>>      /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
>>> -   tb[1].tc_ptr. Return NULL if not found */
>>> + * tb[1].tc_ptr. Return NULL if not found
>>> + *
>>> + * Called with tb_lock held.
>>> + */
>>>    static TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
>>>    {
>>>        int m_min, m_max, m;
>>> @@ -1476,6 +1496,7 @@ void tb_invalidate_phys_addr(AddressSpace *as,
>>> hwaddr addr)
>>>    }
>>>    #endif /* !defined(CONFIG_USER_ONLY) */
>>>    +/* Called with tb_lock held.  */
>>>    void tb_check_watchpoint(CPUState *cpu)
>>>    {
>>>        TranslationBlock *tb;

Re: [Qemu-devel] [PATCH 05/10] cpu-exec: elide more icount code if CONFIG_USER_ONLY

[Paolo Bonzini]

On 13/08/2015 15:14, Frederic Konrad wrote:
> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   cpu-exec.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
> What about the icount part in CPUState and the tb_start/end ?
> Can't this be removed as well?

Yes, here I'm just dropping the parts that needs to be adjusted for
multi-threading---and won't be until mttcg goes in.

Paolo

Re: [Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Paolo Bonzini]

On 13/08/2015 15:32, Frederic Konrad wrote:
>> I've now extracted parts of your patch "tcg: protect TBContext with
>> tb_lock" into a separate "tcg: move tb_find_fast outside the tb_lock
>> critical section" that also applies to user-mode emulation.  That way I
>> get good scalability on Dhrystone, same as with your branch.
> 
> I guess with the whole tlb/tb flush safe?

Yes, that should go before the lock-free tb_find_fast.

> Which is theorically
> protecting tb_jmp_cache (or at least let only the right thread
> accessing it). The drawback of all that is I'm not sure this is
> faster when we have a lot of context switches. For tb_flush it's not
> really a problem as it happen approximately never but the
> tb_invalidate, tlb_*_flush are more regular.

TBs are physically-tagged so invalidates are not too frequent unless the
guest does self-modifying code or swaps.  They shouldn't be a source of
tb_lock contention except if you do SMC (not just dynamic recompilation:
really self-modifying code).

TBs are a good match for RCU overall.  TB data is immutable if you
sacrifice the tb_phys_hash optimization, so there's no need to copy
anything, hence modifications to the lists are rare and reclamations
(tb_flush) are extremely rare.

TLB shootdowns of course will slow down things, but those are a separate
issue and we don't really care: the ARM flush-all-CPUs is probably
faster than an IPI anyway.

Paolo

Re: [Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Peter Maydell]

On 13 August 2015 at 15:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> TBs are physically-tagged so invalidates are not too frequent unless the
> guest does self-modifying code or swaps.  They shouldn't be a source of
> tb_lock contention except if you do SMC (not just dynamic recompilation:
> really self-modifying code).

Do guests with writeable data and code in the same page also
get treated as SMC for these purposes? (That's more common
than real SMC.)

-- PMM

Re: [Qemu-devel] [PATCH 11/10] tcg: comment on which functions have to be called with tb_lock held

[Paolo Bonzini]

On 13/08/2015 17:32, Peter Maydell wrote:
>> > TBs are physically-tagged so invalidates are not too frequent unless the
>> > guest does self-modifying code or swaps.  They shouldn't be a source of
>> > tb_lock contention except if you do SMC (not just dynamic recompilation:
>> > really self-modifying code).
> Do guests with writeable data and code in the same page also
> get treated as SMC for these purposes? (That's more common
> than real SMC.)

Not after a short while, when the code_bitmap kicks in.

Paolo

Re: [Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses

[Emilio G. Cota]

On Thu, Aug 13, 2015 at 10:13:32 +0200, Paolo Bonzini wrote:
> On 12/08/2015 22:37, Emilio G. Cota wrote:
> > > page_find is reading the radix tree outside all locks, so it has to
> > > use the RCU primitives.  It does not need RCU critical sections
> > > because the PageDescs are never removed, so there is never a need
> > > to wait for the end of code sections that use a PageDesc.
> >
> > Note that rcu_find_alloc might end up writing to the tree, see below.
> 
> Yes, but in that case it's always called with the mmap_lock held, see
> patch 7.

Oh I see. Didn't have much time to take a deep look at the patchset; the
commit message got me confused.

> page_find_alloc is only called by tb_alloc_page (called by tb_link_page
> which takes mmap_lock), or by page_set_flags (called with mmap_lock held
> by linux-user/mmap.c).
> 
> > BTW the fact that there are no removals makes the use of RCU unnecessary.
> 
> It only makes it not use the RCU synchronization primitives.  You still
> need the memory barriers.

Yes. Personally I find it confusing to use the RCU macros just
for the convenience that they bring in barriers we need; I'd prefer to
explicitly have the barriers in the code.

> > I argue however that it is better to call page_find/_alloc with a mutex held,
> > since otherwise we'd have to add per-PageDesc locks (it's very common to
> > call page_find and then update the PageDesc). 
> 
> The fields are protected by either the mmap_lock (e.g. the flags, see
> page_unprotect and tb_alloc_page) or the tb_lock (e.g. the tb lists).
> 
> The code is complicated and could definitely use more documentation,
> especially for struct PageDesc, but it seems correct to me apart from
> the lock inversion fixed in patch 10.

OK. I have a bit of time today and tomorrow so I'll rebase my work on
top of this patchset and then submit it.

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH 00/10] translate-all.c thread-safety

[Frederic Konrad]

On 12/08/2015 18:40, Paolo Bonzini wrote:
> Hi, this is my attempt at 1) extracting upstreamable parts out of Fred's
> MTTCG,

Can you take this one as well after the replace spinlock by QemuMutex:
"remove unused spinlock."

Thanks,
Fred
>   and 2) documenting what's going on in user-mode MTTCG 3) fix
> one bug in the process.  I couldn't find any other locking problem
> from reading the code.
>
> The final two patches are not really upstreamable because they
> add some still unnecessary locks to system emulation, but I included
> them to show what's going on.  With this locking logic I do not need
> tb_lock to be recursive anymore.
>
> Paolo
>
> KONRAD Frederic (4):
>    cpus: protect work list with work_mutex
>    cpus: remove tcg_halt_cond global variable.
>    replace spinlock by QemuMutex.
>    tcg: protect TBContext with tb_lock.
>
> Paolo Bonzini (8):
>    exec-all: remove non-TCG stuff from exec-all.h header.
>    cpu-exec: elide more icount code if CONFIG_USER_ONLY
>    tcg: code_bitmap is not used by user-mode emulation
>    tcg: comment on which functions have to be called with mmap_lock held
>    tcg: add memory barriers in page_find_alloc accesses
>    exec: make mmap_lock/mmap_unlock globally available
>    cpu-exec: fix lock hierarchy for user-mode emulation
>    tcg: comment on which functions have to be called with tb_lock held
>
>   bsd-user/qemu.h          |   2 -
>   cpu-exec.c               | 107 +++++++++++++++++++++----------
>   cpus.c                   |  34 ++++++----
>   exec.c                   |   4 ++
>   hw/i386/kvmvapic.c       |   2 +
>   include/exec/exec-all.h  |  19 +++---
>   include/exec/ram_addr.h  |   1 +
>   include/qom/cpu.h        |   9 ++-
>   include/sysemu/sysemu.h  |   3 +
>   linux-user/main.c        |   6 +-
>   linux-user/qemu.h        |   2 -
>   qom/cpu.c                |   1 +
>   target-i386/cpu.h        |   3 +
>   target-i386/mem_helper.c |  25 +++++++-
>   target-i386/translate.c  |   2 +
>   tcg/tcg.h                |   6 ++
>   translate-all.c          | 161 +++++++++++++++++++++++++++++++++++++----------
>   17 files changed, 290 insertions(+), 97 deletions(-)
>

Re: [Qemu-devel] [PATCH 01/10] cpus: protect work list with work_mutex

[Peter Maydell]

On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Protect the list of queued work items with something other than
> the BQL, as a preparation for running the work items outside it.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/10] cpus: remove tcg_halt_cond global variable.

[Peter Maydell]

On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This removes tcg_halt_cond global variable.
> We need one QemuCond per virtual cpu for multithread TCG.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Message-Id: <1439220437-23957-9-git-send-email-fred.konrad@greensocs.com>
> [Keep tcg_halt_cond for bisectability, while making it static. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpus.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 9224488..8884278 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -813,7 +813,6 @@ static unsigned iothread_requesting_mutex;
>  static QemuThread io_thread;
>
>  static QemuThread *tcg_cpu_thread;
> -static QemuCond *tcg_halt_cond;
>

> @@ -1265,6 +1262,7 @@ void resume_all_vcpus(void)
>  static void qemu_tcg_init_vcpu(CPUState *cpu)
>  {
>      char thread_name[VCPU_THREAD_NAME_SIZE];
> +    static QemuCond *tcg_halt_cond;
>
>      tcg_cpu_address_space_init(cpu, cpu->as);

With this patch, code-wise tcg_halt_cond and tcg_cpu_thread
are used in pretty much parallel ways (first call into
qemu_tcg_init_vcpu() sets them up, all the rest just copy
them into the CPU struct). The only difference is that one
of them a static at file scope and the other one is a static
at function scope. It seems a shame to not have them be
exactly parallel...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/10] replace spinlock by QemuMutex.

[Peter Maydell]

On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> spinlock is only used in two cases:
>   * cpu-exec.c: to protect TranslationBlock
>   * mem_helper.c: for lock helper in target-i386 (which seems broken).
>
> It's a pthread_mutex_t in user-mode so better using QemuMutex directly in this
> case.
> It allows as well to reuse tb_lock mutex of TBContext in case of multithread
> TCG.

The line wrapping in this commit message (and the grammar) looks a bit off...

> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> Message-Id: <1439220437-23957-5-git-send-email-fred.konrad@greensocs.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpu-exec.c               | 15 +++------------
>  include/exec/exec-all.h  |  4 ++--
>  linux-user/main.c        |  6 +++---
>  target-i386/cpu.h        |  3 +++
>  target-i386/mem_helper.c | 25 ++++++++++++++++++++++---
>  target-i386/translate.c  |  2 ++
>  tcg/tcg.h                |  4 ++++
>  translate-all.c          | 34 ++++++++++++++++++++++++++++++++++
>  8 files changed, 73 insertions(+), 20 deletions(-)

After this commit it looks like we have no users of spinlock_t
at all. It would be good to have a followup patch which deleted
include/exec/spinlock.h.

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/10] replace spinlock by QemuMutex.

[Frederic Konrad]

On 28/08/2015 16:49, Peter Maydell wrote:
> On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> spinlock is only used in two cases:
>>    * cpu-exec.c: to protect TranslationBlock
>>    * mem_helper.c: for lock helper in target-i386 (which seems broken).
>>
>> It's a pthread_mutex_t in user-mode so better using QemuMutex directly in this
>> case.
>> It allows as well to reuse tb_lock mutex of TBContext in case of multithread
>> TCG.
> The line wrapping in this commit message (and the grammar) looks a bit off...
>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> Message-Id: <1439220437-23957-5-git-send-email-fred.konrad@greensocs.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   cpu-exec.c               | 15 +++------------
>>   include/exec/exec-all.h  |  4 ++--
>>   linux-user/main.c        |  6 +++---
>>   target-i386/cpu.h        |  3 +++
>>   target-i386/mem_helper.c | 25 ++++++++++++++++++++++---
>>   target-i386/translate.c  |  2 ++
>>   tcg/tcg.h                |  4 ++++
>>   translate-all.c          | 34 ++++++++++++++++++++++++++++++++++
>>   8 files changed, 73 insertions(+), 20 deletions(-)
> After this commit it looks like we have no users of spinlock_t
> at all. It would be good to have a followup patch which deleted
> include/exec/spinlock.h.

Seems Paolo forget to pick up the following patch from the mttcg tree:
"remove unused spinlock."
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg01129.html

Thanks,
Fred
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 04/10] exec-all: remove non-TCG stuff from exec-all.h header.

[Peter Maydell]

On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The header is included from basically everywhere, thanks to cpu.h.
> It should be moved to the (TCG only) files that actually need it.
> As a start, remove non-TCG stuff.
>
> While adding a #ifndef CONFIG_USER_ONLY include section to cpu-exec.c,
> move memory API files under it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -22,12 +22,15 @@
>  #include "disas/disas.h"
>  #include "tcg.h"
>  #include "qemu/atomic.h"
> -#include "sysemu/qtest.h"
>  #include "qemu/timer.h"
> +#include "exec/tb-hash.h"
> +#include "qemu/rcu.h"
> +
> +#if !defined(CONFIG_USER_ONLY)
>  #include "exec/address-spaces.h"
>  #include "exec/memory-internal.h"
> -#include "qemu/rcu.h"
> -#include "exec/tb-hash.h"
> +#include "sysemu/sysemu.h"
> +#endif

> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -205,6 +205,9 @@ QemuOpts *qemu_get_machine_opts(void);
>  bool defaults_enabled(void);
>  bool usb_enabled(void);
>
> +bool qemu_in_vcpu_thread(void);
> +void phys_mem_set_alloc(void *(*alloc)(size_t, uint64_t *align));
> +
>  extern QemuOptsList qemu_legacy_drive_opts;
>  extern QemuOptsList qemu_common_drive_opts;
>  extern QemuOptsList qemu_drive_opts;

sysemu.h is a bit of a huge grab-bag of stuff, so it seems
a shame to put qemu_in_vcpu_thread() into it, given that
(a) that function is only used by a couple of files and
(b) the files that do want that function don't want most of
what's in sysemu.h...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/10] cpu-exec: elide more icount code if CONFIG_USER_ONLY

[Peter Maydell]

On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpu-exec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 599e64d..bde5fd1 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -228,6 +228,7 @@ static inline tcg_target_ulong cpu_tb_exec(CPUState *cpu, uint8_t *tb_ptr)
>      return next_tb;
>  }
>
> +#if defined(CONFIG_SOFTMMU)
>  /* Execute the code without caching the generated code. An interpreter
>     could be used if available. */
>  static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
> @@ -251,6 +252,7 @@ static void cpu_exec_nocache(CPUState *cpu, int max_cycles,
>      tb_phys_invalidate(tb, -1);
>      tb_free(tb);
>  }
> +#endif
>
>  static TranslationBlock *tb_find_slow(CPUState *cpu,
>                                        target_ulong pc,
> @@ -523,6 +525,9 @@ int cpu_exec(CPUState *cpu)
>                      case TB_EXIT_ICOUNT_EXPIRED:
>                      {
>                          /* Instruction counter expired.  */
> +#ifdef CONFIG_USER_ONLY
> +                        abort();
> +#else
>                          int insns_left = cpu->icount_decr.u32;
>                          if (cpu->icount_extra && insns_left >= 0) {
>                              /* Refill decrementer and continue execution.  */
> @@ -542,6 +547,7 @@ int cpu_exec(CPUState *cpu)
>                              cpu_loop_exit(cpu);
>                          }
>                          break;
> +#endif
>                      }
>                      default:
>                          break;

What's the rationale for this? Mostly we prefer not to
add ifdefs in code if we can get away with compiling it for
both cases, even if the resulting code isn't used.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/10] tcg: code_bitmap is not used by user-mode emulation

[Peter Maydell]

On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> More #ifdefs are not nice, but this clarifies why its usage is not
> protected by tb_lock.

Does it? I thought the idea of this series was to add locking
which we needed for adding multi-threading to softmmu, in
which case presumably we need to protect this with a lock
of some kind whether we're using softmmu or usermode.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/10] tcg: comment on which functions have to be called with mmap_lock held

[Peter Maydell]

On 12 August 2015 at 17:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  translate-all.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)

Some overall documentation on what the mmap_lock is protecting
(and thus when it needs to be taken) would be nice.

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

I couldn't see where we take the mmap_lock when we call
tb_gen_code() from tb_find_slow() -- how does that work?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses

[Peter Maydell]

On 12 August 2015 at 17:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> page_find is reading the radix tree outside all locks, so it has to
> use the RCU primitives.  It does not need RCU critical sections
> because the PageDescs are never removed, so there is never a need
> to wait for the end of code sections that use a PageDesc.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  translate-all.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index 7727091..78a787d 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -437,14 +437,14 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>
>      /* Level 2..N-1.  */
>      for (i = V_L1_SHIFT / V_L2_BITS - 1; i > 0; i--) {
> -        void **p = *lp;
> +        void **p = atomic_rcu_read(lp);
>
>          if (p == NULL) {
>              if (!alloc) {
>                  return NULL;
>              }
>              p = g_new0(void *, V_L2_SIZE);
> -            *lp = p;
> +            atomic_rcu_set(lp, p);
>          }
>
>          lp = p + ((index >> (i * V_L2_BITS)) & (V_L2_SIZE - 1));
> @@ -456,7 +456,7 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
>              return NULL;
>          }
>          pd = g_new0(PageDesc, V_L2_SIZE);
> -        *lp = pd;
> +        atomic_rcu_set(lp, pd);
>      }
>
>      return pd + (index & (V_L2_SIZE - 1));

Don't we also need to use an atomic_rcu_read() for the load
    pd = *lp;
(which is between hunk 1 and 2 in this patch) ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/10] exec: make mmap_lock/mmap_unlock globally available

[Peter Maydell]

On 12 August 2015 at 17:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> There is some iffy lock hierarchy going on in translate-all.c.  To
> fix it, we need to take the mmap_lock in cpu-exec.c.  Make the
> functions globally available.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  bsd-user/qemu.h         | 2 --
>  include/exec/exec-all.h | 7 ++++++-
>  linux-user/qemu.h       | 2 --
>  translate-all.c         | 5 -----
>  4 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index 5362297..5902614 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -213,8 +213,6 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
>                         abi_ulong new_addr);
>  int target_msync(abi_ulong start, abi_ulong len, int flags);
>  extern unsigned long last_brk;
> -void mmap_lock(void);
> -void mmap_unlock(void);
>  void cpu_list_lock(void);
>  void cpu_list_unlock(void);
>  #if defined(CONFIG_USE_NPTL)
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index eb77373..b3f900a 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -318,7 +318,6 @@ extern uintptr_t tci_tb_ptr;
>  #define GETPC()  (GETRA() - GETPC_ADJ)
>
>  #if !defined(CONFIG_USER_ONLY)
> -
>  struct MemoryRegion *iotlb_to_region(CPUState *cpu,
>                                       hwaddr index);
>

Stray whitespace change.

Otherwise

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 10/10] cpu-exec: fix lock hierarchy for user-mode emulation

[Peter Maydell]

On 12 August 2015 at 17:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> tb_lock has to be taken inside the mmap_lock (example:
> tb_invalidate_phys_range is called by target_mmap), but
> tb_link_page is taking the mmap_lock and it is called
> with the tb_lock held.
>
> To fix this, take the mmap_lock in tb_find_slow, not
> in tb_link_page.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Dropping the outer lock and continuing to hold the inner
one looks rather weird, but I think this is all OK.

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/10] replace spinlock by QemuMutex.

[Paolo Bonzini]

On 28/08/2015 16:53, Frederic Konrad wrote:
>> After this commit it looks like we have no users of spinlock_t
>> at all. It would be good to have a followup patch which deleted
>> include/exec/spinlock.h.
> 
> Seems Paolo forget to pick up the following patch from the mttcg tree:
> "remove unused spinlock."
> https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg01129.html

Yes, that patch does the trick. :)

Paolo

Re: [Qemu-devel] [PATCH 02/10] cpus: remove tcg_halt_cond global variable.

[Paolo Bonzini]

On 28/08/2015 16:36, Peter Maydell wrote:
> With this patch, code-wise tcg_halt_cond and tcg_cpu_thread
> are used in pretty much parallel ways (first call into
> qemu_tcg_init_vcpu() sets them up, all the rest just copy
> them into the CPU struct). The only difference is that one
> of them a static at file scope and the other one is a static
> at function scope. It seems a shame to not have them be
> exactly parallel...

Good point, this does the trick:

diff --git a/cpus.c b/cpus.c
index 105b914..054dd68 100644
--- a/cpus.c
+++ b/cpus.c
@@ -786,8 +786,6 @@ static unsigned iothread_requesting_mutex;
 
 static QemuThread io_thread;
 
-static QemuThread *tcg_cpu_thread;
-
 /* cpu creation */
 static QemuCond qemu_cpu_cond;
 /* system init */
@@ -1222,6 +1220,7 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
     static QemuCond *tcg_halt_cond;
+    static QemuThread *tcg_cpu_thread;
 
     tcg_cpu_address_space_init(cpu, cpu->as);
 
Paolo

Re: [Qemu-devel] [PATCH 04/10] exec-all: remove non-TCG stuff from exec-all.h header.

[Paolo Bonzini]

On 28/08/2015 16:53, Peter Maydell wrote:
> sysemu.h is a bit of a huge grab-bag of stuff, so it seems
> a shame to put qemu_in_vcpu_thread() into it, given that
> (a) that function is only used by a couple of files and
> (b) the files that do want that function don't want most of
> what's in sysemu.h...

Ok, I'll replace it with a different patch that makes
qemu_in_vcpu_thread() static.

Paolo

Re: [Qemu-devel] [PATCH 07/10] tcg: comment on which functions have to be called with mmap_lock held

[Paolo Bonzini]

On 28/08/2015 17:33, Peter Maydell wrote:
> Some overall documentation on what the mmap_lock is protecting
> (and thus when it needs to be taken) would be nice.
> 
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I couldn't see where we take the mmap_lock when we call
> tb_gen_code() from tb_find_slow() -- how does that work?

It's the bug that's fixed in patch 10.

Paolo

Re: [Qemu-devel] [PATCH 08/10] tcg: add memory barriers in page_find_alloc accesses

[Paolo Bonzini]

On 28/08/2015 17:40, Peter Maydell wrote:
> Don't we also need to use an atomic_rcu_read() for the load
>     pd = *lp;
> (which is between hunk 1 and 2 in this patch) ?

Yes.

Paolo

Re: [Qemu-devel] [PATCH 05/10] cpu-exec: elide more icount code if CONFIG_USER_ONLY

[Paolo Bonzini]

On 28/08/2015 16:56, Peter Maydell wrote:
> What's the rationale for this? Mostly we prefer not to
> add ifdefs in code if we can get away with compiling it for
> both cases, even if the resulting code isn't used.

True.  The rationale is three-fold:

1) It makes sense to abort if TB_EXIT_ICOUNT_EXPIRED is returned from
user-mode emulation TCG code.  Then you need to either leave the
unreachable code after abort() or #ifdef cpu_exec_nocache out.

2) Dually, cpu_exec_nocache's future locking requirements (take tb_lock,
mostly) are not yet respected as of this patch.  Marking the function as
#ifdef CONFIG_SOFTMMU explains why.  Once you do that, you have to
abort() either in cpu_exec_nocache or in the caller.


Reason (2) is stronger for me.  You mentioned a few times your hope that
MTTCG would lead to more correct and more documented thread support in
user-mode emulation, and I think this patch does improve the
documentation slightly.

Paolo

Re: [Qemu-devel] [PATCH 06/10] tcg: code_bitmap is not used by user-mode emulation

[Paolo Bonzini]

On 28/08/2015 16:57, Peter Maydell wrote:
>> > More #ifdefs are not nice, but this clarifies why its usage is not
>> > protected by tb_lock.
> Does it? I thought the idea of this series was to add locking
> which we needed for adding multi-threading to softmmu, in
> which case presumably we need to protect this with a lock
> of some kind whether we're using softmmu or usermode.

No, the idea of the series was to base softmmu multi-threading on
user-mode multi-threading, and document what else needs to be done with
respect to common code.

Adding the necessary locks for softmmu only comes in the "extra" patches
11 and 12, and protecting code_bitmap with a lock (presumably tb_lock,
or its own lock, I don't know) would belong there.  However I left this
job to Fred, and similarly for breakpoints.

I'm not sure if it makes sense to add locking (like patches 11 and 12)
in the absence of actual multi-threading.  On one hand it would make it
simpler to commit the MTTCG work in steps.  On the other hand it risks
bitrotting.  The possibility of bitrot is another point in favor of
making more code #ifdef CONFIG_SOFTMMU; it would clearly identify areas
where locking correctness never matters in practice, and thus bugs may
be hidden.

Paolo