[PATCH 0/4] Some watchpoint-related patches

[PATCH 0/4] Some watchpoint-related patches

[Pavel Dovgalyuk]

The series includes several watchpoint-related patches.

---

Pavel Dovgalyuk (4):
      softmmu: fix watchpoint processing in icount mode
      softmmu: remove useless condition in watchpoint check
      softmmu: fix for "after access" watchpoints
      icount: preserve cflags when custom tb is about to execute


 accel/tcg/cpu-exec.c | 10 ++++++++++
 softmmu/physmem.c    | 41 ++++++++++++++++++++---------------------
 2 files changed, 30 insertions(+), 21 deletions(-)

--
Pavel Dovgalyuk

[PATCH 1/4] softmmu: fix watchpoint processing in icount mode

[Pavel Dovgalyuk]

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>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 softmmu/physmem.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index f67ad29981..fd1b3b2088 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -938,18 +938,16 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
                 cpu->watchpoint_hit = wp;
 
                 mmap_lock();
+                /* This call also restores vCPU state */
                 tb_check_watchpoint(cpu, ra);
                 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);
                 }
             }

[PATCH 2/4] softmmu: remove useless condition in watchpoint check

[Pavel Dovgalyuk]

cpu_check_watchpoint function checks cpu->watchpoint_hit at the entry.
But then it also does the same in the middle of the function,
while this field can't change.
That is why this patch removes this useless condition.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
---
 softmmu/physmem.c |   41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index fd1b3b2088..94eda44459 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -929,27 +929,26 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
             }
             wp->hitaddr = MAX(addr, wp->vaddr);
             wp->hitattrs = attrs;
-            if (!cpu->watchpoint_hit) {
-                if (wp->flags & BP_CPU && cc->tcg_ops->debug_check_watchpoint &&
-                    !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
-                    wp->flags &= ~BP_WATCHPOINT_HIT;
-                    continue;
-                }
-                cpu->watchpoint_hit = wp;
-
-                mmap_lock();
-                /* This call also restores vCPU state */
-                tb_check_watchpoint(cpu, ra);
-                if (wp->flags & BP_STOP_BEFORE_ACCESS) {
-                    cpu->exception_index = EXCP_DEBUG;
-                    mmap_unlock();
-                    cpu_loop_exit(cpu);
-                } else {
-                    /* Force execution of one insn next time.  */
-                    cpu->cflags_next_tb = 1 | curr_cflags(cpu);
-                    mmap_unlock();
-                    cpu_loop_exit_noexc(cpu);
-                }
+
+            if (wp->flags & BP_CPU && cc->tcg_ops->debug_check_watchpoint &&
+                !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
+                wp->flags &= ~BP_WATCHPOINT_HIT;
+                continue;
+            }
+            cpu->watchpoint_hit = wp;
+
+            mmap_lock();
+            /* This call also restores vCPU state */
+            tb_check_watchpoint(cpu, ra);
+            if (wp->flags & BP_STOP_BEFORE_ACCESS) {
+                cpu->exception_index = EXCP_DEBUG;
+                mmap_unlock();
+                cpu_loop_exit(cpu);
+            } else {
+                /* Force execution of one insn next time.  */
+                cpu->cflags_next_tb = 1 | curr_cflags(cpu);
+                mmap_unlock();
+                cpu_loop_exit_noexc(cpu);
             }
         } else {
             wp->flags &= ~BP_WATCHPOINT_HIT;

[PATCH 3/4] softmmu: fix for "after access" watchpoints

[Pavel Dovgalyuk]

Watchpoints that should fire after the memory access
break an execution of the current block, try to
translate current instruction into the separate block,
which then causes debug interrupt.
But cpu_interrupt can't be called in such block when
icount is enabled, because interrupts muse be allowed
explicitly.
This patch sets CF_LAST_IO flag for retranslated block,
allowing interrupt request for the last instruction.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
---
 softmmu/physmem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 94eda44459..482d80708f 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -946,7 +946,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
                 cpu_loop_exit(cpu);
             } else {
                 /* Force execution of one insn next time.  */
-                cpu->cflags_next_tb = 1 | curr_cflags(cpu);
+                cpu->cflags_next_tb = 1 | CF_LAST_IO | curr_cflags(cpu);
                 mmap_unlock();
                 cpu_loop_exit_noexc(cpu);
             }

