[Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[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 <pavel.dovgaluk@ispras.ru>
---
 target-i386/translate.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 1284173..193cf9f 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -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);

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Frederic Konrad]

On 22/10/2014 13:38, Pavel Dovgalyuk wrote:

Hi Pavel,
> 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 <pavel.dovgaluk@ispras.ru>
> ---
>   target-i386/translate.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 1284173..193cf9f 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -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?

>                   }
>               }
>           }
> @@ -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?

>       gen_tb_end(tb, num_insns);
>
>

I'll give it a try later and I'll let you know.

Thanks,
Fred

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Pavel Dovgaluk]

> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
> On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
> 
> Hi Pavel,
> > 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 <pavel.dovgaluk@ispras.ru>
> > ---
> >   target-i386/translate.c |    3 ++-
> >   1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/target-i386/translate.c b/target-i386/translate.c
> > index 1284173..193cf9f 100644
> > --- a/target-i386/translate.c
> > +++ b/target-i386/translate.c
> > @@ -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 :)

> >                   }
> >               }
> >           }
> > @@ -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?

Shouldn't we switch off can_do_io flag if it was switched on?

> 
> >       gen_tb_end(tb, num_insns);
> >
> >
> 
> I'll give it a try later and I'll let you know.

Thanks.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Frederic Konrad]

On 23/10/2014 07:57, Pavel Dovgaluk wrote:
>> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
>> On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
>>
>> Hi Pavel,
>>> 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 <pavel.dovgaluk@ispras.ru>
>>> ---
>>>    target-i386/translate.c |    3 ++-
>>>    1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/target-i386/translate.c b/target-i386/translate.c
>>> index 1284173..193cf9f 100644
>>> --- a/target-i386/translate.c
>>> +++ b/target-i386/translate.c
>>> @@ -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 :)

Yes that's what I mentioned.
>
>>>                    }
>>>                }
>>>            }
>>> @@ -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?
> Shouldn't we switch off can_do_io flag 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
>
>>>        gen_tb_end(tb, num_insns);
>>>
>>>
>> I'll give it a try later and I'll let you know.
> Thanks.
>
> Pavel Dovgalyuk
>
>

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Pavel Dovgaluk]

> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
> On 23/10/2014 07:57, Pavel Dovgaluk wrote:
> >> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
> >> On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
> >>
> >> Hi Pavel,
> >>> 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 <pavel.dovgaluk@ispras.ru>
> >>> ---
> >>>    target-i386/translate.c |    3 ++-
> >>>    1 files changed, 2 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/target-i386/translate.c b/target-i386/translate.c
> >>> index 1284173..193cf9f 100644
> >>> --- a/target-i386/translate.c
> >>> +++ b/target-i386/translate.c
> >>> @@ -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 :)
> 
> Yes that's what I mentioned.
> >
> >>>                    }
> >>>                }
> >>>            }
> >>> @@ -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?
> > Shouldn't we switch off can_do_io flag 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?

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

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Frederic Konrad]

On 23/10/2014 09:52, Pavel Dovgaluk wrote:
>> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
>> On 23/10/2014 07:57, Pavel Dovgaluk wrote:
>>>> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
>>>> On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
>>>>
>>>> Hi Pavel,
>>>>> 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 <pavel.dovgaluk@ispras.ru>
>>>>> ---
>>>>>     target-i386/translate.c |    3 ++-
>>>>>     1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/target-i386/translate.c b/target-i386/translate.c
>>>>> index 1284173..193cf9f 100644
>>>>> --- a/target-i386/translate.c
>>>>> +++ b/target-i386/translate.c
>>>>> @@ -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 :)
>> Yes that's what I mentioned.
>>>>>                     }
>>>>>                 }
>>>>>             }
>>>>> @@ -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?
>>> Shouldn't we switch off can_do_io flag 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?
> 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
>
>
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();
}

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Pavel Dovgaluk]

> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
> On 23/10/2014 09:52, Pavel Dovgaluk wrote:
> >> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
> >> On 23/10/2014 07:57, Pavel Dovgaluk wrote:
> >>>> From: Frederic Konrad [mailto:fred.konrad@greensocs.com]
> >>>> On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
> >>>>
> >>>> Hi Pavel,
> >>>>> 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 <pavel.dovgaluk@ispras.ru>
> >>>>> ---
> >>>>>     target-i386/translate.c |    3 ++-
> >>>>>     1 files changed, 2 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/target-i386/translate.c b/target-i386/translate.c
> >>>>> index 1284173..193cf9f 100644
> >>>>> --- a/target-i386/translate.c
> >>>>> +++ b/target-i386/translate.c
> >>>>> @@ -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 :)
> >> Yes that's what I mentioned.
> >>>>>                     }
> >>>>>                 }
> >>>>>             }
> >>>>> @@ -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?
> >>> Shouldn't we switch off can_do_io flag 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?
> > 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
> >
> >
> 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?

