diff mbox

[2/7] target/openrisc: add shutdown logic

Message ID fb69c137317a365dcb549dfef1ecd2fbff48e92c.1492384862.git.shorne@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stafford Horne April 16, 2017, 11:23 p.m. UTC
In openrisc simulators we use hooks like 'l.nop 1' to cause the
simulator to exit.  Implement that for qemu too.

Reported-by: Waldemar Brodkorb <wbx@openadk.org>
Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 target/openrisc/helper.h     |  1 +
 target/openrisc/sys_helper.c | 17 +++++++++++++++++
 target/openrisc/translate.c  |  5 ++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

Comments

Richard Henderson April 18, 2017, 7:52 a.m. UTC | #1
On 04/16/2017 04:23 PM, Stafford Horne wrote:
> In openrisc simulators we use hooks like 'l.nop 1' to cause the
> simulator to exit.  Implement that for qemu too.
>
> Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> Signed-off-by: Stafford Horne <shorne@gmail.com>

As I said the first time this was posted: This is horrible.

If you want to do something like this, it needs to be buried under a special 
run mode like -semihosting.

>          case 0x01:    /* l.nop */
>              LOG_DIS("l.nop %d\n", I16);
> +            {
> +                TCGv_i32 arg = tcg_const_i32(I16);
> +                gen_helper_nop(arg);
> +            }

You also really really must special-case l.nop 0 so that it doesn't generate a 
function call.  Just think of all the extra calls you're adding for every delay 
slot that couldn't be filled.


r~
Stafford Horne April 18, 2017, 2:20 p.m. UTC | #2
On Tue, Apr 18, 2017 at 12:52:52AM -0700, Richard Henderson wrote:
> On 04/16/2017 04:23 PM, Stafford Horne wrote:
> > In openrisc simulators we use hooks like 'l.nop 1' to cause the
> > simulator to exit.  Implement that for qemu too.
> > 
> > Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> 
> As I said the first time this was posted: This is horrible.
> 
> If you want to do something like this, it needs to be buried under a special
> run mode like -semihosting.

Understood,  I will revise this.  I didnt know this was posted before.

> >          case 0x01:    /* l.nop */
> >              LOG_DIS("l.nop %d\n", I16);
> > +            {
> > +                TCGv_i32 arg = tcg_const_i32(I16);
> > +                gen_helper_nop(arg);
> > +            }
> 
> You also really really must special-case l.nop 0 so that it doesn't generate
> a function call.  Just think of all the extra calls you're adding for every
> delay slot that couldn't be filled.

Yeah, that makes sense.  Ill add that for l.nop 0.

-Stafford

> 
> r~
Stafford Horne April 22, 2017, 10:09 a.m. UTC | #3
On Tue, Apr 18, 2017 at 11:20:55PM +0900, Stafford Horne wrote:
> On Tue, Apr 18, 2017 at 12:52:52AM -0700, Richard Henderson wrote:
> > On 04/16/2017 04:23 PM, Stafford Horne wrote:
> > > In openrisc simulators we use hooks like 'l.nop 1' to cause the
> > > simulator to exit.  Implement that for qemu too.
> > > 
> > > Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > 
> > As I said the first time this was posted: This is horrible.
> > 
> > If you want to do something like this, it needs to be buried under a special
> > run mode like -semihosting.
> 
> Understood,  I will revise this.  I didnt know this was posted before.
> 
> > >          case 0x01:    /* l.nop */
> > >              LOG_DIS("l.nop %d\n", I16);
> > > +            {
> > > +                TCGv_i32 arg = tcg_const_i32(I16);
> > > +                gen_helper_nop(arg);
> > > +            }
> > 
> > You also really really must special-case l.nop 0 so that it doesn't generate
> > a function call.  Just think of all the extra calls you're adding for every
> > delay slot that couldn't be filled.
> 
> Yeah, that makes sense.  Ill add that for l.nop 0.

FYI,

I am going to drop this patch for now.  I think Waldemar can apply this
patch for the time being.

I looked through the semihosting route and I don't think poking l.nop
through there makes much sense since that looks mainly for syscalls.  I
also considered making another flag like `-or1k-hacks`, but I figured that
wouldnt be appropriate.

I discussed a bit on #qemu and Alexander Graf suggested to properly define
shutdown semantics for openrisc. Some examples were shown from ppc, s390
and arm.

  s390x
  http://git.qemu.org/?p=qemu.git;a=blob;f=target/s390x/helper.c;hb=HEAD#l265
  Detects the cpu is in WAIT state and shutsdown qemu.

  ppc - platform
  http://git.qemu.org/?p=qemu.git;a=blob;f=hw/ppc/e500.c;hb=HEAD#l936
  Registers hardware device mpc8xxx_gpio which handles shutdown via gpio

I will have a thought about this, it will require some kernel changes.

