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

[David Hildenbrand]

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.

Reviewed-by: David Hildenbrand <david@redhat.com>

--
Thanks,

David / dhildenb