[Qemu-devel] [PATCH v2 00/11] MTTCG fixups for 2.9

[Qemu-devel] [PATCH v2 00/11] MTTCG fixups for 2.9

[Alex Bennée]

Hi Peter,

So perhaps predictably the merging of MTTCG has thrown up some issues
during soft-freeze. The following patches fix up some of those:

The following where in v1 and just have r-b tags:

  vl/cpus: be smarter with icount and MTTCG
  target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
  cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG

The next change downgrades the asserts to tcg_debug_asserts. This will
reduce the instances of production builds failing on non-MTTCG enabled
guests. The races still need to be fixed but you now have to
--enable-debug-tcg to go hunting for them:

  translate: downgrade IRQ BQL asserts to tcg_debug_assert

This never came up in my testing but it seems some guests can
translate while doing a tlb_fill. The call to cpu_restore_state never
worked before (as we are translating the block you are looking for)
but by bugging out we avoid the double tb_lock().

  translate-all: exit cpu_restore_state early if translating

Then we have a bunch of fixes from the reports on the list. They are
safe to merge but I'll leave that up to the maintainers. There may be
an argument for holding these patches back until the guest is
converted to be MTTCG aware and a full audit of the CPU<->HW emulation
boundaries is done for each one:

  sparc/sparc64: grab BQL before calling cpu_check_irqs
  s390x/misc_helper.c: wrap IO instructions in BQL
  target/xtensa: hold BQL for interrupt processing
  target/mips/op_helper: hold BQL before calling cpu_mips_get_count

Finally these are just tiny debug fixes while investigating the icount
regression:

  target/arm/helper: make it clear the EC field is also in hex
  hw/intc/arm_gic: modernise the DPRINTF

Going forward I think 1-5 are ready to be merged now. For 6-9 we
should await decisions from the target maintainers. The last two are
entirely up to you.

It looks like the -icount regression is a poor interaction between
icount timers and the BQL but this is going to require discussion with
Paolo in the morning.

Cheers,


Alex Bennée (11):
  vl/cpus: be smarter with icount and MTTCG
  target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
  cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
  translate: downgrade IRQ BQL asserts to tcg_debug_assert
  translate-all: exit cpu_restore_state early if translating
  sparc/sparc64: grab BQL before calling cpu_check_irqs
  s390x/misc_helper.c: wrap IO instructions in BQL
  target/xtensa: hold BQL for interrupt processing
  target/mips/op_helper: hold BQL before calling cpu_mips_get_count
  target/arm/helper: make it clear the EC field is also in hex
  hw/intc/arm_gic: modernise the DPRINTF

 cpus.c                      | 11 +++++++----
 hw/intc/arm_gic.c           | 13 +++++++++----
 hw/sparc/sun4m.c            |  3 +++
 hw/sparc64/sparc64.c        |  3 +++
 target/arm/helper.c         |  2 +-
 target/i386/cpu.h           |  3 +++
 target/mips/op_helper.c     | 17 ++++++++++++++---
 target/s390x/misc_helper.c  | 21 +++++++++++++++++++++
 target/sparc/int64_helper.c |  3 +++
 target/sparc/win_helper.c   | 11 +++++++++++
 target/xtensa/helper.c      |  1 +
 target/xtensa/op_helper.c   |  7 +++++++
 translate-all.c             |  9 ++++++++-
 translate-common.c          |  3 ++-
 vl.c                        |  7 ++-----
 15 files changed, 95 insertions(+), 19 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 01/11] vl/cpus: be smarter with icount and MTTCG

[Alex Bennée]

