Re: [PATCH] softmmu: fix watchpoint processing in icount mode

[David Hildenbrand]

On 10.09.21 15:34, Richard Henderson wrote:
> On 9/10/21 1:15 PM, David Hildenbrand wrote:
> > On 07.09.21 13:30, Pavel Dovgalyuk wrote:
> > > Watchpoint processing code restores vCPU state twice:
> > > in tb_check_watchpoint and in cpu_loop_exit_restore/cpu_restore_state.
> > > Normally it does not affect anything, but in icount mode instruction
> > > counter is incremented twice and becomes incorrect.
> > > This patch eliminates unneeded CPU state restore.
> > >
> > > Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> > > ---
> > >    softmmu/physmem.c |    5 +----
> > >    1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > > index 23e77cb771..4025dfab11 100644
> > > --- a/softmmu/physmem.c
> > > +++ b/softmmu/physmem.c
> > > @@ -941,14 +941,11 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, 
> > > vaddr len,
> > >                    if (wp->flags & BP_STOP_BEFORE_ACCESS) {
> > >                        cpu->exception_index = EXCP_DEBUG;
> > >                        mmap_unlock();
> > > -                    cpu_loop_exit_restore(cpu, ra);
> > > +                    cpu_loop_exit(cpu);
> > >                    } else {
> > >                        /* Force execution of one insn next time.  */
> > >                        cpu->cflags_next_tb = 1 | curr_cflags(cpu);
> > >                        mmap_unlock();
> > > -                    if (ra) {
> > > -                        cpu_restore_state(cpu, ra, true);
> > > -                    }
> > >                        cpu_loop_exit_noexc(cpu);
> > >                    }
> > >                }
> >
> > I'm not an expert on that code, but it looks good to me.
> >
> > Maybe we could have added a comment above the tb_check_watchpoint() call to 
> > highlight that
> > the restore will happen in there.
>
> Hmm.  Curious.
>
> Looking at tb_check_watchpoint, I have trouble seeing how it could be correct.
> Watchpoints can happen at any memory reference within the TB.  We should be 
> rolling back
> to the cpu state at the memory reference (cpu_retore_state) and not the cpu 
> state at the
> start of the TB (cpu_restore_state_from_tb).

cpu_restore_state() ends up calling cpu_restore_state_from_tb() with essentially
the same parameters or what am I missing?

So AFAIU this patch shouldn't change the situation -- but valid point that the
current behavior might be bogus.

> I'm also not sure why we're invalidating tb's.  Why does watchpoint hit imply 
> that we
> should want to ditch the TB?  If we want different behaviour from the next 
> execution, we
> should be adjusting cflags.

It goes back to

commit 06d55cc19ac84e799d2df8c750049e51798b00a4
Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Tue Nov 18 20:24:06 2008 +0000

    Restore pc on watchpoint hits (Jan Kiszka)
    
    In order to provide accurate information about the triggering
    instruction, this patch adds the required bits to restore the pc if the
    access happened inside a TB. With the BP_STOP_BEFORE_ACCESS flag, the
    watchpoint user can control if the debug trap should be issued on or
    after the accessing instruction.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>


*trying to rememebr what we do on watchpoints* I think we want to
make sure that we end up with a single-instruction TB, right? So we
want to make sure to remove the old one.


--
Thanks,

David / dhildenb