[Qemu-devel] [PATCH v1 0/3] MTTCG regression fixes for 2.9-rc1

[Qemu-devel] [PATCH v1 0/3] MTTCG regression fixes for 2.9-rc1

[Alex Bennée]

Hi,

Here are a few more fixes for regressions caused by the MTTCG merge.
There is still one regression I'm aware of left to fix (record/replay
breakage) but I thought it would be worth getting these posted now for
review.

Another fix for graphic artefacts has already been merged via Gerd's
graphics tree.

The first fix is really just for completeness. It wasn't spotted in
the original work and while I'm not aware of a regression attached to
it I fixed it while investigating the other bits.

The user-exec fix solves the looping assert Laurent found with running
LTP tests.

Finally the bsd-user fix is a compile tested only fix to ensure
mmap_lock and friends do actually work. I wasn't able to complete
building on my set-up due to an unrelated optionrom problem but I'm
confident the MTTCG regression is addressed.

Regards,

Alex.

Alex Bennée (3):
  cputlb: ensure tbl_set_dirty1 updates addr_write atomically
  user-exec: handle synchronous signals from QEMU gracefully
  bsd-user: align use of mmap_lock to that of linux-user

 bsd-user/mmap.c | 13 +------------
 bsd-user/qemu.h |  2 --
 cputlb.c        |  8 ++++++++
 user-exec.c     | 18 +++++++++++++++---
 4 files changed, 24 insertions(+), 17 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v1 1/3] cputlb: ensure tbl_set_dirty1 updates addr_write atomically

[Alex Bennée]

This was an oversight when the rest of cputlb was being updated. As
before it falls back to the non-atomic version when the host can't
support wider-than-bus atomics.

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

diff --git a/cputlb.c b/cputlb.c
index f5d056cc08..0d52e45dfd 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -540,9 +540,17 @@ void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length)
 
 static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
 {
+#if TCG_OVERSIZED_GUEST
     if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
         tlb_entry->addr_write = vaddr;
     }
+#else
+    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
+
+    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
+        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
+    }
+#endif
 }
 
 /* update the TLB corresponding to virtual page vaddr
-- 
2.11.0

Re: [Qemu-devel] [PATCH v1 1/3] cputlb: ensure tbl_set_dirty1 updates addr_write atomically

[Paolo Bonzini]

On 20/03/2017 16:34, Alex Bennée wrote:
>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
>  {
> +#if TCG_OVERSIZED_GUEST
>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
>          tlb_entry->addr_write = vaddr;
>      }
> +#else
> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);

atomic_read is enough, since we don't care at all about cases where the
address doesn't match.  Otherwise

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

Paolo

> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
> +    }
> +#endif
>  }

Re: [Qemu-devel] [PATCH v1 1/3] cputlb: ensure tbl_set_dirty1 updates addr_write atomically

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/03/2017 16:34, Alex Bennée wrote:
>>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
>>  {
>> +#if TCG_OVERSIZED_GUEST
>>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
>>          tlb_entry->addr_write = vaddr;
>>      }
>> +#else
>> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
>
> atomic_read is enough, since we don't care at all about cases where the
> address doesn't match.  Otherwise

Good catch. Will fix for my pullreq

>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Paolo
>
>> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
>> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
>> +    }
>> +#endif
>>  }


--
Alex Bennée

Re: [Qemu-devel] [PATCH v1 1/3] cputlb: ensure tbl_set_dirty1 updates addr_write atomically

[Richard Henderson]

On 03/21/2017 01:34 AM, Alex Bennée wrote:
> This was an oversight when the rest of cputlb was being updated. As
> before it falls back to the non-atomic version when the host can't
> support wider-than-bus atomics.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  cputlb.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/cputlb.c b/cputlb.c
> index f5d056cc08..0d52e45dfd 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -540,9 +540,17 @@ void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length)
>
>  static inline void tlb_set_dirty1(CPUTLBEntry *tlb_entry, target_ulong vaddr)
>  {
> +#if TCG_OVERSIZED_GUEST
>      if (tlb_entry->addr_write == (vaddr | TLB_NOTDIRTY)) {
>          tlb_entry->addr_write = vaddr;
>      }
> +#else
> +    uintptr_t orig_addr = atomic_mb_read(&tlb_entry->addr_write);
> +
> +    if (orig_addr == (vaddr | TLB_NOTDIRTY)) {
> +        atomic_cmpxchg(&tlb_entry->addr_write, orig_addr, vaddr);
> +    }

What's the state machine here?  How can the per-cpu tlb change in a way other 
than dirty->clean / clean->dirty?  AFAIK, we shouldn't be swapping out the tlb 
entry to somthing completely different.

So how does cmpxchg improve over atomic_write?


r~

[Qemu-devel] [PATCH v1 2/3] 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>
---
 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

Re: [Qemu-devel] [PATCH v1 2/3] user-exec: handle synchronous signals from QEMU gracefully

[Richard Henderson]

On 03/21/2017 01:34 AM, Alex Bennée wrote:
> 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>
> ---
>  user-exec.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

[Qemu-devel] [PATCH v1 3/3] 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 ee5907330f..9182a8ef39 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