[Qemu-devel] [PATCH v2 0/6] MTTCG fixes for rc1

[Qemu-devel] [PATCH v2 0/6] MTTCG fixes for rc1

[Alex Bennée]

Hi,

Here is the current batch of fixes for MTTCG regressions for the next
release candidate. I've dropped the speculative atomic cputlb patch
and included 2 fixes to the console code. The first fixes the taking
of the BQL (which I missed) and the second uses the exclusive
mechanism directly in the main thread rather than queuing the work to
run in the vCPU context. I kept them separate as they are logically
discreet changes.

The user-exec and bsd-user fixes have been posted separately before in
the various report threads.

Finally I claim a checkpatch exception for Pranith's newline fix. It
didn't seem worth splitting the string for the sake of a semicolon on
column 81 ;-)

Gerd,

Do you want to take the console patches via your tree or have me
include it in a pull request?

Alex Bennée (5):
  user-exec: handle synchronous signals from QEMU gracefully
  bsd-user: align use of mmap_lock to that of linux-user
  ui/console: ensure do_safe_dpy_refresh holds BQL
  ui/console: use exclusive mechanism directly
  replay/replay.c: bump REPLAY_VERSION

Pranith Kumar (1):
  tcg: Add a new line after incompatibility warning

 bsd-user/mmap.c   | 13 +------------
 bsd-user/qemu.h   |  2 --
 cpu-exec-common.c |  2 +-
 cpus.c            |  2 +-
 replay/replay.c   |  2 +-
 ui/console.c      | 18 +++++++++++-------
 user-exec.c       | 18 +++++++++++++++---
 7 files changed, 30 insertions(+), 27 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/6] user-exec: handle synchronous signals from QEMU gracefully

[Alex Bennée]

When "tcg: enable thread-per-vCPU" (commit 3725794) was merged the
lifetime of current_cpu was changed. Previously a broken linux-user
call might abort() which can eventually escalate into a SIGSEGV which
would then crash qemu as it attempted to deref a NULL current_cpu.
After commit 3725794 it would attempt to fixup state and re-start the
run-loop and much hilarity (i.e. a looping lockup) would ensue from
jumping into a stale jmp_env.

As we can actually tell if we are in the run-loop from looking at the
cpu->running flag we should catch this badness first and abort()
cleanly rather than try to soldier on. There is a theoretical race
between the flag being set and sigsetjmp refreshing the jump buffer
but we can try really hard to not introduce crashes into that code.

[LV: setgroups03 fails on powerpc LTP]
Reported-by: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 user-exec.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/user-exec.c b/user-exec.c
index 6db075884d..a8f95fa1e1 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -57,10 +57,23 @@ static void cpu_exit_tb_from_sighandler(CPUState *cpu, sigset_t *old_set)
 static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
                                     int is_write, sigset_t *old_set)
 {
-    CPUState *cpu;
+    CPUState *cpu = current_cpu;
     CPUClass *cc;
     int ret;
 
+    /* For synchronous signals we expect to be coming from the vCPU
+     * thread (so current_cpu should be valid) and either from running
+     * code or during translation which can fault as we cross pages.
+     *
+     * If neither is true then something has gone wrong and we should
+     * abort rather than try and restart the vCPU execution.
+     */
+    if (!cpu || !cpu->running) {
+        printf("qemu:%s received signal outside vCPU context @ pc=0x%"
+               PRIxPTR "\n",  __func__, pc);
+        abort();
+    }
+
 #if defined(DEBUG_SIGNAL)
     printf("qemu: SIGSEGV pc=0x%08lx address=%08lx w=%d oldset=0x%08lx\n",
            pc, address, is_write, *(unsigned long *)old_set);
@@ -83,7 +96,7 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
              * currently executing TB was modified and must be exited
              * immediately.
              */
-            cpu_exit_tb_from_sighandler(current_cpu, old_set);
+            cpu_exit_tb_from_sighandler(cpu, old_set);
             g_assert_not_reached();
         default:
             g_assert_not_reached();
@@ -94,7 +107,6 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
        are still valid segv ones */
     address = h2g_nocheck(address);
 
-    cpu = current_cpu;
     cc = CPU_GET_CLASS(cpu);
     /* see if it is an MMU fault */
     g_assert(cc->handle_mmu_fault);
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/6] bsd-user: align use of mmap_lock to that of linux-user

