[PATCH v2 0/7] semihosting: proper QEMU exit on semihosted exit syscall

[PATCH v2 0/7] semihosting: proper QEMU exit on semihosted exit syscall

[Luc Michel]

v2:
  - fix linux-user compilation. Declare semihosting_exit_request "static
    inline G_NORETURN" on CONFIG_USER_ONLY side. Use
    g_assert_not_reached() to enforce the G_NORETURN since this function
    is unused in linux-user mode.
  - do not call gdb_exit() in semihosting_exit_request() as it is called in
    qemu_cleanup().
  - pass qemu_get_exit_status() to gdb_exit() in qemu_cleanup() instead
    of 0.

Hi,

This series implements a clean way for semihosted exit syscalls to
terminate QEMU with a given return code.

Until now, exit syscalls implementations consisted in calling exit()
with the wanted return code. The problem with this approach is that
other CPUs are not properly stopped, leading to possible crashes in
MTTCG mode, especially when at_exit callbacks have been registered. This
can be the case e.g., when plugins are in use. Plugins can register
at_exit callbacks. Those will be called on the CPU thread the exit
syscall is comming from, while other CPUs can continue to run and thus
call other plugin callbacks.

The semihosting_exit_request function provides a mean to cleanly
terminate QEMU. It introduces an new exit reason
(SHUTDOWN_CAUSE_GUEST_SEMI_EXIT) used in this case. The CPU is stopped
and returns to the main CPU loop so that no more instruction get
executed (the semihosting_exit_request is declared G_NORETURN).

All targets are converted to use this new function.

Thanks,
Luc

Luc Michel (7):
  softmmu: add qemu_[set|get]_exit_status functions
  semihosting: add the semihosting_exit_request function
  semihosting/arm-compat-semi: use semihosting_exit_request
  target/m68k: use semihosting_exit_request on semihosted exit syscall
  target/mips: use semihosting_exit_request on semihosted exit syscall
  target/nios2: use semihosting_exit_request on semihosted exit syscall
  target/xtensa: use semihosting_exit_request on semihosted exit syscall

 qapi/run-state.json                |  4 +++-
 include/semihosting/semihost.h     |  5 +++++
 include/sysemu/sysemu.h            |  2 ++
 semihosting/arm-compat-semi.c      |  3 +--
 semihosting/config.c               | 16 ++++++++++++++++
 softmmu/main.c                     |  2 +-
 softmmu/runstate.c                 | 13 ++++++++++++-
 target/m68k/m68k-semi.c            |  4 ++--
 target/mips/tcg/sysemu/mips-semi.c |  2 +-
 target/nios2/nios2-semi.c          |  4 ++--
 target/xtensa/xtensa-semi.c        |  2 +-
 11 files changed, 46 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v2 1/7] softmmu: add qemu_[set|get]_exit_status functions

[Luc Michel]

Add the two function qemu_set_exit_status() and qemu_get_exit_status().
Use qemu_get_exit_status() in main instead of 0 as the return value.

This is in preparation for the semihosting exit request implementation.

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
 include/sysemu/sysemu.h |  2 ++
 softmmu/main.c          |  2 +-
 softmmu/runstate.c      | 13 ++++++++++++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 812f66a31a..49b6970d0e 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -103,10 +103,12 @@ void qemu_boot_set(const char *boot_order, Error **errp);
 bool defaults_enabled(void);
 
 void qemu_init(int argc, char **argv, char **envp);
 void qemu_main_loop(void);
 void qemu_cleanup(void);
+void qemu_set_exit_status(int status);
+int qemu_get_exit_status(void);
 
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
 extern QemuOptsList qemu_drive_opts;
 extern QemuOptsList bdrv_runtime_opts;
