All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
@ 2017-03-05 16:59 Alexander Boettcher
  2017-03-05 21:32 ` Alex Bennée
  2017-03-06 13:15 ` Alex Bennée
  0 siblings, 2 replies; 16+ messages in thread
From: Alexander Boettcher @ 2017-03-05 16:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, rth, fred.konrad, pbonzini, crosthwaite.peter

Hello,

beginning with commit 3bd1d74576bacb120949e13cdeded7a0c792c685

"cputlb: introduce tlb_flush_* async work"

using Qemu with SoftMMU+SVM virtualization deadlocks because tb_lock is
taken second time in cputlb.c tlb_flush_nocheck() function. The first
time tb_lock is taken, according to my debugging, in cpu-exex.c
tb_find() line 361.

I'm using Qemu with:

qemu-system-x86_64 -s -no-kvm -display sdl -m 512 -cpu phenom -nographic
-cdrom genode.iso

When building with
./configure --target-list=x86_64-softmmu --enable-debug --disable-pie
--enable-debug-tcg

I get also a

translate-all.c:165: tb_lock: Assertion `!have_tb_lock' failed.

beginning with commit 3bd1d74576bacb120949e13cdeded7a0c792c685. Before
the commit all is fine.

Since I'm not very familiar with Qemu internals, it is not clear to me
whether this commit breaks things or whether something must be
handled/added special somewhere else. I attached below the backtrace of
Qemu when it hangs in tb_lock.

In [0] my branch based on 3bd1d74576bacb120949e13cdeded7a0c792c685 is
used and [1] contains the iso image, if somebody wants try to reproduce it.

[0] https://github.com/alex-ab/qemu/commits/genode_svm_issue
[1]
https://github.com/alex-ab/qemu/commit/1130fee3b04dd2bee576241de9a5771d6855b327

Thanks in advance,

Alex.

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth




[init -> log_terminal]
[init -> log_terminal] [ 0] CORE:0:0:0 10:2:3:0 [0] AMD Phenom(tm) 9550
Quad-Core Processor
qemu-system-x86_64: qemu.git/translate-all.c:165: tb_lock: Assertion
`!have_tb_lock' failed.




(gdb) info threads
  Id   Target Id         Frame
* 1    Thread 0x7ffbc19d3c00 (LWP 8396) "qemu-system-x86"
0x00007ffbbfd2ac21 in __GI_ppoll (fds=0x273a330, nfds=6,
    timeout=<optimized out>, sigmask=0x0) at
../sysdeps/unix/sysv/linux/ppoll.c:50
  2    Thread 0x7ffbbb970700 (LWP 8397) "qemu-system-x86" syscall () at
../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
  3    Thread 0x7ffbb8206700 (LWP 8399) "qemu-system-x86" __lll_lock_wait ()
    at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
(gdb) thread 3
[Switching to thread 3 (Thread 0x7ffbb8206700 (LWP 8399))]
#0  __lll_lock_wait () at
../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
135	../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S: No such file or
directory.
(gdb) bt
#0  __lll_lock_wait () at
../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffbc0002dbd in __GI___pthread_mutex_lock (mutex=0xf6ea18
<tcg_ctx+280>) at ../nptl/pthread_mutex_lock.c:80
#2  0x000000000089852c in qemu_mutex_lock (mutex=0xf6ea18 <tcg_ctx+280>)
at util/qemu-thread-posix.c:60
#3  0x0000000000416103 in tb_lock () at qemu.git/translate-all.c:166
#4  0x000000000046e8d7 in tlb_flush_nocheck (cpu=0x164a360) at
qemu.git/cputlb.c:93
#5  0x000000000046ea2e in tlb_flush (cpu=0x164a360) at qemu.git/cputlb.c:121
#6  0x0000000000538987 in cpu_x86_update_cr4 (env=0x16525f0, new_cr4=1784)
    at qemu.git/target/i386/helper.c:660
#7  0x000000000055e318 in cpu_vmexit (env=0x16525f0, exit_code=78,
exit_info_1=4, retaddr=0)
    at qemu.git/target/i386/svm_helper.c:689
#8  0x000000000055d9b7 in cpu_svm_check_intercept_param (env=0x16525f0,
type=78, param=4, retaddr=0)
    at qemu.git/target/i386/svm_helper.c:511
#9  0x0000000000541acf in raise_interrupt2 (env=0x16525f0, intno=14,
is_int=0, error_code=4, next_eip_addend=0, retaddr=0)
    at qemu.git/target/i386/excp_helper.c:96
#10 0x0000000000541c0d in raise_exception_err_ra (env=0x16525f0,
exception_index=14, error_code=4, retaddr=0)
    at qemu.git/target/i386/excp_helper.c:127
#11 0x00000000005621a9 in tlb_fill (cs=0x164a360, addr=1245184,
access_type=MMU_INST_FETCH, mmu_idx=1, retaddr=0)
    at qemu.git/target/i386/mem_helper.c:212
#12 0x0000000000476c15 in helper_ret_ldb_cmmu (env=0x16525f0,
addr=1245184, oi=1, retaddr=0)
    at qemu.git/softmmu_template.h:127
#13 0x000000000051c86e in cpu_ldub_code_ra (env=0x16525f0, ptr=1245184,
retaddr=0)
    at qemu.git/include/exec/cpu_ldst_template.h:102
#14 0x000000000051c8e4 in cpu_ldub_code (env=0x16525f0, ptr=1245184)
    at qemu.git/include/exec/cpu_ldst_template.h:114
#15 0x0000000000522182 in insn_get (env=0x16525f0, s=0x7ffbb82057e0,
ot=MO_8)
    at qemu.git/target/i386/translate.c:2107
#16 0x000000000052ff3c in disas_insn (env=0x16525f0, s=0x7ffbb82057e0,
pc_start=1245183)
    at qemu.git/target/i386/translate.c:6520
#17 0x0000000000536458 in gen_intermediate_code (env=0x16525f0,
tb=0x7ffbb9ce3a38)
    at qemu.git/target/i386/translate.c:8449
---Type <return> to continue, or q <return> to quit---
#18 0x0000000000417616 in tb_gen_code (cpu=0x164a360, pc=1245179,
cs_base=0, flags=7342771, cflags=0)
    at qemu.git/translate-all.c:1281
