i386: fix breakpoints handling in icount mode
PATCH 1/1 i386: fix breakpoints handling in icount mode
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
target-i386/translate.c +2 -1
@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating; } } }
@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();gen_tb_end (tb, num_insns);
On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
Hi Pavel,
Quoted 15 lines
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)target-i386/translate.c +0
-0
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8003
Quoted 5 lines
target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating;
This makes sense to me.
But I don't see why you don't just "break" like the other instruction in this loop?
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8054
Quoted 10 lines
target-i386/translate.c +1
-0} } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();
Is there any reason why you don't jump over this two lines in case of a breakpoint?
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8055
Quoted 3 lines
target-i386/translate.c +0
-0gen_tb_end (tb, num_insns);
I'll give it a try later and I'll let you know.
Thanks,
Fred
Quoted 14 lines
From: Frederic Konrad [mailto:[email protected]] On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
Hi Pavel,
Quoted 10 lines
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
Quoted 13 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8003
Quoted 10 lines
target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating;This makes sense to me.
But I don't see why you don't just "break" like the other instruction in this loop?
Single break will just exit the breakpoints iteration loop. I'll need an additional flag to break the translation loop. ARM does the same thing, anyway :)
Quoted 4 lines
Quoted 4 lines
target-i386/translate.c +0
-0} } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,
Quoted 8 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8054
Quoted 6 lines
target-i386/translate.c +1
-0break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();Is there any reason why you don't jump over this two lines in case of a breakpoint?
Shouldn't we switch off can_do_io flag if it was switched on?
Quoted 6 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8055
Quoted 4 lines
target-i386/translate.c +0
-0gen_tb_end (tb, num_insns);I'll give it a try later and I'll let you know.
Thanks.
Pavel Dovgalyuk
Quoted 27 lines · Pavel Dovgaluk
Quoted 14 lines
From: Frederic Konrad [mailto:[email protected]] On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
Hi Pavel,
Quoted 10 lines
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)Quoted 13 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8003
Quoted 10 lines
target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating;This makes sense to me.
But I don't see why you don't just "break" like the other instruction in this loop?
Quoted 2 lines
Single break will just exit the breakpoints iteration loop. I'll need an additional flag to break the translation loop. ARM does the same thing, anyway :)
Yes that's what I mentioned.
Quoted 13 lines
Quoted 13 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8054
Quoted 11 lines
target-i386/translate.c +1
-0} } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();Is there any reason why you don't jump over this two lines in case of a breakpoint?
Quoted 1 line
Shouldn't we switch off
can_do_ioflag if it was switched on?
Yes but can we switch on can_do_io if we have a breakpoint?
The code is not shown in this patch but there is:
if (num_insns + 1 == max_insns && (tb->cflags & CF_LAST_IO))
gen_io_start();I think you can't reach this code if you exit the translation loop?
Fred
Quoted 10 lines
Quoted 5 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8055
Quoted 4 lines
target-i386/translate.c +0
-0gen_tb_end (tb, num_insns);I'll give it a try later and I'll let you know.
Thanks.
Pavel Dovgalyuk
Quoted 48 lines
From: Frederic Konrad [mailto:[email protected]]
Quoted 27 lines · Pavel Dovgaluk
Quoted 14 lines
From: Frederic Konrad [mailto:[email protected]] On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
Hi Pavel,
Quoted 10 lines
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)Quoted 13 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8003
Quoted 10 lines
target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating;This makes sense to me.
But I don't see why you don't just "break" like the other instruction in this loop?Quoted 2 lines
Single break will just exit the breakpoints iteration loop. I'll need an additional flag to break the translation loop. ARM does the same thing, anyway :)
Yes that's what I mentioned.
Quoted 13 lines
Quoted 13 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8054
Quoted 11 lines
target-i386/translate.c +1
-0} } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();Is there any reason why you don't jump over this two lines in case of a breakpoint?
Quoted 1 line
Shouldn't we switch off
can_do_ioflag if it was switched on?
Quoted 8 lines
Yes but can we switch on
can_do_ioif we have a breakpoint?The code is not shown in this patch but there is:
if (num_insns + 1 == max_insns && (tb->cflags & CF_LAST_IO)) gen_io_start();I think you can't reach this code if you exit the translation loop?
This is not the only gen_io_start call. It is called from some of the instructions' translation functions, that could precede the breakpoint.
Pavel Dovgalyuk
Quoted 49 lines · Pavel Dovgaluk
Quoted 49 lines
From: Frederic Konrad [mailto:[email protected]]
Quoted 27 lines · Pavel Dovgaluk
Quoted 14 lines
From: Frederic Konrad [mailto:[email protected]] On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
Hi Pavel,
Quoted 10 lines
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)Quoted 13 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8003
Quoted 10 lines
target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating;This makes sense to me.
But I don't see why you don't just "break" like the other instruction in this loop?Quoted 2 lines
Single break will just exit the breakpoints iteration loop. I'll need an additional flag to break the translation loop. ARM does the same thing, anyway :)
Yes that's what I mentioned.
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
target-i386/translate.c +0
-0} } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,Quoted 8 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8054
Quoted 6 lines
target-i386/translate.c +1
-0break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();Is there any reason why you don't jump over this two lines in case of a breakpoint?
Quoted 1 line
Shouldn't we switch off
can_do_ioflag if it was switched on?Yes but can we switch on
can_do_ioif we have a breakpoint?The code is not shown in this patch but there is:
Quoted 10 lines
Quoted 4 lines
if (num_insns + 1 == max_insns && (tb->cflags & CF_LAST_IO)) gen_io_start();I think you can't reach this code if you exit the translation loop?
This is not the only
gen_io_startcall. It is called from some of the instructions' translation functions, that could precede the breakpoint.Pavel Dovgalyuk
True, there are 8 others place where gen_io_start is called in this file, but they seems to be each time followed by a gen_io_end?
eg:
static inline void gen_ins(DisasContext *s, TCGMemOp ot)
{
if (use_icount)
gen_io_start();
gen_string_movl_A0_EDI(s);
/* Note: we must do this dummy write first to be restartable in
case of page fault. */
tcg_gen_movi_tl(cpu_T[0], 0);
gen_op_st_v(s, ot, cpu_T[0], cpu_A0);
tcg_gen_trunc_tl_i32(cpu_tmp2_i32, cpu_regs[R_EDX]);
tcg_gen_andi_i32(cpu_tmp2_i32, cpu_tmp2_i32, 0xffff);
gen_helper_in_func(ot, cpu_T[0], cpu_tmp2_i32);
gen_op_st_v(s, ot, cpu_T[0], cpu_A0);
gen_op_movl_T0_Dshift(ot);
gen_op_add_reg_T0(s->aflag, R_EDI);
if (use_icount)
gen_io_end();
}Quoted 58 lines
From: Frederic Konrad [mailto:[email protected]]
Quoted 56 lines · Pavel Dovgaluk
Quoted 45 lines
From: Frederic Konrad [mailto:[email protected]]
Quoted 27 lines · Pavel Dovgaluk
Quoted 14 lines
From: Frederic Konrad [mailto:[email protected]] On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
Hi Pavel,
Quoted 10 lines
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)Quoted 13 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8003
Quoted 10 lines
target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating;This makes sense to me.
But I don't see why you don't just "break" like the other instruction in this loop?Quoted 2 lines
Single break will just exit the breakpoints iteration loop. I'll need an additional flag to break the translation loop. ARM does the same thing, anyway :)
Yes that's what I mentioned.
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
target-i386/translate.c +0
-0} } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,Quoted 8 lines
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8054
Quoted 6 lines
target-i386/translate.c +1
-0break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();Is there any reason why you don't jump over this two lines in case of a breakpoint?
Quoted 1 line
Shouldn't we switch off
can_do_ioflag if it was switched on?Quoted 8 lines
Yes but can we switch on
can_do_ioif we have a breakpoint?The code is not shown in this patch but there is:
if (num_insns + 1 == max_insns && (tb->cflags & CF_LAST_IO)) gen_io_start();I think you can't reach this code if you exit the translation loop?
This is not the only
gen_io_startcall. It is called from some of the instructions' translation functions, that could precede the breakpoint.
Quoted 6 lines
Quoted 3 lines
Pavel Dovgalyuk
True, there are 8 others place where
gen_io_startis called in this file, but they
seems to be each time followed by agen_io_end?
Right. Here is the updated patch:
target-i386/translate.c +2 -1
@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating; } } }
@@ -8051,6 +8051,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu, }if (tb->cflags &CF_LAST_IO )gen_io_end (); +done_generating:gen_tb_end (tb, num_insns); *tcg_ctx.gen_opc_ptr = INDEX_op_end;/* we don't forget to fill the last values */
Paolo Bonzini
AppliedQuoted 25 lines · Pavel Dovgalyuk
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating; } } }
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8055
Quoted 7 lines
target-i386/translate.c +1
-0@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();gen_tb_end (tb, num_insns);
Applied, thanks.
Paolo
Quoted 27 lines · Pavel Dovgalyuk
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)target-i386/translate.c +1
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating; } } }
target-i386/translate.c static inline void gen_intermediate_code_internal(X86CPU *cpu, · line 8055
Quoted 7 lines
target-i386/translate.c +1
-0@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();gen_tb_end (tb, num_insns);
Didn't looked into why, just bisected that this patch breaks at least certain guest-originated break- or watchpoints in TCG mode. Can be triggered by booting a Linux kernel with kgdb self-tests enabled. The result is some false reporting of a host-originated debug stop to gdb_set_stop_cpu while gdbserver_state is NULL -> SEGV.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux
Quoted 34 lines
From: Jan Kiszka [mailto:[email protected]]
Quoted 32 lines · Pavel Dovgalyuk
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)target-i386/translate.c +2
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating; } } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();gen_tb_end (tb, num_insns);
Quoted 8 lines
Quoted 2 lines
Didn't looked into why, just bisected that this patch breaks at least certain guest-originated break- or watchpoints in TCG mode. Can be triggered by booting a Linux kernel with kgdb self-tests enabled. The result is some false reporting of a host-originated debug stop to
gdb_set_stop_cpuwhilegdbserver_stateis NULL -> SEGV.
It seems that kernel sets some hardware breakpoints and QEMU tries to process them with GDB stub. Modifying gdb_set_stop_cpu should help:
gdbstub.c +4 -2
@@ -1202,8 +1202,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)void gdb_set_stop_cpu (CPUState *cpu) { - gdbserver_state->c_cpu = cpu; - gdbserver_state->g_cpu = cpu; +if (gdbserver_state) { + gdbserver_state->c_cpu = cpu; + gdbserver_state->g_cpu = cpu; + } }#ifndef CONFIG_USER_ONLY
Pavel Dovgalyuk
Quoted 53 lines · Pavel Dovgaluk
Quoted 34 lines
From: Jan Kiszka [mailto:[email protected]]
Quoted 32 lines · Pavel Dovgalyuk
This patch fixes instructions counting when execution is stopped on breakpoint (e.g. set from gdb). Without a patch extra instruction is translated and icount is incremented by invalid value (which equals to number of executed instructions + 1).
Signed-off-by Pavel Dovgalyuk <[email protected]>--- target-i386/translate.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)target-i386/translate.c +2
-1@@ -8000,7 +8000,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); -break ; + goto done_generating; } } }@@ -8049,6 +8049,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,break ; } } +done_generating:if (tb->cflags &CF_LAST_IO )gen_io_end ();gen_tb_end (tb, num_insns);Quoted 8 lines
Quoted 2 lines
Didn't looked into why, just bisected that this patch breaks at least certain guest-originated break- or watchpoints in TCG mode. Can be triggered by booting a Linux kernel with kgdb self-tests enabled. The result is some false reporting of a host-originated debug stop to
gdb_set_stop_cpuwhilegdbserver_stateis NULL -> SEGV.It seems that kernel sets some hardware breakpoints and QEMU tries to process them with GDB stub. Modifying
gdb_set_stop_cpushould help:gdbstub.c +0
-0@@ -1202,8 +1202,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)void gdb_set_stop_cpu (CPUState *cpu)
gdbstub.c static int gdb_handle_packet(GDBState *s, const char *line_buf) · line 1211
Quoted 10 lines
gdbstub.c +4
-2{ - gdbserver_state->c_cpu = cpu; - gdbserver_state->g_cpu = cpu; +if (gdbserver_state) { + gdbserver_state->c_cpu = cpu; + gdbserver_state->g_cpu = cpu; + } }#ifndef CONFIG_USER_ONLY
I think this would only cure a symptom, but it doesn't explain why we now hit cpu_handle_guest_debug which we do not before the patch:
(gdb) bt
#0 gdb_set_stop_cpu (cpu=cpu@entry=0x55555663ac40) at /data/qemu/gdbstub.c:1193 #1 0x000055555562dfdf in cpu_handle_guest_debug (cpu=0x55555663ac40) at /data/qemu/cpus.c:636 #2 tcg_exec_all () at /data/qemu/cpus.c:1389 #3 qemu_tcg_cpu_thread_fn (arg=<optimized out>) at /data/qemu/cpus.c:1033 #4 0x00007ffff2c3d0a4 in start_thread () from /lib64/libpthread.so.0 #5 0x00007fffeea4a7fd in clone () from /lib64/libc.so.6
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux
Quoted 2 lines · Jan Kiszka
I think this would only cure a symptom, but it doesn't explain why we now hit
cpu_handle_guest_debugwhich we do not before the patch:
That means we now exit with EXCP_DEBUG and we didn't before?
Something like this would be a more complete fix (it works if you have both gdb and CPU breakpoints), but I'm not sure if it's also a band-aid for the symptoms.
cpu-exec.c +10 -7
@@ -302,7 +302,7 @@ static inline TranslationBlock *tb_find_fast(CPUArchState *env)return tb; } -static void cpu_handle_debug_exception (CPUArchState *env) +static int cpu_handle_debug_exception (CPUArchState *env) {CPUState *cpu =ENV_GET_CPU (env);CPUClass *cc =CPU_GET_CLASS (cpu);
@@ -314,7 +314,7 @@ static void cpu_handle_debug_exception(CPUArchState *env) } } - cc->debug_excp_handler (cpu); +return cc->debug_excp_handler (cpu); }/* main execution loop */
@@ -375,12 +375,15 @@ int cpu_exec(CPUArchState *env)if (cpu->exception_index >=0 ) {if (cpu->exception_index >=EXCP_INTERRUPT ) {/* exit request from the cpu execution loop */ - ret = cpu->exception_index; -if (ret ==EXCP_DEBUG ) { -cpu_handle_debug_exception (env); +if (cpu->exception_index ==EXCP_DEBUG ) { + ret =cpu_handle_debug_exception (env); + }else { + ret = cpu->exception_index; + } +if (ret >=0 ) { + cpu->exception_index =-1 ; +break ; } - cpu->exception_index =-1 ; -break ; }else {#if defined(CONFIG_USER_ONLY )/* if user mode only, we simulate a fake exception
include/qom/cpu.h +3 -2
@@ -95,7 +95,8 @@ struct TranslationBlock;* @get_phys_page_debug: Callback for obtaining a physical address. * @gdb_read_register: Callback for letting GDB read a register. * @gdb_write_register: Callback for letting GDB write a register. -* @debug_excp_handler: Callback for handling debug exceptions. +* @debug_excp_handler: Callback for handling debug exceptions. Should +* return either #EXCP_DEBUG or zero. * @vmsd: State description for migration. * @gdb_num_core_regs: Number of core registers accessible to GDB. * @gdb_core_xml_file: File name for core registers GDB XML description.
@@ -140,7 +141,7 @@ typedef struct CPUClass {hwaddr (*get_phys_page_debug)(CPUState *cpu,vaddr addr);int (*gdb_read_register)(CPUState *cpu,uint8_t *buf,int reg);int (*gdb_write_register)(CPUState *cpu,uint8_t *buf,int reg); -void (*debug_excp_handler)(CPUState *cpu); +int (*debug_excp_handler)(CPUState *cpu);int (*write_elf64_note)(WriteCoreDumpFunction f,CPUState *cpu,int cpuid,void *opaque);
qom/cpu.c +6 -1
@@ -193,6 +193,11 @@ static bool cpu_common_virtio_is_big_endian(CPUState *cpu)return target_words_bigendian (); } +static int cpu_common_debug_excp_handler (CPUState *cpu) +{ +return EXCP_DEBUG ; +} +static void cpu_common_noop (CPUState *cpu) { }
@@ -340,7 +345,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) k->gdb_read_register = cpu_common_gdb_read_register; k->gdb_write_register = cpu_common_gdb_write_register; k->virtio_is_big_endian = cpu_common_virtio_is_big_endian; - k->debug_excp_handler = cpu_common_noop; + k->debug_excp_handler = cpu_common_debug_excp_handler; k->cpu_exec_enter = cpu_common_noop; k->cpu_exec_exit = cpu_common_noop; k->cpu_exec_interrupt = cpu_common_exec_interrupt;
target-arm/op_helper.c +5 -3
@@ -732,7 +732,7 @@ static bool check_breakpoints(ARMCPU *cpu)return false; } -void arm_debug_excp_handler (CPUState *cs) +int arm_debug_excp_handler (CPUState *cs) {/* Called by core code when a watchpoint or breakpoint fires; * need to check which one and raise the appropriate exception.
@@ -756,9 +756,9 @@ void arm_debug_excp_handler(CPUState *cs) } env->exception.vaddress = wp_hit->hitaddr;raise_exception (env,EXCP_DATA_ABORT ); - }else { -cpu_resume_from_signal (cs,NULL ); +return 0 ; } +cpu_resume_from_signal (cs,NULL ); } }else {if (check_breakpoints (cpu)) {
@@ -771,8 +771,10 @@ void arm_debug_excp_handler(CPUState *cs) }/* FAR is UNKNOWN, so doesn't need setting */ raise_exception (env,EXCP_PREFETCH_ABORT ); +return 0 ; } } +return EXCP_DEBUG ; }/* ??? Flag setting arithmetic is awkward because we need to do comparisons.
target-i386/helper.c +5 -1
@@ -1004,17 +1004,19 @@ bool check_hw_breakpoints(CPUX86State *env, bool force_dr6_update)return hit_enabled; } -void breakpoint_handler (CPUState *cs) +int breakpoint_handler (CPUState *cs) {X86CPU *cpu =X86_CPU (cs);CPUX86State *env = &cpu->env;CPUBreakpoint *bp; +int ret =EXCP_DEBUG ;if (cs->watchpoint_hit) {if (cs->watchpoint_hit->flags &BP_CPU ) { cs->watchpoint_hit =NULL ;if (check_hw_breakpoints (env, false)) {raise_exception (env,EXCP01_DB ); + ret =0 ; }else {cpu_resume_from_signal (cs,NULL ); }
@@ -1025,11 +1027,13 @@ void breakpoint_handler(CPUState *cs)if (bp->flags &BP_CPU ) {check_hw_breakpoints (env, true);raise_exception (env,EXCP01_DB ); + ret =0 ; }break ; } } } +return ret; } typedef struct MCEInjectionParams {
target-lm32/helper.c +5 -1
@@ -125,17 +125,19 @@ static bool check_watchpoints(CPULM32State *env)return false; } -void lm32_debug_excp_handler (CPUState *cs) +int lm32_debug_excp_handler (CPUState *cs) {LM32CPU *cpu =LM32_CPU (cs);CPULM32State *env = &cpu->env;CPUBreakpoint *bp; +int ret =EXCP_DEBUG ;if (cs->watchpoint_hit) {if (cs->watchpoint_hit->flags &BP_CPU ) { cs->watchpoint_hit =NULL ;if (check_watchpoints (env)) {raise_exception (env,EXCP_WATCHPOINT ); + ret =0 ; }else {cpu_resume_from_signal (cs,NULL ); }
@@ -145,11 +147,13 @@ void lm32_debug_excp_handler(CPUState *cs)if (bp->pc == env->pc) {if (bp->flags &BP_CPU ) {raise_exception (env,EXCP_BREAKPOINT ); + ret =0 ; }break ; } } } +return ret; }void lm32_cpu_do_interrupt (CPUState *cs)
target-xtensa/helper.c +3 -1
@@ -79,7 +79,7 @@ static uint32_t check_hw_breakpoints(CPUXtensaState *env)return 0 ; } -void xtensa_breakpoint_handler (CPUState *cs) +int xtensa_breakpoint_handler (CPUState *cs) {XtensaCPU *cpu =XTENSA_CPU (cs);CPUXtensaState *env = &cpu->env;
@@ -92,10 +92,12 @@ void xtensa_breakpoint_handler(CPUState *cs) cause =check_hw_breakpoints (env);if (cause) {debug_exception_env (env, cause); +return 0 ; }cpu_resume_from_signal (cs,NULL ); } } +return EXCP_DEBUG ; }XtensaCPU *cpu_xtensa_init (const char *cpu_model)
Quoted 35 lines · Paolo Bonzini
Quoted 2 lines · Jan Kiszka
I think this would only cure a symptom, but it doesn't explain why we now hit
cpu_handle_guest_debugwhich we do not before the patch:That means we now exit with
EXCP_DEBUGand we didn't before?Something like this would be a more complete fix (it works if you have both gdb and CPU breakpoints), but I'm not sure if it's also a band-aid for the symptoms.
cpu-exec.c +2
-2@@ -302,7 +302,7 @@ static inline TranslationBlock *tb_find_fast(CPUArchState *env)return tb; } -static void cpu_handle_debug_exception (CPUArchState *env) +static int cpu_handle_debug_exception (CPUArchState *env) {CPUState *cpu =ENV_GET_CPU (env);CPUClass *cc =CPU_GET_CLASS (cpu);@@ -314,7 +314,7 @@ static void cpu_handle_debug_exception(CPUArchState *env) } } - cc->debug_excp_handler (cpu); +return cc->debug_excp_handler (cpu); }/* main execution loop */ @@ -375,12 +375,15 @@ int cpu_exec(CPUArchState *env)if (cpu->exception_index >=0 ) {if (cpu->exception_index >=EXCP_INTERRUPT ) {
cpu-exec.c int cpu_exec(CPUArchState *env) · line 383
Quoted 10 lines
cpu-exec.c +6
-3/* exit request from the cpu execution loop */ - ret = cpu->exception_index; -if (ret ==EXCP_DEBUG ) { -cpu_handle_debug_exception (env); +if (cpu->exception_index ==EXCP_DEBUG ) { + ret =cpu_handle_debug_exception (env); + }else { + ret = cpu->exception_index; + } +if (ret >=0 ) {
This condition is always true for both 0 and EXCP_DEBUG.
Quoted 188 lines
cpu-exec.c +2
-2+ cpu->exception_index =-1 ; +break ; } - cpu->exception_index =-1 ; -break ; }else {#if defined(CONFIG_USER_ONLY )/* if user mode only, we simulate a fake exception include/qom/cpu.h +3
-2@@ -95,7 +95,8 @@ struct TranslationBlock;* @get_phys_page_debug: Callback for obtaining a physical address. * @gdb_read_register: Callback for letting GDB read a register. * @gdb_write_register: Callback for letting GDB write a register. -* @debug_excp_handler: Callback for handling debug exceptions. +* @debug_excp_handler: Callback for handling debug exceptions. Should +* return either #EXCP_DEBUG or zero. * @vmsd: State description for migration. * @gdb_num_core_regs: Number of core registers accessible to GDB. * @gdb_core_xml_file: File name for core registers GDB XML description. @@ -140,7 +141,7 @@ typedef struct CPUClass {hwaddr (*get_phys_page_debug)(CPUState *cpu,vaddr addr);int (*gdb_read_register)(CPUState *cpu,uint8_t *buf,int reg);int (*gdb_write_register)(CPUState *cpu,uint8_t *buf,int reg); -void (*debug_excp_handler)(CPUState *cpu); +int (*debug_excp_handler)(CPUState *cpu);int (*write_elf64_note)(WriteCoreDumpFunction f,CPUState *cpu,int cpuid,void *opaque);qom/cpu.c +6
-1@@ -193,6 +193,11 @@ static bool cpu_common_virtio_is_big_endian(CPUState *cpu)return target_words_bigendian (); } +static int cpu_common_debug_excp_handler (CPUState *cpu) +{ +return EXCP_DEBUG ; +} +static void cpu_common_noop (CPUState *cpu) { }@@ -340,7 +345,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) k->gdb_read_register = cpu_common_gdb_read_register; k->gdb_write_register = cpu_common_gdb_write_register; k->virtio_is_big_endian = cpu_common_virtio_is_big_endian; - k->debug_excp_handler = cpu_common_noop; + k->debug_excp_handler = cpu_common_debug_excp_handler; k->cpu_exec_enter = cpu_common_noop; k->cpu_exec_exit = cpu_common_noop; k->cpu_exec_interrupt = cpu_common_exec_interrupt;target-arm/op_helper.c +5
-3@@ -732,7 +732,7 @@ static bool check_breakpoints(ARMCPU *cpu)return false; } -void arm_debug_excp_handler (CPUState *cs) +int arm_debug_excp_handler (CPUState *cs) {/* Called by core code when a watchpoint or breakpoint fires; * need to check which one and raise the appropriate exception. @@ -756,9 +756,9 @@ void arm_debug_excp_handler(CPUState *cs) } env->exception.vaddress = wp_hit->hitaddr;raise_exception (env,EXCP_DATA_ABORT ); - }else { -cpu_resume_from_signal (cs,NULL ); +return 0 ; } +cpu_resume_from_signal (cs,NULL ); } }else {if (check_breakpoints (cpu)) {@@ -771,8 +771,10 @@ void arm_debug_excp_handler(CPUState *cs) }/* FAR is UNKNOWN, so doesn't need setting */ raise_exception (env,EXCP_PREFETCH_ABORT ); +return 0 ; } } +return EXCP_DEBUG ; }/* ??? Flag setting arithmetic is awkward because we need to do comparisons. target-i386/helper.c +5
-1@@ -1004,17 +1004,19 @@ bool check_hw_breakpoints(CPUX86State *env, bool force_dr6_update)return hit_enabled; } -void breakpoint_handler (CPUState *cs) +int breakpoint_handler (CPUState *cs) {X86CPU *cpu =X86_CPU (cs);CPUX86State *env = &cpu->env;CPUBreakpoint *bp; +int ret =EXCP_DEBUG ;if (cs->watchpoint_hit) {if (cs->watchpoint_hit->flags &BP_CPU ) { cs->watchpoint_hit =NULL ;if (check_hw_breakpoints (env, false)) {raise_exception (env,EXCP01_DB ); + ret =0 ; }else {cpu_resume_from_signal (cs,NULL ); }@@ -1025,11 +1027,13 @@ void breakpoint_handler(CPUState *cs)if (bp->flags &BP_CPU ) {check_hw_breakpoints (env, true);raise_exception (env,EXCP01_DB ); + ret =0 ; }break ; } } } +return ret; } typedef struct MCEInjectionParams {target-lm32/helper.c +5
-1@@ -125,17 +125,19 @@ static bool check_watchpoints(CPULM32State *env)return false; } -void lm32_debug_excp_handler (CPUState *cs) +int lm32_debug_excp_handler (CPUState *cs) {LM32CPU *cpu =LM32_CPU (cs);CPULM32State *env = &cpu->env;CPUBreakpoint *bp; +int ret =EXCP_DEBUG ;if (cs->watchpoint_hit) {if (cs->watchpoint_hit->flags &BP_CPU ) { cs->watchpoint_hit =NULL ;if (check_watchpoints (env)) {raise_exception (env,EXCP_WATCHPOINT ); + ret =0 ; }else {cpu_resume_from_signal (cs,NULL ); }@@ -145,11 +147,13 @@ void lm32_debug_excp_handler(CPUState *cs)if (bp->pc == env->pc) {if (bp->flags &BP_CPU ) {raise_exception (env,EXCP_BREAKPOINT ); + ret =0 ; }break ; } } } +return ret; }void lm32_cpu_do_interrupt (CPUState *cs)target-xtensa/helper.c +1
-1@@ -79,7 +79,7 @@ static uint32_t check_hw_breakpoints(CPUXtensaState *env)return 0 ; } -void xtensa_breakpoint_handler (CPUState *cs) +int xtensa_breakpoint_handler (CPUState *cs) {XtensaCPU *cpu =XTENSA_CPU (cs);CPUXtensaState *env = &cpu->env;@@ -92,10 +92,12 @@ void xtensa_breakpoint_handler(CPUState *cs) cause =check_hw_breakpoints (env);if (cause) {
target-xtensa/helper.c void xtensa_breakpoint_handler(CPUState *cs) · line 103
Quoted 10 lines
target-xtensa/helper.c +2
-0debug_exception_env (env, cause); +return 0 ; }cpu_resume_from_signal (cs,NULL ); } } +return EXCP_DEBUG ; }XtensaCPU *cpu_xtensa_init (const char *cpu_model)
Lost track of this: This doesn't build (EXCP_DEBUG not available to qom/cpu.c, wrong breakpoint_handler prototype) and then, when the build issues are fixed, it doesn't work.
Playing a bit with the code, I found out that this cures the issue:
target-i386/translate.c +1 -0
@@ -8006,6 +8006,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); + pc_ptr =disas_insn (env, dc, pc_ptr); goto done_generating; } }
pc_ptr is used at the end of the function to calculate the tb size. I suspect that the difference prevents that the breakpoint event is associated with the stored location. Can someone explain this more properly? Then I would happily pass patch credits.
Jan
Quoted 13 lines · Jan Kiszka
target-i386/translate.c +1
-0@@ -8006,6 +8006,7 @@ static inline void gen_intermediate_code_internal(X86CPU *cpu,if (bp->pc == pc_ptr && !((bp->flags &BP_CPU ) && (tb->flags &HF_RF_MASK ))) {gen_debug (dc, pc_ptr - dc->cs_base); + pc_ptr =disas_insn (env, dc, pc_ptr); goto done_generating; } }
Quoted 4 lines
pc_ptris used at the end of the function to calculate the tb size. I suspect that the difference prevents that the breakpoint event is associated with the stored location. Can someone explain this more properly? Then I would happily pass patch credits.
So when a breakpoint is removed at address X, you have to also remove translation blocks that end exactly at X? That is:
translate-all.c +1 -1
@@ -1113,7 +1116,7 @@ tb_start = tb->page_addr[1 ]; tb_end = tb_start + ((tb->pc + tb->size) & ~TARGET_PAGE_MASK ); } -if (!(tb_end <= start || tb_start >= end)) { +if (tb_start < end && tb_end >= start) {#ifdef TARGET_HAS_PRECISE_SMC if (current_tb_not_found) { current_tb_not_found =0 ;
Does this fix the bug? Is there any other case where this is desirable? Should tb_invalidate_phys_page_range grow another argument to choose between "tb_end > start" and "tb_end >= start"?
Paolo