diff --git a/softmmu/main.c b/softmmu/main.c
index c00432ff09..67b4bb111e 100644
--- a/softmmu/main.c
+++ b/softmmu/main.c
@@ -34,11 +34,11 @@ int qemu_main(int argc, char **argv, char **envp)
 {
     qemu_init(argc, argv, envp);
     qemu_main_loop();
     qemu_cleanup();
 
-    return 0;
+    return qemu_get_exit_status();
 }
 
 #ifndef CONFIG_COCOA
 int main(int argc, char **argv)
 {
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index fac7b63259..a86ffa91e5 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -336,10 +336,11 @@ void vm_state_notify(bool running, RunState state)
 }
 
 static ShutdownCause reset_requested;
 static ShutdownCause shutdown_requested;
 static int shutdown_signal;
+static int exit_status;
 static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
 static int suspend_requested;
 static WakeupReason wakeup_reason;
@@ -351,10 +352,20 @@ static NotifierList wakeup_notifiers =
     NOTIFIER_LIST_INITIALIZER(wakeup_notifiers);
 static NotifierList shutdown_notifiers =
     NOTIFIER_LIST_INITIALIZER(shutdown_notifiers);
 static uint32_t wakeup_reason_mask = ~(1 << QEMU_WAKEUP_REASON_NONE);
 
+void qemu_set_exit_status(int status)
+{
+    exit_status = status;
+}
+
+int qemu_get_exit_status(void)
+{
+    return exit_status;
+}
+
 ShutdownCause qemu_shutdown_requested_get(void)
 {
     return shutdown_requested;
 }
 