#19 0x000000000041993c in tb_find (cpu=0x164a360, last_tb=0x0,
tb_exit=0) at qemu.git/cpu-exec.c:370
#20 0x000000000041a25b in cpu_exec (cpu=0x164a360) at
qemu.git/cpu-exec.c:685
#21 0x000000000044b078 in tcg_cpu_exec (cpu=0x164a360) at
qemu.git/cpus.c:1251
#22 0x000000000044b2e7 in qemu_tcg_rr_cpu_thread_fn (arg=0x164a360) at
qemu.git/cpus.c:1347
#23 0x00007ffbc00006ba in start_thread (arg=0x7ffbb8206700) at
pthread_create.c:333
#24 0x00007ffbbfd3682d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:109

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-05 16:59 [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU Alexander Boettcher
@ 2017-03-05 21:32 ` Alex Bennée
  2017-03-06  1:34   ` Richard Henderson
  2017-03-06 13:15 ` Alex Bennée
  1 sibling, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2017-03-05 21:32 UTC (permalink / raw)
  To: Alexander Boettcher
  Cc: qemu-devel, rth, fred.konrad, pbonzini, crosthwaite.peter


Alexander Boettcher <alexander.boettcher@genode-labs.com> writes:

> Hello,
>
> beginning with commit 3bd1d74576bacb120949e13cdeded7a0c792c685
>
> "cputlb: introduce tlb_flush_* async work"
>
> using Qemu with SoftMMU+SVM virtualization deadlocks because tb_lock is
> taken second time in cputlb.c tlb_flush_nocheck() function. The first
> time tb_lock is taken, according to my debugging, in cpu-exex.c
> tb_find() line 361.

<inlining the backtrace>
> (gdb) info threads
>   Id   Target Id         Frame
> * 1    Thread 0x7ffbc19d3c00 (LWP 8396) "qemu-system-x86"
> 0x00007ffbbfd2ac21 in __GI_ppoll (fds=0x273a330, nfds=6,
>     timeout=<optimized out>, sigmask=0x0) at
> ../sysdeps/unix/sysv/linux/ppoll.c:50
>   2    Thread 0x7ffbbb970700 (LWP 8397) "qemu-system-x86" syscall () at
> ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
>   3    Thread 0x7ffbb8206700 (LWP 8399) "qemu-system-x86" __lll_lock_wait ()
>     at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> (gdb) thread 3
> [Switching to thread 3 (Thread 0x7ffbb8206700 (LWP 8399))]
> #0  __lll_lock_wait () at
> ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> 135	../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S: No such file or
> directory.
> (gdb) bt
> #0  __lll_lock_wait () at
> ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> #1  0x00007ffbc0002dbd in __GI___pthread_mutex_lock (mutex=0xf6ea18
> <tcg_ctx+280>) at ../nptl/pthread_mutex_lock.c:80
> #2  0x000000000089852c in qemu_mutex_lock (mutex=0xf6ea18 <tcg_ctx+280>)
> at util/qemu-thread-posix.c:60
> #3  0x0000000000416103 in tb_lock () at qemu.git/translate-all.c:166
> #4  0x000000000046e8d7 in tlb_flush_nocheck (cpu=0x164a360) at
> qemu.git/cputlb.c:93
> #5  0x000000000046ea2e in tlb_flush (cpu=0x164a360) at qemu.git/cputlb.c:121
> #6  0x0000000000538987 in cpu_x86_update_cr4 (env=0x16525f0, new_cr4=1784)
>     at qemu.git/target/i386/helper.c:660
> #7  0x000000000055e318 in cpu_vmexit (env=0x16525f0, exit_code=78,
> exit_info_1=4, retaddr=0)
>     at qemu.git/target/i386/svm_helper.c:689
> #8  0x000000000055d9b7 in cpu_svm_check_intercept_param (env=0x16525f0,
> type=78, param=4, retaddr=0)
>     at qemu.git/target/i386/svm_helper.c:511
> #9  0x0000000000541acf in raise_interrupt2 (env=0x16525f0, intno=14,
> is_int=0, error_code=4, next_eip_addend=0, retaddr=0)
>     at qemu.git/target/i386/excp_helper.c:96
> #10 0x0000000000541c0d in raise_exception_err_ra (env=0x16525f0,
> exception_index=14, error_code=4, retaddr=0)
>     at qemu.git/target/i386/excp_helper.c:127
> #11 0x00000000005621a9 in tlb_fill (cs=0x164a360, addr=1245184,
> access_type=MMU_INST_FETCH, mmu_idx=1, retaddr=0)
>     at qemu.git/target/i386/mem_helper.c:212

Richard,

So this looks like another path through the SoftMMU code during
code-generation (which is why tb_lock() is held in the first place). I'm
not sure if the correct thing to do is bug out earlier or to defer the
exception raising part to async work and exit the loop.


> #12 0x0000000000476c15 in helper_ret_ldb_cmmu (env=0x16525f0,
> addr=1245184, oi=1, retaddr=0)
>     at qemu.git/softmmu_template.h:127
> #13 0x000000000051c86e in cpu_ldub_code_ra (env=0x16525f0, ptr=1245184,
> retaddr=0)
>     at qemu.git/include/exec/cpu_ldst_template.h:102
> #14 0x000000000051c8e4 in cpu_ldub_code (env=0x16525f0, ptr=1245184)
>     at qemu.git/include/exec/cpu_ldst_template.h:114
> #15 0x0000000000522182 in insn_get (env=0x16525f0, s=0x7ffbb82057e0,
> ot=MO_8)
>     at qemu.git/target/i386/translate.c:2107
> #16 0x000000000052ff3c in disas_insn (env=0x16525f0, s=0x7ffbb82057e0,
> pc_start=1245183)
>     at qemu.git/target/i386/translate.c:6520
> #17 0x0000000000536458 in gen_intermediate_code (env=0x16525f0,
> tb=0x7ffbb9ce3a38)
>     at qemu.git/target/i386/translate.c:8449
> ---Type <return> to continue, or q <return> to quit---
> #18 0x0000000000417616 in tb_gen_code (cpu=0x164a360, pc=1245179,
> cs_base=0, flags=7342771, cflags=0)
>     at qemu.git/translate-all.c:1281
> #19 0x000000000041993c in tb_find (cpu=0x164a360, last_tb=0x0,
> tb_exit=0) at qemu.git/cpu-exec.c:370
> #20 0x000000000041a25b in cpu_exec (cpu=0x164a360) at
> qemu.git/cpu-exec.c:685
> #21 0x000000000044b078 in tcg_cpu_exec (cpu=0x164a360) at
> qemu.git/cpus.c:1251
> #22 0x000000000044b2e7 in qemu_tcg_rr_cpu_thread_fn (arg=0x164a360) at
> qemu.git/cpus.c:1347
> #23 0x00007ffbc00006ba in start_thread (arg=0x7ffbb8206700) at
> pthread_create.c:333
> #24 0x00007ffbbfd3682d in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
>

>
> I'm using Qemu with:
>
> qemu-system-x86_64 -s -no-kvm -display sdl -m 512 -cpu phenom -nographic
> -cdrom genode.iso
>
> When building with
> ./configure --target-list=x86_64-softmmu --enable-debug --disable-pie
> --enable-debug-tcg
>
> I get also a
>
> translate-all.c:165: tb_lock: Assertion `!have_tb_lock' failed.
>
> beginning with commit 3bd1d74576bacb120949e13cdeded7a0c792c685. Before
> the commit all is fine.
>
> Since I'm not very familiar with Qemu internals, it is not clear to me
> whether this commit breaks things or whether something must be
> handled/added special somewhere else. I attached below the backtrace of
> Qemu when it hangs in tb_lock.
>
> In [0] my branch based on 3bd1d74576bacb120949e13cdeded7a0c792c685 is
> used and [1] contains the iso image, if somebody wants try to reproduce it.
>
> [0] https://github.com/alex-ab/qemu/commits/genode_svm_issue
> [1]
> https://github.com/alex-ab/qemu/commit/1130fee3b04dd2bee576241de9a5771d6855b327
>
> Thanks in advance,
>
> Alex.


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-05 21:32 ` Alex Bennée
@ 2017-03-06  1:34   ` Richard Henderson
  2017-03-06 16:58     ` Paolo Bonzini
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Henderson @ 2017-03-06  1:34 UTC (permalink / raw)
  To: Alex Bennée, Alexander Boettcher
  Cc: qemu-devel, fred.konrad, pbonzini, crosthwaite.peter

On 03/06/2017 08:32 AM, Alex Bennée wrote:
>> #5  0x000000000046ea2e in tlb_flush (cpu=0x164a360) at qemu.git/cputlb.c:121
>> #6  0x0000000000538987 in cpu_x86_update_cr4 (env=0x16525f0, new_cr4=1784)
>>     at qemu.git/target/i386/helper.c:660
>> #7  0x000000000055e318 in cpu_vmexit (env=0x16525f0, exit_code=78,
>> exit_info_1=4, retaddr=0)
>>     at qemu.git/target/i386/svm_helper.c:689
>> #8  0x000000000055d9b7 in cpu_svm_check_intercept_param (env=0x16525f0,
>> type=78, param=4, retaddr=0)
>>     at qemu.git/target/i386/svm_helper.c:511
>> #9  0x0000000000541acf in raise_interrupt2 (env=0x16525f0, intno=14,
>> is_int=0, error_code=4, next_eip_addend=0, retaddr=0)
>>     at qemu.git/target/i386/excp_helper.c:96
>> #10 0x0000000000541c0d in raise_exception_err_ra (env=0x16525f0,
>> exception_index=14, error_code=4, retaddr=0)
>>     at qemu.git/target/i386/excp_helper.c:127
>> #11 0x00000000005621a9 in tlb_fill (cs=0x164a360, addr=1245184,
>> access_type=MMU_INST_FETCH, mmu_idx=1, retaddr=0)
>>     at qemu.git/target/i386/mem_helper.c:212
> Richard,
>
> So this looks like another path through the SoftMMU code during
> code-generation (which is why tb_lock() is held in the first place). I'm
> not sure if the correct thing to do is bug out earlier or to defer the
> exception raising part to async work and exit the loop.

My guess is that everything from cpu_svm_check_intercept_param on should be 
done from do_interrupt instead of during raise_interrupt.


r~

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-05 16:59 [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU Alexander Boettcher
  2017-03-05 21:32 ` Alex Bennée
@ 2017-03-06 13:15 ` Alex Bennée
  2017-03-06 13:21   ` Alexander Boettcher
  1 sibling, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2017-03-06 13:15 UTC (permalink / raw)
  To: Alexander Boettcher
  Cc: qemu-devel, rth, fred.konrad, pbonzini, crosthwaite.peter


Alexander Boettcher <alexander.boettcher@genode-labs.com> writes:

> Hello,
>
> beginning with commit 3bd1d74576bacb120949e13cdeded7a0c792c685
>
> "cputlb: introduce tlb_flush_* async work"
>
> using Qemu with SoftMMU+SVM virtualization deadlocks because tb_lock is
> taken second time in cputlb.c tlb_flush_nocheck() function. The first
> time tb_lock is taken, according to my debugging, in cpu-exex.c
> tb_find() line 361.
>
> I'm using Qemu with:
>
> qemu-system-x86_64 -s -no-kvm -display sdl -m 512 -cpu phenom -nographic
> -cdrom genode.iso

I found the virtualbox.iso on your branch but I can't trigger the
assert. Running:

#!/bin/sh

QEMU="$@"

${QEMU} -s -no-kvm -display sdl -m 512 -cpu phenom -nographic -cdrom virtualbox.iso

I get:

NOVA Microhypervisor v7-2436fe2 (x86_32): Feb 25 2017 17:58:48 [gcc 4.9.2]
[init -> log_terminal]
[init -> log_terminal] [ 0] CORE:0:0:0 10:2:3:0 [0] AMD Phenom(tm) 9550 Quad-Core Processor
qemu: fatal: invalid tss type
EAX=00000000 EBX=00000000 ECX=bffff000 EDX=00100000
ESI=00000000 EDI=00000000 EBP=00000000 ESP=bffff000
EIP=00100000 EFL=00000246 [---Z-P-] CPL=3 II=0 A20=1 SMM=0 HLT=0
ES =0023 00000000 ffffffff 00c0f300 DPL=3 DS   [-WA]
CS =001b 00000000 ffffffff 00c0fb00 DPL=3 CS32 [-RA]
SS =0023 00000000 ffffffff 00c0f300 DPL=3 DS   [-WA]
DS =0023 00000000 ffffffff 00c0f300 DPL=3 DS   [-WA]
FS =0023 00000000 ffffffff 00c0f300 DPL=3 DS   [-WA]
GS =0023 00000000 ffffffff 00c0f300 DPL=3 DS   [-WA]
LDT=0000 00000000 00000000 00000000
TR =0030 cffffac0 00002540 00008b00 DPL=0 TSS32-busy
GDT=     cffff780 0000004f
IDT=     c001cb68 0000054f
CR0=8001003b CR2=00100000 CR3=026e9000 CR4=00000678
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
DR6=0000000000000000 DR7=0000000000000400
CCS=00000044 CCD=00000000 CCO=EFLAGS
EFER=0000000000001000
FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
XMM00=00000000cffff7800000004f00000000 XMM01=00000000c001cb680000054f00000000
XMM02=0000000000000000ffffffff0cf30023 XMM03=0000000000000000ffffffff0cf30023
XMM04=0000000000000000ffffffff0c9b0008 XMM05=0000000000000000ffffffff0c930010
XMM06=00000000cffffac000002540008b0030 XMM07=00000000000000000000000010000000
Aborted (core dumped)

Which is a guest error?


>
> When building with
> ./configure --target-list=x86_64-softmmu --enable-debug --disable-pie
> --enable-debug-tcg
>
> I get also a
>
> translate-all.c:165: tb_lock: Assertion `!have_tb_lock' failed.
>
> beginning with commit 3bd1d74576bacb120949e13cdeded7a0c792c685. Before
> the commit all is fine.
>
> Since I'm not very familiar with Qemu internals, it is not clear to me
> whether this commit breaks things or whether something must be
> handled/added special somewhere else. I attached below the backtrace of
> Qemu when it hangs in tb_lock.
>
> In [0] my branch based on 3bd1d74576bacb120949e13cdeded7a0c792c685 is
> used and [1] contains the iso image, if somebody wants try to reproduce it.
>
> [0] https://github.com/alex-ab/qemu/commits/genode_svm_issue
> [1]
> https://github.com/alex-ab/qemu/commit/1130fee3b04dd2bee576241de9a5771d6855b327
>
> Thanks in advance,
>
> Alex.


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 13:15 ` Alex Bennée
@ 2017-03-06 13:21   ` Alexander Boettcher
  2017-03-06 14:42     ` Alex Bennée
  0 siblings, 1 reply; 16+ messages in thread
From: Alexander Boettcher @ 2017-03-06 13:21 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, rth, fred.konrad, pbonzini, crosthwaite.peter

Hi,

On 06.03.2017 14:15, Alex Bennée wrote:
> I found the virtualbox.iso on your branch but I can't trigger the
> assert. Running:
> 
> #!/bin/sh
> 
> QEMU="$@"
> 
> ${QEMU} -s -no-kvm -display sdl -m 512 -cpu phenom -nographic -cdrom virtualbox.iso
> 
> I get:
> 
> NOVA Microhypervisor v7-2436fe2 (x86_32): Feb 25 2017 17:58:48 [gcc 4.9.2]
> [init -> log_terminal]
> [init -> log_terminal] [ 0] CORE:0:0:0 10:2:3:0 [0] AMD Phenom(tm) 9550 Quad-Core Processor
> qemu: fatal: invalid tss type


> Which is a guest error?

It's a race which happens in Qemu (not on hardware), so I still
investigating it within Qemu. Because of that on [0] the commit before
the virtualbox.iso is one commit to avoid this for the moment in order
to continue.

>> [0] https://github.com/alex-ab/qemu/commits/genode_svm_issue

Alex.

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 13:21   ` Alexander Boettcher
@ 2017-03-06 14:42     ` Alex Bennée
  2017-03-06 15:11       ` Alexander Boettcher
  0 siblings, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2017-03-06 14:42 UTC (permalink / raw)
  To: Alexander Boettcher
  Cc: qemu-devel, rth, fred.konrad, pbonzini, crosthwaite.peter


Alexander Boettcher <alexander.boettcher@genode-labs.com> writes:

> Hi,
>
> On 06.03.2017 14:15, Alex Bennée wrote:
>> I found the virtualbox.iso on your branch but I can't trigger the
>> assert. Running:
>>
>> #!/bin/sh
>>
>> QEMU="$@"
>>
>> ${QEMU} -s -no-kvm -display sdl -m 512 -cpu phenom -nographic -cdrom virtualbox.iso
>>
>> I get:
>>
>> NOVA Microhypervisor v7-2436fe2 (x86_32): Feb 25 2017 17:58:48 [gcc 4.9.2]
>> [init -> log_terminal]
>> [init -> log_terminal] [ 0] CORE:0:0:0 10:2:3:0 [0] AMD Phenom(tm) 9550 Quad-Core Processor
>> qemu: fatal: invalid tss type
>
>
>> Which is a guest error?
>
> It's a race which happens in Qemu (not on hardware), so I still
> investigating it within Qemu. Because of that on [0] the commit before
> the virtualbox.iso is one commit to avoid this for the moment in order
> to continue.

How can a single-threaded system emulation race with itself?

>
>>> [0] https://github.com/alex-ab/qemu/commits/genode_svm_issue
>
> Alex.


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 14:42     ` Alex Bennée
@ 2017-03-06 15:11       ` Alexander Boettcher
  2017-03-06 15:57         ` [Qemu-devel] [PATCH] target/i386: move nested exception check to x86_cpu_exec_interrupt Alex Bennée
  2017-03-06 16:24         ` [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU Alex Bennée
  0 siblings, 2 replies; 16+ messages in thread
From: Alexander Boettcher @ 2017-03-06 15:11 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, rth, fred.konrad, pbonzini, crosthwaite.peter

Hi,

On 06.03.2017 15:42, Alex Bennée wrote:
> 
> Alexander Boettcher <alexander.boettcher@genode-labs.com> writes:
> 
>> Hi,
>>
>> On 06.03.2017 14:15, Alex Bennée wrote:
>>> I found the virtualbox.iso on your branch but I can't trigger the
>>> assert. Running:
>>>
>>> #!/bin/sh
>>>
>>> QEMU="$@"
>>>
>>> ${QEMU} -s -no-kvm -display sdl -m 512 -cpu phenom -nographic -cdrom virtualbox.iso
>>>
>>> I get:
>>>
>>> NOVA Microhypervisor v7-2436fe2 (x86_32): Feb 25 2017 17:58:48 [gcc 4.9.2]
>>> [init -> log_terminal]
>>> [init -> log_terminal] [ 0] CORE:0:0:0 10:2:3:0 [0] AMD Phenom(tm) 9550 Quad-Core Processor
>>> qemu: fatal: invalid tss type
>>
>>
>>> Which is a guest error?
>>
>> It's a race which happens in Qemu (not on hardware), so I still
>> investigating it within Qemu. Because of that on [0] the commit before
>> the virtualbox.iso is one commit to avoid this for the moment in order
>> to continue.
> 
> How can a single-threaded system emulation race with itself?

Good question, I would say interrupts/timing in general. State must be
saved and restored when switching between the modes. Depending on where
you run when an interrupt (e.g. timer) happens (in the hypervisor, or in
user-level VMM respectively in some user-level applications or you run
in the VM in ring-0/ring-3.), different code paths are executed within
Qemu. As said, I'm still investigating, looks like something is not
saved/restored correctly everytime.

Sometimes Qemu just succeeds if the timing is well, will see.

But this is not the point of this issue atm.

Alex.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH] target/i386: move nested exception check to x86_cpu_exec_interrupt
  2017-03-06 15:11       ` Alexander Boettcher
@ 2017-03-06 15:57         ` Alex Bennée
  2017-03-06 19:24           ` Richard Henderson
  2017-03-06 16:24         ` [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU Alex Bennée
  1 sibling, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2017-03-06 15:57 UTC (permalink / raw)
  To: alexander.boettcher
  Cc: rth, qemu-devel, Alex Bennée, Paolo Bonzini, Eduardo Habkost

(DUMB CODE MOTION COMMIT WITHOUT FULL UNDERSTANDING OF X86 NEEDS REVIEW)

During code generation cpu_svm_check_intercept_param can through a
sequence of events generated a tlb_fill which fails due to a double
tb_lock(). Moving the checking to x86_cpu_exec_interrupt where it can
take the lock at will.

Reported-by: Alexander Boettcher <alexander.boettcher@genode-labs.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
CC: Richard Henderson <rth@twiddle.net>
---
 target/i386/cpu.h         |  1 +
 target/i386/excp_helper.c | 54 +------------------------------------------
 target/i386/seg_helper.c  | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 53 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index fb09aee7f8..07c591672c 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1105,6 +1105,7 @@ typedef struct CPUX86State {
     /* exception/interrupt handling */
     int error_code;
     int exception_is_int;
+    uintptr_t exception_retaddr;
     target_ulong exception_next_eip;
     target_ulong dr[8]; /* debug registers; note dr4 and dr5 are unused */
     union {
diff --git a/target/i386/excp_helper.c b/target/i386/excp_helper.c
index ee596c6082..0a1a02cdd4 100644
--- a/target/i386/excp_helper.c
+++ b/target/i386/excp_helper.c
@@ -35,51 +35,6 @@ void helper_raise_exception(CPUX86State *env, int exception_index)
 }
 
 /*
- * Check nested exceptions and change to double or triple fault if
- * needed. It should only be called, if this is not an interrupt.
- * Returns the new exception number.
- */
-static int check_exception(CPUX86State *env, int intno, int *error_code,
-                           uintptr_t retaddr)
-{
-    int first_contributory = env->old_exception == 0 ||
-                              (env->old_exception >= 10 &&
-                               env->old_exception <= 13);
-    int second_contributory = intno == 0 ||
-                               (intno >= 10 && intno <= 13);
-
-    qemu_log_mask(CPU_LOG_INT, "check_exception old: 0x%x new 0x%x\n",
-                env->old_exception, intno);
-
-#if !defined(CONFIG_USER_ONLY)
-    if (env->old_exception == EXCP08_DBLE) {
-        if (env->hflags & HF_SVMI_MASK) {
-            cpu_vmexit(env, SVM_EXIT_SHUTDOWN, 0, retaddr); /* does not return */
-        }
-
-        qemu_log_mask(CPU_LOG_RESET, "Triple fault\n");
-
-        qemu_system_reset_request();
-        return EXCP_HLT;
-    }
-#endif
-
-    if ((first_contributory && second_contributory)
-        || (env->old_exception == EXCP0E_PAGE &&
-            (second_contributory || (intno == EXCP0E_PAGE)))) {
-        intno = EXCP08_DBLE;
-        *error_code = 0;
-    }
-
-    if (second_contributory || (intno == EXCP0E_PAGE) ||
-        (intno == EXCP08_DBLE)) {
-        env->old_exception = intno;
-    }
-
-    return intno;
-}
-
-/*
  * Signal an interruption. It is executed in the main CPU loop.
  * is_int is TRUE if coming from the int instruction. next_eip is the
  * env->eip value AFTER the interrupt instruction. It is only relevant if
@@ -92,18 +47,11 @@ static void QEMU_NORETURN raise_interrupt2(CPUX86State *env, int intno,
 {
     CPUState *cs = CPU(x86_env_get_cpu(env));
 
-    if (!is_int) {
-        cpu_svm_check_intercept_param(env, SVM_EXIT_EXCP_BASE + intno,
-                                      error_code, retaddr);
-        intno = check_exception(env, intno, &error_code, retaddr);
-    } else {
-        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0, retaddr);
-    }
-
     cs->exception_index = intno;
     env->error_code = error_code;
     env->exception_is_int = is_int;
     env->exception_next_eip = env->eip + next_eip_addend;
+    env->exception_retaddr = retaddr;
     cpu_loop_exit_restore(cs, retaddr);
 }
 
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index 5c845dc25c..055b6d4025 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -25,6 +25,7 @@
 #include "exec/exec-all.h"
 #include "exec/cpu_ldst.h"
 #include "exec/log.h"
+#include "sysemu/sysemu.h"
 
 //#define DEBUG_PCALL
 
@@ -1314,12 +1315,70 @@ void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw)
     do_interrupt_all(x86_env_get_cpu(env), intno, 0, 0, 0, is_hw);
 }
 
+/*
+ * Check nested exceptions and change to double or triple fault if
+ * needed. It should only be called, if this is not an interrupt.
+ * Returns the new exception number.
+ */
+static int check_exception(CPUX86State *env, int intno, int *error_code,
+                           uintptr_t retaddr)
+{
+    int first_contributory = env->old_exception == 0 ||
+                              (env->old_exception >= 10 &&
+                               env->old_exception <= 13);
+    int second_contributory = intno == 0 ||
+                               (intno >= 10 && intno <= 13);
+
+    qemu_log_mask(CPU_LOG_INT, "check_exception old: 0x%x new 0x%x\n",
+                env->old_exception, intno);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->old_exception == EXCP08_DBLE) {
+        if (env->hflags & HF_SVMI_MASK) {
+            cpu_vmexit(env, SVM_EXIT_SHUTDOWN, 0, retaddr); /* does not return */
+        }
+
+        qemu_log_mask(CPU_LOG_RESET, "Triple fault\n");
+
+        qemu_system_reset_request();
+        return EXCP_HLT;
+    }
+#endif
+
+    if ((first_contributory && second_contributory)
+        || (env->old_exception == EXCP0E_PAGE &&
+            (second_contributory || (intno == EXCP0E_PAGE)))) {
+        intno = EXCP08_DBLE;
+        *error_code = 0;
+    }
+
+    if (second_contributory || (intno == EXCP0E_PAGE) ||
+        (intno == EXCP08_DBLE)) {
+        env->old_exception = intno;
+    }
+
+    return intno;
+}
+
 bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
     bool ret = false;
 
+    if (!env->exception_is_int) {
+        cpu_svm_check_intercept_param(env,
+                                      SVM_EXIT_EXCP_BASE + cs->exception_index,
+                                      env->error_code,
+                                      env->exception_retaddr);
+        cs->exception_index = check_exception(env, cs->exception_index,
+                                              &env->error_code,
+                                              env->exception_retaddr);
+    } else {
+        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0,
+                                      env->exception_retaddr);
+    }
+
 #if !defined(CONFIG_USER_ONLY)
     if (interrupt_request & CPU_INTERRUPT_POLL) {
         cs->interrupt_request &= ~CPU_INTERRUPT_POLL;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 15:11       ` Alexander Boettcher
  2017-03-06 15:57         ` [Qemu-devel] [PATCH] target/i386: move nested exception check to x86_cpu_exec_interrupt Alex Bennée
@ 2017-03-06 16:24         ` Alex Bennée
  2017-03-06 20:11           ` Alexander Boettcher
  1 sibling, 1 reply; 16+ messages in thread
From: Alex Bennée @ 2017-03-06 16:24 UTC (permalink / raw)
  To: Alexander Boettcher
  Cc: qemu-devel, rth, fred.konrad, pbonzini, crosthwaite.peter


Alexander Boettcher <alexander.boettcher@genode-labs.com> writes:

> Hi,
<snip>
>
> Sometimes Qemu just succeeds if the timing is well, will see.
>
> But this is not the point of this issue atm.

I've sent a patch that works as Richard suggested. However your test
image seems to run into other issues. I have tested it on my known good
x86 image and it works.

However my knowledge of x86's interrupt architecture is fairly minimal
so I'm looking for testing/feedback on the patch.

>
> Alex.


--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06  1:34   ` Richard Henderson
@ 2017-03-06 16:58     ` Paolo Bonzini
  2017-03-06 19:21       ` Richard Henderson
  2017-03-06 20:03       ` Alexander Boettcher
  0 siblings, 2 replies; 16+ messages in thread
From: Paolo Bonzini @ 2017-03-06 16:58 UTC (permalink / raw)
  To: Richard Henderson, Alex Bennée, Alexander Boettcher
  Cc: qemu-devel, fred.konrad, crosthwaite.peter



On 06/03/2017 02:34, Richard Henderson wrote:
> On 03/06/2017 08:32 AM, Alex Bennée wrote:
>>> #5  0x000000000046ea2e in tlb_flush (cpu=0x164a360) at
>>> qemu.git/cputlb.c:121
>>> #6  0x0000000000538987 in cpu_x86_update_cr4 (env=0x16525f0,
>>> new_cr4=1784)
>>>     at qemu.git/target/i386/helper.c:660
>>> #7  0x000000000055e318 in cpu_vmexit (env=0x16525f0, exit_code=78,
>>> exit_info_1=4, retaddr=0)
>>>     at qemu.git/target/i386/svm_helper.c:689
>>> #8  0x000000000055d9b7 in cpu_svm_check_intercept_param (env=0x16525f0,
>>> type=78, param=4, retaddr=0)
>>>     at qemu.git/target/i386/svm_helper.c:511
>>> #9  0x0000000000541acf in raise_interrupt2 (env=0x16525f0, intno=14,
>>> is_int=0, error_code=4, next_eip_addend=0, retaddr=0)
>>>     at qemu.git/target/i386/excp_helper.c:96
>>> #10 0x0000000000541c0d in raise_exception_err_ra (env=0x16525f0,
>>> exception_index=14, error_code=4, retaddr=0)
>>>     at qemu.git/target/i386/excp_helper.c:127
>>> #11 0x00000000005621a9 in tlb_fill (cs=0x164a360, addr=1245184,
>>> access_type=MMU_INST_FETCH, mmu_idx=1, retaddr=0)
>>>     at qemu.git/target/i386/mem_helper.c:212
>> Richard,
>>
>> So this looks like another path through the SoftMMU code during
>> code-generation (which is why tb_lock() is held in the first place). I'm
>> not sure if the correct thing to do is bug out earlier or to defer the
>> exception raising part to async work and exit the loop.
> 
> My guess is that everything from cpu_svm_check_intercept_param on should
> be done from do_interrupt instead of during raise_interrupt.

>From cpu_svm_check_intercept_param, or from cpu_vmexit?  The former seems
to be too early because it will usually not even do anything, but treating
cpu_vmexit like an exception is a very good idea indeed.  This is my
uncompiled take.

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 12a39d5..ef319cc 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -694,6 +694,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 
 #define EXCP_SYSCALL    0x100 /* only happens in user only emulation
                                  for syscall instruction */
+#define EXCP_VMEXIT     0x100
 
 /* i386-specific interrupt pending bits.  */
 #define CPU_INTERRUPT_POLL      CPU_INTERRUPT_TGT_EXT_1
@@ -1626,6 +1627,7 @@ void cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
                                    uint64_t param, uintptr_t retaddr);
 void cpu_vmexit(CPUX86State *nenv, uint32_t exit_code, uint64_t exit_info_1,
                 uintptr_t retaddr);
+void do_vmexit(CPUX86State *env, uint32_t exit_code, uint64 exit_info_1);
 
 /* seg_helper.c */
 void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw);
diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index 5c845dc..0374031 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -1297,15 +1297,17 @@ void x86_cpu_do_interrupt(CPUState *cs)
     /* successfully delivered */
     env->old_exception = -1;
 #else
-    /* simulate a real cpu exception. On i386, it can
-       trigger new exceptions, but we do not handle
-       double or triple faults yet. */
-    do_interrupt_all(cpu, cs->exception_index,
-                     env->exception_is_int,
-                     env->error_code,
-                     env->exception_next_eip, 0);
-    /* successfully delivered */
-    env->old_exception = -1;
+    if (cs->exception_index >= EXCP_VMEXIT) {
+        assert(env->old_exception == -1);
+        do_vmexit(env, cs->exception_index - EXCP_VMEXIT, env->error_code);
+    } else {
+        do_interrupt_all(cpu, cs->exception_index,
+                         env->exception_is_int,
+                         env->error_code,
+                         env->exception_next_eip, 0);
+        /* successfully delivered */
+        env->old_exception = -1;
+    }
 #endif
 }
 
diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
index 78d8df4..b49cd6d 100644
--- a/target/i386/svm_helper.c
+++ b/target/i386/svm_helper.c
@@ -580,12 +580,10 @@ void helper_svm_check_io(CPUX86State *env, uint32_t port, uint32_t param,
     }
 }
 
-/* Note: currently only 32 bits of exit_code are used */
 void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
                 uintptr_t retaddr)
 {
     CPUState *cs = CPU(x86_env_get_cpu(env));
-    uint32_t int_ctl;
 
     if (retaddr) {
         cpu_restore_state(cs, retaddr);
@@ -598,6 +596,19 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
                                                    control.exit_info_2)),
                   env->eip);
 
+    cs->exception_index = EXCP_VMEXIT + exit_code;
+    env->error_code = exit_info_1;
+
+    /* remove any pending exception */
+    env->old_exception = -1;
+    cpu_loop_exit(cs);
+}
+
+void do_vmexit(CPUX86State *env, uint32_t exit_code, uint64 exit_info_1)
+{
+    CPUState *cs = CPU(x86_env_get_cpu(env));
+    uint32_t int_ctl;
+
     if (env->hflags & HF_INHIBIT_IRQ_MASK) {
         x86_stl_phys(cs,
                  env->vm_vmcb + offsetof(struct vmcb, control.int_state),
@@ -759,13 +770,6 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
     /* If the host's rIP reloaded by #VMEXIT is outside the limit of the
        host's code segment or non-canonical (in the case of long mode), a
        #GP fault is delivered inside the host. */
-
-    /* remove any pending exception */
-    cs->exception_index = -1;
-    env->error_code = 0;
-    env->old_exception = -1;
-
-    cpu_loop_exit(cs);
 }
 
 #endif

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 16:58     ` Paolo Bonzini
@ 2017-03-06 19:21       ` Richard Henderson
  2017-03-06 20:03       ` Alexander Boettcher
  1 sibling, 0 replies; 16+ messages in thread
From: Richard Henderson @ 2017-03-06 19:21 UTC (permalink / raw)
  To: Paolo Bonzini, Alex Bennée, Alexander Boettcher
  Cc: qemu-devel, fred.konrad, crosthwaite.peter

On 03/07/2017 03:58 AM, Paolo Bonzini wrote:
> On 06/03/2017 02:34, Richard Henderson wrote:
>> My guess is that everything from cpu_svm_check_intercept_param on should
>> be done from do_interrupt instead of during raise_interrupt.
>
>>From cpu_svm_check_intercept_param, or from cpu_vmexit?  The former seems
> to be too early because it will usually not even do anything, but treating
> cpu_vmexit like an exception is a very good idea indeed.  This is my
> uncompiled take.

I hadn't considered that approach.  But it looks very plausible.


r~

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH] target/i386: move nested exception check to x86_cpu_exec_interrupt
  2017-03-06 15:57         ` [Qemu-devel] [PATCH] target/i386: move nested exception check to x86_cpu_exec_interrupt Alex Bennée
@ 2017-03-06 19:24           ` Richard Henderson
  2017-03-07 15:03             ` Alex Bennée
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Henderson @ 2017-03-06 19:24 UTC (permalink / raw)
  To: Alex Bennée, alexander.boettcher
  Cc: qemu-devel, Paolo Bonzini, Eduardo Habkost

On 03/07/2017 02:57 AM, Alex Bennée wrote:
>      cs->exception_index = intno;
>      env->error_code = error_code;
>      env->exception_is_int = is_int;
>      env->exception_next_eip = env->eip + next_eip_addend;
> +    env->exception_retaddr = retaddr;
>      cpu_loop_exit_restore(cs, retaddr);

Since you've restored state here, I don't see that you need to save the retaddr 
at all ...

> +    if (!env->exception_is_int) {
> +        cpu_svm_check_intercept_param(env,
> +                                      SVM_EXIT_EXCP_BASE + cs->exception_index,
> +                                      env->error_code,
> +                                      env->exception_retaddr);
> +        cs->exception_index = check_exception(env, cs->exception_index,
> +                                              &env->error_code,
> +                                              env->exception_retaddr);
> +    } else {
> +        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0,
> +                                      env->exception_retaddr);
> +    }

... simply pass zero here instead.

But do I see correctly from the thread that we don't have a reproducer?


r~

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 16:58     ` Paolo Bonzini
  2017-03-06 19:21       ` Richard Henderson
@ 2017-03-06 20:03       ` Alexander Boettcher
  1 sibling, 0 replies; 16+ messages in thread
