| Message ID | 163542170287.2127597.18369415404458239885.stgit@pasha-ThinkPad-X280 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some watchpoint-related patches | expand |
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~
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~
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
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; }
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(+)