target/arm/cpu.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
With this patch, we allow loading a ROM image at an aliased address,
when it is located in a memory region for which an alias exists.
Signed-off-by: Jean-Hugues Deschenes <jean-hugues.deschenes@ossiaco.com>
---
target/arm/cpu.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7a4ac9339b..00d89f8c38 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -313,13 +313,32 @@ static void arm_cpu_reset(CPUState *s)
initial_msp = ldl_p(rom);
initial_pc = ldl_p(rom + 4);
} else {
- /* Address zero not covered by a ROM blob, or the ROM blob
- * is in non-modifiable memory and this is a second reset after
- * it got copied into memory. In the latter case, rom_ptr
- * will return a NULL pointer and we should use ldl_phys instead.
- */
- initial_msp = ldl_phys(s->as, vecbase);
- initial_pc = ldl_phys(s->as, vecbase + 4);
+ /* See if the ROM blob is aliased somewhere */
+ hwaddr len = 0, xlat = 0;
+ MemoryRegion *mr = address_space_translate(s->as, vecbase, &xlat,
+ &len, false, MEMTXATTRS_UNSPECIFIED);
+
+ if (mr) {
+ rom = rom_ptr(mr->addr + xlat, 8);
+ } else {
+ rom = NULL;
+ }
+
+ if (rom) {
+ initial_msp = ldl_p(rom);
+ initial_pc = ldl_p(rom + 4);
+ } else {
+
+ /*
+ * Address zero not covered by a ROM blob, or the ROM blob
+ * is in non-modifiable memory and this is a second reset after
+ * it got copied into memory. In the latter case, rom_ptr
+ * will return a NULL pointer and we should use ldl_phys
+ * instead.
+ */
+ initial_msp = ldl_phys(s->as, vecbase);
+ initial_pc = ldl_phys(s->as, vecbase + 4);
+ }
}
env->regs[13] = initial_msp & 0xFFFFFFFC;
--
2.17.1
On 11/25/19 12:41 PM, Jean-Hugues Deschênes wrote:
> initial_msp = ldl_p(rom);
> initial_pc = ldl_p(rom + 4);
> } else {
> - /* Address zero not covered by a ROM blob, or the ROM blob
> - * is in non-modifiable memory and this is a second reset after
> - * it got copied into memory. In the latter case, rom_ptr
> - * will return a NULL pointer and we should use ldl_phys instead.
> - */
> - initial_msp = ldl_phys(s->as, vecbase);
> - initial_pc = ldl_phys(s->as, vecbase + 4);
> + /* See if the ROM blob is aliased somewhere */
> + hwaddr len = 0, xlat = 0;
> + MemoryRegion *mr = address_space_translate(s->as, vecbase, &xlat,
> + &len, false, MEMTXATTRS_UNSPECIFIED);
> +
> + if (mr) {
> + rom = rom_ptr(mr->addr + xlat, 8);
> + } else {
> + rom = NULL;
> + }
> +
> + if (rom) {
> + initial_msp = ldl_p(rom);
> + initial_pc = ldl_p(rom + 4);
> + } else {
> +
> + /*
> + * Address zero not covered by a ROM blob, or the ROM blob
> + * is in non-modifiable memory and this is a second reset after
> + * it got copied into memory. In the latter case, rom_ptr
> + * will return a NULL pointer and we should use ldl_phys
> + * instead.
> + */
> + initial_msp = ldl_phys(s->as, vecbase);
> + initial_pc = ldl_phys(s->as, vecbase + 4);
> + }
> }
Can this entire section, including the rom_ptr thing just above, be replaced
with two address_space_read()?
r~
On Sun, 1 Dec 2019 at 20:13, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/25/19 12:41 PM, Jean-Hugues Deschênes wrote:
> > initial_msp = ldl_p(rom);
> > initial_pc = ldl_p(rom + 4);
> > } else {
> > - /* Address zero not covered by a ROM blob, or the ROM blob
> > - * is in non-modifiable memory and this is a second reset after
> > - * it got copied into memory. In the latter case, rom_ptr
> > - * will return a NULL pointer and we should use ldl_phys instead.
> > - */
> > - initial_msp = ldl_phys(s->as, vecbase);
> > - initial_pc = ldl_phys(s->as, vecbase + 4);
> > + /* See if the ROM blob is aliased somewhere */
> > + hwaddr len = 0, xlat = 0;
> > + MemoryRegion *mr = address_space_translate(s->as, vecbase, &xlat,
> > + &len, false, MEMTXATTRS_UNSPECIFIED);
> > +
> > + if (mr) {
> > + rom = rom_ptr(mr->addr + xlat, 8);
> > + } else {
> > + rom = NULL;
> > + }
> > +
> > + if (rom) {
> > + initial_msp = ldl_p(rom);
> > + initial_pc = ldl_p(rom + 4);
> > + } else {
> > +
> > + /*
> > + * Address zero not covered by a ROM blob, or the ROM blob
> > + * is in non-modifiable memory and this is a second reset after
> > + * it got copied into memory. In the latter case, rom_ptr
> > + * will return a NULL pointer and we should use ldl_phys
> > + * instead.
> > + */
> > + initial_msp = ldl_phys(s->as, vecbase);
> > + initial_pc = ldl_phys(s->as, vecbase + 4);
> > + }
> > }
>
> Can this entire section, including the rom_ptr thing just above, be replaced
> with two address_space_read()?
No. This is a reset ordering problem. The CPU reset happens
before the 'rom blob loader' reset, so at this point the
rom data (usually an ELF file segment) has not been written
into ram, and doing an address_space_read() will just read
zeroes. This is also why the aliasing issue happens at all --
the rom blob is at a particular address, but if the address
we use here to try to read the data is an aliased variant
of it then rom_ptr() does the wrong thing.
My preference for fixing this properly is:
* get Damien's three-phase-reset patchset into master
* make the ROM blob loader write its data into ram
in phase 2 ('hold')
* make the arm CPU reset read the data in phase 3 ('exit')
This last matches better what the hardware does -- the
M-profile CPU reads the vector table in the first couple
of clock cycles when it *leaves* reset, not while the
CPU is being *held* in reset. This kind of thing is
always awkward to model in an emulator, especially if
you were hoping to also handle allowing the PC to be
set from an ELF file entrypoint or by the user in the
gdbstub on startup...
thanks
-- PMM
> No. This is a reset ordering problem. The CPU reset happens before the
> 'rom blob loader' reset, so at this point the rom data (usually an ELF file
> segment) has not been written into ram, and doing an
> address_space_read() will just read zeroes. This is also why the aliasing
> issue happens at all -- the rom blob is at a particular address, but if the
> address we use here to try to read the data is an aliased variant of it
> then rom_ptr() does the wrong thing.
>
> My preference for fixing this properly is:
> * get Damien's three-phase-reset patchset into master
> * make the ROM blob loader write its data into ram
> in phase 2 ('hold')
> * make the arm CPU reset read the data in phase 3 ('exit')
>
> This last matches better what the hardware does -- the M-profile CPU
> reads the vector table in the first couple of clock cycles when it *leaves*
> reset, not while the CPU is being *held* in reset. This kind of thing is
> always awkward to model in an emulator, especially if you were hoping
> to also handle allowing the PC to be set from an ELF file entrypoint or
> by the user in the gdbstub on startup...
Makes perfect sense. Feel free to drop the patch.
Thanks,
Jh
On Sun, 1 Dec 2019 at 22:50, Jean-Hugues Deschênes
<Jean-Hugues.Deschenes@ossiaco.com> wrote:
> > My preference for fixing this properly is:
> > * get Damien's three-phase-reset patchset into master
> > * make the ROM blob loader write its data into ram
> > in phase 2 ('hold')
> > * make the arm CPU reset read the data in phase 3 ('exit')
> Makes perfect sense. Feel free to drop the patch.
Well, I'm still vaguely tracking this patch; we might want
a temporary-fix if it looks like the phased-reset approach
is going to take too long to get into master.
thanks
-- PMM
© 2016 - 2025 Red Hat, Inc.