diff mbox series

[v2,2/3] softmmu: fix watchpoint-interrupt races

Message ID 163662451431.125458.14945698834107669531.stgit@pasha-ThinkPad-X280 (mailing list archive)
State New, archived
Headers show
Series Some watchpoint-related patches | expand

Commit Message

Pavel Dovgalyuk Nov. 11, 2021, 9:55 a.m. UTC
Watchpoint may be processed in two phases. First one is detecting
the instruction with target memory access. And the second one is
executing only one instruction and setting the debug interrupt flag.
Hardware interrupts can break this sequence when they happen after
the first watchpoint phase.
This patch postpones the interrupt request until watchpoint is
processed.

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

Comments

Alex Bennée Nov. 11, 2021, 1:15 p.m. UTC | #1
Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> writes:

> Watchpoint may be processed in two phases. First one is detecting
> the instruction with target memory access. And the second one is
> executing only one instruction and setting the debug interrupt flag.
> Hardware interrupts can break this sequence when they happen after
> the first watchpoint phase.
> This patch postpones the interrupt request until watchpoint is
> processed.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
>  accel/tcg/cpu-exec.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index df12452b8f..e4526c2f5e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>              qemu_mutex_unlock_iothread();
>              return true;
>          }
> +        /* Process watchpoints first, or interrupts will ruin everything */
> +        if (cpu->watchpoint_hit) {
> +            qemu_mutex_unlock_iothread();
> +            return false;
> +        }

side note: I wonder if it is time to wrap up the locks up with
QEMU_LOCK_GUARD or something similar?

Anyway seems reasonable:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
David Hildenbrand Nov. 12, 2021, 10:10 a.m. UTC | #2
On 11.11.21 10:55, Pavel Dovgalyuk wrote:
> Watchpoint may be processed in two phases. First one is detecting
> the instruction with target memory access. And the second one is
> executing only one instruction and setting the debug interrupt flag.
> Hardware interrupts can break this sequence when they happen after
> the first watchpoint phase.
> This patch postpones the interrupt request until watchpoint is
> processed.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
> ---
>  accel/tcg/cpu-exec.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index df12452b8f..e4526c2f5e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
>              qemu_mutex_unlock_iothread();
>              return true;
>          }
> +        /* Process watchpoints first, or interrupts will ruin everything */
> +        if (cpu->watchpoint_hit) {
> +            qemu_mutex_unlock_iothread();
> +            return false;
> +        }
>  #if !defined(CONFIG_USER_ONLY)
>          if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
>              /* Do nothing */
> 

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

Patch

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index df12452b8f..e4526c2f5e 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -742,6 +742,11 @@  static inline bool cpu_handle_interrupt(CPUState *cpu,
             qemu_mutex_unlock_iothread();
             return true;
         }
+        /* Process watchpoints first, or interrupts will ruin everything */
+        if (cpu->watchpoint_hit) {
+            qemu_mutex_unlock_iothread();
+            return false;
+        }
 #if !defined(CONFIG_USER_ONLY)
         if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
             /* Do nothing */