Right. Here is the updated patch:

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 1284173..4d5dfb3 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -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 */

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Paolo Bonzini]

On 22/10/2014 13:38, Pavel Dovgalyuk wrote:
> 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 <pavel.dovgaluk@ispras.ru>
> ---
>  target-i386/translate.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 1284173..193cf9f 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -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);
> 

Applied, thanks.

Paolo

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Jan Kiszka]

On 2014-10-22 13:38, Pavel Dovgalyuk wrote:
> 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 <pavel.dovgaluk@ispras.ru>
> ---
>  target-i386/translate.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 1284173..193cf9f 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -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);
> 
> 
> 

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Pavel Dovgaluk]

> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
> On 2014-10-22 13:38, Pavel Dovgalyuk wrote:
> > 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 <pavel.dovgaluk@ispras.ru>
> > ---
> >  target-i386/translate.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/target-i386/translate.c b/target-i386/translate.c
> > index 1284173..193cf9f 100644
> > --- a/target-i386/translate.c
> > +++ b/target-i386/translate.c
> > @@ -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);
> >
> >
> >
> 
> 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.

It seems that kernel sets some hardware breakpoints and QEMU tries to process 
them with GDB stub. Modifying gdb_set_stop_cpu should help:

diff --git a/gdbstub.c b/gdbstub.c
index e4a1a79..e8ef546 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -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

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Jan Kiszka]

On 2015-01-12 09:26, Pavel Dovgaluk wrote:
>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
>> On 2014-10-22 13:38, Pavel Dovgalyuk wrote:
>>> 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 <pavel.dovgaluk@ispras.ru>
>>> ---
>>>  target-i386/translate.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/target-i386/translate.c b/target-i386/translate.c
>>> index 1284173..193cf9f 100644
>>> --- a/target-i386/translate.c
>>> +++ b/target-i386/translate.c
>>> @@ -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);
>>>
>>>
>>>
>>
>> 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.
> 
> It seems that kernel sets some hardware breakpoints and QEMU tries to process 
> them with GDB stub. Modifying gdb_set_stop_cpu should help:
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index e4a1a79..e8ef546 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -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

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Paolo Bonzini]

On 12/01/2015 09:30, Jan Kiszka wrote:
> 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:

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.

diff --git a/cpu-exec.c b/cpu-exec.c
index a4f0eff..56139ac 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -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
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 2098f1c..c1d6c20 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -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);
diff --git a/qom/cpu.c b/qom/cpu.c
index 9c68fa4..e86fec5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -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;
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 2bed914..40b7f79 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -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.
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 4f1ddf7..a313424 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -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 {
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index 7a41f29..088d3fa 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -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)
diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
index d84d259..52f50a2 100644
--- a/target-xtensa/helper.c
+++ b/target-xtensa/helper.c
@@ -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)

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 10322 bytes --]

On 2015-01-12 09:55, Paolo Bonzini wrote:
> On 12/01/2015 09:30, Jan Kiszka wrote:
>> 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:
> 
> 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.
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index a4f0eff..56139ac 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -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) {

This condition is always true for both 0 and EXCP_DEBUG.

> +                        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
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 2098f1c..c1d6c20 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -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);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 9c68fa4..e86fec5 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -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;
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 2bed914..40b7f79 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -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.
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 4f1ddf7..a313424 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -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 {
> diff --git a/target-lm32/helper.c b/target-lm32/helper.c
> index 7a41f29..088d3fa 100644
> --- a/target-lm32/helper.c
> +++ b/target-lm32/helper.c
> @@ -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)
> diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
> index d84d259..52f50a2 100644
> --- a/target-xtensa/helper.c
> +++ b/target-xtensa/helper.c
> @@ -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)
> 
> 

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:

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 305ce50..57b607d 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH] i386: fix breakpoints handling in icount mode

[Paolo Bonzini]

On 24/05/2015 16:43, Jan Kiszka wrote:
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 305ce50..57b607d 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -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.

So when a breakpoint is removed at address X, you have to also remove 
translation blocks that end exactly at X?  That is:

diff --git a/translate-all.c b/translate-all.c
index 536008f..6e50b8f 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -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