[Qemu-devel] [PATCH] gdbstub: shutdown guest when the target is killed

[Qemu-devel] [PATCH] gdbstub: shutdown guest when the target is killed

[KONRAD Frederic]

Under MinGW when the target is killed no "W00" packet are received by GDB
because gdbstub takes the "exit(0)" path. So replace the "exit(0)" call by
a normal guest shutdown so the "W00" packet has a chance to be sent in
"gdb_cleanup".

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 gdbstub.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index bfc7afb..c91a909 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1389,7 +1389,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
     case 'k':
         /* Kill the target */
         error_report("QEMU: Terminated via GDBstub");
+#ifdef CONFIG_USER_ONLY
         exit(0);
+#else
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+#endif
+        break;
     case 'D':
         /* Detach packet */
         pid = 1;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] gdbstub: shutdown guest when the target is killed

[Peter Maydell]

On Thu, 24 Jan 2019 at 12:47, KONRAD Frederic
<frederic.konrad@adacore.com> wrote:
>
> Under MinGW when the target is killed no "W00" packet are received by GDB
> because gdbstub takes the "exit(0)" path.

Why is this a problem? The gdb protocol documentation for the 'k'
packet says:

# If the target system immediately closes the connection in response to 'k',
# GDB does not consider the lack of packet acknowledgment to be an error,
# and assumes the kill was successful.

So it shouldn't matter whether we send the W00 packet or not.
(In fact it's not clear to me that it's right to send the W00
packet unconditionally -- W00 is a "stop reply packet" so it's
only correct to send it on QEMU exit if the stub was currently
executing one of the continue/run commands, not if the guest CPU
was already stopped.)

This also isn't specific to mingw as far as I can see -- we behave
the same on all hosts for system emulation.

> So replace the "exit(0)" call by
> a normal guest shutdown so the "W00" packet has a chance to be sent in
> "gdb_cleanup".

On the other hand I think that triggering a clean shutdown in
system emulation mode is arguably a more reasonable choice than
just bailing out immediately. I'm just not clear on what the
problem is that you're trying to solve with the change...

> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  gdbstub.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index bfc7afb..c91a909 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1389,7 +1389,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>      case 'k':
>          /* Kill the target */
>          error_report("QEMU: Terminated via GDBstub");

I think we could usefully expand the comment.
    /*
     * Kill the target. The protocol does not specify the exact effect
     * of this packet, but notes that usually for a single-process target
     * it will kill the process, and for a bare-metal target it may
     * power-cycle or reset the system, so we follow that suggestion.
     * Note that this packet has no reply.
     */

> +#ifdef CONFIG_USER_ONLY
>          exit(0);
> +#else
> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +#endif
> +        break;
>      case 'D':
>          /* Detach packet */
>          pid = 1;

thanks
-- PMM

Re: [Qemu-devel] [PATCH] gdbstub: shutdown guest when the target is killed

[KONRAD Frederic]

Le 1/24/19 à 2:04 PM, Peter Maydell a écrit :
> On Thu, 24 Jan 2019 at 12:47, KONRAD Frederic
> <frederic.konrad@adacore.com> wrote:
>>
>> Under MinGW when the target is killed no "W00" packet are received by GDB
>> because gdbstub takes the "exit(0)" path.
> 
> Why is this a problem? The gdb protocol documentation for the 'k'
> packet says:
> 
> # If the target system immediately closes the connection in response to 'k',
> # GDB does not consider the lack of packet acknowledgment to be an error,
> # and assumes the kill was successful.
> 
> So it shouldn't matter whether we send the W00 packet or not.
> (In fact it's not clear to me that it's right to send the W00
> packet unconditionally -- W00 is a "stop reply packet" so it's
> only correct to send it on QEMU exit if the stub was currently
> executing one of the continue/run commands, not if the guest CPU
> was already stopped.)

Ah yes good point.. I didn't found this doc when I wrote this patch.. So maybe
there is something odd with my GDB...

> 
> This also isn't specific to mingw as far as I can see -- we behave
> the same on all hosts for system emulation.

Yes I wondered the same.. I'm not sure how the CharBackend behave when calling
exit(..) under Windows though and I think I already had issue with some buffer
not beeing flushed correctly.

> 
>> So replace the "exit(0)" call by
>> a normal guest shutdown so the "W00" packet has a chance to be sent in
>> "gdb_cleanup".
> 
> On the other hand I think that triggering a clean shutdown in
> system emulation mode is arguably a more reasonable choice than
> just bailing out immediately. I'm just not clear on what the
> problem is that you're trying to solve with the change...

True.. I can just make this commit has a cleanup commit if you like..

Thanks,
Fred

> 
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>> ---
>>   gdbstub.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index bfc7afb..c91a909 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1389,7 +1389,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>>       case 'k':
>>           /* Kill the target */
>>           error_report("QEMU: Terminated via GDBstub");
> 
> I think we could usefully expand the comment.
>      /*
>       * Kill the target. The protocol does not specify the exact effect
>       * of this packet, but notes that usually for a single-process target
>       * it will kill the process, and for a bare-metal target it may
>       * power-cycle or reset the system, so we follow that suggestion.
>       * Note that this packet has no reply.
>       */
> 
>> +#ifdef CONFIG_USER_ONLY
>>           exit(0);
>> +#else
>> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +#endif
>> +        break;
>>       case 'D':
>>           /* Detach packet */
>>           pid = 1;
> 
> thanks
> -- PMM
>