-Stafford
Richard Henderson April 22, 2017, 3:25 p.m. UTC | #4
On 04/22/2017 12:09 PM, Stafford Horne wrote:
> I discussed a bit on #qemu and Alexander Graf suggested to properly define
> shutdown semantics for openrisc. Some examples were shown from ppc, s390
> and arm.

Yes, properly defining this in the spec goes a long way toward fixing the 
underlying problem.

It's probably worth thinking about a wait-state condition as well, so that qemu 
can avoid staying pegged at 100% host cpu for an idle guest cpu.

>    ppc - platform
>    http://git.qemu.org/?p=qemu.git;a=blob;f=hw/ppc/e500.c;hb=HEAD#l936
>    Registers hardware device mpc8xxx_gpio which handles shutdown via gpio

That's one possibility.  Another is to define an SPR.


r~
Jason A. Donenfeld April 27, 2022, 5:44 p.m. UTC | #5
Hey Stafford,

On Mon, Apr 17, 2017 at 08:23:51AM +0900, Stafford Horne wrote:
> In openrisc simulators we use hooks like 'l.nop 1' to cause the
> simulator to exit.  Implement that for qemu too.
> 
> Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> Signed-off-by: Stafford Horne <shorne@gmail.com>

I'm curious as to why this never got merged. I noticed I'm entirely able
to shutdown or to reboot (which is mostly what I care about) Linux from
OpenRISC. It just hangs.

Thanks,
Jason
Peter Maydell April 27, 2022, 6:47 p.m. UTC | #6
On Wed, 27 Apr 2022 at 18:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hey Stafford,
>
> On Mon, Apr 17, 2017 at 08:23:51AM +0900, Stafford Horne wrote:
> > In openrisc simulators we use hooks like 'l.nop 1' to cause the
> > simulator to exit.  Implement that for qemu too.
> >
> > Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
>
> I'm curious as to why this never got merged. I noticed I'm entirely able
> to shutdown or to reboot (which is mostly what I care about) Linux from
> OpenRISC. It just hangs.

This kind of thing needs to be either:
 (1) we're modelling real hardware and that real hardware has a
device or other mechanism guest code can prod to cause a power-off
or reboot. Then we model that device, and guest code triggers a
shutdown or reboot exactly as it would on the real hardware.
 (2) there is an architecturally defined ABI for simulators, debug
stubs, etc, that includes various operations typically including
an "exit the simulator" function. (Arm semihosting is an example
of this.) In that case we can implement that functionality,
guarded by and controlled by the appropriate command line options.
(This is generally not as nice as option 1, because the guest code
has to be compiled to have support for semihosting and also because
turning it on is usually also giving implicit permission for the
guest code to read and write arbitrary host files, etc.)

Either way, undocumented random hacks aren't a good idea, which
is why this wasn't merged.

thanks
-- PMM
Stafford Horne April 27, 2022, 9:48 p.m. UTC | #7
On Wed, Apr 27, 2022 at 07:47:33PM +0100, Peter Maydell wrote:
> On Wed, 27 Apr 2022 at 18:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hey Stafford,
> >
> > On Mon, Apr 17, 2017 at 08:23:51AM +0900, Stafford Horne wrote:
> > > In openrisc simulators we use hooks like 'l.nop 1' to cause the
> > > simulator to exit.  Implement that for qemu too.
> > >
> > > Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> >
> > I'm curious as to why this never got merged. I noticed I'm entirely able
> > to shutdown or to reboot (which is mostly what I care about) Linux from
> > OpenRISC. It just hangs.
> 
> This kind of thing needs to be either:
>  (1) we're modelling real hardware and that real hardware has a
> device or other mechanism guest code can prod to cause a power-off
> or reboot. Then we model that device, and guest code triggers a
> shutdown or reboot exactly as it would on the real hardware.
>  (2) there is an architecturally defined ABI for simulators, debug
> stubs, etc, that includes various operations typically including
> an "exit the simulator" function. (Arm semihosting is an example
> of this.) In that case we can implement that functionality,
> guarded by and controlled by the appropriate command line options.
> (This is generally not as nice as option 1, because the guest code
> has to be compiled to have support for semihosting and also because
> turning it on is usually also giving implicit permission for the
> guest code to read and write arbitrary host files, etc.)
> 
> Either way, undocumented random hacks aren't a good idea, which
> is why this wasn't merged.

Yes, this is what was brought up before.  At that time semihosting was mentioned
and I tried to understand what it was but didn't really understand it as a general
concept.  Is this something arm specific?

Since the qemu or1k-sim defines our "simulator", I suspect I could add a
definition of our simulator ABI to the OpenRISC architecture specification.  The
simulation uses of l.nop N as ABI hooks is a de-facto standard for OpenRISC.
From the way you describe this now I take it if we document this as a
architecture simulation ABI the patch would be accepted.

