[Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

This adds a reset opcode for sifive_test device to trigger a system
reset for testing purpose.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 hw/riscv/sifive_test.c         | 4 ++++
 include/hw/riscv/sifive_test.h | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
index 24a04d7..cd86831 100644
--- a/hw/riscv/sifive_test.c
+++ b/hw/riscv/sifive_test.c
@@ -21,6 +21,7 @@
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "qemu/module.h"
+#include "sysemu/sysemu.h"
 #include "target/riscv/cpu.h"
 #include "hw/riscv/sifive_test.h"
 
@@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
             exit(code);
         case FINISHER_PASS:
             exit(0);
+        case FINISHER_RESET:
+            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            return;
         default:
             break;
         }
diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
index 71d4c9f..c186a31 100644
--- a/include/hw/riscv/sifive_test.h
+++ b/include/hw/riscv/sifive_test.h
@@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
 
 enum {
     FINISHER_FAIL = 0x3333,
-    FINISHER_PASS = 0x5555
+    FINISHER_PASS = 0x5555,
+    FINISHER_RESET = 0x7777
 };
 
 DeviceState *sifive_test_create(hwaddr addr);
-- 
2.7.4

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Alistair Francis]

On Fri, Jun 14, 2019 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> This adds a reset opcode for sifive_test device to trigger a system
> reset for testing purpose.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  hw/riscv/sifive_test.c         | 4 ++++
>  include/hw/riscv/sifive_test.h | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> index 24a04d7..cd86831 100644
> --- a/hw/riscv/sifive_test.c
> +++ b/hw/riscv/sifive_test.c
> @@ -21,6 +21,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "qemu/module.h"
> +#include "sysemu/sysemu.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/sifive_test.h"
>
> @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>              exit(code);
>          case FINISHER_PASS:
>              exit(0);
> +        case FINISHER_RESET:
> +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            return;
>          default:
>              break;
>          }
> diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> index 71d4c9f..c186a31 100644
> --- a/include/hw/riscv/sifive_test.h
> +++ b/include/hw/riscv/sifive_test.h
> @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
>
>  enum {
>      FINISHER_FAIL = 0x3333,
> -    FINISHER_PASS = 0x5555
> +    FINISHER_PASS = 0x5555,
> +    FINISHER_RESET = 0x7777

Do you mind sharing where you got this value from? I can't find
details on this device in the SiFive manuals.

Alistair

>  };
>
>  DeviceState *sifive_test_create(hwaddr addr);
> --
> 2.7.4
>
>

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

Hi Alistair,

On Tue, Jun 18, 2019 at 1:15 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Jun 14, 2019 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > This adds a reset opcode for sifive_test device to trigger a system
> > reset for testing purpose.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  hw/riscv/sifive_test.c         | 4 ++++
> >  include/hw/riscv/sifive_test.h | 3 ++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> > index 24a04d7..cd86831 100644
> > --- a/hw/riscv/sifive_test.c
> > +++ b/hw/riscv/sifive_test.c
> > @@ -21,6 +21,7 @@
> >  #include "qemu/osdep.h"
> >  #include "hw/sysbus.h"
> >  #include "qemu/module.h"
> > +#include "sysemu/sysemu.h"
> >  #include "target/riscv/cpu.h"
> >  #include "hw/riscv/sifive_test.h"
> >
> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
> >              exit(code);
> >          case FINISHER_PASS:
> >              exit(0);
> > +        case FINISHER_RESET:
> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > +            return;
> >          default:
> >              break;
> >          }
> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> > index 71d4c9f..c186a31 100644
> > --- a/include/hw/riscv/sifive_test.h
> > +++ b/include/hw/riscv/sifive_test.h
> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
> >
> >  enum {
> >      FINISHER_FAIL = 0x3333,
> > -    FINISHER_PASS = 0x5555
> > +    FINISHER_PASS = 0x5555,
> > +    FINISHER_RESET = 0x7777
>
> Do you mind sharing where you got this value from? I can't find
> details on this device in the SiFive manuals.
>

I don't think this is a device that actually exists on SiFive's
chipset. It's hypothetical.

Regards,
Bin

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Palmer Dabbelt]

