[PATCH v3 0/5] Risc-V/gdb: replace exit calls with proper shutdown

[PATCH v3 0/5] Risc-V/gdb: replace exit calls with proper shutdown

[Clément Chigot]

This series replaces some of the call to exit in hardware used by
Risc-V boards or made when gdb is requested to exit by shutdown
requests. Otherwise, the gdb connection can be abruptly disconnected
resulting in the last gdb packet "Wxx" being not sent.

For the gdbstub modification, gdb_exit calls ensure that the "Wxx"
packet is sent before exiting. However, some features (see
net/vhost-vdpa.c: vhost_vdpa_cleanup for example) are expecting 
that a cleanup is being made before exiting. This, it's probably
safer to follow the same logic here as well.

Difference with v2:
 - Add support to request a shutdown with a specific exit code.
 - Pass the exit code of the main loop to gdb_exit call in qemu_cleanup
 - gdbstub: move the request shutdown in a new new function to avoid
   having to worry about the request having already been sent.

Clément Chigot (5):
  softmmu: add means to pass an exit code when requesting a shutdown
  softmmu: pass the main loop status to gdb "Wxx" packet
  hw/misc/sifive_test.c: replace exit calls with proper shutdown
  hw/char: riscv_htif: replace exit calls with proper shutdown
  gdbstub: replace exit calls with proper shutdown for softmmu

 gdbstub/gdbstub.c          |  5 +++--
 gdbstub/softmmu.c          |  6 ++++++
 gdbstub/user.c             |  6 ++++++
 hw/char/riscv_htif.c       |  5 ++++-
 hw/misc/sifive_test.c      |  9 +++++++--
 include/gdbstub/syscalls.h |  9 +++++++++
 include/sysemu/runstate.h  |  2 ++
 include/sysemu/sysemu.h    |  2 +-
 softmmu/main.c             |  2 +-
 softmmu/runstate.c         | 16 +++++++++++++---
 10 files changed, 52 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v3 1/5] softmmu: add means to pass an exit code when requesting a shutdown

[Clément Chigot]

As of now, the exit code was either EXIT_FAILURE when a panic shutdown
was requested or EXIT_SUCCESS otherwise.
However, some hardware could want to pass more complex exit codes. Thus,
introduce a new shutdown request function allowing that.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 include/sysemu/runstate.h |  2 ++
 softmmu/runstate.c        | 12 +++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 7beb29c2e2..1e59e0b12b 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -61,6 +61,8 @@ void qemu_system_wakeup_request(WakeupReason reason, Error **errp);
 void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
 void qemu_register_wakeup_notifier(Notifier *notifier);
 void qemu_register_wakeup_support(void);
+void qemu_system_shutdown_request_with_code(ShutdownCause reason,
+                                            int exit_code);
 void qemu_system_shutdown_request(ShutdownCause reason);
 void qemu_system_powerdown_request(void);
 void qemu_register_powerdown_notifier(Notifier *notifier);
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index f3bd862818..ee27e26048 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -345,6 +345,7 @@ void vm_state_notify(bool running, RunState state)
 
 static ShutdownCause reset_requested;
 static ShutdownCause shutdown_requested;
+static int shutdown_exit_code = EXIT_SUCCESS;
 static int shutdown_signal;
 static pid_t shutdown_pid;
 static int powerdown_requested;
@@ -624,6 +625,13 @@ void qemu_system_killed(int signal, pid_t pid)
     qemu_notify_event();
 }
 