-Stafford
Jason A. Donenfeld April 28, 2022, 12:04 a.m. UTC | #8
Hi Stafford,

On Thu, Apr 28, 2022 at 06:48:27AM +0900, Stafford Horne wrote:
> On Wed, Apr 27, 2022 at 07:47:33PM +0100, Peter Maydell wrote:
> > On Wed, 27 Apr 2022 at 18:46, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > Hey Stafford,
> > >
> > > On Mon, Apr 17, 2017 at 08:23:51AM +0900, Stafford Horne wrote:
> > > > In openrisc simulators we use hooks like 'l.nop 1' to cause the
> > > > simulator to exit.  Implement that for qemu too.
> > > >
> > > > Reported-by: Waldemar Brodkorb <wbx@openadk.org>
> > > > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > >
> > > I'm curious as to why this never got merged. I noticed I'm entirely able
> > > to shutdown or to reboot (which is mostly what I care about) Linux from
> > > OpenRISC. It just hangs.
> > 
> > This kind of thing needs to be either:
> >  (1) we're modelling real hardware and that real hardware has a
> > device or other mechanism guest code can prod to cause a power-off
> > or reboot. Then we model that device, and guest code triggers a
> > shutdown or reboot exactly as it would on the real hardware.
> >  (2) there is an architecturally defined ABI for simulators, debug
> > stubs, etc, that includes various operations typically including
> > an "exit the simulator" function. (Arm semihosting is an example
> > of this.) In that case we can implement that functionality,
> > guarded by and controlled by the appropriate command line options.
> > (This is generally not as nice as option 1, because the guest code
> > has to be compiled to have support for semihosting and also because
> > turning it on is usually also giving implicit permission for the
> > guest code to read and write arbitrary host files, etc.)
> > 
> > Either way, undocumented random hacks aren't a good idea, which
> > is why this wasn't merged.
> 
> Yes, this is what was brought up before.  At that time semihosting was mentioned
> and I tried to understand what it was but didn't really understand it as a general
> concept.  Is this something arm specific?
> 
> Since the qemu or1k-sim defines our "simulator", I suspect I could add a
> definition of our simulator ABI to the OpenRISC architecture specification.  The
> simulation uses of l.nop N as ABI hooks is a de-facto standard for OpenRISC.
> From the way you describe this now I take it if we document this as a
> architecture simulation ABI the patch would be accepted.

If that's what it takes, then that'd make sense.

By the way, would this also help the reboot case? That's
`reboot(RB_AUTOBOOT);`, which does:

machine_restart() ->
  do_kernel_restart() ->
    atomic_notifier_chain_register(&restart_handler_list, nb) ->
      ???

As far as I can tell, nothing is wired into the reboot case for
OpenRISC. Is this something that could be fixed in the kernel without
having to patch QEMU? If so, then I could effectively get shutdown for
my CI with the -no-reboot option, which is what I'm already doing for a
few platforms.

Jason
Peter Maydell April 28, 2022, 9:19 a.m. UTC | #9
On Wed, 27 Apr 2022 at 23:27, Stafford Horne <shorne@gmail.com> wrote:
> Yes, this is what was brought up before.  At that time semihosting was mentioned
> and I tried to understand what it was but didn't really understand it as a general
> concept.  Is this something arm specific?

QEMU uses "semihosting" for the general concept of a syscall-like
ABI provided by the model that allows guest code written to use it
to access some facilities as if it were a program running on the host
rather than running on bare metal. (I think the term derives originally
from the Arm term for this kind of functionality, but the concept is
not Arm-specific.)

Arm defines an ABI which looks basically like a set of syscalls:
code sets up some registers and executes a specific SVC or HLT
or other magic instruction, and the implementation is supposed to
then act on that. You can do things like "open file", "read file",
"exit", etc.
 https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst
The idea is that simulators and also debug stubs or debuggers can
implement this, and then bare-metal code can be written to use it,
mainly for debugging and test case purposes.

The risc-v folks decided they needed similar functionality, and
that the easiest way to do this was to align with the Arm specification
and document the risc-v specific bits:
https://github.com/riscv/riscv-semihosting-spec

Some other architectures have an equivalent thing but which isn't
the same set of functions as the Arm version; eg the nios2 version
is documented here:
https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=libgloss/nios2/nios2-semi.txt;hb=HEAD

> Since the qemu or1k-sim defines our "simulator", I suspect I could add a
> definition of our simulator ABI to the OpenRISC architecture specification.  The
> simulation uses of l.nop N as ABI hooks is a de-facto standard for OpenRISC.
> From the way you describe this now I take it if we document this as a
> architecture simulation ABI the patch would be accepted.