On Wed, 19 Jun 2019 06:42:21 PDT (-0700), bmeng.cn@gmail.com wrote:
> Hi Alistair,
>
> On Tue, Jun 18, 2019 at 1:15 AM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Fri, Jun 14, 2019 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>> >
>> > This adds a reset opcode for sifive_test device to trigger a system
>> > reset for testing purpose.
>> >
>> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> > ---
>> >
>> >  hw/riscv/sifive_test.c         | 4 ++++
>> >  include/hw/riscv/sifive_test.h | 3 ++-
>> >  2 files changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
>> > index 24a04d7..cd86831 100644
>> > --- a/hw/riscv/sifive_test.c
>> > +++ b/hw/riscv/sifive_test.c
>> > @@ -21,6 +21,7 @@
>> >  #include "qemu/osdep.h"
>> >  #include "hw/sysbus.h"
>> >  #include "qemu/module.h"
>> > +#include "sysemu/sysemu.h"
>> >  #include "target/riscv/cpu.h"
>> >  #include "hw/riscv/sifive_test.h"
>> >
>> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>> >              exit(code);
>> >          case FINISHER_PASS:
>> >              exit(0);
>> > +        case FINISHER_RESET:
>> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> > +            return;
>> >          default:
>> >              break;
>> >          }
>> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
>> > index 71d4c9f..c186a31 100644
>> > --- a/include/hw/riscv/sifive_test.h
>> > +++ b/include/hw/riscv/sifive_test.h
>> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
>> >
>> >  enum {
>> >      FINISHER_FAIL = 0x3333,
>> > -    FINISHER_PASS = 0x5555
>> > +    FINISHER_PASS = 0x5555,
>> > +    FINISHER_RESET = 0x7777
>>
>> Do you mind sharing where you got this value from? I can't find
>> details on this device in the SiFive manuals.
>>
>
> I don't think this is a device that actually exists on SiFive's
> chipset. It's hypothetical.

The device actually does exist in the hardware, but that's just an
implementation quirk.  Essentially what's going on here is that the RTL
contains this device, which has a register and then a behavioral verilog block
that causes simulations to terminate.  This is how we exit from tests in RTL
simulation, and we've just gone ahead and implemented the same device in QEMU
in order to make it easy to have compatibility with those bare-metal tests.
Due to how our design flow is set up we end up with exactly the same block in
the ASIC.  The register is still there, but the behavioral code to exit
simulations doesn't do anything so it's essentially just a useless device.
Since it's useless we don't bother writing it up in the ASIC documentation, but
it should be in the RTL documentation.

I'm not opposed to extending the interface in the suggested fashion, but I
wanted to check with the hardware team first to see if they're doing anything
with the other numbers.  I'm out of the office (and somewhat backed up on code
review) until July, so it might take a bit to dig through this.

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

Hi Palmer,