[Alex Bennée]

The introduction of stricter mmap_lock checking in translate-all broke
the BSD user build. The working mmap_lock functions were hidden behind
CONFIG_USE_NPTL which is never defined. This patch brings them inline
with linux-user.

Despite the disapearence of the comment "We aren't threadsafe to start
with..." this doesn't make bsd-user so. It will still need the rest of
the fixes that have been done in linux-user ported over.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 bsd-user/mmap.c | 13 +------------
 bsd-user/qemu.h |  2 --
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 1ad018a127..7f2018ede0 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -24,8 +24,7 @@
 
 //#define DEBUG_MMAP
 
-#if defined(CONFIG_USE_NPTL)
-pthread_mutex_t mmap_mutex;
+static pthread_mutex_t mmap_mutex = PTHREAD_MUTEX_INITIALIZER;
 static int __thread mmap_lock_count;
 
 void mmap_lock(void)
@@ -62,16 +61,6 @@ void mmap_fork_end(int child)
     else
         pthread_mutex_unlock(&mmap_mutex);
 }
-#else
-/* We aren't threadsafe to start with, so no need to worry about locking.  */
-void mmap_lock(void)
-{
-}
-
-void mmap_unlock(void)
-{
-}
-#endif
 
 /* NOTE: all the constants are the HOST ones, but addresses are target. */
 int target_mprotect(abi_ulong start, abi_ulong len, int prot)
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 2b2b9184e0..b550cee0cb 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -209,10 +209,8 @@ 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;
-#if defined(CONFIG_USE_NPTL)
 void mmap_fork_start(void);
 void mmap_fork_end(int child);
-#endif
 
 /* main.c */
 extern unsigned long x86_stack_size;
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/6] ui/console: ensure do_safe_dpy_refresh holds BQL

[Alex Bennée]

I missed the fact that when an exclusive work item runs it drops the
BQL to ensure all no vCPUs are stuck waiting for it, hence causing a
deadlock. However the actual helper needs to take the BQL especially
as we'll be messing with device emulation bits during the update which
all assume BQL is held.

We make a minor cpu_reloading_memory_map which must try and unlock the
RCU if we are actually outside the running context.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpu-exec-common.c | 2 +-
 ui/console.c      | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cpu-exec-common.c b/cpu-exec-common.c