The sense of the test was inverted. Make it simple, if icount is
enabled then we disabled MTTCG by default. If the user tries to force
MTTCG upon us then we tell them "no".

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 cpus.c | 7 +++----
 vl.c   | 7 ++-----
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/cpus.c b/cpus.c
index 8200ac6b75..e9aab9f70f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -185,10 +185,7 @@ static bool check_tcg_memory_orders_compatible(void)
 
 static bool default_mttcg_enabled(void)
 {
-    QemuOpts *icount_opts = qemu_find_opts_singleton("icount");
-    const char *rr = qemu_opt_get(icount_opts, "rr");
-
-    if (rr || TCG_OVERSIZED_GUEST) {
+    if (use_icount || TCG_OVERSIZED_GUEST) {
         return false;
     } else {
 #ifdef TARGET_SUPPORTS_MTTCG
@@ -206,6 +203,8 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp)
         if (strcmp(t, "multi") == 0) {
             if (TCG_OVERSIZED_GUEST) {
                 error_setg(errp, "No MTTCG when guest word size > hosts");
+            } else if (use_icount) {
+                error_setg(errp, "No MTTCG when icount is enabled");
             } else {
                 if (!check_tcg_memory_orders_compatible()) {
                     error_report("Guest expects a stronger memory ordering "
diff --git a/vl.c b/vl.c
index e10a27bdd6..bbbf1baadf 100644
--- a/vl.c
+++ b/vl.c
@@ -4025,8 +4025,6 @@ int main(int argc, char **argv, char **envp)
 
     replay_configure(icount_opts);
 
-    qemu_tcg_configure(accel_opts, &error_fatal);
-
     machine_class = select_machine();
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
@@ -4393,14 +4391,13 @@ int main(int argc, char **argv, char **envp)
         if (!tcg_enabled()) {
             error_report("-icount is not allowed with hardware virtualization");
             exit(1);
-        } else if (qemu_tcg_mttcg_enabled()) {
-            error_report("-icount does not currently work with MTTCG");
-            exit(1);
         }
         configure_icount(icount_opts, &error_abort);
         qemu_opts_del(icount_opts);
     }
 
+    qemu_tcg_configure(accel_opts, &error_fatal);
+
     if (default_net) {
         QemuOptsList *net = qemu_find_opts("net");
         qemu_opts_set(net, NULL, "type", "nic", &error_abort);
-- 
2.11.0

[Qemu-devel] [PATCH v2 02/11] target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO

[Alex Bennée]

This suppresses the incorrect warning when forcing MTTCG for x86
guests on x86 hosts. A future patch will still warn when
TARGET_SUPPORT_MTTCG hasn't been defined for the guest (which is still
pending for x86).

Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 target/i386/cpu.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 12a39d590f..ecdd3bbc2a 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -30,6 +30,9 @@
 #define TARGET_LONG_BITS 32
 #endif
 
+/* The x86 has a strong memory model with some store-after-load re-ordering */
+#define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
+
 /* Maximum instruction code size */
 #define TARGET_MAX_INSN_SIZE 16
 
-- 
2.11.0

[Qemu-devel] [PATCH v2 03/11] cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG

[Alex Bennée]

While we may fail the memory ordering check later that can be
confusing. So in cases where TARGET_SUPPORT_MTTCG has yet to be
defined we should say so specifically.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 cpus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/cpus.c b/cpus.c
index e9aab9f70f..2459f564ea 100644
--- a/cpus.c
+++ b/cpus.c
@@ -206,6 +206,10 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp)
             } else if (use_icount) {
                 error_setg(errp, "No MTTCG when icount is enabled");
             } else {
+#ifndef TARGET_SUPPORT_MTTCG
+                error_report("Guest not yet converted to MTTCG - "
+                             "you may get unexpected results");
+#endif
                 if (!check_tcg_memory_orders_compatible()) {
                     error_report("Guest expects a stronger memory ordering "
                                  "than the host provides");
-- 
2.11.0

[Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Alex Bennée]

While on MTTCG hosts it is very important that updates to
cpu->interrupt_request are protected by the BQL not all guests have
been converted to the correct locking yet. As a result we are seeing
breaking on non-MTTCG enabled guests in production builds.

The locking in the guests needs to be fixed but while running single
threaded they will continue to work. By moving the asserts to
tcg_debug_asserts() they will still be useful during conversion
work (much like the existing assert_memory_lock/assert_tb_lock
asserts).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 translate-all.c    | 2 +-
 translate-common.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index 9bac061c9b..7ee273410d 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1928,7 +1928,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf)
 
 void cpu_interrupt(CPUState *cpu, int mask)
 {
-    g_assert(qemu_mutex_iothread_locked());
+    tcg_debug_assert(qemu_mutex_iothread_locked());
     cpu->interrupt_request |= mask;
     cpu->tcg_exit_req = 1;
 }
diff --git a/translate-common.c b/translate-common.c
index d504dd0d33..24e05c077a 100644
--- a/translate-common.c
+++ b/translate-common.c
@@ -22,6 +22,7 @@
 #include "qom/cpu.h"
 #include "sysemu/cpus.h"
 #include "qemu/main-loop.h"
+#include "tcg.h"
 
 uintptr_t qemu_real_host_page_size;
 intptr_t qemu_real_host_page_mask;
@@ -31,7 +32,7 @@ intptr_t qemu_real_host_page_mask;
 static void tcg_handle_interrupt(CPUState *cpu, int mask)
 {
     int old_mask;
-    g_assert(qemu_mutex_iothread_locked());
+    tcg_debug_assert(qemu_mutex_iothread_locked());
 
     old_mask = cpu->interrupt_request;
     cpu->interrupt_request |= mask;
-- 
2.11.0

[Qemu-devel] [PATCH v2 05/11] translate-all: exit cpu_restore_state early if translating

[Alex Bennée]

The translation code uses cpu_ld*_code which can trigger a tlb_fill
which if it fails will attempt a fault resolution. This never works
during translation as the TB being generated hasn't been added yet.
However with the new locking regime we end up double locking the
tb_lock(). As the tcg_ctx.cpu is only set during translation we use
this to short circuit the restore code and return with a fail.

Most front-ends seem to ignore the pass/fail result anyway but
tolerate not having the cpu environment updated. This is arguably ugly
but will do for now.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 translate-all.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/translate-all.c b/translate-all.c
index 7ee273410d..956d54b882 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -333,6 +333,13 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t retaddr)
     TranslationBlock *tb;
     bool r = false;
 
+    /* Don't attempt to restore state if we are translating already */
+    if (tcg_ctx.cpu == cpu) {
+        qemu_log_mask(LOG_UNIMP, "Attempt to resolve CPU state @ 0x%" PRIxPTR
+                      " while translating\n", retaddr);
+        return r;
+    }
+
     tb_lock();
     tb = tb_find_pc(retaddr);
     if (tb) {
-- 
2.11.0

[Qemu-devel] [PATCH v2 06/11] sparc/sparc64: grab BQL before calling cpu_check_irqs

[Alex Bennée]

IRQ modification is part of device emulation and should be done while
the BQL is held to prevent races when MTTCG is enabled. This adds
assertions in the hw emulation layer and wraps the calls from helpers
in the BQL.

Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/sparc/sun4m.c            |  3 +++
 hw/sparc64/sparc64.c        |  3 +++
 target/sparc/int64_helper.c |  3 +++
 target/sparc/win_helper.c   | 11 +++++++++++
 4 files changed, 20 insertions(+)

diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 61416a6426..873cd7df9a 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -142,6 +142,9 @@ void cpu_check_irqs(CPUSPARCState *env)
 {
     CPUState *cs;
 
+    /* We should be holding the BQL before we mess with IRQs */
+    g_assert(qemu_mutex_iothread_locked());
+
     if (env->pil_in && (env->interrupt_index == 0 ||
                         (env->interrupt_index & ~15) == TT_EXTINT)) {
         unsigned int i;
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index b3d219c769..4e4fdab065 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -55,6 +55,9 @@ void cpu_check_irqs(CPUSPARCState *env)
     uint32_t pil = env->pil_in |
                   (env->softint & ~(SOFTINT_TIMER | SOFTINT_STIMER));
 
+    /* We should be holding the BQL before we mess with IRQs */
+    g_assert(qemu_mutex_iothread_locked());
+
     /* TT_IVEC has a higher priority (16) than TT_EXTINT (31..17) */
     if (env->ivec_status & 0x20) {
         return;
diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
index 605747c93c..f942973c22 100644
--- a/target/sparc/int64_helper.c
+++ b/target/sparc/int64_helper.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/log.h"
@@ -208,7 +209,9 @@ static bool do_modify_softint(CPUSPARCState *env, uint32_t value)
         env->softint = value;
 #if !defined(CONFIG_USER_ONLY)
         if (cpu_interrupts_enabled(env)) {
+            qemu_mutex_lock_iothread();
             cpu_check_irqs(env);
+            qemu_mutex_unlock_iothread();
         }
 #endif
         return true;
diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
index 71b3dd37e8..b387b026d8 100644
--- a/target/sparc/win_helper.c
+++ b/target/sparc/win_helper.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
@@ -86,7 +87,9 @@ void cpu_put_psr(CPUSPARCState *env, target_ulong val)
 {
     cpu_put_psr_raw(env, val);
 #if ((!defined(TARGET_SPARC64)) && !defined(CONFIG_USER_ONLY))
+    qemu_mutex_lock_iothread();
     cpu_check_irqs(env);
+    qemu_mutex_unlock_iothread();
 #endif
 }
 
@@ -368,7 +371,9 @@ void helper_wrpstate(CPUSPARCState *env, target_ulong new_state)
 
 #if !defined(CONFIG_USER_ONLY)
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }
@@ -381,7 +386,9 @@ void helper_wrpil(CPUSPARCState *env, target_ulong new_pil)
     env->psrpil = new_pil;
 
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }
@@ -408,7 +415,9 @@ void helper_done(CPUSPARCState *env)
 
 #if !defined(CONFIG_USER_ONLY)
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }
@@ -435,7 +444,9 @@ void helper_retry(CPUSPARCState *env)
 
 #if !defined(CONFIG_USER_ONLY)
     if (cpu_interrupts_enabled(env)) {
+        qemu_mutex_lock_iothread();
         cpu_check_irqs(env);
+        qemu_mutex_unlock_iothread();
     }
 #endif
 }
-- 
2.11.0

[Qemu-devel] [PATCH v2 07/11] s390x/misc_helper.c: wrap IO instructions in BQL

[Alex Bennée]

Helpers that can trigger IO events (including interrupts) need to be
protected by the BQL. I've updated all the helpers that call into an
ioinst_handle_* functions.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/s390x/misc_helper.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 3cb942e8bb..93b0e61366 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/memory.h"
 #include "qemu/host-utils.h"
@@ -551,61 +552,81 @@ uint32_t HELPER(sigp)(CPUS390XState *env, uint64_t order_code, uint32_t r1,
 void HELPER(xsch)(CPUS390XState *env, uint64_t r1)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_xsch(cpu, r1);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(csch)(CPUS390XState *env, uint64_t r1)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_csch(cpu, r1);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(hsch)(CPUS390XState *env, uint64_t r1)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_hsch(cpu, r1);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(msch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_msch(cpu, r1, inst >> 16);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(rchp)(CPUS390XState *env, uint64_t r1)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_rchp(cpu, r1);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(rsch)(CPUS390XState *env, uint64_t r1)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_rsch(cpu, r1);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(ssch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_ssch(cpu, r1, inst >> 16);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_stsch(cpu, r1, inst >> 16);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(tsch)(CPUS390XState *env, uint64_t r1, uint64_t inst)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_tsch(cpu, r1, inst >> 16);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(chsc)(CPUS390XState *env, uint64_t inst)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
+    qemu_mutex_lock_iothread();
     ioinst_handle_chsc(cpu, inst >> 16);
+    qemu_mutex_unlock_iothread();
 }
 #endif
 
-- 
2.11.0

[Qemu-devel] [PATCH v2 08/11] target/xtensa: hold BQL for interrupt processing

[Alex Bennée]

Make sure we have the BQL held when processing interrupts.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/xtensa/helper.c    | 1 +
 target/xtensa/op_helper.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/target/xtensa/helper.c b/target/xtensa/helper.c
index c67d715c4b..bcd0b7738d 100644
--- a/target/xtensa/helper.c
+++ b/target/xtensa/helper.c
@@ -217,6 +217,7 @@ static void handle_interrupt(CPUXtensaState *env)
     }
 }
 
+/* Called from cpu_handle_interrupt with BQL held */
 void xtensa_cpu_do_interrupt(CPUState *cs)
 {
     XtensaCPU *cpu = XTENSA_CPU(cs);
diff --git a/target/xtensa/op_helper.c b/target/xtensa/op_helper.c
index af2723445d..519fbeddd6 100644
--- a/target/xtensa/op_helper.c
+++ b/target/xtensa/op_helper.c
@@ -26,6 +26,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "qemu/host-utils.h"
@@ -381,7 +382,11 @@ void HELPER(waiti)(CPUXtensaState *env, uint32_t pc, uint32_t intlevel)
     env->pc = pc;
     env->sregs[PS] = (env->sregs[PS] & ~PS_INTLEVEL) |
         (intlevel << PS_INTLEVEL_SHIFT);
+
+    qemu_mutex_lock_iothread();
     check_interrupts(env);
+    qemu_mutex_unlock_iothread();
+
     if (env->pending_irq_level) {
         cpu_loop_exit(CPU(xtensa_env_get_cpu(env)));
         return;
@@ -426,7 +431,9 @@ void HELPER(update_ccompare)(CPUXtensaState *env, uint32_t i)
 
 void HELPER(check_interrupts)(CPUXtensaState *env)
 {
+    qemu_mutex_lock_iothread();
     check_interrupts(env);
+    qemu_mutex_unlock_iothread();
 }
 
 void HELPER(itlb_hit_test)(CPUXtensaState *env, uint32_t vaddr)
-- 
2.11.0

[Qemu-devel] [PATCH v2 09/11] target/mips/op_helper: hold BQL before calling cpu_mips_get_count

[Alex Bennée]

We should hold the BQL before we transition to HW emulation. This is
because all HW emulation needs to be serialised under MTTCG
conditions. This is picked up by asserts that fire when
cpu_mips_get_count triggers and IRQ.

Reported-by: Yongbok Kim <yongbok.kim@imgtec.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/mips/op_helper.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index b683fcb025..38bca03f52 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "qemu/host-utils.h"
 #include "exec/helper-proto.h"
@@ -827,7 +828,13 @@ target_ulong helper_mftc0_tcschefback(CPUMIPSState *env)
 
 target_ulong helper_mfc0_count(CPUMIPSState *env)
 {
-    return (int32_t)cpu_mips_get_count(env);
+    int32_t count;
+
+    qemu_mutex_lock_iothread();
+    count = (int32_t)cpu_mips_get_count(env);
+    qemu_mutex_unlock_iothread();
+
+    return count;
 }
 
 target_ulong helper_mftc0_entryhi(CPUMIPSState *env)
@@ -2296,12 +2303,16 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env)
 
 target_ulong helper_rdhwr_cc(CPUMIPSState *env)
 {
+    int32_t count;
     check_hwrena(env, 2, GETPC());
 #ifdef CONFIG_USER_ONLY
-    return env->CP0_Count;
+    count = env->CP0_Count;
 #else
-    return (int32_t)cpu_mips_get_count(env);
+    qemu_mutex_lock_iothread();
+    count = (int32_t)cpu_mips_get_count(env);
+    qemu_mutex_unlock_iothread();
 #endif
+    return count;
 }
 
 target_ulong helper_rdhwr_ccres(CPUMIPSState *env)
-- 
2.11.0

[Qemu-devel] [PATCH v2 10/11] target/arm/helper: make it clear the EC field is also in hex

[Alex Bennée]

..just like the rest of the displayed ESR register. Otherwise people
might scratch their heads if a not obviously hex number is displayed
for the EC field.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3f4211b572..76b608f0ba 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6857,7 +6857,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                   new_el);
     if (qemu_loglevel_mask(CPU_LOG_INT)
         && !excp_is_internal(cs->exception_index)) {
-        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
+        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%x/0x%" PRIx32 "\n",
                       env->exception.syndrome >> ARM_EL_EC_SHIFT,
                       env->exception.syndrome);
     }
-- 
2.11.0

[Qemu-devel] [PATCH v2 11/11] hw/intc/arm_gic: modernise the DPRINTF

[Alex Bennée]

While I was debugging the icount issues I realised a bunch of the
messages look quite similar. I've fixed this by including __func__ in
the debug print. At the same time I move the a modern if (GATE) style
printf which ensures the compiler can check for format string errors
even if the code gets optimised away in the non-DEBUG_GIC case.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/intc/arm_gic.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 8e5a9d8a3e..b305d9032a 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -26,15 +26,20 @@
 #include "qemu/log.h"
 #include "trace.h"
 
-//#define DEBUG_GIC
+/* #define DEBUG_GIC */
 
 #ifdef DEBUG_GIC
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
+#define DEBUG_GIC_GATE 1
 #else
-#define DPRINTF(fmt, ...) do {} while(0)
+#define DEBUG_GIC_GATE 0
 #endif
 
+#define DPRINTF(fmt, ...) do {                                          \
+        if (DEBUG_GIC_GATE) {                                           \
+            fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__);      \
+        }                                                               \
+    } while (0)
+
 static const uint8_t gic_id_11mpcore[] = {
     0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
 };
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 05/11] translate-all: exit cpu_restore_state early if translating