On Fri, Jun 21, 2019 at 10:53 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 19 Jun 2019 06:42:21 PDT (-0700), bmeng.cn@gmail.com wrote:
> > Hi Alistair,
> >
> > On Tue, Jun 18, 2019 at 1:15 AM Alistair Francis <alistair23@gmail.com> wrote:
> >>
> >> On Fri, Jun 14, 2019 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >> >
> >> > This adds a reset opcode for sifive_test device to trigger a system
> >> > reset for testing purpose.
> >> >
> >> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >> > ---
> >> >
> >> >  hw/riscv/sifive_test.c         | 4 ++++
> >> >  include/hw/riscv/sifive_test.h | 3 ++-
> >> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> >> > index 24a04d7..cd86831 100644
> >> > --- a/hw/riscv/sifive_test.c
> >> > +++ b/hw/riscv/sifive_test.c
> >> > @@ -21,6 +21,7 @@
> >> >  #include "qemu/osdep.h"
> >> >  #include "hw/sysbus.h"
> >> >  #include "qemu/module.h"
> >> > +#include "sysemu/sysemu.h"
> >> >  #include "target/riscv/cpu.h"
> >> >  #include "hw/riscv/sifive_test.h"
> >> >
> >> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
> >> >              exit(code);
> >> >          case FINISHER_PASS:
> >> >              exit(0);
> >> > +        case FINISHER_RESET:
> >> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >> > +            return;
> >> >          default:
> >> >              break;
> >> >          }
> >> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> >> > index 71d4c9f..c186a31 100644
> >> > --- a/include/hw/riscv/sifive_test.h
> >> > +++ b/include/hw/riscv/sifive_test.h
> >> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
> >> >
> >> >  enum {
> >> >      FINISHER_FAIL = 0x3333,
> >> > -    FINISHER_PASS = 0x5555
> >> > +    FINISHER_PASS = 0x5555,
> >> > +    FINISHER_RESET = 0x7777
> >>
> >> Do you mind sharing where you got this value from? I can't find
> >> details on this device in the SiFive manuals.
> >>
> >
> > I don't think this is a device that actually exists on SiFive's
> > chipset. It's hypothetical.
>
> The device actually does exist in the hardware, but that's just an
> implementation quirk.  Essentially what's going on here is that the RTL
> contains this device, which has a register and then a behavioral verilog block
> that causes simulations to terminate.  This is how we exit from tests in RTL
> simulation, and we've just gone ahead and implemented the same device in QEMU
> in order to make it easy to have compatibility with those bare-metal tests.
> Due to how our design flow is set up we end up with exactly the same block in
> the ASIC.  The register is still there, but the behavioral code to exit
> simulations doesn't do anything so it's essentially just a useless device.
> Since it's useless we don't bother writing it up in the ASIC documentation, but
> it should be in the RTL documentation.
>
> I'm not opposed to extending the interface in the suggested fashion, but I
> wanted to check with the hardware team first to see if they're doing anything
> with the other numbers.  I'm out of the office (and somewhat backed up on code
> review) until July, so it might take a bit to dig through this.

Thanks for the clarification. The main reason of adding this
functionality is to provide software a way of rebooting the whole
system. Please provide update after you consult SiFive hardware guys
:)

Regards,
Bin

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Palmer Dabbelt]

