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 <address@hidden> 
> > 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