[Richard Henderson]

On 03/03/2017 06:53 AM, Alex Bennée wrote:
> The translation code uses cpu_ld*_code which can trigger a tlb_fill
> which if it fails will attempt a fault resolution. This never works
> during translation as the TB being generated hasn't been added yet.
> However with the new locking regime we end up double locking the
> tb_lock(). As the tcg_ctx.cpu is only set during translation we use
> this to short circuit the restore code and return with a fail.

We *should* have retaddr == 0 for this case, which indicates that we should not 
attempt to restore state.  Are you seeing a non-zero value?

Hmm.. Or rather we should not have called cpu_restore_state in the first place. 
  What is the call chain leading to this point?

Is this in fact linux-user, not softmmu, as you imply from tlb_fill?  Because 
handle_cpu_signal will in fact pass a genuine non-zero retaddr for the SIGSEGV 
resulting from a cpu_ld*_code from a non-mapped address.

So... for linux-user I think the qemu_log is unnecessary -- that's just the way 
things are.  For softmmu, I don't know what to think except that we shouldn't 
have gotten here.


r~

Re: [Qemu-devel] [PATCH v2 05/11] translate-all: exit cpu_restore_state early if translating

[Alex Bennée]

Richard Henderson <rth@twiddle.net> writes:

> On 03/03/2017 06:53 AM, Alex Bennée wrote:
>> The translation code uses cpu_ld*_code which can trigger a tlb_fill
>> which if it fails will attempt a fault resolution. This never works
>> during translation as the TB being generated hasn't been added yet.
>> However with the new locking regime we end up double locking the
>> tb_lock(). As the tcg_ctx.cpu is only set during translation we use
>> this to short circuit the restore code and return with a fail.
>
> We *should* have retaddr == 0 for this case, which indicates that we
> should not attempt to restore state.  Are you seeing a non-zero value?

Actually looking at xtensa I see:

  Attempt to resolve CPU state @ 0x0 while translating

So maybe I should check just that - but I don't see where we ensure we
always pass zero.

>
> Hmm.. Or rather we should not have called cpu_restore_state in the
> first place. What is the call chain leading to this point?


