[PATCH v2 1/2] hw/core: Support device reset handler priority

[PATCH v2 1/2] hw/core: Support device reset handler priority

[Stephanos Ioannidis]

The device reset handler invocation order is currently dependent on
the order of handler registration, and this is less than ideal because
there may exist dependencies among the handlers that require them to
be invoked in a specific order.

This commit adds the `priority` field to the reset entry struct and
introduces the `qemu_register_reset_with_priority` function that
implements descending-order insertion into the reset handler list based
on the priority value, in order to allow the reset handler invocation
order to be specified by the caller.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
---
 hw/core/reset.c        | 32 ++++++++++++++++++++++++++++++--
 include/sysemu/reset.h | 24 ++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/hw/core/reset.c b/hw/core/reset.c
index 9c477f2bf5..74f3677d96 100644
--- a/hw/core/reset.c
+++ b/hw/core/reset.c
@@ -31,6 +31,7 @@
 
 typedef struct QEMUResetEntry {
     QTAILQ_ENTRY(QEMUResetEntry) entry;
+    uint16_t priority;
     QEMUResetHandler *func;
     void *opaque;
 } QEMUResetEntry;
@@ -38,13 +39,40 @@ typedef struct QEMUResetEntry {
 static QTAILQ_HEAD(, QEMUResetEntry) reset_handlers =
     QTAILQ_HEAD_INITIALIZER(reset_handlers);
 
-void qemu_register_reset(QEMUResetHandler *func, void *opaque)
+void qemu_register_reset_with_priority(uint16_t priority,
+    QEMUResetHandler *func, void *opaque)
 {
+    /* Initialise reset entry */
     QEMUResetEntry *re = g_malloc0(sizeof(QEMUResetEntry));
 
+    re->priority = priority;
     re->func = func;
     re->opaque = opaque;
-    QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
+
+    /* Insert sorted by priority into the reset entry list */
+    if (QTAILQ_EMPTY(&reset_handlers) ||
+        QTAILQ_LAST(&reset_handlers)->priority >= priority)
+    {
+        QTAILQ_INSERT_TAIL(&reset_handlers, re, entry);
+    }
+    else
+    {
+        QEMUResetEntry *cur = QTAILQ_LAST(&reset_handlers);
+
+        while (QTAILQ_PREV(cur, entry) != NULL &&
+               QTAILQ_PREV(cur, entry)->priority < priority)
+        {
+            cur = QTAILQ_PREV(cur, entry);
+        }
+
+        QTAILQ_INSERT_BEFORE(cur, re, entry);
+    }
+}
+
+void qemu_register_reset(QEMUResetHandler *func, void *opaque)
+{
+    qemu_register_reset_with_priority(
+        QEMU_RESET_PRIORITY_DEFAULT, func, opaque);
 }
 
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
diff --git a/include/sysemu/reset.h b/include/sysemu/reset.h
index 0b0d6d7598..39a0fe55f0 100644
--- a/include/sysemu/reset.h
+++ b/include/sysemu/reset.h
@@ -1,8 +1,32 @@
 #ifndef QEMU_SYSEMU_RESET_H
 #define QEMU_SYSEMU_RESET_H
 
+/*
+ * The device reset handlers are executed in descending order of their priority
+ * values (i.e. the reset handler with the greatest numerical priority value
+ * will be executed first).
+ */
+#define QEMU_RESET_PRIORITY_DEFAULT     (0x8000)
+
+/*
+ * The priority level can range from -8 to +7, where the reset handler with the
+ * highest priority level is executed first.
+ */
+#define QEMU_RESET_PRIORITY_LEVEL(l) \
+    (QEMU_RESET_PRIORITY_DEFAULT + (l * 0x1000))
+
+/*
+ * Each priority level may be further divided into a maximum of 4096 sub-levels
+ * (0 to 4095).
+ */
+#define QEMU_RESET_PRIORITY_SUBLEVEL(l, s) \
+    (QEMU_RESET_PRIORITY_DEFAULT + (l * 0x1000) + s)
+
 typedef void QEMUResetHandler(void *opaque);
 
+void qemu_register_reset_with_priority(uint16_t priority,
+    QEMUResetHandler *func, void *opaque);
+
 void qemu_register_reset(QEMUResetHandler *func, void *opaque);
 void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
 void qemu_devices_reset(void);
-- 
2.17.1

[PATCH v2 2/2] hw/arm/armv7m: Downgrade CPU reset handler priority

[Stephanos Ioannidis]

The ARMv7-M CPU reset handler, which loads the initial SP and PC
register values from the vector table, is currently executed before
the ROM reset handler (rom_reset), and this causes the devices that
alias low memory region (e.g. STM32F405 that aliases the flash memory
located at 0x8000000 to 0x0) to load an invalid reset vector of 0 when
the kernel image is linked to be loaded at the high memory address.

For instance, it is norm for the STM32F405 firmware ELF image to have
the text and rodata sections linked at 0x8000000, as this facilitates
proper image loading by the firmware burning utility, and the processor
can execute in place from the high flash memory address region as well.

In order to resolve this issue, this commit downgrades the ARMCPU reset
handler invocation priority level to -1 such that it is always executed
after the ROM reset handler, which has a priority level of 0.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
---
 hw/arm/armv7m.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 7531b97ccd..8b7c4b12a6 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -352,7 +352,8 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
      * way A-profile does it. Note that this means that every M profile
      * board must call this function!
      */
-    qemu_register_reset(armv7m_reset, cpu);
+    qemu_register_reset_with_priority(
+        QEMU_RESET_PRIORITY_LEVEL(-1), armv7m_reset, cpu);
 }
 
 static Property bitband_properties[] = {
-- 
2.17.1

Re: [PATCH v2 2/2] hw/arm/armv7m: Downgrade CPU reset handler priority

[Philippe Mathieu-Daudé]

Hi Stephanos,

On 2/27/20 12:51 PM, Stephanos Ioannidis wrote:
> The ARMv7-M CPU reset handler, which loads the initial SP and PC
> register values from the vector table, is currently executed before
> the ROM reset handler (rom_reset), and this causes the devices that
> alias low memory region (e.g. STM32F405 that aliases the flash memory
> located at 0x8000000 to 0x0) to load an invalid reset vector of 0 when
> the kernel image is linked to be loaded at the high memory address.

So we have armv7m_load_kernel -> load_elf_as -> rom_add_blob_fixed_as -> 
rom_add_blob -> rom_insert.

arm_cpu_reset is called before rom_reset, rom_ptr is NULL, we call 
initial_pc = ldl_phys(cpu_as) from an empty flash.

Then later rom_reset -> address_space_write_rom.

I think Alistair and myself use the 'loader' device with Cortex-M boards 
and never hit this problem.

> 
> For instance, it is norm for the STM32F405 firmware ELF image to have
> the text and rodata sections linked at 0x8000000, as this facilitates
> proper image loading by the firmware burning utility, and the processor
> can execute in place from the high flash memory address region as well.
> 
> In order to resolve this issue, this commit downgrades the ARMCPU reset
> handler invocation priority level to -1 such that it is always executed
> after the ROM reset handler, which has a priority level of 0.
> 
> Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
> ---
>   hw/arm/armv7m.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 7531b97ccd..8b7c4b12a6 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -352,7 +352,8 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
>        * way A-profile does it. Note that this means that every M profile
>        * board must call this function!
>        */
> -    qemu_register_reset(armv7m_reset, cpu);
> +    qemu_register_reset_with_priority(
> +        QEMU_RESET_PRIORITY_LEVEL(-1), armv7m_reset, cpu);
>   }
>   
>   static Property bitband_properties[] = {
> 

Re: [PATCH v2 2/2] hw/arm/armv7m: Downgrade CPU reset handler priority

[Alistair Francis]

On Thu, Feb 27, 2020 at 5:32 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Stephanos,
>
> On 2/27/20 12:51 PM, Stephanos Ioannidis wrote:
> > The ARMv7-M CPU reset handler, which loads the initial SP and PC
> > register values from the vector table, is currently executed before
> > the ROM reset handler (rom_reset), and this causes the devices that
> > alias low memory region (e.g. STM32F405 that aliases the flash memory
> > located at 0x8000000 to 0x0) to load an invalid reset vector of 0 when
> > the kernel image is linked to be loaded at the high memory address.
>
> So we have armv7m_load_kernel -> load_elf_as -> rom_add_blob_fixed_as ->
> rom_add_blob -> rom_insert.
>
> arm_cpu_reset is called before rom_reset, rom_ptr is NULL, we call
> initial_pc = ldl_phys(cpu_as) from an empty flash.
>
> Then later rom_reset -> address_space_write_rom.
>
> I think Alistair and myself use the 'loader' device with Cortex-M boards
> and never hit this problem.

I do hit this problem, Peter described a workaround in the previous
version of this patch, that is to link at address 0 instead of the
alias address.

Alistair

>
> >
> > For instance, it is norm for the STM32F405 firmware ELF image to have
> > the text and rodata sections linked at 0x8000000, as this facilitates
> > proper image loading by the firmware burning utility, and the processor
> > can execute in place from the high flash memory address region as well.
> >
> > In order to resolve this issue, this commit downgrades the ARMCPU reset
> > handler invocation priority level to -1 such that it is always executed
> > after the ROM reset handler, which has a priority level of 0.
> >
> > Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
> > ---
> >   hw/arm/armv7m.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> > index 7531b97ccd..8b7c4b12a6 100644
> > --- a/hw/arm/armv7m.c
> > +++ b/hw/arm/armv7m.c
> > @@ -352,7 +352,8 @@ void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int mem_size)
> >        * way A-profile does it. Note that this means that every M profile
> >        * board must call this function!
> >        */
> > -    qemu_register_reset(armv7m_reset, cpu);
> > +    qemu_register_reset_with_priority(
> > +        QEMU_RESET_PRIORITY_LEVEL(-1), armv7m_reset, cpu);
> >   }
> >
> >   static Property bitband_properties[] = {
> >
>
>

Re: [PATCH v2 2/2] hw/arm/armv7m: Downgrade CPU reset handler priority

[Peter Maydell]

On Thu, 27 Feb 2020 at 21:37, Alistair Francis <alistair23@gmail.com> wrote:
> I do hit this problem, Peter described a workaround in the previous
> version of this patch, that is to link at address 0 instead of the
> alias address.

Do you happen to have a simple test case you can send me
that demonstrates the bug? That will save me a bit of
messing around when I come to try to fix it...

thanks
-- PMM

Re: [PATCH v2 2/2] hw/arm/armv7m: Downgrade CPU reset handler priority

[Alistair Francis]

On Thu, Feb 27, 2020 at 1:44 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 27 Feb 2020 at 21:37, Alistair Francis <alistair23@gmail.com> wrote:
> > I do hit this problem, Peter described a workaround in the previous
> > version of this patch, that is to link at address 0 instead of the
> > alias address.
>
> Do you happen to have a simple test case you can send me
> that demonstrates the bug? That will save me a bit of
> messing around when I come to try to fix it...

Yep!

This repo: https://github.com/alistair23/CSSE3010-QEMU-Examples

Run: np2_env.sh to setup variables

Then build an example in the examples directory.

That repo will hard fault on QEMU currently (it doesn't have the address fix).

Alistair

>
> thanks
> -- PMM

[PATCH v2 0/2] Support device reset handler priority configuration

[Stephanos Ioannidis]

This series adds support for configuring device reset handler priority, and uses it to ensure that the ARMv7-M CPU reset handler is invoked after the ROM reset handler.

This fixes the STM32F405 boot failure issue when the kernel ELF image has the text and rodata sections linked at the non-aliased flash memory address (0x8000000), which is the default configuration used by many toolchains.

Stephanos Ioannidis (2):
  hw/core: Support device reset handler priority
  hw/arm/armv7m: Downgrade CPU reset handler priority

 hw/arm/armv7m.c        |  3 ++-
 hw/core/reset.c        | 32 ++++++++++++++++++++++++++++++--
 include/sysemu/reset.h | 24 ++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 3 deletions(-)

--
2.17.1