+void qemu_system_shutdown_request_with_code(ShutdownCause reason,
+                                            int exit_code)
+{
+    shutdown_exit_code = exit_code;
+    qemu_system_shutdown_request(reason);
+}
+
 void qemu_system_shutdown_request(ShutdownCause reason)
 {
     trace_qemu_system_shutdown_request(reason);
@@ -685,7 +693,9 @@ static bool main_loop_should_exit(int *status)
         if (shutdown_action == SHUTDOWN_ACTION_PAUSE) {
             vm_stop(RUN_STATE_SHUTDOWN);
         } else {
-            if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
+            if (shutdown_exit_code != EXIT_SUCCESS) {
+                *status = shutdown_exit_code;
+            } else if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
                 panic_action == PANIC_ACTION_EXIT_FAILURE) {
                 *status = EXIT_FAILURE;
             }
-- 
2.25.1

Re: [PATCH v3 1/5] softmmu: add means to pass an exit code when requesting a shutdown

[Alistair Francis]

On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
>
> As of now, the exit code was either EXIT_FAILURE when a panic shutdown
> was requested or EXIT_SUCCESS otherwise.
> However, some hardware could want to pass more complex exit codes. Thus,
> introduce a new shutdown request function allowing that.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/sysemu/runstate.h |  2 ++
>  softmmu/runstate.c        | 12 +++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index 7beb29c2e2..1e59e0b12b 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -61,6 +61,8 @@ void qemu_system_wakeup_request(WakeupReason reason, Error **errp);
>  void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
>  void qemu_register_wakeup_notifier(Notifier *notifier);
>  void qemu_register_wakeup_support(void);
> +void qemu_system_shutdown_request_with_code(ShutdownCause reason,
> +                                            int exit_code);
>  void qemu_system_shutdown_request(ShutdownCause reason);
>  void qemu_system_powerdown_request(void);
>  void qemu_register_powerdown_notifier(Notifier *notifier);
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index f3bd862818..ee27e26048 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -345,6 +345,7 @@ void vm_state_notify(bool running, RunState state)
>
>  static ShutdownCause reset_requested;
>  static ShutdownCause shutdown_requested;
> +static int shutdown_exit_code = EXIT_SUCCESS;
>  static int shutdown_signal;
>  static pid_t shutdown_pid;
>  static int powerdown_requested;
> @@ -624,6 +625,13 @@ void qemu_system_killed(int signal, pid_t pid)
>      qemu_notify_event();
>  }
>
> +void qemu_system_shutdown_request_with_code(ShutdownCause reason,
> +                                            int exit_code)
> +{
> +    shutdown_exit_code = exit_code;
> +    qemu_system_shutdown_request(reason);
> +}
> +
>  void qemu_system_shutdown_request(ShutdownCause reason)
>  {
>      trace_qemu_system_shutdown_request(reason);
> @@ -685,7 +693,9 @@ static bool main_loop_should_exit(int *status)
>          if (shutdown_action == SHUTDOWN_ACTION_PAUSE) {
>              vm_stop(RUN_STATE_SHUTDOWN);
>          } else {
> -            if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
> +            if (shutdown_exit_code != EXIT_SUCCESS) {
> +                *status = shutdown_exit_code;
> +            } else if (request == SHUTDOWN_CAUSE_GUEST_PANIC &&
>                  panic_action == PANIC_ACTION_EXIT_FAILURE) {
>                  *status = EXIT_FAILURE;
>              }
> --
> 2.25.1
>

[PATCH v3 2/5] softmmu: pass the main loop status to gdb "Wxx" packet

[Clément Chigot]

gdb_exit function aims to close gdb sessions and sends the exit code of
the current execution. It's being called by qemu_cleanup once the main
loop is over.
Until now, the exit code sent was always 0. Now that hardware can
shutdown this main loop with custom exit codes, these codes must be
transfered to gdb as well.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 include/sysemu/sysemu.h | 2 +-
 softmmu/main.c          | 2 +-
 softmmu/runstate.c      | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 25be2a692e..73a37949c2 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -101,7 +101,7 @@ bool defaults_enabled(void);
 
 void qemu_init(int argc, char **argv);
 int qemu_main_loop(void);
-void qemu_cleanup(void);
+void qemu_cleanup(int);
 
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
diff --git a/softmmu/main.c b/softmmu/main.c
index 694388bd7f..9b91d21ea8 100644
--- a/softmmu/main.c
+++ b/softmmu/main.c
@@ -35,7 +35,7 @@ int qemu_default_main(void)
     int status;
 
     status = qemu_main_loop();
-    qemu_cleanup();
+    qemu_cleanup(status);
 
     return status;
 }
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index ee27e26048..d4e2e59e45 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -794,9 +794,9 @@ void qemu_init_subsystems(void)
 }
 
 
