* [Qemu-devel] "cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64 @ 2011-06-30 16:47 Jan Kiszka 2011-06-30 21:17 ` Stefan Weil 2011-07-01 1:44 ` TeLeMan 0 siblings, 2 replies; 18+ messages in thread From: Jan Kiszka @ 2011-06-30 16:47 UTC (permalink / raw) To: Blue Swirl; +Cc: qemu-devel Hi Blue, commit cea5f9a28f breaks here, just starting qemu without any parameters: Starting program: qemu-system-x86_64 [Thread debugging using libthread_db enabled] Program received signal SIGSEGV, Segmentation fault. 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 (gdb) bt #0 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 #1 0x00000000004eb96c in cpu_x86_exec (env=0x11d09a0) at cpu-exec.c:233 #2 0x000000000040f056 in tcg_cpu_exec (env=0x11d09a0) at cpus.c:1059 #3 cpu_exec_all () at cpus.c:1100 #4 0x000000000058cfcb in main_loop () at vl.c:1380 #5 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at vl.c:3318 Please have a look. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] "cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64 2011-06-30 16:47 [Qemu-devel] "cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64 Jan Kiszka @ 2011-06-30 21:17 ` Stefan Weil 2011-07-01 1:44 ` TeLeMan 1 sibling, 0 replies; 18+ messages in thread From: Stefan Weil @ 2011-06-30 21:17 UTC (permalink / raw) To: Jan Kiszka; +Cc: Blue Swirl, qemu-devel Am 30.06.2011 18:47, schrieb Jan Kiszka: > Hi Blue, > > commit cea5f9a28f breaks here, just starting qemu without any > parameters: > > Starting program: qemu-system-x86_64 > [Thread debugging using libthread_db enabled] > > Program received signal SIGSEGV, Segmentation fault. > 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 > (gdb) bt > #0 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 > #1 0x00000000004eb96c in cpu_x86_exec (env=0x11d09a0) at cpu-exec.c:233 > #2 0x000000000040f056 in tcg_cpu_exec (env=0x11d09a0) at cpus.c:1059 > #3 cpu_exec_all () at cpus.c:1100 > #4 0x000000000058cfcb in main_loop () at vl.c:1380 > #5 main (argc=<value optimized out>, argv=<value optimized out>, > envp=<value optimized out>) at vl.c:3318 > > Please have a look. > > Jan Hi, the same commit works here. Maybe this is caused by different timing? You might try -singlestep. Does QEMU emulate any guest code, or is logging output empty when QEMU was run with -d in_asm? Regards, Stefan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] "cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64 2011-06-30 16:47 [Qemu-devel] "cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64 Jan Kiszka 2011-06-30 21:17 ` Stefan Weil @ 2011-07-01 1:44 ` TeLeMan 2011-07-01 20:15 ` Blue Swirl 1 sibling, 1 reply; 18+ messages in thread From: TeLeMan @ 2011-07-01 1:44 UTC (permalink / raw) To: Jan Kiszka; +Cc: Blue Swirl, qemu-devel On Fri, Jul 1, 2011 at 00:47, Jan Kiszka <jan.kiszka@siemens.com> wrote: > Hi Blue, > > commit cea5f9a28f breaks here, just starting qemu without any > parameters: > > Starting program: qemu-system-x86_64 > [Thread debugging using libthread_db enabled] > > Program received signal SIGSEGV, Segmentation fault. > 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 > (gdb) bt > #0 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 > #1 0x00000000004eb96c in cpu_x86_exec (env=0x11d09a0) at cpu-exec.c:233 > #2 0x000000000040f056 in tcg_cpu_exec (env=0x11d09a0) at cpus.c:1059 > #3 cpu_exec_all () at cpus.c:1100 > #4 0x000000000058cfcb in main_loop () at vl.c:1380 > #5 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at vl.c:3318 > > Please have a look. > > Jan > > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux > > It works on gcc 4.4.5 x64. Which version is your gcc? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] "cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64 2011-07-01 1:44 ` TeLeMan @ 2011-07-01 20:15 ` Blue Swirl 2011-07-02 7:50 ` [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp Jan Kiszka 0 siblings, 1 reply; 18+ messages in thread From: Blue Swirl @ 2011-07-01 20:15 UTC (permalink / raw) To: Jan Kiszka, qemu-devel; +Cc: TeLeMan On Fri, Jul 1, 2011 at 4:44 AM, TeLeMan <geleman@gmail.com> wrote: > On Fri, Jul 1, 2011 at 00:47, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> Hi Blue, >> >> commit cea5f9a28f breaks here, just starting qemu without any >> parameters: >> >> Starting program: qemu-system-x86_64 >> [Thread debugging using libthread_db enabled] >> >> Program received signal SIGSEGV, Segmentation fault. >> 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 >> (gdb) bt >> #0 0x00007ffff39ac770 in __sigsetjmp () from /lib64/libc.so.6 >> #1 0x00000000004eb96c in cpu_x86_exec (env=0x11d09a0) at cpu-exec.c:233 >> #2 0x000000000040f056 in tcg_cpu_exec (env=0x11d09a0) at cpus.c:1059 >> #3 cpu_exec_all () at cpus.c:1100 >> #4 0x000000000058cfcb in main_loop () at vl.c:1380 >> #5 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at vl.c:3318 >> >> Please have a look. >> >> Jan >> >> -- >> Siemens AG, Corporate Technology, CT T DE IT 1 >> Corporate Competence Center Embedded Linux >> >> > It works on gcc 4.4.5 x64. Which version is your gcc? Works for me too. The change added a second argument to tcg_qemu_tb_exec() and in tcg/i386/* we assume that the second register is %rsi. Isn't that so also with your gcc? ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-07-01 20:15 ` Blue Swirl @ 2011-07-02 7:50 ` Jan Kiszka 2011-07-02 9:08 ` Blue Swirl ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Jan Kiszka @ 2011-07-02 7:50 UTC (permalink / raw) To: Blue Swirl; +Cc: TeLeMan, qemu-devel From: Jan Kiszka <jan.kiszka@siemens.com> Recent compilers look deep into cpu_exec, find longjmp as a noreturn function and decide to smash some stack variables as they won't be used again. This may lead to env becoming invalid after return from setjmp, causing crashes. Fix it by reloading env from cpu_single_env in that case. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- cpu-exec.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/cpu-exec.c b/cpu-exec.c index 20e3ec4..de0d716 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -587,6 +587,10 @@ int cpu_exec(CPUState *env) /* reset soft MMU for next block (it can currently only be set by a memory fault) */ } /* for(;;) */ + } else { + /* Reload env after longjmp - the compiler may have smashed all + * local variables as longjmp is marked 'noreturn'. */ + env = cpu_single_env; } } /* for(;;) */ ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-07-02 7:50 ` [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp Jan Kiszka @ 2011-07-02 9:08 ` Blue Swirl 2011-07-02 9:43 ` Jan Kiszka 2011-07-12 20:56 ` Blue Swirl 2011-08-11 11:30 ` Peter Maydell 2 siblings, 1 reply; 18+ messages in thread From: Blue Swirl @ 2011-07-02 9:08 UTC (permalink / raw) To: Jan Kiszka; +Cc: TeLeMan, qemu-devel On Sat, Jul 2, 2011 at 10:50 AM, Jan Kiszka <jan.kiszka@web.de> wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Recent compilers look deep into cpu_exec, find longjmp as a noreturn > function and decide to smash some stack variables as they won't be used > again. This may lead to env becoming invalid after return from setjmp, > causing crashes. Fix it by reloading env from cpu_single_env in that > case. Nice. Could you try if gcc flag -Wclobbered catches something using your compiler without your patch: commit f826f0d0f5cf5dd18a0d34159c1a3bc8f2e6ddf4 Author: Blue Swirl <blauwirbel@gmail.com> Date: Sun Sep 26 11:58:38 2010 +0000 Add gcc warning -Wclobbered Signed-off-by: Blue Swirl <blauwirbel@gmail.com> diff --git a/configure b/configure index 88159ac..2417205 100755 --- a/configure +++ b/configure @@ -1038,7 +1038,7 @@ fi gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits" gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags" gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" -gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags" +gcc_flags="-fstack-protector-all -Wendif-labels -Wclobbered $gcc_flags" cat > $TMPC << EOF int main(void) { return 0; } EOF > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > cpu-exec.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/cpu-exec.c b/cpu-exec.c > index 20e3ec4..de0d716 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -587,6 +587,10 @@ int cpu_exec(CPUState *env) > /* reset soft MMU for next block (it can currently > only be set by a memory fault) */ > } /* for(;;) */ > + } else { > + /* Reload env after longjmp - the compiler may have smashed all > + * local variables as longjmp is marked 'noreturn'. */ > + env = cpu_single_env; > } > } /* for(;;) */ > > ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-07-02 9:08 ` Blue Swirl @ 2011-07-02 9:43 ` Jan Kiszka 2011-07-03 14:09 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Jan Kiszka @ 2011-07-02 9:43 UTC (permalink / raw) To: Blue Swirl; +Cc: TeLeMan, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2596 bytes --] On 2011-07-02 11:08, Blue Swirl wrote: > On Sat, Jul 2, 2011 at 10:50 AM, Jan Kiszka <jan.kiszka@web.de> wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> Recent compilers look deep into cpu_exec, find longjmp as a noreturn >> function and decide to smash some stack variables as they won't be used >> again. This may lead to env becoming invalid after return from setjmp, >> causing crashes. Fix it by reloading env from cpu_single_env in that >> case. > > Nice. Could you try if gcc flag -Wclobbered catches something using > your compiler without your patch: > > commit f826f0d0f5cf5dd18a0d34159c1a3bc8f2e6ddf4 > Author: Blue Swirl <blauwirbel@gmail.com> > Date: Sun Sep 26 11:58:38 2010 +0000 > > Add gcc warning -Wclobbered > > Signed-off-by: Blue Swirl <blauwirbel@gmail.com> > > diff --git a/configure b/configure > index 88159ac..2417205 100755 > --- a/configure > +++ b/configure > @@ -1038,7 +1038,7 @@ fi > gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits" > gcc_flags="-Wformat-security -Wformat-y2k -Winit-self > -Wignored-qualifiers $gcc_flags" > gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags" > -gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags" > +gcc_flags="-fstack-protector-all -Wendif-labels -Wclobbered $gcc_flags" > cat > $TMPC << EOF > int main(void) { return 0; } > EOF Neither native gcc 4.5.0 nor mingw's 4.6.0 detect this issue. However, 4.6 found this - and 3 false positives: diff --git a/monitor.c b/monitor.c index 67ceb46..d7edbbb 100644 --- a/monitor.c +++ b/monitor.c @@ -3253,6 +3253,7 @@ static const mon_cmd_t qmp_query_cmds[] = { /*******************************************************************/ static const char *pch; +static char *saved_key; static jmp_buf expr_env; #define MD_TLONG 0 @@ -4254,8 +4255,11 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, } typestr++; } - if (get_expr(mon, &val, &p)) + saved_key = key; + if (get_expr(mon, &val, &p)) { + key = saved_key; goto fail; + } /* Check if 'i' is greater than 32-bit */ if ((c == 'i') && ((val >> 32) & 0xffffffff)) { monitor_printf(mon, "\'%s\' has failed: ", cmdname); But the right way to deal with it is to return error codes and get rid of this setjmp/longjmp mess for get_expr. Jan [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-07-02 9:43 ` Jan Kiszka @ 2011-07-03 14:09 ` Paolo Bonzini 0 siblings, 0 replies; 18+ messages in thread From: Paolo Bonzini @ 2011-07-03 14:09 UTC (permalink / raw) To: Jan Kiszka; +Cc: Blue Swirl, TeLeMan, qemu-devel On 07/02/2011 11:43 AM, Jan Kiszka wrote: > static const char *pch; > +static char *saved_key; > static jmp_buf expr_env; > > #define MD_TLONG 0 > @@ -4254,8 +4255,11 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon, > } > typestr++; > } > - if (get_expr(mon,&val,&p)) > + saved_key = key; > + if (get_expr(mon,&val,&p)) { > + key = saved_key; > goto fail; > + } Please make saved_key a volatile local instead. Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-07-02 7:50 ` [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp Jan Kiszka 2011-07-02 9:08 ` Blue Swirl @ 2011-07-12 20:56 ` Blue Swirl 2011-08-11 11:30 ` Peter Maydell 2 siblings, 0 replies; 18+ messages in thread From: Blue Swirl @ 2011-07-12 20:56 UTC (permalink / raw) To: Jan Kiszka; +Cc: TeLeMan, qemu-devel Thanks, applied. On Sat, Jul 2, 2011 at 10:50 AM, Jan Kiszka <jan.kiszka@web.de> wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Recent compilers look deep into cpu_exec, find longjmp as a noreturn > function and decide to smash some stack variables as they won't be used > again. This may lead to env becoming invalid after return from setjmp, > causing crashes. Fix it by reloading env from cpu_single_env in that > case. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > cpu-exec.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/cpu-exec.c b/cpu-exec.c > index 20e3ec4..de0d716 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -587,6 +587,10 @@ int cpu_exec(CPUState *env) > /* reset soft MMU for next block (it can currently > only be set by a memory fault) */ > } /* for(;;) */ > + } else { > + /* Reload env after longjmp - the compiler may have smashed all > + * local variables as longjmp is marked 'noreturn'. */ > + env = cpu_single_env; > } > } /* for(;;) */ > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-07-02 7:50 ` [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp Jan Kiszka 2011-07-02 9:08 ` Blue Swirl 2011-07-12 20:56 ` Blue Swirl @ 2011-08-11 11:30 ` Peter Maydell 2011-08-11 12:16 ` Paolo Bonzini 2 siblings, 1 reply; 18+ messages in thread From: Peter Maydell @ 2011-08-11 11:30 UTC (permalink / raw) To: Jan Kiszka; +Cc: Blue Swirl, TeLeMan, David Gilbert, qemu-devel On 2 July 2011 08:50, Jan Kiszka <jan.kiszka@web.de> wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Recent compilers look deep into cpu_exec, find longjmp as a noreturn > function and decide to smash some stack variables as they won't be used > again. This may lead to env becoming invalid after return from setjmp, > causing crashes. Fix it by reloading env from cpu_single_env in that > case. Can you give more details of what compiler/platform this was a problem for? My reading of the C standard is that the compiler isn't allowed to trash env across this longjmp, because it's a variable of automatic scope which isn't modified between the setjmp and the longjmp... (We've been looking at this because reloading from cpu_single_env is the wrong fix in the case of user-mode where there are multiple-threads.) Thanks -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 11:30 ` Peter Maydell @ 2011-08-11 12:16 ` Paolo Bonzini 2011-08-11 12:40 ` Peter Maydell 0 siblings, 1 reply; 18+ messages in thread From: Paolo Bonzini @ 2011-08-11 12:16 UTC (permalink / raw) To: Peter Maydell; +Cc: Blue Swirl, TeLeMan, Jan Kiszka, qemu-devel, David Gilbert On 08/11/2011 01:30 PM, Peter Maydell wrote: >> > Recent compilers look deep into cpu_exec, find longjmp as a noreturn >> > function and decide to smash some stack variables as they won't be used >> > again. This may lead to env becoming invalid after return from setjmp, >> > causing crashes. Fix it by reloading env from cpu_single_env in that >> > case. > Can you give more details of what compiler/platform this was > a problem for? My reading of the C standard is that the compiler > isn't allowed to trash env across this longjmp, because it's > a variable of automatic scope which isn't modified between the > setjmp and the longjmp... longjmp can destroy any non-volatile variable (-Wclobbered warns about this). Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 12:16 ` Paolo Bonzini @ 2011-08-11 12:40 ` Peter Maydell 2011-08-11 13:13 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Peter Maydell @ 2011-08-11 12:40 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Blue Swirl, TeLeMan, Jan Kiszka, qemu-devel, David Gilbert On 11 August 2011 13:16, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 08/11/2011 01:30 PM, Peter Maydell wrote: >> Can you give more details of what compiler/platform this was >> a problem for? My reading of the C standard is that the compiler >> isn't allowed to trash env across this longjmp, because it's >> a variable of automatic scope which isn't modified between the >> setjmp and the longjmp... > > longjmp can destroy any non-volatile variable (-Wclobbered warns about > this). "All accessible objects have values [...] as of the time the longjmp function was called, except that the values of objects of automatic storage duration that are local to the function containing the invocation of the corresponding setjmp macro that do not have volatile-qualified type and have been changed between the setjmp invocation and longjmp call are indeterminate." -- C99 section 7.13.2.1 para 3. So variables may only be destroyed if they are all of: * local to the function calling setjmp * not volatile * changed between setjmp and longjmp We don't change env between the setjmp and longjmp so the compiler should not trash it. (Indeed according to Jan in http://lists.gnu.org/archive/html/qemu-devel/2011-07/msg00144.html -Wclobbered doesn't complain about this code.) -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 12:40 ` Peter Maydell @ 2011-08-11 13:13 ` Paolo Bonzini 2011-08-11 13:31 ` Peter Maydell 0 siblings, 1 reply; 18+ messages in thread From: Paolo Bonzini @ 2011-08-11 13:13 UTC (permalink / raw) To: Peter Maydell; +Cc: Blue Swirl, TeLeMan, Jan Kiszka, qemu-devel, David Gilbert On 08/11/2011 02:40 PM, Peter Maydell wrote: > "All accessible objects have values [...] as of the time the longjmp > function was called, except that the values of objects of automatic > storage duration that are local to the function containing the > invocation of the corresponding setjmp macro that do not have > volatile-qualified type and have been changed between the setjmp > invocation and longjmp call are indeterminate." > -- C99 section 7.13.2.1 para 3. > > So variables may only be destroyed if they are all of: > * local to the function calling setjmp > * not volatile > * changed between setjmp and longjmp I didn't remember this third part. Thanks for bringing up facts. :) > We don't change env between the setjmp and longjmp so the compiler > should not trash it. (Indeed according to Jan in > http://lists.gnu.org/archive/html/qemu-devel/2011-07/msg00144.html > -Wclobbered doesn't complain about this code.) Then it's a compiler bug, not smartness. Making env volatile (or making a volatile copy if there is a performance impact) should still be enough to work around it. Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 13:13 ` Paolo Bonzini @ 2011-08-11 13:31 ` Peter Maydell 2011-08-11 14:10 ` Paolo Bonzini 0 siblings, 1 reply; 18+ messages in thread From: Peter Maydell @ 2011-08-11 13:31 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Blue Swirl, TeLeMan, Jan Kiszka, qemu-devel, David Gilbert On 11 August 2011 14:13, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 08/11/2011 02:40 PM, Peter Maydell wrote: >> We don't change env between the setjmp and longjmp so the compiler >> should not trash it. (Indeed according to Jan in >> http://lists.gnu.org/archive/html/qemu-devel/2011-07/msg00144.html >> -Wclobbered doesn't complain about this code.) > > Then it's a compiler bug, not smartness. Making env volatile (or making a > volatile copy if there is a performance impact) should still be enough to > work around it. Yes. (It would have to be a volatile copy, I think, env is a function parameter and I don't think you can make those volatile.) https://bugs.launchpad.net/qemu/+bug/823902 includes some discussion of the effects on the test of adding the volatile copy. -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 13:31 ` Peter Maydell @ 2011-08-11 14:10 ` Paolo Bonzini 2011-08-11 14:12 ` David Gilbert 2011-08-11 14:24 ` Peter Maydell 0 siblings, 2 replies; 18+ messages in thread From: Paolo Bonzini @ 2011-08-11 14:10 UTC (permalink / raw) To: Peter Maydell; +Cc: Blue Swirl, TeLeMan, Jan Kiszka, qemu-devel, David Gilbert On 08/11/2011 03:31 PM, Peter Maydell wrote: >>> >>> Then it's a compiler bug, not smartness. Making env volatile >>> (or making a volatile copy if there is a performance impact) >>> should still be enough to work around it. > Yes. (It would have to be a volatile copy, I think, env is a function > parameter and I don't think you can make those volatile.) > https://bugs.launchpad.net/qemu/+bug/823902 includes some discussion > of the effects on the test of adding the volatile copy. I'm not sure about what to read from there: > If I make cpu_single_env thread local with __thread and leave > 0d101... in, then again it works reliably on 32bit Lucid, and is > flaky on 64 bit Oneiric (5/10 2 hangs, 3 segs) > > I've also tried using a volatile local variable in cpu_exec to hold > a copy of env and restore that rather than cpu_single_env. With this > it's solid on 32bit lucid and flaky on 64bit Oneirc; these failures > on 64bit OO look like it running off the end of the code buffer (all > 0 code), jumping to non-existent code addresses and a seg in > tb_reset_jump_recursive2. It looks like neither a thread-local cpu_single_env nor a volatile copy fix the bug?!? I cannot think off-hand of a reason why thread-local cpu_single_env should not work for iothread under Unix, BTW. Since cpu_single_env is only set/used by a thread at a time (under the global lock), its users cannot distinguish between a thread-local variable and a global. The only problem would be Windows, which runs cpu_signal in a thread different than the CPU thread. But that can be fixed easily in qemu_cpu_kick_thread. Paolo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 14:10 ` Paolo Bonzini @ 2011-08-11 14:12 ` David Gilbert 2011-08-11 14:24 ` Peter Maydell 1 sibling, 0 replies; 18+ messages in thread From: David Gilbert @ 2011-08-11 14:12 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Blue Swirl, Peter Maydell, Jan Kiszka, qemu-devel, TeLeMan On 11 August 2011 15:10, Paolo Bonzini <pbonzini@redhat.com> wrote: > I'm not sure about what to read from there: > >> If I make cpu_single_env thread local with __thread and leave >> 0d101... in, then again it works reliably on 32bit Lucid, and is >> flaky on 64 bit Oneiric (5/10 2 hangs, 3 segs) >> >> I've also tried using a volatile local variable in cpu_exec to hold >> a copy of env and restore that rather than cpu_single_env. With this >> it's solid on 32bit lucid and flaky on 64bit Oneirc; these failures >> on 64bit OO look like it running off the end of the code buffer (all >> 0 code), jumping to non-existent code addresses and a seg in >> tb_reset_jump_recursive2. > > It looks like neither a thread-local cpu_single_env nor a volatile copy fix > the bug?!? As I say at the bottom of that bug I'm assuming I'm hitting multiple bugs. Although it's not clear to me why I don't hit them on 32bit lucid. Dave ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 14:10 ` Paolo Bonzini 2011-08-11 14:12 ` David Gilbert @ 2011-08-11 14:24 ` Peter Maydell 2011-08-11 14:32 ` Paolo Bonzini 1 sibling, 1 reply; 18+ messages in thread From: Peter Maydell @ 2011-08-11 14:24 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Blue Swirl, TeLeMan, Jan Kiszka, qemu-devel, David Gilbert On 11 August 2011 15:10, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 08/11/2011 03:31 PM, Peter Maydell wrote: >>>> >>>> Then it's a compiler bug, not smartness. Making env volatile >>>> (or making a volatile copy if there is a performance impact) >>>> should still be enough to work around it. >> >> Yes. (It would have to be a volatile copy, I think, env is a function >> parameter and I don't think you can make those volatile.) >> https://bugs.launchpad.net/qemu/+bug/823902 includes some discussion >> of the effects on the test of adding the volatile copy. > > I'm not sure about what to read from there: > >> If I make cpu_single_env thread local with __thread and leave >> 0d101... in, then again it works reliably on 32bit Lucid, and is >> flaky on 64 bit Oneiric (5/10 2 hangs, 3 segs) >> >> I've also tried using a volatile local variable in cpu_exec to hold >> a copy of env and restore that rather than cpu_single_env. With this >> it's solid on 32bit lucid and flaky on 64bit Oneirc; these failures >> on 64bit OO look like it running off the end of the code buffer (all >> 0 code), jumping to non-existent code addresses and a seg in >> tb_reset_jump_recursive2. > > It looks like neither a thread-local cpu_single_env nor a volatile copy fix > the bug?!? My guess is that we're running into the various other problems qemu has with significantly multithreaded programs in user-mode, which makes it a bit hard to disentangle this specific regression. (In particular segfaults in tb_reset_jump_recursive2 are almost certainly bug 668799.) > I cannot think off-hand of a reason why thread-local cpu_single_env should > not work for iothread under Unix, BTW. Since cpu_single_env is only > set/used by a thread at a time (under the global lock), its users cannot > distinguish between a thread-local variable and a global. Thanks for the clarification. As you say, as long as we don't ever try to access it from another thread we're fine... > The only problem would be Windows, which runs cpu_signal in a thread > different than the CPU thread. But that can be fixed easily in > qemu_cpu_kick_thread. ...and we just need to fix this. -- PMM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp 2011-08-11 14:24 ` Peter Maydell @ 2011-08-11 14:32 ` Paolo Bonzini 0 siblings, 0 replies; 18+ messages in thread From: Paolo Bonzini @ 2011-08-11 14:32 UTC (permalink / raw) To: Peter Maydell; +Cc: Blue Swirl, TeLeMan, Jan Kiszka, qemu-devel, David Gilbert On 08/11/2011 04:24 PM, Peter Maydell wrote: >> I cannot think off-hand of a reason why thread-local cpu_single_env should >> not work for iothread under Unix, BTW. Since cpu_single_env is only >> set/used by a thread at a time (under the global lock), its users cannot >> distinguish between a thread-local variable and a global. > > Thanks for the clarification. As you say, as long as we don't ever > try to access it from another thread we're fine... Yes, and the current usage of the lock should be enough of a guarantee. >> The only problem would be Windows, which runs cpu_signal in a thread >> different than the CPU thread. But that can be fixed easily in >> qemu_cpu_kick_thread. > > ...and we just need to fix this. Untested (uncompiled) patch follows: diff --git a/cpus.c b/cpus.c index 6bf4e3f..04e52fe 100644 --- a/cpus.c +++ b/cpus.c @@ -179,10 +179,10 @@ static void cpu_handle_guest_debug(CPUState *env) } #ifdef CONFIG_IOTHREAD -static void cpu_signal(int sig) +static inline void do_cpu_kick(CPUState *env) { - if (cpu_single_env) { - cpu_exit(cpu_single_env); + if (env) { + cpu_exit(env); } exit_request = 1; } @@ -476,6 +476,13 @@ static void qemu_kvm_init_cpu_signals(CPUState *env) } } +#ifdef CONFIG_IOTHREAD +static void cpu_signal(int sig) +{ + do_cpu_kick(cpu_single_env); +} +#endif + static void qemu_tcg_init_cpu_signals(void) { #ifdef CONFIG_IOTHREAD @@ -858,7 +865,7 @@ static void qemu_cpu_kick_thread(CPUState *env) #else /* _WIN32 */ if (!qemu_cpu_is_self(env)) { SuspendThread(env->thread->thread); - cpu_signal(0); + do_cpu_kick(env); ResumeThread(env->thread->thread); } #endif ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2011-08-11 14:33 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-30 16:47 [Qemu-devel] "cpu-exec.c: avoid AREG0 use" breaks x86 emulation on x86-64 Jan Kiszka 2011-06-30 21:17 ` Stefan Weil 2011-07-01 1:44 ` TeLeMan 2011-07-01 20:15 ` Blue Swirl 2011-07-02 7:50 ` [Qemu-devel] [PATCH] tcg: Reload local variables after return from longjmp Jan Kiszka 2011-07-02 9:08 ` Blue Swirl 2011-07-02 9:43 ` Jan Kiszka 2011-07-03 14:09 ` Paolo Bonzini 2011-07-12 20:56 ` Blue Swirl 2011-08-11 11:30 ` Peter Maydell 2011-08-11 12:16 ` Paolo Bonzini 2011-08-11 12:40 ` Peter Maydell 2011-08-11 13:13 ` Paolo Bonzini 2011-08-11 13:31 ` Peter Maydell 2011-08-11 14:10 ` Paolo Bonzini 2011-08-11 14:12 ` David Gilbert 2011-08-11 14:24 ` Peter Maydell 2011-08-11 14:32 ` 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.