On Thu, 20 Jun 2019 22:40:24 PDT (-0700), bmeng.cn@gmail.com wrote:
> Hi Palmer,
>
> On Fri, Jun 21, 2019 at 10:53 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 19 Jun 2019 06:42:21 PDT (-0700), bmeng.cn@gmail.com wrote:
>> > Hi Alistair,
>> >
>> > On Tue, Jun 18, 2019 at 1:15 AM Alistair Francis <alistair23@gmail.com> wrote:
>> >>
>> >> On Fri, Jun 14, 2019 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>> >> >
>> >> > This adds a reset opcode for sifive_test device to trigger a system
>> >> > reset for testing purpose.
>> >> >
>> >> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> >> > ---
>> >> >
>> >> >  hw/riscv/sifive_test.c         | 4 ++++
>> >> >  include/hw/riscv/sifive_test.h | 3 ++-
>> >> >  2 files changed, 6 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
>> >> > index 24a04d7..cd86831 100644
>> >> > --- a/hw/riscv/sifive_test.c
>> >> > +++ b/hw/riscv/sifive_test.c
>> >> > @@ -21,6 +21,7 @@
>> >> >  #include "qemu/osdep.h"
>> >> >  #include "hw/sysbus.h"
>> >> >  #include "qemu/module.h"
>> >> > +#include "sysemu/sysemu.h"
>> >> >  #include "target/riscv/cpu.h"
>> >> >  #include "hw/riscv/sifive_test.h"
>> >> >
>> >> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>> >> >              exit(code);
>> >> >          case FINISHER_PASS:
>> >> >              exit(0);
>> >> > +        case FINISHER_RESET:
>> >> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> >> > +            return;
>> >> >          default:
>> >> >              break;
>> >> >          }
>> >> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
>> >> > index 71d4c9f..c186a31 100644
>> >> > --- a/include/hw/riscv/sifive_test.h
>> >> > +++ b/include/hw/riscv/sifive_test.h
>> >> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
>> >> >
>> >> >  enum {
>> >> >      FINISHER_FAIL = 0x3333,
>> >> > -    FINISHER_PASS = 0x5555
>> >> > +    FINISHER_PASS = 0x5555,
>> >> > +    FINISHER_RESET = 0x7777
>> >>
>> >> Do you mind sharing where you got this value from? I can't find
>> >> details on this device in the SiFive manuals.
>> >>
>> >
>> > I don't think this is a device that actually exists on SiFive's
>> > chipset. It's hypothetical.
>>
>> The device actually does exist in the hardware, but that's just an
>> implementation quirk.  Essentially what's going on here is that the RTL
>> contains this device, which has a register and then a behavioral verilog block
>> that causes simulations to terminate.  This is how we exit from tests in RTL
>> simulation, and we've just gone ahead and implemented the same device in QEMU
>> in order to make it easy to have compatibility with those bare-metal tests.
>> Due to how our design flow is set up we end up with exactly the same block in
>> the ASIC.  The register is still there, but the behavioral code to exit
>> simulations doesn't do anything so it's essentially just a useless device.
>> Since it's useless we don't bother writing it up in the ASIC documentation, but
>> it should be in the RTL documentation.
>>
>> I'm not opposed to extending the interface in the suggested fashion, but I
>> wanted to check with the hardware team first to see if they're doing anything
>> with the other numbers.  I'm out of the office (and somewhat backed up on code
>> review) until July, so it might take a bit to dig through this.
>
> Thanks for the clarification. The main reason of adding this
> functionality is to provide software a way of rebooting the whole
> system. Please provide update after you consult SiFive hardware guys
> :)

Ya, it makes sense.  My only worry here is that we have some way of doing this
already, in which case I'd just want to make sure it matches.

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

Hi Palmer,

