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

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

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
> 

--

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

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>