[PATCH 4/4] icount: preserve cflags when custom tb is about to execute

[Pavel Dovgalyuk]

When debugging with the watchpoints, qemu may need to create
TB with single instruction. This is achieved by setting cpu->cflags_next_tb.
But when this block is about to execute, it may be interrupted by another
thread. In this case cflags will be lost and next executed TB will not
be the special one.
This patch checks TB exit reason and restores cflags_next_tb to allow
finding the interrupted block.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
---
 accel/tcg/cpu-exec.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index c9764c1325..af1c6e6ba3 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -842,6 +842,16 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
          * cpu_handle_interrupt.  cpu_handle_interrupt will also
          * clear cpu->icount_decr.u16.high.
          */
+        if (cpu->cflags_next_tb == -1
+            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
+                || cpu_neg(cpu)->icount_decr.u16.low >= tb->icount)) {
+            /*
+             * icount is disabled or there are enough instructions
+             * in the budget, do not retranslate this block with
+             * different parameters.
+             */
+            cpu->cflags_next_tb = tb->cflags;
+        }
         return;
     }
 

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

[Richard Henderson]

On 10/28/21 4:48 AM, 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>
> Reviewed-by: David Hildenbrand<david@redhat.com>
> ---
>   softmmu/physmem.c |    6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/4] softmmu: remove useless condition in watchpoint check

[Richard Henderson]

On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
> cpu_check_watchpoint function checks cpu->watchpoint_hit at the entry.
> But then it also does the same in the middle of the function,
> while this field can't change.
> That is why this patch removes this useless condition.
> 
> Signed-off-by: Pavel Dovgalyuk<Pavel.Dovgalyuk@ispras.ru>
> ---
>   softmmu/physmem.c |   41 ++++++++++++++++++++---------------------
>   1 file changed, 20 insertions(+), 21 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/4] softmmu: fix for "after access" watchpoints

[Richard Henderson]

On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
> Watchpoints that should fire after the memory access
> break an execution of the current block, try to
> translate current instruction into the separate block,
> which then causes debug interrupt.
> But cpu_interrupt can't be called in such block when
> icount is enabled, because interrupts muse be allowed
> explicitly.
> This patch sets CF_LAST_IO flag for retranslated block,
> allowing interrupt request for the last instruction.
> 
> Signed-off-by: Pavel Dovgalyuk<Pavel.Dovgalyuk@ispras.ru>
> ---
>   softmmu/physmem.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Indeed, the other such assignment, about 30 lines up, already does this.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 4/4] icount: preserve cflags when custom tb is about to execute

[Richard Henderson]

On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
> +        if (cpu->cflags_next_tb == -1
> +            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->icount)) {
> +            /*
> +             * icount is disabled or there are enough instructions
> +             * in the budget, do not retranslate this block with
> +             * different parameters.
> +             */
> +            cpu->cflags_next_tb = tb->cflags;
> +        }

I can't see that this will work.

We've been asked to exit to the main loop; probably for an interrupt.  The next thing 
that's likely to happen is that cpu_cc->do_interrupt will adjust cpu state to continue in 
the guest interrupt handler.  The cflags_next_tb flag that you're setting up is not 
relevant to that context.

This seems related to Phil's reported problem

   https://gitlab.com/qemu-project/qemu/-/issues/245

in which an interrupt arrives before we finish processing the watchpoint.

I *think* we need to make cflags_next_tb != -1 be treated like any other interrupt disable 
bit, and delay delivery of the interrupt.  Which also means that we should not check for 
exit_tb at the beginning of any TB we generate for the watchpoint step.

