Re: [PATCH] softmmu: fix watchpoint processing in icount mode
On 10.09.2021 17:41, Richard Henderson wrote:
On 9/10/21 3:46 PM, David Hildenbrand wrote:
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
---
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?
Whoops, yes. I must have been thinking of a different function.
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
Date: Tue Nov 18 20:24:06 2008 +
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
Signed-off-by: Anthony Liguori
*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.
When the watchpoint needs to trigger after the insn, we do indeed want
to execute a single insn, which we do with the cflags there in the patch
context. But when we want to stop before the insn, we're already done
-- so what was the invalidate supposed to achieve?
Right, this really looks strange.
Do you think that this function also has to be rewritten?
Or this should be done with another patch?
(Then of course there's the problem that Phillipe filed (#245) in which
we set cflags as per above, then take an interrupt before using it, then
wind up with garbage. Ho hum.)
r~
r~
Re: [PATCH] softmmu: fix watchpoint processing in icount mode
On 9/10/21 3:46 PM, David Hildenbrand wrote:
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
---
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?
Whoops, yes. I must have been thinking of a different function.
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
Date: Tue Nov 18 20:24:06 2008 +
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
Signed-off-by: Anthony Liguori
*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.
When the watchpoint needs to trigger after the insn, we do indeed want to execute a single
insn, which we do with the cflags there in the patch context. But when we want to stop
before the insn, we're already done -- so what was the invalidate supposed to achieve?
(Then of course there's the problem that Phillipe filed (#245) in which we set cflags as
per above, then take an interrupt before using it, then wind up with garbage. Ho hum.)
r~
r~
Re: [PATCH] softmmu: fix watchpoint processing in icount mode
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
---
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
Date: Tue Nov 18 20:24:06 2008 +
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
Signed-off-by: Anthony Liguori
*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
Re: [PATCH] softmmu: fix watchpoint processing in icount mode
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
---
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).
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.
r~
Re: [PATCH] softmmu: fix watchpoint processing in icount mode
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
---
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
--
Thanks,
David / dhildenb
[PATCH] softmmu: fix watchpoint processing in icount mode
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
---
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);
}
}