@@ -779,11 +790,11 @@ void qemu_init_subsystems(void)
 }
 
 
 void qemu_cleanup(void)
 {
-    gdb_exit(0);
+    gdb_exit(qemu_get_exit_status());
 
     /*
      * cleaning up the migration object cancels any existing migration
      * try to do this early so that it also stops using devices.
      */
-- 
2.17.1

[PATCH v2 2/7] semihosting: add the semihosting_exit_request function

[Luc Michel]

Add the semihosting_exit_request function to be used by targets when
handling an `exit' semihosted syscall. This function calls gdb_exit to
close existing GDB connections, and qemu_system_shutdown_request with
the new `guest-semi-exit' exit reason. It sets the QEMU exit status
given by the exit syscall parameter. Finally it stops the CPU to prevent
further execution, and exit the CPU loop as the syscall caller expects
this syscall to not return.

This function is meant to be used in place of a raw exit() call when
handling semihosted `exit' syscalls. Such a call is not safe because
it does not allow other CPU threads to exit properly, leading to e.g.
at_exit callbacks being called while other CPUs still run. This can lead
to strange bugs, especially in plugins with a registered at_exit function.

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
 qapi/run-state.json            |  4 +++-
 include/semihosting/semihost.h |  5 +++++
 semihosting/config.c           | 16 ++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 6e2162d7b3..a4f08dd32e 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -80,20 +80,22 @@
 # @guest-reset: Guest reset request, and command line turns that into
 #               a shutdown
 #
 # @guest-panic: Guest panicked, and command line turns that into a shutdown
 #
+# @guest-semi-exit: Guest exit request via a semi-hosted exit syscall
+#
 # @subsystem-reset: Partial guest reset that does not trigger QMP events and
 #                   ignores --no-reboot. This is useful for sanitizing
 #                   hypercalls on s390 that are used during kexec/kdump/boot
 #
 ##
 { 'enum': 'ShutdownCause',
   # Beware, shutdown_caused_by_guest() depends on enumeration order
   'data': [ 'none', 'host-error', 'host-qmp-quit', 'host-qmp-system-reset',
             'host-signal', 'host-ui', 'guest-shutdown', 'guest-reset',
-            'guest-panic', 'subsystem-reset'] }
+            'guest-panic', 'guest-semi-exit', 'subsystem-reset'] }
 
 ##
 # @StatusInfo:
 #
 # Information about VCPU run state
diff --git a/include/semihosting/semihost.h b/include/semihosting/semihost.h
index 0c55ade3ac..63b5641241 100644
--- a/include/semihosting/semihost.h
+++ b/include/semihosting/semihost.h
@@ -54,10 +54,14 @@ static inline const char *semihosting_get_cmdline(void)
 
 static inline Chardev *semihosting_get_chardev(void)
 {
     return NULL;
 }
+static inline G_NORETURN void semihosting_exit_request(int status)
+{
+    g_assert_not_reached();
+}
 static inline void qemu_semihosting_console_init(void)
 {
 }
 #else /* !CONFIG_USER_ONLY */
 bool semihosting_enabled(void);
@@ -65,10 +69,11 @@ SemihostingTarget semihosting_get_target(void);
 const char *semihosting_get_arg(int i);
 int semihosting_get_argc(void);
 const char *semihosting_get_cmdline(void);
 void semihosting_arg_fallback(const char *file, const char *cmd);
 Chardev *semihosting_get_chardev(void);
+G_NORETURN void semihosting_exit_request(int status);
 /* for vl.c hooks */
 void qemu_semihosting_enable(void);
 int qemu_semihosting_config_options(const char *opt);
 void qemu_semihosting_connect_chardevs(void);
 void qemu_semihosting_console_init(void);
diff --git a/semihosting/config.c b/semihosting/config.c
index 3afacf54ab..e60a32a3f7 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -22,10 +22,15 @@
 #include "qemu/option.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "semihosting/semihost.h"
 #include "chardev/char.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/runstate.h"
+#include "sysemu/cpus.h"
+#include "exec/exec-all.h"
+#include "exec/gdbstub.h"
 
 QemuOptsList qemu_semihosting_config_opts = {
     .name = "semihosting-config",
     .merge_lists = true,
     .implied_opt_name = "enable",
@@ -183,5 +188,16 @@ void qemu_semihosting_connect_chardevs(void)
             exit(1);
         }
         semihosting.chardev = chr;
     }
 }
+
+void semihosting_exit_request(int status)
+{
+    qemu_set_exit_status(status);
+    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SEMI_EXIT);
+    cpu_stop_current();
+
+    current_cpu->exception_index = EXCP_HLT;
+    current_cpu->halted = 1;
+    cpu_loop_exit(current_cpu);
+}
-- 
2.17.1

[PATCH v2 3/7] semihosting/arm-compat-semi: use semihosting_exit_request

[Luc Michel]

Use the new semihosting_exit_request instead of a call to exit when
handling a semihosted exit syscall.

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
 semihosting/arm-compat-semi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index b6ddaf863a..fad5116f3c 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -1253,12 +1253,11 @@ target_ulong do_common_semihosting(CPUState *cs)
              * allow the guest to specify the exit status code.
              * Everything else is considered an error.
              */
             ret = (args == ADP_Stopped_ApplicationExit) ? 0 : 1;
         }
-        gdb_exit(ret);
-        exit(ret);
+        semihosting_exit_request(ret);
     case TARGET_SYS_ELAPSED:
         elapsed = get_clock() - clock_start;
         if (sizeof(target_ulong) == 8) {
             SET_ARG(0, elapsed);
         } else {
-- 
2.17.1

[PATCH v2 4/7] target/m68k: use semihosting_exit_request on semihosted exit syscall

[Luc Michel]

Use the new semihosting_exit_request instead of a call to exit when
handling a semihosted exit syscall.

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
 target/m68k/m68k-semi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
index 37343d47e2..b3de3c6874 100644
--- a/target/m68k/m68k-semi.c
+++ b/target/m68k/m68k-semi.c
@@ -27,10 +27,11 @@
 #else
 #include "exec/softmmu-semi.h"
 #include "hw/boards.h"
 #endif
 #include "qemu/log.h"
+#include "semihosting/semihost.h"
 
 #define HOSTED_EXIT  0
 #define HOSTED_INIT_SIM 1
 #define HOSTED_OPEN 2
 #define HOSTED_CLOSE 3
@@ -193,12 +194,11 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
     uint32_t result;
 
     args = env->dregs[1];
     switch (nr) {
     case HOSTED_EXIT:
-        gdb_exit(env->dregs[0]);
-        exit(env->dregs[0]);
+        semihosting_exit_request(env->dregs[0]);
     case HOSTED_OPEN:
         GET_ARG(0);
         GET_ARG(1);
         GET_ARG(2);
         GET_ARG(3);
-- 
2.17.1

[PATCH v2 5/7] target/mips: use semihosting_exit_request on semihosted exit syscall

[Luc Michel]

Use the new semihosting_exit_request instead of a call to exit when
handling a semihosted exit syscall.

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
 target/mips/tcg/sysemu/mips-semi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/mips/tcg/sysemu/mips-semi.c b/target/mips/tcg/sysemu/mips-semi.c
index b4a383ae90..94be486925 100644
--- a/target/mips/tcg/sysemu/mips-semi.c
+++ b/target/mips/tcg/sysemu/mips-semi.c
@@ -245,11 +245,11 @@ void helper_do_semihosting(CPUMIPSState *env)
     char *p, *p2;
 
     switch (op) {
     case UHI_exit:
         qemu_log("UHI(%d): exit(%d)\n", op, (int)gpr[4]);
-        exit(gpr[4]);
+        semihosting_exit_request(gpr[4]);
     case UHI_open:
         GET_TARGET_STRING(p, gpr[4]);
         if (!strcmp("/dev/stdin", p)) {
             gpr[2] = 0;
         } else if (!strcmp("/dev/stdout", p)) {
-- 
2.17.1

[PATCH v2 6/7] target/nios2: use semihosting_exit_request on semihosted exit syscall

[Luc Michel]

Use the new semihosting_exit_request instead of a call to exit when
handling a semihosted exit syscall.

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
 target/nios2/nios2-semi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/nios2/nios2-semi.c b/target/nios2/nios2-semi.c
index ec88474a73..2624ef1539 100644
--- a/target/nios2/nios2-semi.c
+++ b/target/nios2/nios2-semi.c
@@ -29,10 +29,11 @@
 #include "qemu.h"
 #else
 #include "exec/softmmu-semi.h"
 #endif
 #include "qemu/log.h"
+#include "semihosting/semihost.h"
 
 #define HOSTED_EXIT  0
 #define HOSTED_INIT_SIM 1
 #define HOSTED_OPEN 2
 #define HOSTED_CLOSE 3
@@ -212,12 +213,11 @@ void do_nios2_semihosting(CPUNios2State *env)
 
     nr = env->regs[R_ARG0];
     args = env->regs[R_ARG1];
     switch (nr) {
     case HOSTED_EXIT:
-        gdb_exit(env->regs[R_ARG0]);
-        exit(env->regs[R_ARG0]);
+        semihosting_exit_request(env->regs[R_ARG0]);
     case HOSTED_OPEN:
         GET_ARG(0);
         GET_ARG(1);
         GET_ARG(2);
         GET_ARG(3);
-- 
2.17.1

[PATCH v2 7/7] target/xtensa: use semihosting_exit_request on semihosted exit syscall

[Luc Michel]

Use the new semihosting_exit_request instead of a call to exit when
handling a semihosted exit syscall.

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
 target/xtensa/xtensa-semi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/xtensa/xtensa-semi.c b/target/xtensa/xtensa-semi.c
index fa21b7e11f..0e9a9edc16 100644
--- a/target/xtensa/xtensa-semi.c
+++ b/target/xtensa/xtensa-semi.c
@@ -193,11 +193,11 @@ void HELPER(simcall)(CPUXtensaState *env)
     CPUState *cs = env_cpu(env);
     uint32_t *regs = env->regs;
 
     switch (regs[2]) {
     case TARGET_SYS_exit:
-        exit(regs[3]);
+        semihosting_exit_request(regs[3]);
         break;
 
     case TARGET_SYS_read:
     case TARGET_SYS_write:
         {
-- 
2.17.1

Re: [PATCH v2 1/7] softmmu: add qemu_[set|get]_exit_status functions

[Laurent Vivier]

Le 21/06/2022 à 14:59, Luc Michel a écrit :
> Add the two function qemu_set_exit_status() and qemu_get_exit_status().
> Use qemu_get_exit_status() in main instead of 0 as the return value.
> 
> This is in preparation for the semihosting exit request implementation.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>   include/sysemu/sysemu.h |  2 ++
>   softmmu/main.c          |  2 +-
>   softmmu/runstate.c      | 13 ++++++++++++-
>   3 files changed, 15 insertions(+), 2 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 2/7] semihosting: add the semihosting_exit_request function

[Laurent Vivier]

Le 21/06/2022 à 14:59, Luc Michel a écrit :
> Add the semihosting_exit_request function to be used by targets when
> handling an `exit' semihosted syscall. This function calls gdb_exit to
> close existing GDB connections, and qemu_system_shutdown_request with
> the new `guest-semi-exit' exit reason. It sets the QEMU exit status
> given by the exit syscall parameter. Finally it stops the CPU to prevent
> further execution, and exit the CPU loop as the syscall caller expects
> this syscall to not return.
> 
> This function is meant to be used in place of a raw exit() call when
> handling semihosted `exit' syscalls. Such a call is not safe because
> it does not allow other CPU threads to exit properly, leading to e.g.
> at_exit callbacks being called while other CPUs still run. This can lead
> to strange bugs, especially in plugins with a registered at_exit function.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>   qapi/run-state.json            |  4 +++-
>   include/semihosting/semihost.h |  5 +++++
>   semihosting/config.c           | 16 ++++++++++++++++
>   3 files changed, 24 insertions(+), 1 deletion(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 3/7] semihosting/arm-compat-semi: use semihosting_exit_request

[Laurent Vivier]

Le 21/06/2022 à 14:59, Luc Michel a écrit :
> Use the new semihosting_exit_request instead of a call to exit when
> handling a semihosted exit syscall.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>   semihosting/arm-compat-semi.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 5/7] target/mips: use semihosting_exit_request on semihosted exit syscall