I simply haven't taken the time to investigate this properly.


r~

Re: [PATCH 0/4] Some watchpoint-related patches

[Richard Henderson]

On 10/28/21 4:47 AM, Pavel Dovgalyuk wrote:
> 
> Pavel Dovgalyuk (4):
>        softmmu: fix watchpoint processing in icount mode
>        softmmu: remove useless condition in watchpoint check
>        softmmu: fix for "after access" watchpoints
>        icount: preserve cflags when custom tb is about to execute

I'm going to queue the first 3 patches to tcg-next.


r~

Re: [PATCH 4/4] icount: preserve cflags when custom tb is about to execute

[Pavel Dovgalyuk]

On 28.10.2021 22:26, Richard Henderson wrote:
> On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
>> +        if (cpu->cflags_next_tb == -1
>> +            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
>> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->icount)) {
>> +            /*
>> +             * icount is disabled or there are enough instructions
>> +             * in the budget, do not retranslate this block with
>> +             * different parameters.
>> +             */
>> +            cpu->cflags_next_tb = tb->cflags;
>> +        }
> 
> I can't see that this will work.

The issue was the following:
- icount is enabled
- watchpoint hit
- next block should contain a single instruction to stop the execution 
after memory access
- tb with one instruction is translated, cflags_next_tb is reset
- main loop breaks the execution for some reason
- after resuming the execution, we are trying to find new tb without any 
filter in cflags
- tb with multiple instructions is executed (instead of previously 
generated)

> 
> We've been asked to exit to the main loop; probably for an interrupt.  
> The next thing that's likely to happen is that cpu_cc->do_interrupt will 
> adjust cpu state to continue in the guest interrupt handler.  The 
> cflags_next_tb flag that you're setting up is not relevant to that context.
> 
> This seems related to Phil's reported problem
> 
>    https://gitlab.com/qemu-project/qemu/-/issues/245
> 
> in which an interrupt arrives before we finish processing the watchpoint.

Right, this is similar.

> 
> I *think* we need to make cflags_next_tb != -1 be treated like any other 
> interrupt disable bit, and delay delivery of the interrupt.  Which also 
> means that we should not check for exit_tb at the beginning of any TB we 
> generate for the watchpoint step.
> 
> I simply haven't taken the time to investigate this properly.
> 
> 
> r~

Re: [PATCH 4/4] icount: preserve cflags when custom tb is about to execute

[Pavel Dovgalyuk]

On 28.10.2021 22:26, Richard Henderson wrote:
> On 10/28/21 4:48 AM, Pavel Dovgalyuk wrote:
>> +        if (cpu->cflags_next_tb == -1
>> +            && (!use_icount || !(tb->cflags & CF_USE_ICOUNT)
>> +                || cpu_neg(cpu)->icount_decr.u16.low >= tb->icount)) {
>> +            /*
>> +             * icount is disabled or there are enough instructions
>> +             * in the budget, do not retranslate this block with
>> +             * different parameters.
>> +             */
>> +            cpu->cflags_next_tb = tb->cflags;
>> +        }
> 
> I can't see that this will work.
> 
> We've been asked to exit to the main loop; probably for an interrupt.  
> The next thing that's likely to happen is that cpu_cc->do_interrupt will 
> adjust cpu state to continue in the guest interrupt handler.  The 
> cflags_next_tb flag that you're setting up is not relevant to that context.
> 
> This seems related to Phil's reported problem
> 
>    https://gitlab.com/qemu-project/qemu/-/issues/245
> 
> in which an interrupt arrives before we finish processing the watchpoint.
> 
> I *think* we need to make cflags_next_tb != -1 be treated like any other 
> interrupt disable bit, and delay delivery of the interrupt.  Which also 
> means that we should not check for exit_tb at the beginning of any TB we 
> generate for the watchpoint step.
> 
> I simply haven't taken the time to investigate this properly.

Please check the new series, it probably solves the problem described in 
that issue.

Pavel Dovgalyuk