On Sun, Jun 23, 2019 at 10:40 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Thu, 20 Jun 2019 22:40:24 PDT (-0700), bmeng.cn@gmail.com wrote:
> > Hi Palmer,
> >
> > On Fri, Jun 21, 2019 at 10:53 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Wed, 19 Jun 2019 06:42:21 PDT (-0700), bmeng.cn@gmail.com wrote:
> >> > Hi Alistair,
> >> >
> >> > On Tue, Jun 18, 2019 at 1:15 AM Alistair Francis <alistair23@gmail.com> wrote:
> >> >>
> >> >> On Fri, Jun 14, 2019 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >> >> >
> >> >> > This adds a reset opcode for sifive_test device to trigger a system
> >> >> > reset for testing purpose.
> >> >> >
> >> >> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >> >> > ---
> >> >> >
> >> >> >  hw/riscv/sifive_test.c         | 4 ++++
> >> >> >  include/hw/riscv/sifive_test.h | 3 ++-
> >> >> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> >> >> > index 24a04d7..cd86831 100644
> >> >> > --- a/hw/riscv/sifive_test.c
> >> >> > +++ b/hw/riscv/sifive_test.c
> >> >> > @@ -21,6 +21,7 @@
> >> >> >  #include "qemu/osdep.h"
> >> >> >  #include "hw/sysbus.h"
> >> >> >  #include "qemu/module.h"
> >> >> > +#include "sysemu/sysemu.h"
> >> >> >  #include "target/riscv/cpu.h"
> >> >> >  #include "hw/riscv/sifive_test.h"
> >> >> >
> >> >> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
> >> >> >              exit(code);
> >> >> >          case FINISHER_PASS:
> >> >> >              exit(0);
> >> >> > +        case FINISHER_RESET:
> >> >> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >> >> > +            return;
> >> >> >          default:
> >> >> >              break;
> >> >> >          }
> >> >> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> >> >> > index 71d4c9f..c186a31 100644
> >> >> > --- a/include/hw/riscv/sifive_test.h
> >> >> > +++ b/include/hw/riscv/sifive_test.h
> >> >> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
> >> >> >
> >> >> >  enum {
> >> >> >      FINISHER_FAIL = 0x3333,
> >> >> > -    FINISHER_PASS = 0x5555
> >> >> > +    FINISHER_PASS = 0x5555,
> >> >> > +    FINISHER_RESET = 0x7777
> >> >>
> >> >> Do you mind sharing where you got this value from? I can't find
> >> >> details on this device in the SiFive manuals.
> >> >>
> >> >
> >> > I don't think this is a device that actually exists on SiFive's
> >> > chipset. It's hypothetical.
> >>
> >> The device actually does exist in the hardware, but that's just an
> >> implementation quirk.  Essentially what's going on here is that the RTL
> >> contains this device, which has a register and then a behavioral verilog block
> >> that causes simulations to terminate.  This is how we exit from tests in RTL
> >> simulation, and we've just gone ahead and implemented the same device in QEMU
> >> in order to make it easy to have compatibility with those bare-metal tests.
> >> Due to how our design flow is set up we end up with exactly the same block in
> >> the ASIC.  The register is still there, but the behavioral code to exit
> >> simulations doesn't do anything so it's essentially just a useless device.
> >> Since it's useless we don't bother writing it up in the ASIC documentation, but
> >> it should be in the RTL documentation.
> >>
> >> I'm not opposed to extending the interface in the suggested fashion, but I
> >> wanted to check with the hardware team first to see if they're doing anything
> >> with the other numbers.  I'm out of the office (and somewhat backed up on code
> >> review) until July, so it might take a bit to dig through this.
> >
> > Thanks for the clarification. The main reason of adding this
> > functionality is to provide software a way of rebooting the whole
> > system. Please provide update after you consult SiFive hardware guys
> > :)
>
> Ya, it makes sense.  My only worry here is that we have some way of doing this
> already, in which case I'd just want to make sure it matches.

If the SiFive chipset already has the functionality to do the reset
via this test module, I agree let's align the number to what hardware
actually accepts.

Regards,
Bin

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Palmer Dabbelt]

On Fri, 14 Jun 2019 08:15:51 PDT (-0700), bmeng.cn@gmail.com wrote:
> This adds a reset opcode for sifive_test device to trigger a system
> reset for testing purpose.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  hw/riscv/sifive_test.c         | 4 ++++
>  include/hw/riscv/sifive_test.h | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> index 24a04d7..cd86831 100644
> --- a/hw/riscv/sifive_test.c
> +++ b/hw/riscv/sifive_test.c
> @@ -21,6 +21,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "qemu/module.h"
> +#include "sysemu/sysemu.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/sifive_test.h"
>
> @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>              exit(code);
>          case FINISHER_PASS:
>              exit(0);
> +        case FINISHER_RESET:
> +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            return;
>          default:
>              break;
>          }
> diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> index 71d4c9f..c186a31 100644
> --- a/include/hw/riscv/sifive_test.h
> +++ b/include/hw/riscv/sifive_test.h
> @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
>
>  enum {
>      FINISHER_FAIL = 0x3333,
> -    FINISHER_PASS = 0x5555
> +    FINISHER_PASS = 0x5555,
> +    FINISHER_RESET = 0x7777
>  };
>
>  DeviceState *sifive_test_create(hwaddr addr);

Poking around the hardware side, it looks like we're silently ignoring all
codes aside from 0x3333 and 0x5555.  As a result there's really no way to test
if this added reset is going to work, so this should be a "sifive,test1"
instead of a "sifive,test0".  It's backwards compatible, so the compat string
in the device trees should look something like

    compatible = "sifive,test1", "sifive,test0";

