[PATCH for-5.1? 0/3] Fix AIRCR.SYSRESETREQ for most M-profile boards

[PATCH for-5.1? 0/3] Fix AIRCR.SYSRESETREQ for most M-profile boards

[Peter Maydell]

QEMU's NVIC device provides an outbound qemu_irq "SYSRESETREQ" which
it signals when the guest sets the SYSRESETREQ bit in the AIRCR
register.  This matches the hardware design (where the CPU has a
signal of this name and it is up to the SoC to connect that up to an
actual reset mechanism), but in QEMU it mostly results in duplicated
code in SoC objects and bugs where SoC model implementors forget to
wire up the SYSRESETREQ line.

This patchseries provides a default behaviour for the case where
SYSRESETREQ is not actually connected to anything: use
qemu_system_reset_request() to perform a system reset. This is a
much more plausible default than "do nothing". It allows us to
allow us to remove the implementations of SYSRESETREQ handling from
the boards which currently hook up a reset function that just
does that (stellaris, emcraft-sf2), and also fixes the bugs
in these board models which forgot to wire up the signal:

 * microbit
 * mps2-an385
 * mps2-an505
 * mps2-an511
 * mps2-an521
 * musca-a
 * musca-b1
 * netduino
 * netduinoplus2

[I admit that I have not specifically checked for datasheets
and errata notes for all these boards to confirm that AIRCR.SYSRESETREQ
really does reset the system or to look for more complex
reset-control logic... As an example, though, the SSE-200 used in
the mps2-an521 and the musca boards has a RESET_MASK register in the
system control block that allows a guest to suppress reset requests from
one or both CPUs. Since we don't model that register, "always reset"
is still closer to reasonable behaviour than "do nothing".]

We still allow the board to wire up the signal if it needs to, in case
we need to model more complicated reset controller logic (eg if we
wanted to get that SSE-200 corner case right in future) or to model
buggy SoC hardware which forgot to wire up the line itself. But
defaulting to "reset the system" is going to be correct much more
often than "do nothing".

Patch 1 introduces a new API for "check whether my qemu_irq is
actually connected to anything" (the test is trivial but the
encapsulation seems worthwhile); patch 2 provides the new default
and patch 3 removes the now-unnecessary SYSRESETREQ handlers in
board/SoC code.

thanks
-- PMM

Peter Maydell (3):
  include/hw/irq.h: New function qemu_irq_is_connected()
  hw/intc/armv7m_nvic: Provide default "reset the system" behaviour for
    SYSRESETREQ
  msf2-soc, stellaris: Don't wire up SYSRESETREQ

 include/hw/arm/armv7m.h |  4 +++-
 include/hw/irq.h        | 18 ++++++++++++++++++
 hw/arm/msf2-soc.c       | 11 -----------
 hw/arm/stellaris.c      | 12 ------------
 hw/intc/armv7m_nvic.c   | 17 ++++++++++++++++-
 5 files changed, 37 insertions(+), 25 deletions(-)

-- 
2.20.1

[PATCH for-5.1? 1/3] include/hw/irq.h: New function qemu_irq_is_connected()

[Peter Maydell]

Mostly devices don't need to care whether one of their output
qemu_irq lines is connected, because functions like qemu_set_irq()
silently do nothing if there is nothing on the other end.  However
sometimes a device might want to implement default behaviour for the
case where the machine hasn't wired the line up to anywhere.

Provide a function qemu_irq_is_connected() that devices can use for
this purpose.  (The test is trivial but encapsulating it in a
function makes it easier to see where we're doing it in case we need
to change the implementation later.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/irq.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/hw/irq.h b/include/hw/irq.h
index 24ba0ece116..dc7abf199e3 100644
--- a/include/hw/irq.h
+++ b/include/hw/irq.h
@@ -55,4 +55,22 @@ qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2);
    on an existing vector of qemu_irq.  */
 void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n);
 
+/**
+ * qemu_irq_is_connected: Return true if IRQ line is wired up
+ *
+ * If a qemu_irq has a device on the other (receiving) end of it,
+ * return true; otherwise return false.
+ *
+ * Usually device models don't need to care whether the machine model
+ * has wired up their outbound qemu_irq lines, because functions like
+ * qemu_set_irq() silently do nothing if there is nothing on the other
+ * end of the line. However occasionally a device model will want to
+ * provide default behaviour if its output is left floating, and
+ * it can use this function to identify when that is the case.
+ */
+static inline bool qemu_irq_is_connected(qemu_irq irq)
+{
+    return irq != NULL;
+}
+
 #endif