-void qemu_cleanup(void)
+void qemu_cleanup(int status)
 {
-    gdb_exit(0);
+    gdb_exit(status);
 
     /*
      * cleaning up the migration object cancels any existing migration
-- 
2.25.1

Re: [PATCH v3 2/5] softmmu: pass the main loop status to gdb "Wxx" packet

[Alistair Francis]

On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
>
> gdb_exit function aims to close gdb sessions and sends the exit code of
> the current execution. It's being called by qemu_cleanup once the main
> loop is over.
> Until now, the exit code sent was always 0. Now that hardware can
> shutdown this main loop with custom exit codes, these codes must be
> transfered to gdb as well.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/sysemu/sysemu.h | 2 +-
>  softmmu/main.c          | 2 +-
>  softmmu/runstate.c      | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 25be2a692e..73a37949c2 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -101,7 +101,7 @@ bool defaults_enabled(void);
>
>  void qemu_init(int argc, char **argv);
>  int qemu_main_loop(void);
> -void qemu_cleanup(void);
> +void qemu_cleanup(int);
>
>  extern QemuOptsList qemu_legacy_drive_opts;
>  extern QemuOptsList qemu_common_drive_opts;
> diff --git a/softmmu/main.c b/softmmu/main.c
> index 694388bd7f..9b91d21ea8 100644
> --- a/softmmu/main.c
> +++ b/softmmu/main.c
> @@ -35,7 +35,7 @@ int qemu_default_main(void)
>      int status;
>
>      status = qemu_main_loop();
> -    qemu_cleanup();
> +    qemu_cleanup(status);
>
>      return status;
>  }
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index ee27e26048..d4e2e59e45 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -794,9 +794,9 @@ void qemu_init_subsystems(void)
>  }
>
>
> -void qemu_cleanup(void)
> +void qemu_cleanup(int status)
>  {
> -    gdb_exit(0);
> +    gdb_exit(status);
>
>      /*
>       * cleaning up the migration object cancels any existing migration
> --
> 2.25.1
>

[PATCH v3 3/5] hw/misc/sifive_test.c: replace exit calls with proper shutdown

[Clément Chigot]

This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
before its final packet ("Wxx") is being sent. This part, being done
inside qemu_cleanup function, can be reached only when the main loop
exits after a shutdown request.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 hw/misc/sifive_test.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/misc/sifive_test.c b/hw/misc/sifive_test.c
index 56df45bfe5..ad688079c4 100644
--- a/hw/misc/sifive_test.c
+++ b/hw/misc/sifive_test.c
@@ -25,6 +25,7 @@
 #include "qemu/module.h"
 #include "sysemu/runstate.h"
 #include "hw/misc/sifive_test.h"
+#include "sysemu/sysemu.h"
 
 static uint64_t sifive_test_read(void *opaque, hwaddr addr, unsigned int size)
 {
@@ -39,9 +40,13 @@ static void sifive_test_write(void *opaque, hwaddr addr,
         int code = (val64 >> 16) & 0xffff;
         switch (status) {
         case FINISHER_FAIL:
-            exit(code);
+            qemu_system_shutdown_request_with_code(
+                SHUTDOWN_CAUSE_GUEST_PANIC, code);
+            return;
         case FINISHER_PASS:
-            exit(0);
+            qemu_system_shutdown_request_with_code(
+                SHUTDOWN_CAUSE_GUEST_SHUTDOWN, code);
+            return;
         case FINISHER_RESET:
             qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
             return;
-- 
2.25.1

Re: [PATCH v3 3/5] hw/misc/sifive_test.c: replace exit calls with proper shutdown

[Alistair Francis]

On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
>
> This replaces the exit calls by shutdown requests, ensuring a proper
> cleanup of Qemu. Otherwise, some connections like gdb could be broken
> before its final packet ("Wxx") is being sent. This part, being done
> inside qemu_cleanup function, can be reached only when the main loop
> exits after a shutdown request.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/misc/sifive_test.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/sifive_test.c b/hw/misc/sifive_test.c
> index 56df45bfe5..ad688079c4 100644
> --- a/hw/misc/sifive_test.c
> +++ b/hw/misc/sifive_test.c
> @@ -25,6 +25,7 @@
>  #include "qemu/module.h"
>  #include "sysemu/runstate.h"
>  #include "hw/misc/sifive_test.h"
> +#include "sysemu/sysemu.h"
>
>  static uint64_t sifive_test_read(void *opaque, hwaddr addr, unsigned int size)
>  {
> @@ -39,9 +40,13 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>          int code = (val64 >> 16) & 0xffff;
>          switch (status) {
>          case FINISHER_FAIL:
> -            exit(code);
> +            qemu_system_shutdown_request_with_code(
> +                SHUTDOWN_CAUSE_GUEST_PANIC, code);
> +            return;
>          case FINISHER_PASS:
> -            exit(0);
> +            qemu_system_shutdown_request_with_code(
> +                SHUTDOWN_CAUSE_GUEST_SHUTDOWN, code);
> +            return;
>          case FINISHER_RESET:
>              qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>              return;
> --
> 2.25.1
>

[PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown

[Clément Chigot]

This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Otherwise, some connections like gdb could be broken
before its final packet ("Wxx") is being sent. This part, being done
inside qemu_cleanup function, can be reached only when the main loop
exits after a shutdown request.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 hw/char/riscv_htif.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 37d3ccc76b..7e9b6fcc98 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -31,6 +31,7 @@
 #include "qemu/error-report.h"
 #include "exec/address-spaces.h"
 #include "sysemu/dma.h"
+#include "sysemu/runstate.h"
 
 #define RISCV_DEBUG_HTIF 0
 #define HTIF_DEBUG(fmt, ...)                                                   \
@@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
                     g_free(sig_data);
                 }
 
-                exit(exit_code);
+                qemu_system_shutdown_request_with_code(
+                    SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
+                return;
             } else {
                 uint64_t syscall[8];
                 cpu_physical_memory_read(payload, syscall, sizeof(syscall));
-- 
2.25.1

Re: [PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown

[Alistair Francis]

On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
>
> This replaces the exit calls by shutdown requests, ensuring a proper
> cleanup of Qemu. Otherwise, some connections like gdb could be broken
> before its final packet ("Wxx") is being sent. This part, being done
> inside qemu_cleanup function, can be reached only when the main loop
> exits after a shutdown request.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/char/riscv_htif.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 37d3ccc76b..7e9b6fcc98 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -31,6 +31,7 @@
>  #include "qemu/error-report.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/dma.h"
> +#include "sysemu/runstate.h"
>
>  #define RISCV_DEBUG_HTIF 0
>  #define HTIF_DEBUG(fmt, ...)                                                   \
> @@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>                      g_free(sig_data);
>                  }
>
> -                exit(exit_code);
> +                qemu_system_shutdown_request_with_code(
> +                    SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
> +                return;
>              } else {
>                  uint64_t syscall[8];
>                  cpu_physical_memory_read(payload, syscall, sizeof(syscall));
> --
> 2.25.1
>

Re: [PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown

[Alistair Francis]

On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
>
> This replaces the exit calls by shutdown requests, ensuring a proper
> cleanup of Qemu. Otherwise, some connections like gdb could be broken
> before its final packet ("Wxx") is being sent. This part, being done
> inside qemu_cleanup function, can be reached only when the main loop
> exits after a shutdown request.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Do you mind rebasing this on:
https://github.com/alistair23/qemu/tree/riscv-to-apply.next

Alistair

> ---
>  hw/char/riscv_htif.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 37d3ccc76b..7e9b6fcc98 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -31,6 +31,7 @@
>  #include "qemu/error-report.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/dma.h"
> +#include "sysemu/runstate.h"
>
>  #define RISCV_DEBUG_HTIF 0
>  #define HTIF_DEBUG(fmt, ...)                                                   \
> @@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>                      g_free(sig_data);
>                  }
>
> -                exit(exit_code);
> +                qemu_system_shutdown_request_with_code(
> +                    SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
> +                return;
>              } else {
>                  uint64_t syscall[8];
>                  cpu_physical_memory_read(payload, syscall, sizeof(syscall));
> --
> 2.25.1
>

Re: [PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown

[Clément Chigot]

On Fri, Sep 22, 2023 at 7:20 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
> >
> > This replaces the exit calls by shutdown requests, ensuring a proper
> > cleanup of Qemu. Otherwise, some connections like gdb could be broken
> > before its final packet ("Wxx") is being sent. This part, being done
> > inside qemu_cleanup function, can be reached only when the main loop
> > exits after a shutdown request.
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> Do you mind rebasing this on:
> https://github.com/alistair23/qemu/tree/riscv-to-apply.next

Thanks for the review.
Just a quick question on the procedure side, is there any special tag
or something to say in the cover letter to state that it has been
rebased on riscv-to-apply instead of the usual master?

Clément

> Alistair
>
> > ---
> >  hw/char/riscv_htif.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> > index 37d3ccc76b..7e9b6fcc98 100644
> > --- a/hw/char/riscv_htif.c
> > +++ b/hw/char/riscv_htif.c
> > @@ -31,6 +31,7 @@
> >  #include "qemu/error-report.h"
> >  #include "exec/address-spaces.h"
> >  #include "sysemu/dma.h"
> > +#include "sysemu/runstate.h"
> >
> >  #define RISCV_DEBUG_HTIF 0
> >  #define HTIF_DEBUG(fmt, ...)                                                   \
> > @@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
> >                      g_free(sig_data);
> >                  }
> >
> > -                exit(exit_code);
> > +                qemu_system_shutdown_request_with_code(
> > +                    SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
> > +                return;
> >              } else {
> >                  uint64_t syscall[8];
> >                  cpu_physical_memory_read(payload, syscall, sizeof(syscall));
> > --
> > 2.25.1
> >

Re: [PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown

[Alistair Francis]

On Mon, Oct 2, 2023 at 7:32 PM Clément Chigot <chigot@adacore.com> wrote:
>
> On Fri, Sep 22, 2023 at 7:20 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
> > >
> > > This replaces the exit calls by shutdown requests, ensuring a proper
> > > cleanup of Qemu. Otherwise, some connections like gdb could be broken
> > > before its final packet ("Wxx") is being sent. This part, being done
> > > inside qemu_cleanup function, can be reached only when the main loop
> > > exits after a shutdown request.
> > >
> > > Signed-off-by: Clément Chigot <chigot@adacore.com>
> >
> > Do you mind rebasing this on:
> > https://github.com/alistair23/qemu/tree/riscv-to-apply.next
>
> Thanks for the review.
> Just a quick question on the procedure side, is there any special tag
> or something to say in the cover letter to state that it has been
> rebased on riscv-to-apply instead of the usual master?

You can use the "Based-on" tag. There is some more details here:

https://www.qemu.org/docs/master/devel/submitting-a-patch.html#id33

Alistair

>
> Clément
>
> > Alistair
> >
> > > ---
> > >  hw/char/riscv_htif.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> > > index 37d3ccc76b..7e9b6fcc98 100644
> > > --- a/hw/char/riscv_htif.c
> > > +++ b/hw/char/riscv_htif.c
> > > @@ -31,6 +31,7 @@
> > >  #include "qemu/error-report.h"
> > >  #include "exec/address-spaces.h"
> > >  #include "sysemu/dma.h"
> > > +#include "sysemu/runstate.h"
> > >
> > >  #define RISCV_DEBUG_HTIF 0
> > >  #define HTIF_DEBUG(fmt, ...)                                                   \
> > > @@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
> > >                      g_free(sig_data);
> > >                  }
> > >
> > > -                exit(exit_code);
> > > +                qemu_system_shutdown_request_with_code(
> > > +                    SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
> > > +                return;
> > >              } else {
> > >                  uint64_t syscall[8];
> > >                  cpu_physical_memory_read(payload, syscall, sizeof(syscall));
> > > --
> > > 2.25.1
> > >

[PATCH v3 5/5] gdbstub: replace exit calls with proper shutdown for softmmu

[Clément Chigot]

This replaces the exit calls by shutdown requests, ensuring a proper
cleanup of Qemu. Features like net/vhost-vdpa.c are expecting
qemu_cleanup to be called to remove their last residuals.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 gdbstub/gdbstub.c          | 5 +++--
 gdbstub/softmmu.c          | 6 ++++++
 gdbstub/user.c             | 6 ++++++
 include/gdbstub/syscalls.h | 9 +++++++++
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 349d348c7b..1cb6d65306 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1327,7 +1327,7 @@ static void handle_v_kill(GArray *params, void *user_ctx)
     gdb_put_packet("OK");
     error_report("QEMU: Terminated via GDBstub");
     gdb_exit(0);
-    exit(0);
+    gdb_qemu_exit(0);
 }
 
 static const GdbCmdParseEntry gdb_v_commands_table[] = {
@@ -1846,7 +1846,8 @@ static int gdb_handle_packet(const char *line_buf)
         /* Kill the target */
         error_report("QEMU: Terminated via GDBstub");
         gdb_exit(0);
-        exit(0);
+        gdb_qemu_exit(0);
+        break;
     case 'D':
         {
             static const GdbCmdParseEntry detach_cmd_desc = {
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 9f0b8b5497..a5d6e04c79 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -435,6 +435,12 @@ void gdb_exit(int code)
     qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
 }
 
+void gdb_qemu_exit(int code)
+{
+    qemu_system_shutdown_request_with_code(SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
+                                           code);
+}
+
 /*
  * Memory access
  */
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 7ab6e5d975..dbe1d9b887 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -113,6 +113,12 @@ void gdb_exit(int code)
         gdb_put_packet(buf);
         gdbserver_state.allow_stop_reply = false;
     }
+
+}
+
+void gdb_qemu_exit(int code)
+{
+    exit(code);
 }
 
 int gdb_handlesig(CPUState *cpu, int sig)