With that it should be fine, pending the change to our hardware to reserve this
as an error code within "sifive,test0" devices.

Do you mind also submitting a device tree binding to Linux that describes this
device?  That way we can all stay on the same page about what the pair of
interfaces do.

Thanks!

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

On Tue, Jul 9, 2019 at 5:48 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Fri, 14 Jun 2019 08:15:51 PDT (-0700), bmeng.cn@gmail.com wrote:
> > This adds a reset opcode for sifive_test device to trigger a system
> > reset for testing purpose.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  hw/riscv/sifive_test.c         | 4 ++++
> >  include/hw/riscv/sifive_test.h | 3 ++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> > index 24a04d7..cd86831 100644
> > --- a/hw/riscv/sifive_test.c
> > +++ b/hw/riscv/sifive_test.c
> > @@ -21,6 +21,7 @@
> >  #include "qemu/osdep.h"
> >  #include "hw/sysbus.h"
> >  #include "qemu/module.h"
> > +#include "sysemu/sysemu.h"
> >  #include "target/riscv/cpu.h"
> >  #include "hw/riscv/sifive_test.h"
> >
> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
> >              exit(code);
> >          case FINISHER_PASS:
> >              exit(0);
> > +        case FINISHER_RESET:
> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > +            return;
> >          default:
> >              break;
> >          }
> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> > index 71d4c9f..c186a31 100644
> > --- a/include/hw/riscv/sifive_test.h
> > +++ b/include/hw/riscv/sifive_test.h
> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
> >
> >  enum {
> >      FINISHER_FAIL = 0x3333,
> > -    FINISHER_PASS = 0x5555
> > +    FINISHER_PASS = 0x5555,
> > +    FINISHER_RESET = 0x7777
> >  };
> >
> >  DeviceState *sifive_test_create(hwaddr addr);
>
> Poking around the hardware side, it looks like we're silently ignoring all
> codes aside from 0x3333 and 0x5555.  As a result there's really no way to test
> if this added reset is going to work, so this should be a "sifive,test1"
> instead of a "sifive,test0".  It's backwards compatible, so the compat string
> in the device trees should look something like
>
>     compatible = "sifive,test1", "sifive,test0";
>
> With that it should be fine, pending the change to our hardware to reserve this
> as an error code within "sifive,test0" devices.
>
> Do you mind also submitting a device tree binding to Linux that describes this
> device?  That way we can all stay on the same page about what the pair of
> interfaces do.

Thanks for the confirmation. I will try to create a device tree
binding for Linux.

Regards,
Bin

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Palmer Dabbelt]

On Fri, 14 Jun 2019 08:15:51 PDT (-0700), bmeng.cn@gmail.com wrote:
> This adds a reset opcode for sifive_test device to trigger a system
> reset for testing purpose.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  hw/riscv/sifive_test.c         | 4 ++++
>  include/hw/riscv/sifive_test.h | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> index 24a04d7..cd86831 100644
> --- a/hw/riscv/sifive_test.c
> +++ b/hw/riscv/sifive_test.c
> @@ -21,6 +21,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "qemu/module.h"
> +#include "sysemu/sysemu.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/sifive_test.h"
>
> @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>              exit(code);
>          case FINISHER_PASS:
>              exit(0);
> +        case FINISHER_RESET:
> +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            return;
>          default:
>              break;
>          }
> diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> index 71d4c9f..c186a31 100644
> --- a/include/hw/riscv/sifive_test.h
> +++ b/include/hw/riscv/sifive_test.h
> @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
>
>  enum {
>      FINISHER_FAIL = 0x3333,
> -    FINISHER_PASS = 0x5555
> +    FINISHER_PASS = 0x5555,
> +    FINISHER_RESET = 0x7777
>  };
>
>  DeviceState *sifive_test_create(hwaddr addr);