-- 
2.20.1

[PATCH for-5.1? 2/3] hw/intc/armv7m_nvic: Provide default "reset the system" behaviour for SYSRESETREQ

[Peter Maydell]

The NVIC provides an outbound qemu_irq "SYSRESETREQ" which it signals
when the guest sets the SYSRESETREQ bit in the AIRCR register.  This
matches the hardware design (where the CPU has a signal of this name
and it is up to the SoC to connect that up to an actual reset
mechanism), but in QEMU it mostly results in duplicated code in SoC
objects and bugs where SoC model implementors forget to wire up the
SYSRESETREQ line.

Provide a default behaviour for the case where SYSRESETREQ is not
actually connected to anything: use qemu_system_reset_request() to
perform a system reset.  This will allow us to remove the
implementations of SYSRESETREQ handling from the boards where that's
exactly what it does, and also fixes the bugs in the board models
which forgot to wire up the signal:

 * microbit
 * mps2-an385
 * mps2-an505
 * mps2-an511
 * mps2-an521
 * musca-a
 * musca-b1
 * netduino
 * netduinoplus2

We still allow the board to wire up the signal if it needs to, in case
we need to model more complicated reset controller logic or to model
buggy SoC hardware which forgot to wire up the line itself. But
defaulting to "reset the system" is more often going to be correct
than defaulting to "do nothing".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/armv7m.h |  4 +++-
 hw/intc/armv7m_nvic.c   | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