If it's something that (a) is documented officially somewhere and
(b) everybody is using consistently (ie guest code such as GNU newlib,
QEMU, other simulators, etc), then yes, that's OK. It sounds like
you just need to write down the details of your de-facto standard
to turn it into a de-jure one :-)

We would want to guard it behind the existing semihosting command
line option, rather than having it enabled all the time, but that
part should be straightforward.

-- PMM
Jason A. Donenfeld April 28, 2022, 11:16 a.m. UTC | #10
Hey again,

On Thu, Apr 28, 2022 at 02:04:29AM +0200, Jason A. Donenfeld wrote:
> By the way, would this also help the reboot case? That's
> `reboot(RB_AUTOBOOT);`, which does:
> 
> machine_restart() ->
>   do_kernel_restart() ->
>     atomic_notifier_chain_register(&restart_handler_list, nb) ->
>       ???
> 
> As far as I can tell, nothing is wired into the reboot case for
> OpenRISC. Is this something that could be fixed in the kernel without
> having to patch QEMU? If so, then I could effectively get shutdown for
> my CI with the -no-reboot option, which is what I'm already doing for a
> few platforms.

I added 13 for this: https://lists.librecores.org/pipermail/openrisc/2022-April/003884.html

When you go add these nops to the specification, please remember to add
one for reboot too. Then, once that kernel code is merged and the
specification published, it'll be sensible to add shutdown and reboot
support to QEMU, per Peter's description.

Jason
Stafford Horne April 28, 2022, 11:47 a.m. UTC | #11
On Thu, Apr 28, 2022 at 01:16:51PM +0200, Jason A. Donenfeld wrote:
> Hey again,
> 
> On Thu, Apr 28, 2022 at 02:04:29AM +0200, Jason A. Donenfeld wrote:
> > By the way, would this also help the reboot case? That's
> > `reboot(RB_AUTOBOOT);`, which does:
> > 
> > machine_restart() ->
> >   do_kernel_restart() ->
> >     atomic_notifier_chain_register(&restart_handler_list, nb) ->
> >       ???
> > 
> > As far as I can tell, nothing is wired into the reboot case for
> > OpenRISC. Is this something that could be fixed in the kernel without
> > having to patch QEMU? If so, then I could effectively get shutdown for
> > my CI with the -no-reboot option, which is what I'm already doing for a
> > few platforms.
> 
> I added 13 for this: https://lists.librecores.org/pipermail/openrisc/2022-April/003884.html
> 
> When you go add these nops to the specification, please remember to add
> one for reboot too. Then, once that kernel code is merged and the
> specification published, it'll be sensible to add shutdown and reboot
> support to QEMU, per Peter's description.

This sounds fair.

-Stafford
diff mbox

Patch

diff --git a/target/openrisc/helper.h b/target/openrisc/helper.h
index 4fd1a6b..b7838f5 100644
--- a/target/openrisc/helper.h
+++ b/target/openrisc/helper.h
@@ -59,3 +59,4 @@  DEF_HELPER_FLAGS_1(rfe, 0, void, env)
 /* sys */
 DEF_HELPER_FLAGS_4(mtspr, 0, void, env, tl, tl, tl)
 DEF_HELPER_FLAGS_4(mfspr, TCG_CALL_NO_WG, tl, env, tl, tl, tl)
+DEF_HELPER_1(nop, void, i32)
diff --git a/target/openrisc/sys_helper.c b/target/openrisc/sys_helper.c
index 60c3193..2eaff87 100644
--- a/target/openrisc/sys_helper.c
+++ b/target/openrisc/sys_helper.c
@@ -22,6 +22,7 @@ 
 #include "cpu.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
+#include "sysemu/sysemu.h"
 
 #define TO_SPR(group, number) (((group) << 11) + (number))
 
@@ -286,3 +287,19 @@  target_ulong HELPER(mfspr)(CPUOpenRISCState *env,
     /* for rd is passed in, if rd unchanged, just keep it back.  */
     return rd;
 }
+
+/* In openrisc simulators nop can be used to do intersting
+ * things like shut down the simulator */
+void HELPER(nop)(uint32_t arg)
+{
+#ifndef CONFIG_USER_ONLY
+    switch (arg) {
+    case 0x001: /* NOP_EXIT */
+    case 0x00c: /* NOP_EXIT_SILENT */
+        qemu_system_shutdown_request();
+        break;
+    default:
+        break;
+    }
+#endif
+}
diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index 7c4cbf2..74df245 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -755,8 +755,11 @@  static void dec_misc(DisasContext *dc, uint32_t insn)
         switch (op1) {
         case 0x01:    /* l.nop */
             LOG_DIS("l.nop %d\n", I16);
+            {
+                TCGv_i32 arg = tcg_const_i32(I16);
+                gen_helper_nop(arg);
+            }
             break;
-
         default:
             gen_illegal_exception(dc);
             break;