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

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

[Luc Michel]

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     |  4 ++++
 include/sysemu/sysemu.h            |  2 ++
 semihosting/arm-compat-semi.c      |  3 +--
 semihosting/config.c               | 17 +++++++++++++++++
 softmmu/main.c                     |  2 +-
 softmmu/runstate.c                 | 11 +++++++++++
 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, 45 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH] accel/tcg/cpu-exec: fix precise single-stepping after interrupt

[Luc Michel]

In some cases, cpu->exit_request can be false after handling the
interrupt, leading to another TB being executed instead of returning
to the main loop.

Fix this by returning true unconditionally when in single-step mode.

Fixes: ba3c35d9c4026361fd380b269dc6def9510b7166

Signed-off-by: Luc Michel <lmichel@kalray.eu>
---
Coming back on this issue I worked on with Richard in 2020. The issue is
that when debugging the guest with GDB, the first instruction of the IRQ
handler is missed by GDB (it's still executed though).

It happened to me again in TCG RR mode (but not in MTTCG). It seems that
cpu->exit_request can be false in RR mode when returning from
cc->tcg_ops->cpu_exec_interrupt, leading to cpu_handle_interrupt
returning false and the next TB being executed, instead of the EXCP_DEBUG
being handled.
---
 accel/tcg/cpu-exec.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 8b4cd6c59d..74d7f83f34 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -796,13 +796,17 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
                 /*
                  * After processing the interrupt, ensure an EXCP_DEBUG is
                  * raised when single-stepping so that GDB doesn't miss the
                  * next instruction.
                  */
-                cpu->exception_index =
-                    (cpu->singlestep_enabled ? EXCP_DEBUG : -1);
-                *last_tb = NULL;
+                if (unlikely(cpu->singlestep_enabled)) {
+                    cpu->exception_index = EXCP_DEBUG;
+                    return true;
+                } else {
+                    cpu->exception_index = -1;
+                    *last_tb = NULL;
+                }
             }
             /* The target hook may have updated the 'cpu->interrupt_request';
              * reload the 'interrupt_request' value */
             interrupt_request = cpu->interrupt_request;
         }
-- 
2.17.1

[PATCH 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      | 11 +++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

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..d2491b8a59 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;
 }
 
-- 
2.17.1

[PATCH 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 |  4 ++++
 semihosting/config.c           | 17 +++++++++++++++++
 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..9a3214a0c8 100644
--- a/include/semihosting/semihost.h
+++ b/include/semihosting/semihost.h
@@ -54,10 +54,13 @@ static inline const char *semihosting_get_cmdline(void)
 
 static inline Chardev *semihosting_get_chardev(void)
 {
     return NULL;
 }
+void semihosting_exit_request(int status)
+{
+}
 static inline void qemu_semihosting_console_init(void)
 {
 }
 #else /* !CONFIG_USER_ONLY */
 bool semihosting_enabled(void);
@@ -65,10 +68,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..7913c83033 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,17 @@ void qemu_semihosting_connect_chardevs(void)
             exit(1);
         }
         semihosting.chardev = chr;
     }
 }
