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

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(-)
>

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
> 

--

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

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

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