Thread 3 "qemu-system-xte" hit Breakpoint 2, cpu_restore_state (cpu=cpu@entry=0x555556032600, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:338
338	        qemu_log_mask(LOG_UNIMP, "Attempt to resolve CPU state @ 0x%" PRIxPTR
#0  0x00005555555e3712 in cpu_restore_state (cpu=cpu@entry=0x555556032600, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:338
#1  0x000055555564cb38 in tlb_fill (cs=cs@entry=0x555556032600, vaddr=vaddr@entry=537034752, access_type=access_type@entry=MMU_INST_FETCH, mmu_idx=mmu_idx@entry=1, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/target/xtensa/op_helper.c:73
#2  0x000055555562d604 in helper_ret_ldb_cmmu (env=env@entry=0x55555603a890, addr=537034752, oi=<optimised out>, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/softmmu_template.h:127
#3  0x00005555556448df in gen_intermediate_code (retaddr=0, ptr=<optimised out>, env=0x55555603a890) at /home/alex/lsrc/qemu/qemu.git/include/exec/cpu_ldst_template.h:102
#4  0x00005555556448df in gen_intermediate_code (ptr=<optimised out>, env=0x55555603a890) at /home/alex/lsrc/qemu/qemu.git/include/exec/cpu_ldst_template.h:114
#5  0x00005555556448df in gen_intermediate_code (dc=0x7fffcca0f4d0, env=0x55555603a890) at /home/alex/lsrc/qemu/qemu.git/target/xtensa/translate.c:1052
#6  0x00005555556448df in gen_intermediate_code (env=0x55555603a890, tb=0x7fffccc7de00) at /home/alex/lsrc/qemu/qemu.git/target/xtensa/translate.c:3214
#7  0x00005555555e383b in tb_gen_code (cpu=cpu@entry=0x555556032600, pc=pc@entry=537034751, cs_base=cs_base@entry=0, flags=flags@entry=229393, cflags=<optimised out>, cflags@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:1288
#8  0x00005555555e5e41 in cpu_exec (tb_exit=0, last_tb=<optimised out>, cpu=0x38011) at /home/alex/lsrc/qemu/qemu.git/cpu-exec.c:370
#9  0x00005555555e5e41 in cpu_exec (cpu=cpu@entry=0x555556032600) at /home/alex/lsrc/qemu/qemu.git/cpu-exec.c:685
#10 0x0000555555611898 in tcg_cpu_exec (cpu=0x555556032600) at /home/alex/lsrc/qemu/qemu.git/cpus.c:1254
#11 0x0000555555611bd4 in qemu_tcg_rr_cpu_thread_fn (arg=<optimised out>) at /home/alex/lsrc/qemu/qemu.git/cpus.c:1350
#12 0x00007fffdf6606ba in start_thread (arg=0x7fffcca12700) at pthread_create.c:333
#13 0x00007fffdf39682d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
Continuing.

Thread 3 "qemu-system-xte" hit Breakpoint 2, cpu_restore_state (cpu=cpu@entry=0x555556032600, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:338
338	        qemu_log_mask(LOG_UNIMP, "Attempt to resolve CPU state @ 0x%" PRIxPTR
#0  0x00005555555e3712 in cpu_restore_state (cpu=cpu@entry=0x555556032600, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:338
#1  0x000055555564cb38 in tlb_fill (cs=cs@entry=0x555556032600, vaddr=vaddr@entry=4308992, access_type=access_type@entry=MMU_INST_FETCH, mmu_idx=mmu_idx@entry=1, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/target/xtensa/op_helper.c:73
#2  0x000055555562d604 in helper_ret_ldb_cmmu (env=env@entry=0x55555603a890, addr=4308992, oi=<optimised out>, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/softmmu_template.h:127
#3  0x00005555556448df in gen_intermediate_code (retaddr=0, ptr=<optimised out>, env=0x55555603a890) at /home/alex/lsrc/qemu/qemu.git/include/exec/cpu_ldst_template.h:102
#4  0x00005555556448df in gen_intermediate_code (ptr=<optimised out>, env=0x55555603a890) at /home/alex/lsrc/qemu/qemu.git/include/exec/cpu_ldst_template.h:114
#5  0x00005555556448df in gen_intermediate_code (dc=0x7fffcca0f4d0, env=0x55555603a890) at /home/alex/lsrc/qemu/qemu.git/target/xtensa/translate.c:1052
#6  0x00005555556448df in gen_intermediate_code (env=0x55555603a890, tb=0x7fffccd0b1b0) at /home/alex/lsrc/qemu/qemu.git/target/xtensa/translate.c:3214
#7  0x00005555555e383b in tb_gen_code (cpu=cpu@entry=0x555556032600, pc=pc@entry=4308991, cs_base=cs_base@entry=0, flags=flags@entry=229393, cflags=<optimised out>, cflags@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:1288
#8  0x00005555555e5e41 in cpu_exec (tb_exit=0, last_tb=<optimised out>, cpu=0x38011) at /home/alex/lsrc/qemu/qemu.git/cpu-exec.c:370
#9  0x00005555555e5e41 in cpu_exec (cpu=cpu@entry=0x555556032600) at /home/alex/lsrc/qemu/qemu.git/cpu-exec.c:685
#10 0x0000555555611898 in tcg_cpu_exec (cpu=0x555556032600) at /home/alex/lsrc/qemu/qemu.git/cpus.c:1254
#11 0x0000555555611bd4 in qemu_tcg_rr_cpu_thread_fn (arg=<optimised out>) at /home/alex/lsrc/qemu/qemu.git/cpus.c:1350
#12 0x00007fffdf6606ba in start_thread (arg=0x7fffcca12700) at pthread_create.c:333
#13 0x00007fffdf39682d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109


> Is this in fact linux-user, not softmmu, as you imply from tlb_fill?
> Because handle_cpu_signal will in fact pass a genuine non-zero retaddr
> for the SIGSEGV resulting from a cpu_ld*_code from a non-mapped
> address.

I think that is another call chain that might trip us up. Peter
mentioned he'd hit it. This one is definitely softmmu.

> So... for linux-user I think the qemu_log is unnecessary -- that's
> just the way things are.  For softmmu, I don't know what to think
> except that we shouldn't have gotten here.

I kinda agree but all SoftMMU targets have this potential path. Having
saif that I haven't seen it hit ARM, maybe because we take care not to
cross a page boundary?

I agree it would be better to handle fetching code bytes without this
potential for breakage but that would be a much bigger change and need
more testing this close to rc0.

>
>
> r~


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Peter Maydell]

On 2 March 2017 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
> While on MTTCG hosts it is very important that updates to
> cpu->interrupt_request are protected by the BQL not all guests have
> been converted to the correct locking yet. As a result we are seeing
> breaking on non-MTTCG enabled guests in production builds.
>
> The locking in the guests needs to be fixed but while running single
> threaded they will continue to work. By moving the asserts to
> tcg_debug_asserts() they will still be useful during conversion
> work (much like the existing assert_memory_lock/assert_tb_lock
> asserts).
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  translate-all.c    | 2 +-
>  translate-common.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index 9bac061c9b..7ee273410d 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1928,7 +1928,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf)
>
>  void cpu_interrupt(CPUState *cpu, int mask)
>  {
> -    g_assert(qemu_mutex_iothread_locked());
> +    tcg_debug_assert(qemu_mutex_iothread_locked());

If CONFIG_DEBUG_TCG isn't enabled then tcg_debug_assert()
turns into "if (!(X)) { __builtin_unreachable(); }", which
means that instead of asserting we now run straight
into compiler undefined behaviour, don't we?
If what we want is "don't actually check this condition in
the non-tcg-debug config" then we should do something
that means we don't actually check the condition...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Alex Bennée]

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

> On 2 March 2017 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
>> While on MTTCG hosts it is very important that updates to
>> cpu->interrupt_request are protected by the BQL not all guests have
>> been converted to the correct locking yet. As a result we are seeing
>> breaking on non-MTTCG enabled guests in production builds.
>>
>> The locking in the guests needs to be fixed but while running single
>> threaded they will continue to work. By moving the asserts to
>> tcg_debug_asserts() they will still be useful during conversion
>> work (much like the existing assert_memory_lock/assert_tb_lock
>> asserts).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  translate-all.c    | 2 +-
>>  translate-common.c | 3 ++-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/translate-all.c b/translate-all.c
>> index 9bac061c9b..7ee273410d 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -1928,7 +1928,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf)
>>
>>  void cpu_interrupt(CPUState *cpu, int mask)
>>  {
>> -    g_assert(qemu_mutex_iothread_locked());
>> +    tcg_debug_assert(qemu_mutex_iothread_locked());
>
> If CONFIG_DEBUG_TCG isn't enabled then tcg_debug_assert()
> turns into "if (!(X)) { __builtin_unreachable(); }", which
> means that instead of asserting we now run straight
> into compiler undefined behaviour, don't we?

According to the commit that added it
(c552d6c038f7cf4058d1fd5987118ffd41e0e050) it is meant to be a hint to
the compiler. Reading the GCC notes however seems to contradict that.

FWIW I did test it in both builds and we do used tese for a bunch of
other asserts and they haven't blown up.

> If what we want is "don't actually check this condition in
> the non-tcg-debug config" then we should do something
> that means we don't actually check the condition...

Hmm:

28	intptr_t qemu_real_host_page_mask;
29
30	#ifndef CONFIG_USER_ONLY
31	/* mask must never be zero, except for A20 change call */
32	static void tcg_handle_interrupt(CPUState *cpu, int mask)
33	{
34	    int old_mask;
35	    tcg_debug_assert(qemu_mutex_iothread_locked());
36
37	    old_mask = cpu->interrupt_request;
Line 34 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0a <tcg_handle_interrupt+10> but contains no code.
Line 35 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0a <tcg_handle_interrupt+10> and ends at 0x24db0f <tcg_handle_interrupt+15>.
Line 36 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0f <tcg_handle_interrupt+15> but contains no code.
Line 37 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0f <tcg_handle_interrupt+15> and ends at 0x24db15 <tcg_handle_interrupt+21>.
   0x24db0a <tcg_handle_interrupt+10>:	callq  0x27a570 <qemu_mutex_iothread_locked>
   0x24db0f <tcg_handle_interrupt+15>:	mov    0xa8(%rbx),%ebp
   0x24db15 <tcg_handle_interrupt+21>:	mov    %r12d,%eax
   0x24db18 <tcg_handle_interrupt+24>:	mov    %rbx,%rdi
   0x24db1b <tcg_handle_interrupt+27>:	or     %ebp,%eax
   0x24db1d <tcg_handle_interrupt+29>:	mov    %eax,0xa8(%rbx)
   0x24db23 <tcg_handle_interrupt+35>:	callq  0x27a530 <qemu_cpu_is_self>

It certainly looks as though it makes the call but ignores the result?

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 09/11] target/mips/op_helper: hold BQL before calling cpu_mips_get_count

[Yongbok Kim]

On 02/03/2017 19:53, Alex Bennée wrote:
> We should hold the BQL before we transition to HW emulation. This is
> because all HW emulation needs to be serialised under MTTCG
> conditions. This is picked up by asserts that fire when
> cpu_mips_get_count triggers and IRQ.
> 
> Reported-by: Yongbok Kim <yongbok.kim@imgtec.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Hi Alex,

Thanks for this but it is required to have few more locks.
I will send the patch shortly. Please merge it into your patchset.

Regards,
Yongbok

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Peter Maydell]

On 3 March 2017 at 11:05, Alex Bennée <alex.bennee@linaro.org> wrote:
> According to the commit that added it
> (c552d6c038f7cf4058d1fd5987118ffd41e0e050) it is meant to be a hint to
> the compiler. Reading the GCC notes however seems to contradict that.
>
> FWIW I did test it in both builds and we do used tese for a bunch of
> other asserts and they haven't blown up.
>
>> If what we want is "don't actually check this condition in
>> the non-tcg-debug config" then we should do something
>> that means we don't actually check the condition...
>
> Hmm:
>
> 28      intptr_t qemu_real_host_page_mask;
> 29
> 30      #ifndef CONFIG_USER_ONLY
> 31      /* mask must never be zero, except for A20 change call */
> 32      static void tcg_handle_interrupt(CPUState *cpu, int mask)
> 33      {
> 34          int old_mask;
> 35          tcg_debug_assert(qemu_mutex_iothread_locked());
> 36
> 37          old_mask = cpu->interrupt_request;
> Line 34 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0a <tcg_handle_interrupt+10> but contains no code.
> Line 35 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0a <tcg_handle_interrupt+10> and ends at 0x24db0f <tcg_handle_interrupt+15>.
> Line 36 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0f <tcg_handle_interrupt+15> but contains no code.
> Line 37 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0f <tcg_handle_interrupt+15> and ends at 0x24db15 <tcg_handle_interrupt+21>.
>    0x24db0a <tcg_handle_interrupt+10>:  callq  0x27a570 <qemu_mutex_iothread_locked>
>    0x24db0f <tcg_handle_interrupt+15>:  mov    0xa8(%rbx),%ebp
>    0x24db15 <tcg_handle_interrupt+21>:  mov    %r12d,%eax
>    0x24db18 <tcg_handle_interrupt+24>:  mov    %rbx,%rdi
>    0x24db1b <tcg_handle_interrupt+27>:  or     %ebp,%eax
>    0x24db1d <tcg_handle_interrupt+29>:  mov    %eax,0xa8(%rbx)
>    0x24db23 <tcg_handle_interrupt+35>:  callq  0x27a530 <qemu_cpu_is_self>
>
> It certainly looks as though it makes the call but ignores the result?

That is one permitted implementation of the undefined behaviour,
certainly. Not the only one, though. In particular you're telling
the compiler's optimization passes "this can never be reached"
which can result in the optimizer making significantly different
decisions (especially if it manages to inline things or it can
look "inside" the condition being asserted here, etc etc).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 06/11] sparc/sparc64: grab BQL before calling cpu_check_irqs

[Paolo Bonzini]

On 02/03/2017 20:53, Alex Bennée wrote:
> IRQ modification is part of device emulation and should be done while
> the BQL is held to prevent races when MTTCG is enabled. This adds
> assertions in the hw emulation layer and wraps the calls from helpers
> in the BQL.
> 
> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/sparc/sun4m.c            |  3 +++
>  hw/sparc64/sparc64.c        |  3 +++
>  target/sparc/int64_helper.c |  3 +++
>  target/sparc/win_helper.c   | 11 +++++++++++
>  4 files changed, 20 insertions(+)
> 
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 61416a6426..873cd7df9a 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -142,6 +142,9 @@ void cpu_check_irqs(CPUSPARCState *env)
>  {
>      CPUState *cs;
>  
> +    /* We should be holding the BQL before we mess with IRQs */
> +    g_assert(qemu_mutex_iothread_locked());
> +
>      if (env->pil_in && (env->interrupt_index == 0 ||
>                          (env->interrupt_index & ~15) == TT_EXTINT)) {
>          unsigned int i;
> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
> index b3d219c769..4e4fdab065 100644
> --- a/hw/sparc64/sparc64.c
> +++ b/hw/sparc64/sparc64.c
> @@ -55,6 +55,9 @@ void cpu_check_irqs(CPUSPARCState *env)
>      uint32_t pil = env->pil_in |
>                    (env->softint & ~(SOFTINT_TIMER | SOFTINT_STIMER));
>  
> +    /* We should be holding the BQL before we mess with IRQs */
> +    g_assert(qemu_mutex_iothread_locked());
> +
>      /* TT_IVEC has a higher priority (16) than TT_EXTINT (31..17) */
>      if (env->ivec_status & 0x20) {
>          return;
> diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
> index 605747c93c..f942973c22 100644
> --- a/target/sparc/int64_helper.c
> +++ b/target/sparc/int64_helper.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "exec/helper-proto.h"
>  #include "exec/log.h"
> @@ -208,7 +209,9 @@ static bool do_modify_softint(CPUSPARCState *env, uint32_t value)
>          env->softint = value;
>  #if !defined(CONFIG_USER_ONLY)
>          if (cpu_interrupts_enabled(env)) {
> +            qemu_mutex_lock_iothread();
>              cpu_check_irqs(env);
> +            qemu_mutex_unlock_iothread();
>          }
>  #endif
>          return true;
> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
> index 71b3dd37e8..b387b026d8 100644
> --- a/target/sparc/win_helper.c
> +++ b/target/sparc/win_helper.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "exec/exec-all.h"
>  #include "exec/helper-proto.h"
> @@ -86,7 +87,9 @@ void cpu_put_psr(CPUSPARCState *env, target_ulong val)
>  {
>      cpu_put_psr_raw(env, val);
>  #if ((!defined(TARGET_SPARC64)) && !defined(CONFIG_USER_ONLY))
> +    qemu_mutex_lock_iothread();
>      cpu_check_irqs(env);
> +    qemu_mutex_unlock_iothread();
>  #endif
>  }

This can be called from gdbstub, so you need to put the lock/unlock
around helper_wrpsr's call to cpu_put_psr instead.  Also please add a
comment /* Called with BQL held.  */ around cpu_put_psr.

Paolo

> @@ -368,7 +371,9 @@ void helper_wrpstate(CPUSPARCState *env, target_ulong new_state)
>  
>  #if !defined(CONFIG_USER_ONLY)
>      if (cpu_interrupts_enabled(env)) {
> +        qemu_mutex_lock_iothread();
>          cpu_check_irqs(env);
> +        qemu_mutex_unlock_iothread();
>      }
>  #endif
>  }
> @@ -381,7 +386,9 @@ void helper_wrpil(CPUSPARCState *env, target_ulong new_pil)
>      env->psrpil = new_pil;
>  
>      if (cpu_interrupts_enabled(env)) {
> +        qemu_mutex_lock_iothread();
>          cpu_check_irqs(env);
> +        qemu_mutex_unlock_iothread();
>      }
>  #endif
>  }
> @@ -408,7 +415,9 @@ void helper_done(CPUSPARCState *env)
>  
>  #if !defined(CONFIG_USER_ONLY)
>      if (cpu_interrupts_enabled(env)) {
> +        qemu_mutex_lock_iothread();
>          cpu_check_irqs(env);
> +        qemu_mutex_unlock_iothread();
>      }
>  #endif
>  }
> @@ -435,7 +444,9 @@ void helper_retry(CPUSPARCState *env)
>  
>  #if !defined(CONFIG_USER_ONLY)
>      if (cpu_interrupts_enabled(env)) {
> +        qemu_mutex_lock_iothread();
>          cpu_check_irqs(env);
> +        qemu_mutex_unlock_iothread();
>      }
>  #endif
>  }
> 

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Paolo Bonzini]