Reviewed-by: Palmer Dabbelt <palmer@sifive.com>

Sorry this took a while, but it's in the hardware now.  I'll merge this, but
I'm considering it a new feature so it'll be held off a bit.

Thanks!

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

Hi Palmer,

On Sat, Jul 20, 2019 at 9:47 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Fri, 14 Jun 2019 08:15:51 PDT (-0700), bmeng.cn@gmail.com wrote:
> > This adds a reset opcode for sifive_test device to trigger a system
> > reset for testing purpose.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >
> >  hw/riscv/sifive_test.c         | 4 ++++
> >  include/hw/riscv/sifive_test.h | 3 ++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> > index 24a04d7..cd86831 100644
> > --- a/hw/riscv/sifive_test.c
> > +++ b/hw/riscv/sifive_test.c
> > @@ -21,6 +21,7 @@
> >  #include "qemu/osdep.h"
> >  #include "hw/sysbus.h"
> >  #include "qemu/module.h"
> > +#include "sysemu/sysemu.h"
> >  #include "target/riscv/cpu.h"
> >  #include "hw/riscv/sifive_test.h"
> >
> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
> >              exit(code);
> >          case FINISHER_PASS:
> >              exit(0);
> > +        case FINISHER_RESET:
> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > +            return;
> >          default:
> >              break;
> >          }
> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> > index 71d4c9f..c186a31 100644
> > --- a/include/hw/riscv/sifive_test.h
> > +++ b/include/hw/riscv/sifive_test.h
> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
> >
> >  enum {
> >      FINISHER_FAIL = 0x3333,
> > -    FINISHER_PASS = 0x5555
> > +    FINISHER_PASS = 0x5555,
> > +    FINISHER_RESET = 0x7777
> >  };
> >
> >  DeviceState *sifive_test_create(hwaddr addr);
>
> Reviewed-by: Palmer Dabbelt <palmer@sifive.com>

Thanks a lot!

> Sorry this took a while, but it's in the hardware now.  I'll merge this, but
> I'm considering it a new feature so it'll be held off a bit.

"but it's in the hardware now", do you mean the code I added (0x7777)
is now supported by a newer version SiFive test device with compatible
string "sifive,test1", and can actually do the system wide reset?

Regards,
Bin

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Palmer Dabbelt]

On Mon, 22 Jul 2019 22:30:15 PDT (-0700), bmeng.cn@gmail.com wrote:
> Hi Palmer,
>
> On Sat, Jul 20, 2019 at 9:47 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Fri, 14 Jun 2019 08:15:51 PDT (-0700), bmeng.cn@gmail.com wrote:
>> > This adds a reset opcode for sifive_test device to trigger a system
>> > reset for testing purpose.
>> >
>> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> > ---
>> >
>> >  hw/riscv/sifive_test.c         | 4 ++++
>> >  include/hw/riscv/sifive_test.h | 3 ++-
>> >  2 files changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
>> > index 24a04d7..cd86831 100644
>> > --- a/hw/riscv/sifive_test.c
>> > +++ b/hw/riscv/sifive_test.c
>> > @@ -21,6 +21,7 @@
>> >  #include "qemu/osdep.h"
>> >  #include "hw/sysbus.h"
>> >  #include "qemu/module.h"
>> > +#include "sysemu/sysemu.h"
>> >  #include "target/riscv/cpu.h"
>> >  #include "hw/riscv/sifive_test.h"
>> >
>> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
>> >              exit(code);
>> >          case FINISHER_PASS:
>> >              exit(0);
>> > +        case FINISHER_RESET:
>> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> > +            return;
>> >          default:
>> >              break;
>> >          }
>> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
>> > index 71d4c9f..c186a31 100644
>> > --- a/include/hw/riscv/sifive_test.h
>> > +++ b/include/hw/riscv/sifive_test.h
>> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
>> >
>> >  enum {
>> >      FINISHER_FAIL = 0x3333,
>> > -    FINISHER_PASS = 0x5555
>> > +    FINISHER_PASS = 0x5555,
>> > +    FINISHER_RESET = 0x7777
>> >  };
>> >
>> >  DeviceState *sifive_test_create(hwaddr addr);
>>
>> Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
>
> Thanks a lot!
>
>> Sorry this took a while, but it's in the hardware now.  I'll merge this, but
>> I'm considering it a new feature so it'll be held off a bit.
>
> "but it's in the hardware now", do you mean the code I added (0x7777)
> is now supported by a newer version SiFive test device with compatible
> string "sifive,test1", and can actually do the system wide reset?