[Laurent Vivier]

Le 21/06/2022 à 14:59, Luc Michel a écrit :
> Use the new semihosting_exit_request instead of a call to exit when
> handling a semihosted exit syscall.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>   target/mips/tcg/sysemu/mips-semi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/mips/tcg/sysemu/mips-semi.c b/target/mips/tcg/sysemu/mips-semi.c
> index b4a383ae90..94be486925 100644
> --- a/target/mips/tcg/sysemu/mips-semi.c
> +++ b/target/mips/tcg/sysemu/mips-semi.c
> @@ -245,11 +245,11 @@ void helper_do_semihosting(CPUMIPSState *env)
>       char *p, *p2;
>   
>       switch (op) {
>       case UHI_exit:
>           qemu_log("UHI(%d): exit(%d)\n", op, (int)gpr[4]);
> -        exit(gpr[4]);
> +        semihosting_exit_request(gpr[4]);
>       case UHI_open:
>           GET_TARGET_STRING(p, gpr[4]);
>           if (!strcmp("/dev/stdin", p)) {
>               gpr[2] = 0;
>           } else if (!strcmp("/dev/stdout", p)) {


Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 6/7] target/nios2: use semihosting_exit_request on semihosted exit syscall

[Laurent Vivier]

Le 21/06/2022 à 14:59, Luc Michel via a écrit :
> Use the new semihosting_exit_request instead of a call to exit when
> handling a semihosted exit syscall.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>   target/nios2/nios2-semi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.Eu>

Re: [PATCH v2 7/7] target/xtensa: use semihosting_exit_request on semihosted exit syscall

[Laurent Vivier]

Le 21/06/2022 à 14:59, Luc Michel a écrit :
> Use the new semihosting_exit_request instead of a call to exit when
> handling a semihosted exit syscall.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>   target/xtensa/xtensa-semi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 4/7] target/m68k: use semihosting_exit_request on semihosted exit syscall