On 03/03/2017 11:08, Peter Maydell wrote:
> On 2 March 2017 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
>> While on MTTCG hosts it is very important that updates to
>> cpu->interrupt_request are protected by the BQL not all guests have
>> been converted to the correct locking yet. As a result we are seeing
>> breaking on non-MTTCG enabled guests in production builds.
>>
>> The locking in the guests needs to be fixed but while running single
>> threaded they will continue to work. By moving the asserts to
>> tcg_debug_asserts() they will still be useful during conversion
>> work (much like the existing assert_memory_lock/assert_tb_lock
>> asserts).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  translate-all.c    | 2 +-
>>  translate-common.c | 3 ++-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/translate-all.c b/translate-all.c
>> index 9bac061c9b..7ee273410d 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -1928,7 +1928,7 @@ void dump_opcount_info(FILE *f, fprintf_function cpu_fprintf)
>>
>>  void cpu_interrupt(CPUState *cpu, int mask)
>>  {
>> -    g_assert(qemu_mutex_iothread_locked());
>> +    tcg_debug_assert(qemu_mutex_iothread_locked());
> 
> If CONFIG_DEBUG_TCG isn't enabled then tcg_debug_assert()
> turns into "if (!(X)) { __builtin_unreachable(); }", which
> means that instead of asserting we now run straight
> into compiler undefined behaviour, don't we?
> If what we want is "don't actually check this condition in
> the non-tcg-debug config" then we should do something
> that means we don't actually check the condition...

I think we should fix it for good and leave this as a hard assertion.

Paolo

Re: [Qemu-devel] [PATCH v2 09/11] target/mips/op_helper: hold BQL before calling cpu_mips_get_count

[Alex Bennée]

Yongbok Kim <yongbok.kim@imgtec.com> writes:

> On 02/03/2017 19:53, Alex Bennée wrote:
>> We should hold the BQL before we transition to HW emulation. This is
>> because all HW emulation needs to be serialised under MTTCG
>> conditions. This is picked up by asserts that fire when
>> cpu_mips_get_count triggers and IRQ.
>>
>> Reported-by: Yongbok Kim <yongbok.kim@imgtec.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> Hi Alex,
>
> Thanks for this but it is required to have few more locks.
> I will send the patch shortly. Please merge it into your patchset.

Thanks, I'll look out for it.

Have you been tracking the MTTCG work? Once the BQL issues are sorted as
long as atomics, barriers and tlb flushes are updated you can turn on
mttcg by default.

>
> Regards,
> Yongbok


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 09/11] target/mips/op_helper: hold BQL before calling cpu_mips_get_count

[Yongbok Kim]

On 03/03/2017 12:54, Alex Bennée wrote:
> 
> Yongbok Kim <yongbok.kim@imgtec.com> writes:
> 
>> On 02/03/2017 19:53, Alex Bennée wrote:
>>> We should hold the BQL before we transition to HW emulation. This is
>>> because all HW emulation needs to be serialised under MTTCG
>>> conditions. This is picked up by asserts that fire when
>>> cpu_mips_get_count triggers and IRQ.
>>>
>>> Reported-by: Yongbok Kim <yongbok.kim@imgtec.com>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> Hi Alex,
>>
>> Thanks for this but it is required to have few more locks.
>> I will send the patch shortly. Please merge it into your patchset.
> 
> Thanks, I'll look out for it.
> 
> Have you been tracking the MTTCG work? Once the BQL issues are sorted as
> long as atomics, barriers and tlb flushes are updated you can turn on
> mttcg by default.
> 
> 

Yes I have MIPS patches for MTTCG but it has not been rebased to the latest
version yet. I will send it after 2.9.

Regards,
Yongbok

Re: [Qemu-devel] [PATCH v2 11/11] hw/intc/arm_gic: modernise the DPRINTF

[Frederic Konrad]

Hi Alex,

On 03/02/2017 08:53 PM, Alex Bennée wrote:
> While I was debugging the icount issues I realised a bunch of the
> messages look quite similar. I've fixed this by including __func__ in
> the debug print. At the same time I move the a modern if (GATE) style
> printf which ensures the compiler can check for format string errors
> even if the code gets optimised away in the non-DEBUG_GIC case.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/intc/arm_gic.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 8e5a9d8a3e..b305d9032a 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -26,15 +26,20 @@
>  #include "qemu/log.h"
>  #include "trace.h"
>  
> -//#define DEBUG_GIC
> +/* #define DEBUG_GIC */
>  
>  #ifdef DEBUG_GIC
> -#define DPRINTF(fmt, ...) \
> -do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
> +#define DEBUG_GIC_GATE 1
>  #else
> -#define DPRINTF(fmt, ...) do {} while(0)
> +#define DEBUG_GIC_GATE 0
>  #endif
>  
> +#define DPRINTF(fmt, ...) do {                                          \
> +        if (DEBUG_GIC_GATE) {                                           \
> +            fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__);      \
> +        }                                                               \
> +    } while (0)
> +

Seems a prefered way is using qemu_log instead of fprintf?

Thanks,
Fred

>  static const uint8_t gic_id_11mpcore[] = {
>      0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>  };
> 

Re: [Qemu-devel] [PATCH v2 10/11] target/arm/helper: make it clear the EC field is also in hex

[Frederic Konrad]

On 03/02/2017 08:53 PM, Alex Bennée wrote:
> ..just like the rest of the displayed ESR register. Otherwise people
> might scratch their heads if a not obviously hex number is displayed
> for the EC field.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3f4211b572..76b608f0ba 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6857,7 +6857,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>                    new_el);
>      if (qemu_loglevel_mask(CPU_LOG_INT)
>          && !excp_is_internal(cs->exception_index)) {
> -        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
> +        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%x/0x%" PRIx32 "\n",
>                        env->exception.syndrome >> ARM_EL_EC_SHIFT,
>                        env->exception.syndrome);
>      }
> 

This seems OK to me.

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

Re: [Qemu-devel] [PATCH v2 11/11] hw/intc/arm_gic: modernise the DPRINTF

[Peter Maydell]

On 3 March 2017 at 17:05, Frederic Konrad <fred.konrad@greensocs.com> wrote:
> Hi Alex,
>
> On 03/02/2017 08:53 PM, Alex Bennée wrote:
>> While I was debugging the icount issues I realised a bunch of the
>> messages look quite similar. I've fixed this by including __func__ in
>> the debug print. At the same time I move the a modern if (GATE) style
>> printf which ensures the compiler can check for format string errors
>> even if the code gets optimised away in the non-DEBUG_GIC case.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  hw/intc/arm_gic.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 8e5a9d8a3e..b305d9032a 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -26,15 +26,20 @@
>>  #include "qemu/log.h"
>>  #include "trace.h"
>>
>> -//#define DEBUG_GIC
>> +/* #define DEBUG_GIC */
>>
>>  #ifdef DEBUG_GIC
>> -#define DPRINTF(fmt, ...) \
>> -do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
>> +#define DEBUG_GIC_GATE 1
>>  #else
>> -#define DPRINTF(fmt, ...) do {} while(0)
>> +#define DEBUG_GIC_GATE 0
>>  #endif
>>
>> +#define DPRINTF(fmt, ...) do {                                          \
>> +        if (DEBUG_GIC_GATE) {                                           \
>> +            fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__);      \
>> +        }                                                               \
>> +    } while (0)
>> +
>
> Seems a prefered way is using qemu_log instead of fprintf?

This is debug print. Our "preferred" approach is the trace-events
framework (look at how gicv3 does it for example), which has the
nice property of being user-enableable at runtime, but there's
no absolute requirement to update old code to use that instead of
debug printfs. (It's not completely trivial to update, and among
other things you need to consider what trace might be useful to
the user rather than merely whatever random fprintfs you found
helpful during development.)

