Re: [PATCH] target/arm: Allow loading elf from aliased ROM regions

[Peter Maydell]

On Sun, 1 Dec 2019 at 20:13, Richard Henderson
<address@hidden> 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