[Laurent Vivier]

Le 21/06/2022 à 14:59, Luc Michel a écrit :
> Use the new semihosting_exit_request instead of a call to exit when
> handling a semihosted exit syscall.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>   target/m68k/m68k-semi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 0/7] semihosting: proper QEMU exit on semihosted exit syscall

[Richard Henderson]

On 6/21/22 05:59, Luc Michel wrote:
> v2:
>    - fix linux-user compilation. Declare semihosting_exit_request "static
>      inline G_NORETURN" on CONFIG_USER_ONLY side. Use
>      g_assert_not_reached() to enforce the G_NORETURN since this function
>      is unused in linux-user mode.

Not true.  It *is* used with semihosting linux-user.

Anyway, before you go too far down this road, see

https://patchew.org/QEMU/20220607204557.658541-1-richard.henderson@linaro.org/


r~

Re: [PATCH v2 0/7] semihosting: proper QEMU exit on semihosted exit syscall

[Luc Michel]

On 07:37 Tue 21 Jun     , Richard Henderson wrote:
> On 6/21/22 05:59, Luc Michel wrote:
> > v2:
> >    - fix linux-user compilation. Declare semihosting_exit_request "static
> >      inline G_NORETURN" on CONFIG_USER_ONLY side. Use
> >      g_assert_not_reached() to enforce the G_NORETURN since this function
> >      is unused in linux-user mode.
> 
> Not true.  It *is* used with semihosting linux-user.
> 
> Anyway, before you go too far down this road, see
> 
> https://patchew.org/QEMU/20220607204557.658541-1-richard.henderson@linaro.org/