From: Alexander Boettcher @ 2017-03-06 20:03 UTC (permalink / raw)
  To: Paolo Bonzini, Richard Henderson, Alex Bennée
  Cc: qemu-devel, fred.konrad, crosthwaite.peter

Hello,

I applied the patch and beside two uint64 -> uint64_t in do_vmexit() it
compiles and solves the issue for me reliable.

Great !

On 06.03.2017 17:58, Paolo Bonzini wrote:
> 
> 
> On 06/03/2017 02:34, Richard Henderson wrote:
>> On 03/06/2017 08:32 AM, Alex Bennée wrote:
>>>> #5  0x000000000046ea2e in tlb_flush (cpu=0x164a360) at
>>>> qemu.git/cputlb.c:121
>>>> #6  0x0000000000538987 in cpu_x86_update_cr4 (env=0x16525f0,
>>>> new_cr4=1784)
>>>>     at qemu.git/target/i386/helper.c:660
>>>> #7  0x000000000055e318 in cpu_vmexit (env=0x16525f0, exit_code=78,
>>>> exit_info_1=4, retaddr=0)
>>>>     at qemu.git/target/i386/svm_helper.c:689
>>>> #8  0x000000000055d9b7 in cpu_svm_check_intercept_param (env=0x16525f0,
>>>> type=78, param=4, retaddr=0)
>>>>     at qemu.git/target/i386/svm_helper.c:511
>>>> #9  0x0000000000541acf in raise_interrupt2 (env=0x16525f0, intno=14,
>>>> is_int=0, error_code=4, next_eip_addend=0, retaddr=0)
>>>>     at qemu.git/target/i386/excp_helper.c:96
>>>> #10 0x0000000000541c0d in raise_exception_err_ra (env=0x16525f0,
>>>> exception_index=14, error_code=4, retaddr=0)
>>>>     at qemu.git/target/i386/excp_helper.c:127
>>>> #11 0x00000000005621a9 in tlb_fill (cs=0x164a360, addr=1245184,
>>>> access_type=MMU_INST_FETCH, mmu_idx=1, retaddr=0)
>>>>     at qemu.git/target/i386/mem_helper.c:212
>>> Richard,
>>>
>>> So this looks like another path through the SoftMMU code during
>>> code-generation (which is why tb_lock() is held in the first place). I'm
>>> not sure if the correct thing to do is bug out earlier or to defer the
>>> exception raising part to async work and exit the loop.
>>
>> My guess is that everything from cpu_svm_check_intercept_param on should
>> be done from do_interrupt instead of during raise_interrupt.
> 
> From cpu_svm_check_intercept_param, or from cpu_vmexit?  The former seems
> to be too early because it will usually not even do anything, but treating
> cpu_vmexit like an exception is a very good idea indeed.  This is my
> uncompiled take.
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 12a39d5..ef319cc 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -694,6 +694,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
>  
>  #define EXCP_SYSCALL    0x100 /* only happens in user only emulation
>                                   for syscall instruction */
> +#define EXCP_VMEXIT     0x100
>  
>  /* i386-specific interrupt pending bits.  */
>  #define CPU_INTERRUPT_POLL      CPU_INTERRUPT_TGT_EXT_1
> @@ -1626,6 +1627,7 @@ void cpu_svm_check_intercept_param(CPUX86State *env1, uint32_t type,
>                                     uint64_t param, uintptr_t retaddr);
>  void cpu_vmexit(CPUX86State *nenv, uint32_t exit_code, uint64_t exit_info_1,
>                  uintptr_t retaddr);
> +void do_vmexit(CPUX86State *env, uint32_t exit_code, uint64 exit_info_1);
>  
>  /* seg_helper.c */
>  void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw);
> diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> index 5c845dc..0374031 100644
> --- a/target/i386/seg_helper.c
> +++ b/target/i386/seg_helper.c
> @@ -1297,15 +1297,17 @@ void x86_cpu_do_interrupt(CPUState *cs)
>      /* successfully delivered */
>      env->old_exception = -1;
>  #else
> -    /* simulate a real cpu exception. On i386, it can
> -       trigger new exceptions, but we do not handle
> -       double or triple faults yet. */
> -    do_interrupt_all(cpu, cs->exception_index,
> -                     env->exception_is_int,
> -                     env->error_code,
> -                     env->exception_next_eip, 0);
> -    /* successfully delivered */
> -    env->old_exception = -1;
> +    if (cs->exception_index >= EXCP_VMEXIT) {
> +        assert(env->old_exception == -1);
> +        do_vmexit(env, cs->exception_index - EXCP_VMEXIT, env->error_code);
> +    } else {
> +        do_interrupt_all(cpu, cs->exception_index,
> +                         env->exception_is_int,
> +                         env->error_code,
> +                         env->exception_next_eip, 0);
> +        /* successfully delivered */
> +        env->old_exception = -1;
> +    }
>  #endif
>  }
>  
> diff --git a/target/i386/svm_helper.c b/target/i386/svm_helper.c
> index 78d8df4..b49cd6d 100644
> --- a/target/i386/svm_helper.c
> +++ b/target/i386/svm_helper.c
> @@ -580,12 +580,10 @@ void helper_svm_check_io(CPUX86State *env, uint32_t port, uint32_t param,
>      }
>  }
>  
> -/* Note: currently only 32 bits of exit_code are used */
>  void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
>                  uintptr_t retaddr)
>  {
>      CPUState *cs = CPU(x86_env_get_cpu(env));
> -    uint32_t int_ctl;
>  
>      if (retaddr) {
>          cpu_restore_state(cs, retaddr);
> @@ -598,6 +596,19 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
>                                                     control.exit_info_2)),
>                    env->eip);
>  
> +    cs->exception_index = EXCP_VMEXIT + exit_code;
> +    env->error_code = exit_info_1;
> +
> +    /* remove any pending exception */
> +    env->old_exception = -1;
> +    cpu_loop_exit(cs);
> +}
> +
> +void do_vmexit(CPUX86State *env, uint32_t exit_code, uint64 exit_info_1)
> +{
> +    CPUState *cs = CPU(x86_env_get_cpu(env));
> +    uint32_t int_ctl;
> +
>      if (env->hflags & HF_INHIBIT_IRQ_MASK) {
>          x86_stl_phys(cs,
>                   env->vm_vmcb + offsetof(struct vmcb, control.int_state),
> @@ -759,13 +770,6 @@ void cpu_vmexit(CPUX86State *env, uint32_t exit_code, uint64_t exit_info_1,
>      /* If the host's rIP reloaded by #VMEXIT is outside the limit of the
>         host's code segment or non-canonical (in the case of long mode), a
>         #GP fault is delivered inside the host. */
> -
> -    /* remove any pending exception */
> -    cs->exception_index = -1;
> -    env->error_code = 0;
> -    env->old_exception = -1;
> -
> -    cpu_loop_exit(cs);
>  }
>  
>  #endif
> 

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 16:24         ` [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU Alex Bennée
@ 2017-03-06 20:11           ` Alexander Boettcher
  2017-03-06 20:56             ` Paolo Bonzini
  0 siblings, 1 reply; 16+ messages in thread
From: Alexander Boettcher @ 2017-03-06 20:11 UTC (permalink / raw)
  To: Alex Bennée
  Cc: qemu-devel, rth, fred.konrad, pbonzini, crosthwaite.peter

Hello,

thank you for your effort.

With your patch I get now a 0x3f exception shown by the hypervisor,
which I have never encountered so far before. The guest kernel seems not
to start, at least no life sign is shown.

For me the patch of Paolo Bonzini works reliable.

How to proceed ?

Thanks,

Alex.

On 06.03.2017 17:24, Alex Bennée wrote:
> 
> Alexander Boettcher <alexander.boettcher@genode-labs.com> writes:
> 
>> Hi,
> <snip>
>>
>> Sometimes Qemu just succeeds if the timing is well, will see.
>>
>> But this is not the point of this issue atm.
> 
> I've sent a patch that works as Richard suggested. However your test
> image seems to run into other issues. I have tested it on my known good
> x86 image and it works.
> 
> However my knowledge of x86's interrupt architecture is fairly minimal
> so I'm looking for testing/feedback on the patch.
> 
>>
>> Alex.
> 
> 
> --
> Alex Bennée
> 

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU
  2017-03-06 20:11           ` Alexander Boettcher
@ 2017-03-06 20:56             ` Paolo Bonzini
  0 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2017-03-06 20:56 UTC (permalink / raw)
  To: Alexander Boettcher
  Cc: Alex Bennée, qemu-devel, rth, fred konrad, crosthwaite peter



----- Original Message -----
> From: "Alexander Boettcher" <alexander.boettcher@genode-labs.com>
> To: "Alex Bennée" <alex.bennee@linaro.org>
> Cc: qemu-devel@nongnu.org, rth@twiddle.net, "fred konrad" <fred.konrad@greensocs.com>, pbonzini@redhat.com,
> "crosthwaite peter" <crosthwaite.peter@gmail.com>
> Sent: Monday, March 6, 2017 9:11:04 PM
> Subject: Re: Qemu deadlocks in tb_lock when using SVM+SoftMMU
> 
> Hello,
> 
> thank you for your effort.
> 
> With your patch I get now a 0x3f exception shown by the hypervisor,
> which I have never encountered so far before. The guest kernel seems not
> to start, at least no life sign is shown.
> 
> For me the patch of Paolo Bonzini works reliable.

Let's use mine then. :)  I'll post it properly tomorrow.

Paolo

> How to proceed ?
> 
> Thanks,
> 
> Alex.
> 
> On 06.03.2017 17:24, Alex Bennée wrote:
> > 
> > Alexander Boettcher <alexander.boettcher@genode-labs.com> writes:
> > 
> >> Hi,
> > <snip>
> >>
> >> Sometimes Qemu just succeeds if the timing is well, will see.
> >>
> >> But this is not the point of this issue atm.
> > 
> > I've sent a patch that works as Richard suggested. However your test
> > image seems to run into other issues. I have tested it on my known good
> > x86 image and it works.
> > 
> > However my knowledge of x86's interrupt architecture is fairly minimal
> > so I'm looking for testing/feedback on the patch.
> > 
> >>
> >> Alex.
> > 
> > 
> > --
> > Alex Bennée
> > 
> 
> --
> Alexander Boettcher
> Genode Labs
> 
> http://www.genode-labs.com - http://www.genode.org
> 
> Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
> Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH] target/i386: move nested exception check to x86_cpu_exec_interrupt
  2017-03-06 19:24           ` Richard Henderson
@ 2017-03-07 15:03             ` Alex Bennée
  0 siblings, 0 replies; 16+ messages in thread
From: Alex Bennée @ 2017-03-07 15:03 UTC (permalink / raw)
  To: Richard Henderson
  Cc: alexander.boettcher, qemu-devel, Paolo Bonzini, Eduardo Habkost


Richard Henderson <rth@twiddle.net> writes:

> On 03/07/2017 02:57 AM, Alex Bennée wrote:
>>      cs->exception_index = intno;
>>      env->error_code = error_code;
>>      env->exception_is_int = is_int;
>>      env->exception_next_eip = env->eip + next_eip_addend;
>> +    env->exception_retaddr = retaddr;
>>      cpu_loop_exit_restore(cs, retaddr);
>
> Since you've restored state here, I don't see that you need to save
> the retaddr at all ...
>
>> +    if (!env->exception_is_int) {
>> +        cpu_svm_check_intercept_param(env,
>> +                                      SVM_EXIT_EXCP_BASE + cs->exception_index,
>> +                                      env->error_code,
>> +                                      env->exception_retaddr);
>> +        cs->exception_index = check_exception(env, cs->exception_index,
>> +                                              &env->error_code,
>> +                                              env->exception_retaddr);
>> +    } else {
>> +        cpu_svm_check_intercept_param(env, SVM_EXIT_SWINT, 0,
>> +                                      env->exception_retaddr);
>> +    }
>
> ... simply pass zero here instead.
>
> But do I see correctly from the thread that we don't have a reproducer?

Yeah - I've nabbed Paolo's much cleaner and more informed fix and that
seems fine.

--
Alex Bennée

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-03-07 15:03 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-05 16:59 [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU Alexander Boettcher
2017-03-05 21:32 ` Alex Bennée
2017-03-06  1:34   ` Richard Henderson
2017-03-06 16:58     ` Paolo Bonzini
2017-03-06 19:21       ` Richard Henderson
2017-03-06 20:03       ` Alexander Boettcher
2017-03-06 13:15 ` Alex Bennée
2017-03-06 13:21   ` Alexander Boettcher
2017-03-06 14:42     ` Alex Bennée
2017-03-06 15:11       ` Alexander Boettcher
2017-03-06 15:57         ` [Qemu-devel] [PATCH] target/i386: move nested exception check to x86_cpu_exec_interrupt Alex Bennée
2017-03-06 19:24           ` Richard Henderson
2017-03-07 15:03             ` Alex Bennée
2017-03-06 16:24         ` [Qemu-devel] Qemu deadlocks in tb_lock when using SVM+SoftMMU Alex Bennée
2017-03-06 20:11           ` Alexander Boettcher
2017-03-06 20:56             ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.