| Message ID | 20200227112617.66044-2-root@stephanos.io |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] hw/core: Support device reset handler priority | expand |
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
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 >
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
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
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 >>
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[] = {
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(-)