aww OK. Let's have your series merged first then.

> 
> 
> r~
> 
> 
> To declare a filtering error, please use the following link : https://www.security-mail.net/reporter.php?mid=16699.62b1d79b.16c49.0&r=lmichel%40kalray.eu&s=richard.henderson%40linaro.org&o=Re%3A+%5BPATCH+v2+0%2F7%5D+semihosting%3A+proper+QEMU+exit+on+semihosted+exit+syscall&verdict=C&c=71759fc2b76b8b9bc7813e449355fa174c40ee7b
> 

-- 

Re: [PATCH v2 2/7] semihosting: add the semihosting_exit_request function

[Eric Blake]

On Tue, Jun 21, 2022 at 02:59:11PM +0200, Luc Michel wrote:
> Add the semihosting_exit_request function to be used by targets when
> handling an `exit' semihosted syscall. This function calls gdb_exit to
> close existing GDB connections, and qemu_system_shutdown_request with
> the new `guest-semi-exit' exit reason. It sets the QEMU exit status
> given by the exit syscall parameter. Finally it stops the CPU to prevent
> further execution, and exit the CPU loop as the syscall caller expects
> this syscall to not return.
> 
> This function is meant to be used in place of a raw exit() call when
> handling semihosted `exit' syscalls. Such a call is not safe because
> it does not allow other CPU threads to exit properly, leading to e.g.
> at_exit callbacks being called while other CPUs still run. This can lead
> to strange bugs, especially in plugins with a registered at_exit function.
> 
> Signed-off-by: Luc Michel <lmichel@kalray.eu>
> ---
>  qapi/run-state.json            |  4 +++-
>  include/semihosting/semihost.h |  5 +++++
>  semihosting/config.c           | 16 ++++++++++++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 6e2162d7b3..a4f08dd32e 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -80,20 +80,22 @@
>  # @guest-reset: Guest reset request, and command line turns that into
>  #               a shutdown
>  #
>  # @guest-panic: Guest panicked, and command line turns that into a shutdown
>  #
> +# @guest-semi-exit: Guest exit request via a semi-hosted exit syscall

Should include a '(since 7.1)' notation.

> +#
>  # @subsystem-reset: Partial guest reset that does not trigger QMP events and
>  #                   ignores --no-reboot. This is useful for sanitizing
>  #                   hypercalls on s390 that are used during kexec/kdump/boot
>  #
>  ##

As it is, the overall enum is missing a 'Since: 1.0' section.

>  { 'enum': 'ShutdownCause',
>    # Beware, shutdown_caused_by_guest() depends on enumeration order
>    'data': [ 'none', 'host-error', 'host-qmp-quit', 'host-qmp-system-reset',
>              'host-signal', 'host-ui', 'guest-shutdown', 'guest-reset',
> -            'guest-panic', 'subsystem-reset'] }
> +            'guest-panic', 'guest-semi-exit', 'subsystem-reset'] }
>  
>  ##
>  # @StatusInfo:
>  #
>  # Information about VCPU run state

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 2/7] semihosting: add the semihosting_exit_request function

[Peter Maydell]

On Tue, 21 Jun 2022 at 13:59, Luc Michel <lmichel@kalray.eu> wrote:
>
> Add the semihosting_exit_request function to be used by targets when
> handling an `exit' semihosted syscall. This function calls gdb_exit to
> close existing GDB connections, and qemu_system_shutdown_request with
> the new `guest-semi-exit' exit reason. It sets the QEMU exit status
> given by the exit syscall parameter. Finally it stops the CPU to prevent
> further execution, and exit the CPU loop as the syscall caller expects
> this syscall to not return.
>
> This function is meant to be used in place of a raw exit() call when
> handling semihosted `exit' syscalls. Such a call is not safe because
> it does not allow other CPU threads to exit properly, leading to e.g.
> at_exit callbacks being called while other CPUs still run. This can lead
> to strange bugs, especially in plugins with a registered at_exit function.