No, the hardware is still a "sifive,test0" as plumbing through the reset is
trickier than I wanted to take on.  I just reserved the 0x7777 code and
implemented it by triggering an unsupported function error, so we don't
accidentally use it for something else later.

Re: [Qemu-devel] [PATCH] riscv: sifive_test: Add reset functionality

[Bin Meng]

On Tue, Jul 30, 2019 at 3:47 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Mon, 22 Jul 2019 22:30:15 PDT (-0700), bmeng.cn@gmail.com wrote:
> > Hi Palmer,
> >
> > On Sat, Jul 20, 2019 at 9:47 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Fri, 14 Jun 2019 08:15:51 PDT (-0700), bmeng.cn@gmail.com wrote:
> >> > This adds a reset opcode for sifive_test device to trigger a system
> >> > reset for testing purpose.
> >> >
> >> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >> > ---
> >> >
> >> >  hw/riscv/sifive_test.c         | 4 ++++
> >> >  include/hw/riscv/sifive_test.h | 3 ++-
> >> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> >> > index 24a04d7..cd86831 100644
> >> > --- a/hw/riscv/sifive_test.c
> >> > +++ b/hw/riscv/sifive_test.c
> >> > @@ -21,6 +21,7 @@
> >> >  #include "qemu/osdep.h"
> >> >  #include "hw/sysbus.h"
> >> >  #include "qemu/module.h"
> >> > +#include "sysemu/sysemu.h"
> >> >  #include "target/riscv/cpu.h"
> >> >  #include "hw/riscv/sifive_test.h"
> >> >
> >> > @@ -40,6 +41,9 @@ static void sifive_test_write(void *opaque, hwaddr addr,
> >> >              exit(code);
> >> >          case FINISHER_PASS:
> >> >              exit(0);
> >> > +        case FINISHER_RESET:
> >> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >> > +            return;
> >> >          default:
> >> >              break;
> >> >          }
> >> > diff --git a/include/hw/riscv/sifive_test.h b/include/hw/riscv/sifive_test.h
> >> > index 71d4c9f..c186a31 100644
> >> > --- a/include/hw/riscv/sifive_test.h
> >> > +++ b/include/hw/riscv/sifive_test.h
> >> > @@ -34,7 +34,8 @@ typedef struct SiFiveTestState {
> >> >
> >> >  enum {
> >> >      FINISHER_FAIL = 0x3333,
> >> > -    FINISHER_PASS = 0x5555
> >> > +    FINISHER_PASS = 0x5555,
> >> > +    FINISHER_RESET = 0x7777
> >> >  };
> >> >
> >> >  DeviceState *sifive_test_create(hwaddr addr);
> >>
> >> Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
> >
> > Thanks a lot!
> >
> >> Sorry this took a while, but it's in the hardware now.  I'll merge this, but
> >> I'm considering it a new feature so it'll be held off a bit.
> >
> > "but it's in the hardware now", do you mean the code I added (0x7777)
> > is now supported by a newer version SiFive test device with compatible
> > string "sifive,test1", and can actually do the system wide reset?
>
> No, the hardware is still a "sifive,test0" as plumbing through the reset is
> trickier than I wanted to take on.  I just reserved the 0x7777 code and
> implemented it by triggering an unsupported function error, so we don't
> accidentally use it for something else later.

OK, thanks for the clarifications!

Regards,
Bin