+
+void semihosting_exit_request(int status)
+{
+    gdb_exit(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 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 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 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 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 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] accel/tcg/cpu-exec: fix precise single-stepping after interrupt

[Luc Michel]

On 16:24 Mon 20 Jun     , Luc Michel wrote:
> In some cases, cpu->exit_request can be false after handling the
> interrupt, leading to another TB being executed instead of returning
> to the main loop.
> 
> Fix this by returning true unconditionally when in single-step mode.
> 
> Fixes: ba3c35d9c4026361fd380b269dc6def9510b7166
> 

Please ignore this old patch

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

[Peter Maydell]

On Mon, 20 Jun 2022 at 15:25, Luc Michel <lmichel@kalray.eu> wrote:
> 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 other option would be to say "if you register an atexit
callback in your plugin that's your problem to sort out" :-)
There's lots of situations where code inside QEMU might just
call exit(), not just this one. (Mostly these are "we detected
an error and decided to just bail out" codepaths.)

Is there a situation where we get a crash that doesn't involve
code in a plugin doing something odd?

thanks
-- PMM

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

[Luc Michel]

On 15:35 Mon 20 Jun     , Peter Maydell wrote:
> On Mon, 20 Jun 2022 at 15:25, Luc Michel <lmichel@kalray.eu> wrote:
> > 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 other option would be to say "if you register an atexit
> callback in your plugin that's your problem to sort out" :-)
> There's lots of situations where code inside QEMU might just
> call exit(), not just this one. (Mostly these are "we detected
> an error and decided to just bail out" codepaths.)

Sorry I was a bit unclear. I meant plugins using the
qemu_plugin_register_atexit_cb() register function, not directly calling
atexit(). This function documentation stats:

"Plugins should be able to free all their resources at this point much like
after a reset/uninstall callback is called."

If other CPUs are still running, this is not possible. I guess it's
reasonable to assume CPUs have reached a quiescent state when those
callbacks are called.

> 
> Is there a situation where we get a crash that doesn't involve
> code in a plugin doing something odd?

I'm not sure... I always feel a bit uncomfortable calling exit() from
a CPU thread in the middle of a translation block :)
I guess if you're monitoring QEMU through a QMP connection, it's better
to have a proper exit reason than having the connection suddenly
dropped. I guess the semihosting mode is not that popular among QEMU
users so there are probably other corner cases I'm not aware about. 

Thanks,

Luc

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

[Richard Henderson]

On 6/20/22 07:24, Luc Michel wrote:
> 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.

Did you test a complete build?  At a glance I would guess that arm-linux-user will no 
longer link because qemu_set/get_exit_status is missing.


r~

> 
> 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     |  4 ++++
>   include/sysemu/sysemu.h            |  2 ++
>   semihosting/arm-compat-semi.c      |  3 +--
>   semihosting/config.c               | 17 +++++++++++++++++
>   softmmu/main.c                     |  2 +-
>   softmmu/runstate.c                 | 11 +++++++++++
>   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, 45 insertions(+), 10 deletions(-)
> 

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

[Peter Maydell]

On Mon, 20 Jun 2022 at 16:10, Luc Michel <lmichel@kalray.eu> wrote:
>
> On 15:35 Mon 20 Jun     , Peter Maydell wrote:
> > On Mon, 20 Jun 2022 at 15:25, Luc Michel <lmichel@kalray.eu> wrote:
> > > 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 other option would be to say "if you register an atexit
> > callback in your plugin that's your problem to sort out" :-)
> > There's lots of situations where code inside QEMU might just
> > call exit(), not just this one. (Mostly these are "we detected
> > an error and decided to just bail out" codepaths.)
>
> Sorry I was a bit unclear. I meant plugins using the
> qemu_plugin_register_atexit_cb() register function, not directly calling
> atexit(). This function documentation stats:
>
> "Plugins should be able to free all their resources at this point much like
> after a reset/uninstall callback is called."
>
> If other CPUs are still running, this is not possible. I guess it's
> reasonable to assume CPUs have reached a quiescent state when those
> callbacks are called.

Ah, I see. I wonder if we should be handling the system-mode
exit() more along the lines of what we're currently doing
for usermode exit(). In general I don't think it's safe for
the plugins/core.c code to assume that when its atexit() handler
runs that all the TCG CPUs have stopped. Semihosting exit is just
the easiest reliably-reproducible situation when that's false.

thanks
-- PMM

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

[Luc Michel]

On 08:59 Mon 20 Jun     , Richard Henderson wrote:
> On 6/20/22 07:24, Luc Michel wrote:
> > 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.
> 
> Did you test a complete build?  At a glance I would guess that
> arm-linux-user will no longer link because qemu_set/get_exit_status is
> missing.

You are right I forgot to test build *-linux-user. There is a
compilation issue because I forgot "static inline" on the
semihosting_exit_request function on the CONFIG_USER_ONLY side. I'll fix
that in v2.

qemu_set/get_exit_status is fine though as it is only called from
softmmu-only code (and declared in sysemu/sysemu.h).

thanks,
Luc

> 
> 
> r~
> 
> > 
> > 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     |  4 ++++
> >   include/sysemu/sysemu.h            |  2 ++
> >   semihosting/arm-compat-semi.c      |  3 +--
> >   semihosting/config.c               | 17 +++++++++++++++++
> >   softmmu/main.c                     |  2 +-
> >   softmmu/runstate.c                 | 11 +++++++++++
> >   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, 45 insertions(+), 10 deletions(-)
> > 
> 
> 
> 
> To declare a filtering error, please use the following link : https://www.security-mail.net/reporter.php?mid=bb16.62b09954.79e61.0&r=lmichel%40kalray.eu&s=richard.henderson%40linaro.org&o=Re%3A+%5BPATCH+0%2F7%5D+semihosting%3A+proper+QEMU+exit+on+semihosted+exit+syscall&verdict=C&c=d52db680df8df28629e4a26f18787c389730fd78
> 

--