| Message ID | 1609963815-417989-1-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V1] gdbstub: suspended state support | expand |
Steve Sistare <steven.sistare@oracle.com> writes: > Modify the gdb server so a continue command appears to resume execution > when in RUN_STATE_SUSPENDED. Do not print the next gdb prompt, but do not > actually resume instruction fetch. While in this "fake" running mode, a > ctrl-C returns the user to the gdb prompt. What exactly is the purpose of this? To hide the details of the runstate as controlled by the user? I wouldn't expect someone using gdb debugging not to also have control of the HMP/QMP interface. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > gdbstub.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index f3a318c..2f0d9ff 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -461,7 +461,9 @@ static inline void gdb_continue(void) > #else > if (!runstate_needs_reset()) { > trace_gdbstub_op_continue(); > - vm_start(); > + if (!runstate_check(RUN_STATE_SUSPENDED)) { > + vm_start(); > + } > } > #endif > } > @@ -490,7 +492,7 @@ static int gdb_continue_partial(char *newstates) > int flag = 0; > > if (!runstate_needs_reset()) { > - if (vm_prepare_start()) { > + if (!runstate_check(RUN_STATE_SUSPENDED) && vm_prepare_start()) { > return 0; > } > > @@ -2835,6 +2837,9 @@ static void gdb_read_byte(uint8_t ch) > /* when the CPU is running, we cannot do anything except stop > it when receiving a char */ > vm_stop(RUN_STATE_PAUSED); > + } else if (runstate_check(RUN_STATE_SUSPENDED) && ch == 3) { > + /* Received ctrl-c from gdb */ > + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); > } else > #endif > { > @@ -3282,6 +3287,8 @@ static void gdb_sigterm_handler(int signal) > { > if (runstate_is_running()) { > vm_stop(RUN_STATE_PAUSED); > + } else if (runstate_check(RUN_STATE_SUSPENDED)) { > + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); > } > } > #endif
On 1/7/2021 7:40 AM, Alex Bennée wrote: > Steve Sistare <steven.sistare@oracle.com> writes: > >> Modify the gdb server so a continue command appears to resume execution >> when in RUN_STATE_SUSPENDED. Do not print the next gdb prompt, but do not >> actually resume instruction fetch. While in this "fake" running mode, a >> ctrl-C returns the user to the gdb prompt. > > What exactly is the purpose of this? To hide the details of the runstate > as controlled by the user? I wouldn't expect someone using gdb debugging > not to also have control of the HMP/QMP interface. Without this fix, a user that attaches gdb to a suspended guest breaks the guest. The state is RUN_STATE_SUSPENDED. After attaching gdb and typing continue or quit, qemu transitions to RUN_STATE_RUNNING (wrong) and the guest continues execution (wrong). The guest loops polling on an acpi port, deep in a call stack under acpi_suspend_enter(). Sending a system_wakeup request via qmp or hmp fails with the message "Error: Unable to wake up: guest is not in suspended state". With the fix, the state remains RUN_STATE_SUSPENDED throughout, until the system_wakeup request, and the guest pc does not change. gdb interprets RUN_STATE_SUSPENDED as "target is running", without causing instruction fetch to resume. If you are satisfied, I will add this explanation to the commit message. - Steve >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> gdbstub.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index f3a318c..2f0d9ff 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -461,7 +461,9 @@ static inline void gdb_continue(void) >> #else >> if (!runstate_needs_reset()) { >> trace_gdbstub_op_continue(); >> - vm_start(); >> + if (!runstate_check(RUN_STATE_SUSPENDED)) { >> + vm_start(); >> + } >> } >> #endif >> } >> @@ -490,7 +492,7 @@ static int gdb_continue_partial(char *newstates) >> int flag = 0; >> >> if (!runstate_needs_reset()) { >> - if (vm_prepare_start()) { >> + if (!runstate_check(RUN_STATE_SUSPENDED) && vm_prepare_start()) { >> return 0; >> } >> >> @@ -2835,6 +2837,9 @@ static void gdb_read_byte(uint8_t ch) >> /* when the CPU is running, we cannot do anything except stop >> it when receiving a char */ >> vm_stop(RUN_STATE_PAUSED); >> + } else if (runstate_check(RUN_STATE_SUSPENDED) && ch == 3) { >> + /* Received ctrl-c from gdb */ >> + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); >> } else >> #endif >> { >> @@ -3282,6 +3287,8 @@ static void gdb_sigterm_handler(int signal) >> { >> if (runstate_is_running()) { >> vm_stop(RUN_STATE_PAUSED); >> + } else if (runstate_check(RUN_STATE_SUSPENDED)) { >> + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); >> } >> } >> #endif > >
Steven Sistare <steven.sistare@oracle.com> writes: > On 1/7/2021 7:40 AM, Alex Bennée wrote: >> Steve Sistare <steven.sistare@oracle.com> writes: >> >>> Modify the gdb server so a continue command appears to resume execution >>> when in RUN_STATE_SUSPENDED. Do not print the next gdb prompt, but do not >>> actually resume instruction fetch. While in this "fake" running mode, a >>> ctrl-C returns the user to the gdb prompt. >> >> What exactly is the purpose of this? To hide the details of the runstate >> as controlled by the user? I wouldn't expect someone using gdb debugging >> not to also have control of the HMP/QMP interface. > > Without this fix, a user that attaches gdb to a suspended guest breaks the > guest. The state is RUN_STATE_SUSPENDED. After attaching gdb and typing > continue or quit, qemu transitions to RUN_STATE_RUNNING (wrong) and the > guest continues execution (wrong). The guest loops polling on an acpi port, > deep in a call stack under acpi_suspend_enter(). Sending a system_wakeup > request via qmp or hmp fails with the message "Error: Unable to wake up: > guest is not in suspended state". > > With the fix, the state remains RUN_STATE_SUSPENDED throughout, until the > system_wakeup request, and the guest pc does not change. gdb interprets > RUN_STATE_SUSPENDED as "target is running", without causing instruction > fetch to resume. > > If you are satisfied, I will add this explanation to the commit > message. I'm satisfied with the explanation going in the commit message. However I'm not convinced the implementation of pretending we worked. I think if we are not going to start we should probably reply with an O packet explaining why we don't start followed by a S (stop) packet so GDB doesn't get confused. I would also be happier if we could add a test case that works through all the potential state transitions so we don't have test manually (i.e. not). I think we need at least: - -S -> stop -> continue -> start -> Ctrl-C - -S -> continue -> stop -> start -> Ctrl-C - Ctrl-C -> stop -> continue -> start - stop -> Ctrl-C -> start -> continue I suspect it would need to use the acceptance tests given you'll want to change two control points. The reverse debugging tests already do something similar (see tests/acceptance/reverse_debugging.py). > > - Steve > >>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>> --- >>> gdbstub.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/gdbstub.c b/gdbstub.c >>> index f3a318c..2f0d9ff 100644 >>> --- a/gdbstub.c >>> +++ b/gdbstub.c >>> @@ -461,7 +461,9 @@ static inline void gdb_continue(void) >>> #else >>> if (!runstate_needs_reset()) { >>> trace_gdbstub_op_continue(); >>> - vm_start(); >>> + if (!runstate_check(RUN_STATE_SUSPENDED)) { >>> + vm_start(); >>> + } >>> } >>> #endif >>> } >>> @@ -490,7 +492,7 @@ static int gdb_continue_partial(char *newstates) >>> int flag = 0; >>> >>> if (!runstate_needs_reset()) { >>> - if (vm_prepare_start()) { >>> + if (!runstate_check(RUN_STATE_SUSPENDED) && vm_prepare_start()) { >>> return 0; >>> } >>> >>> @@ -2835,6 +2837,9 @@ static void gdb_read_byte(uint8_t ch) >>> /* when the CPU is running, we cannot do anything except stop >>> it when receiving a char */ >>> vm_stop(RUN_STATE_PAUSED); >>> + } else if (runstate_check(RUN_STATE_SUSPENDED) && ch == 3) { >>> + /* Received ctrl-c from gdb */ >>> + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); >>> } else >>> #endif >>> { >>> @@ -3282,6 +3287,8 @@ static void gdb_sigterm_handler(int signal) >>> { >>> if (runstate_is_running()) { >>> vm_stop(RUN_STATE_PAUSED); >>> + } else if (runstate_check(RUN_STATE_SUSPENDED)) { >>> + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); >>> } >>> } >>> #endif >> >>
diff --git a/gdbstub.c b/gdbstub.c index f3a318c..2f0d9ff 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -461,7 +461,9 @@ static inline void gdb_continue(void) #else if (!runstate_needs_reset()) { trace_gdbstub_op_continue(); - vm_start(); + if (!runstate_check(RUN_STATE_SUSPENDED)) { + vm_start(); + } } #endif } @@ -490,7 +492,7 @@ static int gdb_continue_partial(char *newstates) int flag = 0; if (!runstate_needs_reset()) { - if (vm_prepare_start()) { + if (!runstate_check(RUN_STATE_SUSPENDED) && vm_prepare_start()) { return 0; } @@ -2835,6 +2837,9 @@ static void gdb_read_byte(uint8_t ch) /* when the CPU is running, we cannot do anything except stop it when receiving a char */ vm_stop(RUN_STATE_PAUSED); + } else if (runstate_check(RUN_STATE_SUSPENDED) && ch == 3) { + /* Received ctrl-c from gdb */ + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); } else #endif { @@ -3282,6 +3287,8 @@ static void gdb_sigterm_handler(int signal) { if (runstate_is_running()) { vm_stop(RUN_STATE_PAUSED); + } else if (runstate_check(RUN_STATE_SUSPENDED)) { + gdb_vm_state_change(0, 0, RUN_STATE_PAUSED); } } #endif
Modify the gdb server so a continue command appears to resume execution when in RUN_STATE_SUSPENDED. Do not print the next gdb prompt, but do not actually resume instruction fetch. While in this "fake" running mode, a ctrl-C returns the user to the gdb prompt. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- gdbstub.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)