[Qemu-devel] [PULL 0/6] MTTCG fixes for rc2

[Qemu-devel] [PULL 0/6] MTTCG fixes for rc2

[Alex Bennée]

The following changes since commit ea2afcf5b6727a577cf561fd8fe0d8c397ecc927:

  Merge remote-tracking branch 'remotes/stefanha/tags/tracing-pull-request' into staging (2017-03-24 14:14:18 +0000)

are available in the git repository at:

  https://github.com/stsquad/qemu.git tags/pull-mttcg-fixups-for-rc2-280317-1

for you to fetch changes up to 5b12c163c830081cbb78e2de3b42c5fe1b73e74e:

  replay/replay.c: bump REPLAY_VERSION (2017-03-28 10:52:50 +0100)

----------------------------------------------------------------
MTTCG regression fixes for rc2

----------------------------------------------------------------
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] [PULL 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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] [PULL 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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] [PULL 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>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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] [PULL 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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] [PULL 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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] [PULL 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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] [PULL 0/6] MTTCG fixes for rc2

[Peter Maydell]

On 28 March 2017 at 12:09, Alex Bennée <alex.bennee@linaro.org> wrote:
> The following changes since commit ea2afcf5b6727a577cf561fd8fe0d8c397ecc927:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/tracing-pull-request' into staging (2017-03-24 14:14:18 +0000)
>
> are available in the git repository at:
>
>   https://github.com/stsquad/qemu.git tags/pull-mttcg-fixups-for-rc2-280317-1
>
> for you to fetch changes up to 5b12c163c830081cbb78e2de3b42c5fe1b73e74e:
>
>   replay/replay.c: bump REPLAY_VERSION (2017-03-28 10:52:50 +0100)
>
> ----------------------------------------------------------------
> MTTCG regression fixes for rc2
>
> ----------------------------------------------------------------
> 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 warnin

Applied, thanks.

-- PMM