qemu_log is for certain specific kinds of user-enableable tracing
(notably "guest did something wrong" and "we don't implement this").

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 00/11] MTTCG fixups for 2.9

[Frederic Konrad]

Hi All,

I've a strangeness with the "drop iolock" patch.
It seems it has an impact on the MMIO execution patch-set I'm working
on:

Basically modifying the memory tree is causing a Bad Ram Address error.
I wonder if this can't happen with hotplug/unplug model as well.

I'll look into this.
Shoot if you have any evidence of why that doesn't work :).

Thanks,
Fred

On 03/02/2017 08:53 PM, Alex Bennée wrote:
> Hi Peter,
> 
> So perhaps predictably the merging of MTTCG has thrown up some issues
> during soft-freeze. The following patches fix up some of those:
> 
> The following where in v1 and just have r-b tags:
> 
>   vl/cpus: be smarter with icount and MTTCG
>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
> 
> The next change downgrades the asserts to tcg_debug_asserts. This will
> reduce the instances of production builds failing on non-MTTCG enabled
> guests. The races still need to be fixed but you now have to
> --enable-debug-tcg to go hunting for them:
> 
>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
> 
> This never came up in my testing but it seems some guests can
> translate while doing a tlb_fill. The call to cpu_restore_state never
> worked before (as we are translating the block you are looking for)
> but by bugging out we avoid the double tb_lock().
> 
>   translate-all: exit cpu_restore_state early if translating
> 
> Then we have a bunch of fixes from the reports on the list. They are
> safe to merge but I'll leave that up to the maintainers. There may be
> an argument for holding these patches back until the guest is
> converted to be MTTCG aware and a full audit of the CPU<->HW emulation
> boundaries is done for each one:
> 
>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>   s390x/misc_helper.c: wrap IO instructions in BQL
>   target/xtensa: hold BQL for interrupt processing
>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
> 
> Finally these are just tiny debug fixes while investigating the icount
> regression:
> 
>   target/arm/helper: make it clear the EC field is also in hex
>   hw/intc/arm_gic: modernise the DPRINTF
> 
> Going forward I think 1-5 are ready to be merged now. For 6-9 we
> should await decisions from the target maintainers. The last two are
> entirely up to you.
> 
> It looks like the -icount regression is a poor interaction between
> icount timers and the BQL but this is going to require discussion with
> Paolo in the morning.
> 
> Cheers,
> 
> 
> Alex Bennée (11):
>   vl/cpus: be smarter with icount and MTTCG
>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
>   translate-all: exit cpu_restore_state early if translating
>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>   s390x/misc_helper.c: wrap IO instructions in BQL
>   target/xtensa: hold BQL for interrupt processing
>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
>   target/arm/helper: make it clear the EC field is also in hex
>   hw/intc/arm_gic: modernise the DPRINTF
> 
>  cpus.c                      | 11 +++++++----
>  hw/intc/arm_gic.c           | 13 +++++++++----
>  hw/sparc/sun4m.c            |  3 +++
>  hw/sparc64/sparc64.c        |  3 +++
>  target/arm/helper.c         |  2 +-
>  target/i386/cpu.h           |  3 +++
>  target/mips/op_helper.c     | 17 ++++++++++++++---
>  target/s390x/misc_helper.c  | 21 +++++++++++++++++++++
>  target/sparc/int64_helper.c |  3 +++
>  target/sparc/win_helper.c   | 11 +++++++++++
>  target/xtensa/helper.c      |  1 +
>  target/xtensa/op_helper.c   |  7 +++++++
>  translate-all.c             |  9 ++++++++-
>  translate-common.c          |  3 ++-
>  vl.c                        |  7 ++-----
>  15 files changed, 95 insertions(+), 19 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 11/11] hw/intc/arm_gic: modernise the DPRINTF

[Peter Maydell]

On 2 March 2017 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
> While I was debugging the icount issues I realised a bunch of the
> messages look quite similar. I've fixed this by including __func__ in
> the debug print. At the same time I move the a modern if (GATE) style
> printf which ensures the compiler can check for format string errors
> even if the code gets optimised away in the non-DEBUG_GIC case.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 10/11] target/arm/helper: make it clear the EC field is also in hex

[Peter Maydell]

On 2 March 2017 at 19:53, Alex Bennée <alex.bennee@linaro.org> wrote:
> ..just like the rest of the displayed ESR register. Otherwise people
> might scratch their heads if a not obviously hex number is displayed
> for the EC field.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3f4211b572..76b608f0ba 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6857,7 +6857,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>                    new_el);
>      if (qemu_loglevel_mask(CPU_LOG_INT)
>          && !excp_is_internal(cs->exception_index)) {
> -        qemu_log_mask(CPU_LOG_INT, "...with ESR %x/0x%" PRIx32 "\n",
> +        qemu_log_mask(CPU_LOG_INT, "...with ESR 0x%x/0x%" PRIx32 "\n",
>                        env->exception.syndrome >> ARM_EL_EC_SHIFT,
>                        env->exception.syndrome);
>      }
> --

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 02/11] target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO

[Eduardo Habkost]

On Thu, Mar 02, 2017 at 07:53:28PM +0000, Alex Bennée wrote:
> This suppresses the incorrect warning when forcing MTTCG for x86
> guests on x86 hosts. A future patch will still warn when
> TARGET_SUPPORT_MTTCG hasn't been defined for the guest (which is still
> pending for x86).
> 
> Reported-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I assume the whole series is going to be merged through the same
tree, so:

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  target/i386/cpu.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 12a39d590f..ecdd3bbc2a 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -30,6 +30,9 @@
>  #define TARGET_LONG_BITS 32
>  #endif
>  
> +/* The x86 has a strong memory model with some store-after-load re-ordering */
> +#define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
> +
>  /* Maximum instruction code size */
>  #define TARGET_MAX_INSN_SIZE 16
>  
> -- 
> 2.11.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Richard Henderson]

On 03/03/2017 10:19 PM, Peter Maydell wrote:
> On 3 March 2017 at 11:05, Alex Bennée <alex.bennee@linaro.org> wrote:
>> According to the commit that added it
>> (c552d6c038f7cf4058d1fd5987118ffd41e0e050) it is meant to be a hint to
>> the compiler. Reading the GCC notes however seems to contradict that.
>>
>> FWIW I did test it in both builds and we do used tese for a bunch of
>> other asserts and they haven't blown up.
>>
>>> If what we want is "don't actually check this condition in
>>> the non-tcg-debug config" then we should do something
>>> that means we don't actually check the condition...
>>
>> Hmm:
>>
>> 28      intptr_t qemu_real_host_page_mask;
>> 29
>> 30      #ifndef CONFIG_USER_ONLY
>> 31      /* mask must never be zero, except for A20 change call */
>> 32      static void tcg_handle_interrupt(CPUState *cpu, int mask)
>> 33      {
>> 34          int old_mask;
>> 35          tcg_debug_assert(qemu_mutex_iothread_locked());
>> 36
>> 37          old_mask = cpu->interrupt_request;
>> Line 34 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0a <tcg_handle_interrupt+10> but contains no code.
>> Line 35 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0a <tcg_handle_interrupt+10> and ends at 0x24db0f <tcg_handle_interrupt+15>.
>> Line 36 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0f <tcg_handle_interrupt+15> but contains no code.
>> Line 37 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0f <tcg_handle_interrupt+15> and ends at 0x24db15 <tcg_handle_interrupt+21>.
>>    0x24db0a <tcg_handle_interrupt+10>:  callq  0x27a570 <qemu_mutex_iothread_locked>
>>    0x24db0f <tcg_handle_interrupt+15>:  mov    0xa8(%rbx),%ebp
>>    0x24db15 <tcg_handle_interrupt+21>:  mov    %r12d,%eax
>>    0x24db18 <tcg_handle_interrupt+24>:  mov    %rbx,%rdi
>>    0x24db1b <tcg_handle_interrupt+27>:  or     %ebp,%eax
>>    0x24db1d <tcg_handle_interrupt+29>:  mov    %eax,0xa8(%rbx)
>>    0x24db23 <tcg_handle_interrupt+35>:  callq  0x27a530 <qemu_cpu_is_self>
>>
>> It certainly looks as though it makes the call but ignores the result?
>
> That is one permitted implementation of the undefined behaviour,
> certainly. Not the only one, though. In particular you're telling
> the compiler's optimization passes "this can never be reached"
> which can result in the optimizer making significantly different
> decisions (especially if it manages to inline things or it can
> look "inside" the condition being asserted here, etc etc).

Yep, Peter's right.

Which is exactly the point when you have a condition like (X > 0); letting the 
compiler have the same information for the production build that it would have 
gleaned from the debug build.

But that's not the same as dropping the assert, which is what you wanted.

On the other hand, isn't "assert" instead of "g_assert" exactly what you 
wanted?  Don't we define NDEBUG for production builds?


r~

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Eric Blake]

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

On 03/03/2017 01:35 PM, Richard Henderson wrote:
> 
> Which is exactly the point when you have a condition like (X > 0);
> letting the compiler have the same information for the production build
> that it would have gleaned from the debug build.
> 
> But that's not the same as dropping the assert, which is what you wanted.
> 
> On the other hand, isn't "assert" instead of "g_assert" exactly what you
> wanted?  Don't we define NDEBUG for production builds?

No - no one in their right mind defines NDEBUG for qemu.  For better or
worse, there are too many places where we liberally use assert(), and
where the code WILL crash and burn when it falls through to subsequent
code if a failed assert() is not equivalent to a fatal exit.  (I still
try to make sure we avoid any new side-effects in assert in my code
reviews, but there's no way you'll convince me to audit the code base
for NDEBUG-safety violations).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert

[Eric Blake]

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

On 03/03/2017 01:47 PM, Eric Blake wrote:
> On 03/03/2017 01:35 PM, Richard Henderson wrote:
>>
>> Which is exactly the point when you have a condition like (X > 0);
>> letting the compiler have the same information for the production build
>> that it would have gleaned from the debug build.
>>
>> But that's not the same as dropping the assert, which is what you wanted.
>>
>> On the other hand, isn't "assert" instead of "g_assert" exactly what you
>> wanted?  Don't we define NDEBUG for production builds?
> 
> No - no one in their right mind defines NDEBUG for qemu.  For better or
> worse, there are too many places where we liberally use assert(), and
> where the code WILL crash and burn when it falls through to subsequent
> code if a failed assert() is not equivalent to a fatal exit.  (I still
> try to make sure we avoid any new side-effects in assert in my code
> reviews, but there's no way you'll convince me to audit the code base
> for NDEBUG-safety violations).

A quick git grep shows, among others:

hw/scsi/mptsas.c:     * When we do, we might be able to re-enable NDEBUG
below.
hw/scsi/mptsas.c:#ifdef NDEBUG
hw/scsi/mptsas.c:#error building with NDEBUG is not supported
hw/virtio/virtio.c:     * When we do, we might be able to re-enable
NDEBUG below.
hw/virtio/virtio.c:#ifdef NDEBUG
hw/virtio/virtio.c:#error building with NDEBUG is not supported


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 05/11] translate-all: exit cpu_restore_state early if translating

[Richard Henderson]

On 03/03/2017 09:03 PM, Alex Bennée wrote:
>> We *should* have retaddr == 0 for this case, which indicates that we
>> should not attempt to restore state.  Are you seeing a non-zero value?
>
> Actually looking at xtensa I see:
>
>   Attempt to resolve CPU state @ 0x0 while translating
>
> So maybe I should check just that - but I don't see where we ensure we
> always pass zero.

cpu_ld*_cmmu, in include/exec/cpu_ldst_template.h, has

     return glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(env, ptr, 0);

so there's your zero.

> #0  0x00005555555e3712 in cpu_restore_state (cpu=cpu@entry=0x555556032600, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/translate-all.c:338
> #1  0x000055555564cb38 in tlb_fill (cs=cs@entry=0x555556032600, vaddr=vaddr@entry=537034752, access_type=access_type@entry=MMU_INST_FETCH, mmu_idx=mmu_idx@entry=1, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/target/xtensa/op_helper.c:73
> #2  0x000055555562d604 in helper_ret_ldb_cmmu (env=env@entry=0x55555603a890, addr=537034752, oi=<optimised out>, retaddr=retaddr@entry=0) at /home/alex/lsrc/qemu/qemu.git/softmmu_template.h:127

Confirmed.  This is a simple bug in xtensa -- failure to check retaddr == 0 
before calling cpu_restore_state.

That said, I do wonder if it wouldn't be better to move that check inside 
cpu_restore_state instead.  Put the check there now, but leave the follow-on 
cleanup for the next devel cycle.

It would also save auditing the other usages of cpu_restore_state in the tree.

>> Is this in fact linux-user, not softmmu, as you imply from tlb_fill?
>> Because handle_cpu_signal will in fact pass a genuine non-zero retaddr
>> for the SIGSEGV resulting from a cpu_ld*_code from a non-mapped
>> address.
>
> I think that is another call chain that might trip us up. Peter
> mentioned he'd hit it. This one is definitely softmmu.

This is really easy to reproduce on any guest with a call to a null function 
pointer.


r~

Re: [Qemu-devel] [PATCH v2 00/11] MTTCG fixups for 2.9

[Alex Bennée]

Frederic Konrad <fred.konrad@greensocs.com> writes:

> Hi All,
>
> I've a strangeness with the "drop iolock" patch.
> It seems it has an impact on the MMIO execution patch-set I'm working
> on:

Hmm the only real change is the BQL (implicit or explict) being taken on
entry to the mmio functions.

> Basically modifying the memory tree is causing a Bad Ram Address error.
> I wonder if this can't happen with hotplug/unplug model as well.

Isn't this all done under an RCU? I don't think any of this should have
changed for the MTTCG patch series.

> I'll look into this.
> Shoot if you have any evidence of why that doesn't work :).

--enable-tcg-debug will turn on more lock checking (at a performance
  hit). You could try that.

>
> Thanks,
> Fred
>
> On 03/02/2017 08:53 PM, Alex Bennée wrote:
>> Hi Peter,
>>
>> So perhaps predictably the merging of MTTCG has thrown up some issues
>> during soft-freeze. The following patches fix up some of those:
>>
>> The following where in v1 and just have r-b tags:
>>
>>   vl/cpus: be smarter with icount and MTTCG
>>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
>>
>> The next change downgrades the asserts to tcg_debug_asserts. This will
>> reduce the instances of production builds failing on non-MTTCG enabled
>> guests. The races still need to be fixed but you now have to
>> --enable-debug-tcg to go hunting for them:
>>
>>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
>>
>> This never came up in my testing but it seems some guests can
>> translate while doing a tlb_fill. The call to cpu_restore_state never
>> worked before (as we are translating the block you are looking for)
>> but by bugging out we avoid the double tb_lock().
>>
>>   translate-all: exit cpu_restore_state early if translating
>>
>> Then we have a bunch of fixes from the reports on the list. They are
>> safe to merge but I'll leave that up to the maintainers. There may be
>> an argument for holding these patches back until the guest is
>> converted to be MTTCG aware and a full audit of the CPU<->HW emulation
>> boundaries is done for each one:
>>
>>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>>   s390x/misc_helper.c: wrap IO instructions in BQL
>>   target/xtensa: hold BQL for interrupt processing
>>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
>>
>> Finally these are just tiny debug fixes while investigating the icount
>> regression:
>>
>>   target/arm/helper: make it clear the EC field is also in hex
>>   hw/intc/arm_gic: modernise the DPRINTF
>>
>> Going forward I think 1-5 are ready to be merged now. For 6-9 we
>> should await decisions from the target maintainers. The last two are
>> entirely up to you.
>>
>> It looks like the -icount regression is a poor interaction between
>> icount timers and the BQL but this is going to require discussion with
>> Paolo in the morning.
>>
>> Cheers,
>>
>>
>> Alex Bennée (11):
>>   vl/cpus: be smarter with icount and MTTCG
>>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
>>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
>>   translate-all: exit cpu_restore_state early if translating
>>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>>   s390x/misc_helper.c: wrap IO instructions in BQL
>>   target/xtensa: hold BQL for interrupt processing
>>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
>>   target/arm/helper: make it clear the EC field is also in hex
>>   hw/intc/arm_gic: modernise the DPRINTF
>>
>>  cpus.c                      | 11 +++++++----
>>  hw/intc/arm_gic.c           | 13 +++++++++----
>>  hw/sparc/sun4m.c            |  3 +++
>>  hw/sparc64/sparc64.c        |  3 +++
>>  target/arm/helper.c         |  2 +-
>>  target/i386/cpu.h           |  3 +++
>>  target/mips/op_helper.c     | 17 ++++++++++++++---
>>  target/s390x/misc_helper.c  | 21 +++++++++++++++++++++
>>  target/sparc/int64_helper.c |  3 +++
>>  target/sparc/win_helper.c   | 11 +++++++++++
>>  target/xtensa/helper.c      |  1 +
>>  target/xtensa/op_helper.c   |  7 +++++++
>>  translate-all.c             |  9 ++++++++-
>>  translate-common.c          |  3 ++-
>>  vl.c                        |  7 ++-----
>>  15 files changed, 95 insertions(+), 19 deletions(-)
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 06/11] sparc/sparc64: grab BQL before calling cpu_check_irqs

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/03/2017 20:53, Alex Bennée wrote:
>> IRQ modification is part of device emulation and should be done while
>> the BQL is held to prevent races when MTTCG is enabled. This adds
>> assertions in the hw emulation layer and wraps the calls from helpers
>> in the BQL.
>>
>> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  hw/sparc/sun4m.c            |  3 +++
>>  hw/sparc64/sparc64.c        |  3 +++
>>  target/sparc/int64_helper.c |  3 +++
>>  target/sparc/win_helper.c   | 11 +++++++++++
>>  4 files changed, 20 insertions(+)
>>
>> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
>> index 61416a6426..873cd7df9a 100644
>> --- a/hw/sparc/sun4m.c
>> +++ b/hw/sparc/sun4m.c
>> @@ -142,6 +142,9 @@ void cpu_check_irqs(CPUSPARCState *env)
>>  {
>>      CPUState *cs;
>>
>> +    /* We should be holding the BQL before we mess with IRQs */
>> +    g_assert(qemu_mutex_iothread_locked());
>> +
>>      if (env->pil_in && (env->interrupt_index == 0 ||
>>                          (env->interrupt_index & ~15) == TT_EXTINT)) {
>>          unsigned int i;
>> diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
>> index b3d219c769..4e4fdab065 100644
>> --- a/hw/sparc64/sparc64.c
>> +++ b/hw/sparc64/sparc64.c
>> @@ -55,6 +55,9 @@ void cpu_check_irqs(CPUSPARCState *env)
>>      uint32_t pil = env->pil_in |
>>                    (env->softint & ~(SOFTINT_TIMER | SOFTINT_STIMER));
>>
>> +    /* We should be holding the BQL before we mess with IRQs */
>> +    g_assert(qemu_mutex_iothread_locked());
>> +
>>      /* TT_IVEC has a higher priority (16) than TT_EXTINT (31..17) */
>>      if (env->ivec_status & 0x20) {
>>          return;
>> diff --git a/target/sparc/int64_helper.c b/target/sparc/int64_helper.c
>> index 605747c93c..f942973c22 100644
>> --- a/target/sparc/int64_helper.c
>> +++ b/target/sparc/int64_helper.c
>> @@ -18,6 +18,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>  #include "cpu.h"
>>  #include "exec/helper-proto.h"
>>  #include "exec/log.h"
>> @@ -208,7 +209,9 @@ static bool do_modify_softint(CPUSPARCState *env, uint32_t value)
>>          env->softint = value;
>>  #if !defined(CONFIG_USER_ONLY)
>>          if (cpu_interrupts_enabled(env)) {
>> +            qemu_mutex_lock_iothread();
>>              cpu_check_irqs(env);
>> +            qemu_mutex_unlock_iothread();
>>          }
>>  #endif
>>          return true;
>> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
>> index 71b3dd37e8..b387b026d8 100644
>> --- a/target/sparc/win_helper.c
>> +++ b/target/sparc/win_helper.c
>> @@ -18,6 +18,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>>  #include "cpu.h"
>>  #include "exec/exec-all.h"
>>  #include "exec/helper-proto.h"
>> @@ -86,7 +87,9 @@ void cpu_put_psr(CPUSPARCState *env, target_ulong val)
>>  {
>>      cpu_put_psr_raw(env, val);
>>  #if ((!defined(TARGET_SPARC64)) && !defined(CONFIG_USER_ONLY))
>> +    qemu_mutex_lock_iothread();
>>      cpu_check_irqs(env);
>> +    qemu_mutex_unlock_iothread();
>>  #endif
>>  }
>
> This can be called from gdbstub, so you need to put the lock/unlock
> around helper_wrpsr's call to cpu_put_psr instead.  Also please add a
> comment /* Called with BQL held.  */ around cpu_put_psr.

OK will do. I have to say its hard to see the gdbstub being under the
BQL. Is this a feature of the packet handling being done in the main IO
thread?

>
> Paolo
>
>> @@ -368,7 +371,9 @@ void helper_wrpstate(CPUSPARCState *env, target_ulong new_state)
>>
>>  #if !defined(CONFIG_USER_ONLY)
>>      if (cpu_interrupts_enabled(env)) {
>> +        qemu_mutex_lock_iothread();
>>          cpu_check_irqs(env);
>> +        qemu_mutex_unlock_iothread();
>>      }
>>  #endif
>>  }
>> @@ -381,7 +386,9 @@ void helper_wrpil(CPUSPARCState *env, target_ulong new_pil)
>>      env->psrpil = new_pil;
>>
>>      if (cpu_interrupts_enabled(env)) {
>> +        qemu_mutex_lock_iothread();
>>          cpu_check_irqs(env);
>> +        qemu_mutex_unlock_iothread();
>>      }
>>  #endif
>>  }
>> @@ -408,7 +415,9 @@ void helper_done(CPUSPARCState *env)
>>
>>  #if !defined(CONFIG_USER_ONLY)
>>      if (cpu_interrupts_enabled(env)) {
>> +        qemu_mutex_lock_iothread();
>>          cpu_check_irqs(env);
>> +        qemu_mutex_unlock_iothread();
>>      }
>>  #endif
>>  }
>> @@ -435,7 +444,9 @@ void helper_retry(CPUSPARCState *env)
>>
>>  #if !defined(CONFIG_USER_ONLY)
>>      if (cpu_interrupts_enabled(env)) {
>> +        qemu_mutex_lock_iothread();
>>          cpu_check_irqs(env);
>> +        qemu_mutex_unlock_iothread();
>>      }
>>  #endif
>>  }
>>


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 00/11] MTTCG fixups for 2.9

[Frederic Konrad]

On 03/06/2017 10:43 AM, Alex Bennée wrote:
> 
> Frederic Konrad <fred.konrad@greensocs.com> writes:
> 
>> Hi All,
>>
>> I've a strangeness with the "drop iolock" patch.
>> It seems it has an impact on the MMIO execution patch-set I'm working
>> on:
> 
> Hmm the only real change is the BQL (implicit or explict) being taken on
> entry to the mmio functions.

Yes maybe it just trigger a bug more easily..

Fred

> 
>> Basically modifying the memory tree is causing a Bad Ram Address error.
>> I wonder if this can't happen with hotplug/unplug model as well.
> 
> Isn't this all done under an RCU? I don't think any of this should have
> changed for the MTTCG patch series.
> 
>> I'll look into this.
>> Shoot if you have any evidence of why that doesn't work :).
> 
> --enable-tcg-debug will turn on more lock checking (at a performance
>   hit). You could try that.
> 
>>
>> Thanks,
>> Fred
>>
>> On 03/02/2017 08:53 PM, Alex Bennée wrote:
>>> Hi Peter,
>>>
>>> So perhaps predictably the merging of MTTCG has thrown up some issues
>>> during soft-freeze. The following patches fix up some of those:
>>>
>>> The following where in v1 and just have r-b tags:
>>>
>>>   vl/cpus: be smarter with icount and MTTCG
>>>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>>>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
>>>
>>> The next change downgrades the asserts to tcg_debug_asserts. This will
>>> reduce the instances of production builds failing on non-MTTCG enabled
>>> guests. The races still need to be fixed but you now have to
>>> --enable-debug-tcg to go hunting for them:
>>>
>>>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
>>>
>>> This never came up in my testing but it seems some guests can
>>> translate while doing a tlb_fill. The call to cpu_restore_state never
>>> worked before (as we are translating the block you are looking for)
>>> but by bugging out we avoid the double tb_lock().
>>>
>>>   translate-all: exit cpu_restore_state early if translating
>>>
>>> Then we have a bunch of fixes from the reports on the list. They are
>>> safe to merge but I'll leave that up to the maintainers. There may be
>>> an argument for holding these patches back until the guest is
>>> converted to be MTTCG aware and a full audit of the CPU<->HW emulation
>>> boundaries is done for each one:
>>>
>>>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>>>   s390x/misc_helper.c: wrap IO instructions in BQL
>>>   target/xtensa: hold BQL for interrupt processing
>>>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
>>>
>>> Finally these are just tiny debug fixes while investigating the icount
>>> regression:
>>>
>>>   target/arm/helper: make it clear the EC field is also in hex
>>>   hw/intc/arm_gic: modernise the DPRINTF
>>>
>>> Going forward I think 1-5 are ready to be merged now. For 6-9 we
>>> should await decisions from the target maintainers. The last two are
>>> entirely up to you.
>>>
>>> It looks like the -icount regression is a poor interaction between
>>> icount timers and the BQL but this is going to require discussion with
>>> Paolo in the morning.
>>>
>>> Cheers,
>>>
>>>
>>> Alex Bennée (11):
>>>   vl/cpus: be smarter with icount and MTTCG
>>>   target/i386/cpu.h: declare TCG_GUEST_DEFAULT_MO
>>>   cpus.c: add additional error_report when !TARGET_SUPPORT_MTTCG
>>>   translate: downgrade IRQ BQL asserts to tcg_debug_assert
>>>   translate-all: exit cpu_restore_state early if translating
>>>   sparc/sparc64: grab BQL before calling cpu_check_irqs
>>>   s390x/misc_helper.c: wrap IO instructions in BQL
>>>   target/xtensa: hold BQL for interrupt processing
>>>   target/mips/op_helper: hold BQL before calling cpu_mips_get_count
>>>   target/arm/helper: make it clear the EC field is also in hex
>>>   hw/intc/arm_gic: modernise the DPRINTF
>>>
>>>  cpus.c                      | 11 +++++++----
>>>  hw/intc/arm_gic.c           | 13 +++++++++----
>>>  hw/sparc/sun4m.c            |  3 +++
>>>  hw/sparc64/sparc64.c        |  3 +++
>>>  target/arm/helper.c         |  2 +-
>>>  target/i386/cpu.h           |  3 +++
>>>  target/mips/op_helper.c     | 17 ++++++++++++++---
>>>  target/s390x/misc_helper.c  | 21 +++++++++++++++++++++
>>>  target/sparc/int64_helper.c |  3 +++
>>>  target/sparc/win_helper.c   | 11 +++++++++++
>>>  target/xtensa/helper.c      |  1 +
>>>  target/xtensa/op_helper.c   |  7 +++++++
>>>  translate-all.c             |  9 ++++++++-
>>>  translate-common.c          |  3 ++-
>>>  vl.c                        |  7 ++-----
>>>  15 files changed, 95 insertions(+), 19 deletions(-)
>>>
> 
> 
> --
> Alex Bennée
> 

Re: [Qemu-devel] [PATCH v2 06/11] sparc/sparc64: grab BQL before calling cpu_check_irqs

[Paolo Bonzini]

On 06/03/2017 11:28, Alex Bennée wrote:
>> This can be called from gdbstub, so you need to put the lock/unlock
>> around helper_wrpsr's call to cpu_put_psr instead.  Also please add a
>> comment /* Called with BQL held.  */ around cpu_put_psr.
> 
> OK will do. I have to say its hard to see the gdbstub being under the
> BQL. Is this a feature of the packet handling being done in the main IO
> thread?

Yes.

It's probably nigh time to have stronger debug facilities for locks,
including changing our lock policy comments to optional assertions.

I'm even thinking of doing something like C++'s std::unique_lock, for
example

    // Takes a lock and unlocks it when the scope is left.
    // c++: std::lock_guard<std::mutex> sl(some_lock);
    QEMU_SCOPED_MUTEX(QemuMutex, some_lock) sl;


    // The lock is also unlocked correctly when the scope is left.
    // c++: std::unique_lock<std::mutex> sl(some_lock);
    QEMU_SCOPED_MUTEX(QemuMutex, some_lock) sl;
    ...
    for (;;) {
        qemu_scoped_mutex_unlock(&sl);
        if (foo) {
            break;
        }
        ...
        qemu_scoped_mutex_lock(&sl);
    }

    // The lock is taken later.
    // c++: std::unique_lock<std::mutex> sl(some_lock, std::defer_lock);
    QEMU_SCOPED_MUTEX_DEFER(QemuMutex, some_lock) sl;
    ...
    if (x) {
        qemu_scoped_mutex_lock(&sl);
    }

Paolo

Re: [Qemu-devel] [PATCH v2 08/11] target/xtensa: hold BQL for interrupt processing

[Max Filippov]

On Thu, Mar 2, 2017 at 11:53 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> Make sure we have the BQL held when processing interrupts.
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/xtensa/helper.c    | 1 +
>  target/xtensa/op_helper.c | 7 +++++++
>  2 files changed, 8 insertions(+)

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max