diff --git a/include/gdbstub/syscalls.h b/include/gdbstub/syscalls.h
index 243eaf8ce4..54ff7245a1 100644
--- a/include/gdbstub/syscalls.h
+++ b/include/gdbstub/syscalls.h
@@ -110,4 +110,13 @@ int use_gdb_syscalls(void);
  */
 void gdb_exit(int code);
 
+/**
+ * gdb_qemu_exit: ask qemu to exit
+ * @code: exit code reported
+ *
+ * This requests qemu to exit. This function is allowed to return as
+ * the exit request might be processed asynchronously by qemu backend.
+ */
+void gdb_qemu_exit(int code);
+
 #endif /* _SYSCALLS_H_ */
-- 
2.25.1

Re: [PATCH v3 5/5] gdbstub: replace exit calls with proper shutdown for softmmu

[Alistair Francis]

On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot <chigot@adacore.com> wrote:
>
> This replaces the exit calls by shutdown requests, ensuring a proper
> cleanup of Qemu. Features like net/vhost-vdpa.c are expecting
> qemu_cleanup to be called to remove their last residuals.
>
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  gdbstub/gdbstub.c          | 5 +++--
>  gdbstub/softmmu.c          | 6 ++++++
>  gdbstub/user.c             | 6 ++++++
>  include/gdbstub/syscalls.h | 9 +++++++++
>  4 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 349d348c7b..1cb6d65306 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1327,7 +1327,7 @@ static void handle_v_kill(GArray *params, void *user_ctx)
>      gdb_put_packet("OK");
>      error_report("QEMU: Terminated via GDBstub");
>      gdb_exit(0);
> -    exit(0);
> +    gdb_qemu_exit(0);
>  }
>
>  static const GdbCmdParseEntry gdb_v_commands_table[] = {
> @@ -1846,7 +1846,8 @@ static int gdb_handle_packet(const char *line_buf)
>          /* Kill the target */
>          error_report("QEMU: Terminated via GDBstub");
>          gdb_exit(0);
> -        exit(0);
> +        gdb_qemu_exit(0);
> +        break;
>      case 'D':
>          {
>              static const GdbCmdParseEntry detach_cmd_desc = {
> diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
> index 9f0b8b5497..a5d6e04c79 100644
> --- a/gdbstub/softmmu.c
> +++ b/gdbstub/softmmu.c
> @@ -435,6 +435,12 @@ void gdb_exit(int code)
>      qemu_chr_fe_deinit(&gdbserver_system_state.chr, true);
>  }
>
> +void gdb_qemu_exit(int code)
> +{
> +    qemu_system_shutdown_request_with_code(SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
> +                                           code);
> +}
> +
>  /*
>   * Memory access
>   */
> diff --git a/gdbstub/user.c b/gdbstub/user.c
> index 7ab6e5d975..dbe1d9b887 100644
> --- a/gdbstub/user.c
> +++ b/gdbstub/user.c
> @@ -113,6 +113,12 @@ void gdb_exit(int code)
>          gdb_put_packet(buf);
>          gdbserver_state.allow_stop_reply = false;
>      }
> +
> +}
> +
> +void gdb_qemu_exit(int code)
> +{
> +    exit(code);
>  }
>
>  int gdb_handlesig(CPUState *cpu, int sig)
> diff --git a/include/gdbstub/syscalls.h b/include/gdbstub/syscalls.h
> index 243eaf8ce4..54ff7245a1 100644
> --- a/include/gdbstub/syscalls.h
> +++ b/include/gdbstub/syscalls.h
> @@ -110,4 +110,13 @@ int use_gdb_syscalls(void);
>   */
>  void gdb_exit(int code);
>
> +/**
> + * gdb_qemu_exit: ask qemu to exit
> + * @code: exit code reported
> + *
> + * This requests qemu to exit. This function is allowed to return as
> + * the exit request might be processed asynchronously by qemu backend.
> + */
> +void gdb_qemu_exit(int code);
> +
>  #endif /* _SYSCALLS_H_ */
> --
> 2.25.1
>