This is mixing up two things:
 (1) fixing bugs with the plugin code when code (semihosting or
     otherwise) calls exit()
 (2) reporting to the monitor when the guest exits because it
     asked to via semihosting

I remain unconvinced that this series is actually fixing (1),
I think it's just working around the most common cause of it.
For (2), maybe we want it, but that should I think be a
separate patchset with justification of why it's useful to
tell the monitor about it. I think on balance it probably
is a good idea, but I disagree about (1) and would like to
see these two things not tangled up in the same series.

thanks
-- PMM

Re: [PATCH v2 2/7] semihosting: add the semihosting_exit_request function

[Luc Michel]

On 20:09 Wed 22 Jun     , Peter Maydell wrote:
> On Tue, 21 Jun 2022 at 13:59, Luc Michel <lmichel@kalray.eu> wrote:
> >
> > Add the semihosting_exit_request function to be used by targets when
> > handling an `exit' semihosted syscall. This function calls gdb_exit to
> > close existing GDB connections, and qemu_system_shutdown_request with
> > the new `guest-semi-exit' exit reason. It sets the QEMU exit status
> > given by the exit syscall parameter. Finally it stops the CPU to prevent
> > further execution, and exit the CPU loop as the syscall caller expects
> > this syscall to not return.
> >
> > This function is meant to be used in place of a raw exit() call when
> > handling semihosted `exit' syscalls. Such a call is not safe because
> > it does not allow other CPU threads to exit properly, leading to e.g.
> > at_exit callbacks being called while other CPUs still run. This can lead
> > to strange bugs, especially in plugins with a registered at_exit function.
> 
> This is mixing up two things:
>  (1) fixing bugs with the plugin code when code (semihosting or
>      otherwise) calls exit()
>  (2) reporting to the monitor when the guest exits because it
>      asked to via semihosting
> 
> I remain unconvinced that this series is actually fixing (1),
> I think it's just working around the most common cause of it.
> For (2), maybe we want it, but that should I think be a
> separate patchset with justification of why it's useful to
> tell the monitor about it. I think on balance it probably
> is a good idea, but I disagree about (1) and would like to
> see these two things not tangled up in the same series.

OK. I'll rework this once Richard's semihosting cleanup series is
merged.

thanks.

Luc

> 
> thanks
> -- PMM
> 
> 
> To declare a filtering error, please use the following link : https://www.security-mail.net/reporter.php?mid=11a39.62b36915.466b.0&r=lmichel%40kalray.eu&s=peter.maydell%40linaro.org&o=Re%3A+%5BPATCH+v2+2%2F7%5D+semihosting%3A+add+the+semihosting_exit_request+function&verdict=C&c=b75eec0eae9b68db747812558b665a75218eca91
> 

--