index d2c74d3872a..a30e3c64715 100644
--- a/include/hw/arm/armv7m.h
+++ b/include/hw/arm/armv7m.h
@@ -35,7 +35,9 @@ typedef struct {
 
 /* ARMv7M container object.
  * + Unnamed GPIO input lines: external IRQ lines for the NVIC
- * + Named GPIO output SYSRESETREQ: signalled for guest AIRCR.SYSRESETREQ
+ * + Named GPIO output SYSRESETREQ: signalled for guest AIRCR.SYSRESETREQ.
+ *   If this GPIO is not wired up then the NVIC will default to performing
+ *   a qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET).
  * + Property "cpu-type": CPU type to instantiate
  * + Property "num-irq": number of external IRQ lines
  * + Property "memory": MemoryRegion defining the physical address space
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 3c4b6e6d701..277a98b87b9 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -19,6 +19,7 @@
 #include "hw/intc/armv7m_nvic.h"
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
+#include "sysemu/runstate.h"
 #include "target/arm/cpu.h"
 #include "exec/exec-all.h"
 #include "exec/memop.h"
@@ -64,6 +65,20 @@ static const uint8_t nvic_id[] = {
     0x00, 0xb0, 0x1b, 0x00, 0x0d, 0xe0, 0x05, 0xb1
 };
 
+static void signal_sysresetreq(NVICState *s)
+{
+    if (qemu_irq_is_connected(s->sysresetreq)) {
+        qemu_irq_pulse(s->sysresetreq);
+    } else {
+        /*
+         * Default behaviour if the SoC doesn't need to wire up
+         * SYSRESETREQ (eg to a system reset controller of some kind):
+         * perform a system reset via the usual QEMU API.
+         */
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+    }
+}
+
 static int nvic_pending_prio(NVICState *s)
 {
     /* return the group priority of the current pending interrupt,
@@ -1524,7 +1539,7 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
             if (value & R_V7M_AIRCR_SYSRESETREQ_MASK) {
                 if (attrs.secure ||
                     !(cpu->env.v7m.aircr & R_V7M_AIRCR_SYSRESETREQS_MASK)) {
-                    qemu_irq_pulse(s->sysresetreq);
+                    signal_sysresetreq(s);
                 }
             }
             if (value & R_V7M_AIRCR_VECTCLRACTIVE_MASK) {
-- 
2.20.1

[PATCH for-5.1? 3/3] msf2-soc, stellaris: Don't wire up SYSRESETREQ

[Peter Maydell]

The MSF2 SoC model and the Stellaris board code both wire
SYSRESETREQ up to a function that just invokes
    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
This is now the default action that the NVIC does if the line is
not connected, so we can delete the handling code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/msf2-soc.c  | 11 -----------
 hw/arm/stellaris.c | 12 ------------
 2 files changed, 23 deletions(-)

diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index 33ea7df342c..d2c29e82d13 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -30,7 +30,6 @@
 #include "hw/irq.h"
 #include "hw/arm/msf2-soc.h"
 #include "hw/misc/unimp.h"
-#include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
 
 #define MSF2_TIMER_BASE       0x40004000
@@ -59,13 +58,6 @@ static const int spi_irq[MSF2_NUM_SPIS] = { 2, 3 };
 static const int uart_irq[MSF2_NUM_UARTS] = { 10, 11 };
 static const int timer_irq[MSF2_NUM_TIMERS] = { 14, 15 };
 
-static void do_sys_reset(void *opaque, int n, int level)
-{
-    if (level) {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
-    }
-}
-
 static void m2sxxx_soc_initfn(Object *obj)
 {
     MSF2State *s = MSF2_SOC(obj);
@@ -130,9 +122,6 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
         return;
     }
 
-    qdev_connect_gpio_out_named(DEVICE(&s->armv7m.nvic), "SYSRESETREQ", 0,
-                                qemu_allocate_irq(&do_sys_reset, NULL, 0));
-
     system_clock_scale = NANOSECONDS_PER_SECOND / s->m3clk;
 
     for (i = 0; i < MSF2_NUM_UARTS; i++) {
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 28eb15c76ca..5f9d0801807 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -18,7 +18,6 @@
 #include "hw/boards.h"
 #include "qemu/log.h"
 #include "exec/address-spaces.h"
-#include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
 #include "hw/arm/armv7m.h"
 #include "hw/char/pl011.h"
@@ -1206,14 +1205,6 @@ static void stellaris_adc_init(Object *obj)
     qdev_init_gpio_in(dev, stellaris_adc_trigger, 1);
 }
 
-static
-void do_sys_reset(void *opaque, int n, int level)
-{
-    if (level) {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
-    }
-}
-
 /* Board init.  */
 static stellaris_board_info stellaris_boards[] = {
   { "LM3S811EVB",
@@ -1317,9 +1308,6 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
     /* This will exit with an error if the user passed us a bad cpu_type */
     sysbus_realize_and_unref(SYS_BUS_DEVICE(nvic), &error_fatal);
 
-    qdev_connect_gpio_out_named(nvic, "SYSRESETREQ", 0,
-                                qemu_allocate_irq(&do_sys_reset, NULL, 0));
-
     if (board->dc1 & (1 << 16)) {
         dev = sysbus_create_varargs(TYPE_STELLARIS_ADC, 0x40038000,
                                     qdev_get_gpio_in(nvic, 14),
-- 
2.20.1

Re: [PATCH for-5.1? 0/3] Fix AIRCR.SYSRESETREQ for most M-profile boards

[Philippe Mathieu-Daudé]

On 7/28/20 12:37 PM, Peter Maydell wrote:
> QEMU's NVIC device provides an outbound qemu_irq "SYSRESETREQ" which
> it signals when the guest sets the SYSRESETREQ bit in the AIRCR
> register.  This matches the hardware design (where the CPU has a
> signal of this name and it is up to the SoC to connect that up to an
> actual reset mechanism), but in QEMU it mostly results in duplicated
> code in SoC objects and bugs where SoC model implementors forget to
> wire up the SYSRESETREQ line.
> 
> This patchseries provides a default behaviour for the case where
> SYSRESETREQ is not actually connected to anything: use
> qemu_system_reset_request() to perform a system reset. This is a
> much more plausible default than "do nothing". It allows us to
> allow us to remove the implementations of SYSRESETREQ handling from
> the boards which currently hook up a reset function that just
> does that (stellaris, emcraft-sf2), and also fixes the bugs
> in these board models which forgot to wire up the signal:
> 
>  * microbit
>  * mps2-an385
>  * mps2-an505
>  * mps2-an511
>  * mps2-an521
>  * musca-a
>  * musca-b1
>  * netduino
>  * netduinoplus2
> 
> [I admit that I have not specifically checked for datasheets
> and errata notes for all these boards to confirm that AIRCR.SYSRESETREQ
> really does reset the system or to look for more complex
> reset-control logic... As an example, though, the SSE-200 used in
> the mps2-an521 and the musca boards has a RESET_MASK register in the
> system control block that allows a guest to suppress reset requests from
> one or both CPUs. Since we don't model that register, "always reset"
> is still closer to reasonable behaviour than "do nothing".]
> 
> We still allow the board to wire up the signal if it needs to, in case
> we need to model more complicated reset controller logic (eg if we
> wanted to get that SSE-200 corner case right in future) or to model
> buggy SoC hardware which forgot to wire up the line itself. But
> defaulting to "reset the system" is going to be correct much more
> often than "do nothing".
> 
> Patch 1 introduces a new API for "check whether my qemu_irq is
> actually connected to anything" (the test is trivial but the
> encapsulation seems worthwhile); patch 2 provides the new default
> and patch 3 removes the now-unnecessary SYSRESETREQ handlers in
> board/SoC code.

Good cleanup!

Series:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH for-5.1? 0/3] Fix AIRCR.SYSRESETREQ for most M-profile boards

[Alistair Francis]

On Tue, Jul 28, 2020 at 3:38 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> QEMU's NVIC device provides an outbound qemu_irq "SYSRESETREQ" which
> it signals when the guest sets the SYSRESETREQ bit in the AIRCR
> register.  This matches the hardware design (where the CPU has a
> signal of this name and it is up to the SoC to connect that up to an
> actual reset mechanism), but in QEMU it mostly results in duplicated
> code in SoC objects and bugs where SoC model implementors forget to
> wire up the SYSRESETREQ line.
>
> This patchseries provides a default behaviour for the case where
> SYSRESETREQ is not actually connected to anything: use
> qemu_system_reset_request() to perform a system reset. This is a
> much more plausible default than "do nothing". It allows us to
> allow us to remove the implementations of SYSRESETREQ handling from
> the boards which currently hook up a reset function that just
> does that (stellaris, emcraft-sf2), and also fixes the bugs
> in these board models which forgot to wire up the signal:
>
>  * microbit
>  * mps2-an385
>  * mps2-an505
>  * mps2-an511
>  * mps2-an521
>  * musca-a
>  * musca-b1
>  * netduino
>  * netduinoplus2
>
> [I admit that I have not specifically checked for datasheets
> and errata notes for all these boards to confirm that AIRCR.SYSRESETREQ
> really does reset the system or to look for more complex
> reset-control logic... As an example, though, the SSE-200 used in
> the mps2-an521 and the musca boards has a RESET_MASK register in the
> system control block that allows a guest to suppress reset requests from
> one or both CPUs. Since we don't model that register, "always reset"
> is still closer to reasonable behaviour than "do nothing".]
>
> We still allow the board to wire up the signal if it needs to, in case
> we need to model more complicated reset controller logic (eg if we
> wanted to get that SSE-200 corner case right in future) or to model
> buggy SoC hardware which forgot to wire up the line itself. But
> defaulting to "reset the system" is going to be correct much more
> often than "do nothing".
>
> Patch 1 introduces a new API for "check whether my qemu_irq is
> actually connected to anything" (the test is trivial but the
> encapsulation seems worthwhile); patch 2 provides the new default
> and patch 3 removes the now-unnecessary SYSRESETREQ handlers in
> board/SoC code.

I checked the STM Reference Manual and this matches what they expect.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> thanks
> -- PMM
>
> Peter Maydell (3):
>   include/hw/irq.h: New function qemu_irq_is_connected()
>   hw/intc/armv7m_nvic: Provide default "reset the system" behaviour for
>     SYSRESETREQ
>   msf2-soc, stellaris: Don't wire up SYSRESETREQ
>
>  include/hw/arm/armv7m.h |  4 +++-
>  include/hw/irq.h        | 18 ++++++++++++++++++
>  hw/arm/msf2-soc.c       | 11 -----------
>  hw/arm/stellaris.c      | 12 ------------
>  hw/intc/armv7m_nvic.c   | 17 ++++++++++++++++-
>  5 files changed, 37 insertions(+), 25 deletions(-)
>
> --
> 2.20.1
>
>