index 0504a9457b..e81da276bb 100644
--- a/cpu-exec-common.c
+++ b/cpu-exec-common.c
@@ -35,7 +35,7 @@ void cpu_loop_exit_noexc(CPUState *cpu)
 #if defined(CONFIG_SOFTMMU)
 void cpu_reloading_memory_map(void)
 {
-    if (qemu_in_vcpu_thread()) {
+    if (qemu_in_vcpu_thread() && current_cpu->running) {
         /* The guest can in theory prolong the RCU critical section as long
          * as it feels like. The major problem with this is that because it
          * can do multiple reconfigurations of the memory map within the
diff --git a/ui/console.c b/ui/console.c
index 937c950840..dd27c9501b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1586,7 +1586,9 @@ bool dpy_gfx_check_format(QemuConsole *con,
 static void do_safe_dpy_refresh(CPUState *cpu, run_on_cpu_data opaque)
 {
     DisplayChangeListener *dcl = opaque.host_ptr;
+    qemu_mutex_lock_iothread();
     dcl->ops->dpy_refresh(dcl);
+    qemu_mutex_unlock_iothread();
 }
 
 static void dpy_refresh(DisplayState *s)
-- 
2.11.0

[Qemu-devel] [PATCH v2 4/6] ui/console: use exclusive mechanism directly

[Alex Bennée]

The previous commit (8bb93c6f99) using async_safe_run_on_cpu() doesn't
work on graphics sub-system which restrict which threads can do GUI
updates. Rather the special casing MacOS we just directly call the
helper and move all the exclusive handling into do_dafe_dpy_refresh().

The unfortunate bouncing of the BQL is to ensure there is no deadlock
as vCPUs waiting on the BQL are kicked into their quiescent state.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 ui/console.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index dd27c9501b..419b098c11 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1576,19 +1576,22 @@ bool dpy_gfx_check_format(QemuConsole *con,
 }
 
 /*
- * Safe DPY refresh for TCG guests. This runs when the TCG vCPUs are
- * quiescent so we can avoid races between dirty page tracking for
- * direct frame-buffer access by the guest.
+ * Safe DPY refresh for TCG guests. We use the exclusive mechanism to
+ * ensure the TCG vCPUs are quiescent so we can avoid races between
+ * dirty page tracking for direct frame-buffer access by the guest.
  *
  * This is a temporary stopgap until we've fixed the dirty tracking
  * races in display adapters.
  */
-static void do_safe_dpy_refresh(CPUState *cpu, run_on_cpu_data opaque)
+static void do_safe_dpy_refresh(DisplayChangeListener *dcl)
 {
-    DisplayChangeListener *dcl = opaque.host_ptr;
+    qemu_mutex_unlock_iothread();
+    start_exclusive();
     qemu_mutex_lock_iothread();
     dcl->ops->dpy_refresh(dcl);
     qemu_mutex_unlock_iothread();
+    end_exclusive();
+    qemu_mutex_lock_iothread();
 }
 
 static void dpy_refresh(DisplayState *s)
@@ -1598,8 +1601,7 @@ static void dpy_refresh(DisplayState *s)
     QLIST_FOREACH(dcl, &s->listeners, next) {
         if (dcl->ops->dpy_refresh) {
             if (tcg_enabled()) {
-                async_safe_run_on_cpu(first_cpu, do_safe_dpy_refresh,
-                                      RUN_ON_CPU_HOST_PTR(dcl));
+                do_safe_dpy_refresh(dcl);
             } else {
                 dcl->ops->dpy_refresh(dcl);
             }
-- 
2.11.0

[Qemu-devel] [PATCH v2 5/6] tcg: Add a new line after incompatibility warning

[Alex Bennée]

From: Pranith Kumar <bobby.prani@gmail.com>

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 cpus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 167d9615e1..68fdbc40b9 100644
--- a/cpus.c
+++ b/cpus.c
@@ -209,7 +209,7 @@ void qemu_tcg_configure(QemuOpts *opts, Error **errp)
                 if (!check_tcg_memory_orders_compatible()) {
                     error_report("Guest expects a stronger memory ordering "
                                  "than the host provides");
-                    error_printf("This may cause strange/hard to debug errors");
+                    error_printf("This may cause strange/hard to debug errors\n");
                 }
                 mttcg_enabled = true;
             }
-- 
2.11.0

[Qemu-devel] [PATCH v2 6/6] replay/replay.c: bump REPLAY_VERSION

[Alex Bennée]

A previous commit (3d4d16f4) added support for audio record/playback.
However this breaks the logfile ABI due to the re-ordering of the
ReplayEvents enum. The REPLAY_VERSION check is meant to prevent you
from using old log files in newer QEMUs but this is currently broken.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 replay/replay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/replay/replay.c b/replay/replay.c
index 78e2a7e570..9e0724e756 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -22,7 +22,7 @@
 
 /* Current version of the replay mechanism.
    Increase it when file format changes. */
-#define REPLAY_VERSION              0xe02005
+#define REPLAY_VERSION              0xe02006
 /* Size of replay log header */
 #define HEADER_SIZE                 (sizeof(uint32_t) + sizeof(uint64_t))
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 0/6] MTTCG fixes for rc1

[Paolo Bonzini]

On 28/03/2017 10:36, Alex Bennée wrote:
> 
> Here is the current batch of fixes for MTTCG regressions for the next
> release candidate. I've dropped the speculative atomic cputlb patch
> and included 2 fixes to the console code. The first fixes the taking
> of the BQL (which I missed) and the second uses the exclusive
> mechanism directly in the main thread rather than queuing the work to
> run in the vCPU context. I kept them separate as they are logically
> discreet changes.
> 
> The user-exec and bsd-user fixes have been posted separately before in
> the various report threads.
> 
> Finally I claim a checkpatch exception for Pranith's newline fix. It
> didn't seem worth splitting the string for the sake of a semicolon on
> column 81 ;-)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/6] MTTCG fixes for rc1

[Gerd Hoffmann]

  Hi,

> Gerd,
> 
> Do you want to take the console patches via your tree or have me
> include it in a pull request?

Just include them in your pull req.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd