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

[PATCH 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 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 2/2] hw/arm/armv7m: Downgrade CPU reset handler priority

[Peter Maydell]

On Thu, 27 Feb 2020 at 11:27, Stephanos Ioannidis <root@stephanos.io> 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.
>
> 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.


I think we should be able to do this with the new 3-phase
reset API : the rom loader reset should happen in phase 2,
and the Arm CPU should only load the new PC and SP in
phase 3. It's on my todo list to write some code for this
to see if this theory works out.

I'd prefer it if we do it that way, or alternatively find
out for certain that that approach does not work, before
we add a reset-priority concept to the reset APIs.

(In particular, this use of qemu_register_reset to arrange for
the CPU to be reset should ideally go away in favour of having
the CPU reset handled by the SoC which owns the CPU, so it's
not a good long-term way to look at trying to fix ordering issues.)

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 2/27/20 1:13 PM, Peter Maydell wrote:
> On Thu, 27 Feb 2020 at 11:27, Stephanos Ioannidis <root@stephanos.io> 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.
>>
>> 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.
> 
> 
> I think we should be able to do this with the new 3-phase
> reset API : the rom loader reset should happen in phase 2,
> and the Arm CPU should only load the new PC and SP in
> phase 3. It's on my todo list to write some code for this
> to see if this theory works out.
> 
> I'd prefer it if we do it that way, or alternatively find
> out for certain that that approach does not work, before
> we add a reset-priority concept to the reset APIs.

Agreed.

> 
> (In particular, this use of qemu_register_reset to arrange for
> the CPU to be reset should ideally go away in favour of having
> the CPU reset handled by the SoC which owns the CPU, so it's
> not a good long-term way to look at trying to fix ordering issues.)

It would be nice to get ride of qemu_register_reset with the reset API :)

> 
> thanks
> -- PMM
> 

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

[Stephanos Ioannidis]

Hi Peter and Philip,

Thanks for your insight on this matter.

I am okay as long as this issue is going to be eventually fixed in one way or another; the three-phase reset scheme you mentioned does sound like a more manageable approach for this purpose; though, I still believe having the option to specify the invocation priority for reset handlers would come in handy in the future for various purposes.

On 2/27/20 10:31 PM, Philippe Mathieu-Daudé wrote:
> I think Alistair and myself use the 'loader' device with Cortex-M boards and never hit this problem.

I tried both `-kernel [ELF IMAGE]` and `-device loader,file=[ELF IMAGE]` without any success; in both cases, without this patch, QEMU hard-faults during start-up due to the unavailability of the vector table content at the time of CPU reset.

Maybe your firmware image has a load address of 0x0 instead of 0x8000000?

The following is the MAP file for the firmware image I am testing with:
https://gist.github.com/stephanosio/97d1f47f962844479a76e0e909a3b8cf

Regards,

Stephanos 

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

[Peter Maydell]

On Thu, 27 Feb 2020 at 15:08, Stephanos Ioannidis <root@stephanos.io> wrote:
> On 2/27/20 10:31 PM, Philippe Mathieu-Daudé wrote:
> > I think Alistair and myself use the 'loader' device with Cortex-M boards and never hit this problem.
>
> I tried both `-kernel [ELF IMAGE]` and `-device loader,file=[ELF IMAGE]` without any success; in both cases, without this patch, QEMU hard-faults during start-up due to the unavailability of the vector table content at the time of CPU reset.

You only run into this bug if:
 * you're using a Cortex-M CPU
 * and the board model has aliased memory regions so that the
   flash or memory you're loading the ELF file into appears at
   multiple addresses in the memory map
 * and the ELF file loads the data into the aliased address
   rather than the CPU's VTOR register reset value (which is
   0 for CPUs without the Security Extension)
 * but it doesn't matter whether you use -kernel or -device loader

So you can work around it by linking your images to be loaded
at 0 rather than the higher address. It is definitely a bug
that we don't correctly handle these ELF images.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

Hi Peter,

On 2/27/20 2:35 PM, Philippe Mathieu-Daudé wrote:
> On 2/27/20 1:13 PM, Peter Maydell wrote:
>> On Thu, 27 Feb 2020 at 11:27, Stephanos Ioannidis <root@stephanos.io> 
>> 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.
>>>
>>> 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.
>>
>>
>> I think we should be able to do this with the new 3-phase
>> reset API : the rom loader reset should happen in phase 2,
>> and the Arm CPU should only load the new PC and SP in
>> phase 3. It's on my todo list to write some code for this
>> to see if this theory works out.
>>
>> I'd prefer it if we do it that way, or alternatively find
>> out for certain that that approach does not work, before
>> we add a reset-priority concept to the reset APIs.

FYI I hit the same problem testing the RX port which on reset loads $PC 
at 0xfffffffc. Using Stephanos's previous patch and 
qemu_register_reset_with_priority() in cpu_realize(), the issue is fixed.
I plan to carry the patch in the RX series until we find an alternative.

> 
> Agreed.
> 
>>
>> (In particular, this use of qemu_register_reset to arrange for
>> the CPU to be reset should ideally go away in favour of having
>> the CPU reset handled by the SoC which owns the CPU, so it's
>> not a good long-term way to look at trying to fix ordering issues.)

And your "cpu: Use DeviceClass reset instead of a special CPUClass 
reset​" patch goes into that direction :)

> 
> It would be nice to get ride of qemu_register_reset with the reset API :)
> 
>>
>> thanks
>> -- PMM
>>