[for-6.0,2/2] target/arm: Make M-profile VTOR loads on reset handle memory aliasing

For Arm M-profile CPUs, on reset the CPU must load its initial PC and
SP from a vector table in guest memory.  Because we can't guarantee
reset ordering, we have to handle the possibility that the ROM blob
loader's reset function has not yet run when the CPU resets, in which
case the data in an ELF file specified by the user won't be in guest
memory to be read yet.

We work around the reset ordering problem by checking whether the ROM
blob loader has any data for the address where the vector table is,
using rom_ptr().  Unfortunately this does not handle the possibility
of memory aliasing.  For many M-profile boards, memory can be
accessed via multiple possible physical addresses; if the board has
the vector table at address X but the user's ELF file loads data via
a different address Y which is an alias to the same underlying guest
RAM then rom_ptr() will not find it.

Handle the possibility of aliasing by iterating through the whole
FlatView of the CPU's address space checking for other mappings of
the MemoryRegion corresponding to the location of the vector table.
If we find any aliases we use rom_ptr() to see if the ROM blob loader
has any data there.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

Hi Peter,

On 3/12/21 6:29 PM, Peter Maydell wrote:
> For Arm M-profile CPUs, on reset the CPU must load its initial PC and
> SP from a vector table in guest memory.  Because we can't guarantee
> reset ordering, we have to handle the possibility that the ROM blob
> loader's reset function has not yet run when the CPU resets, in which
> case the data in an ELF file specified by the user won't be in guest
> memory to be read yet.
> 
> We work around the reset ordering problem by checking whether the ROM
> blob loader has any data for the address where the vector table is,
> using rom_ptr().  Unfortunately this does not handle the possibility
> of memory aliasing.  For many M-profile boards, memory can be
> accessed via multiple possible physical addresses; if the board has
> the vector table at address X but the user's ELF file loads data via
> a different address Y which is an alias to the same underlying guest
> RAM then rom_ptr() will not find it.
> 
> Handle the possibility of aliasing by iterating through the whole
> FlatView of the CPU's address space checking for other mappings of
> the MemoryRegion corresponding to the location of the vector table.
> If we find any aliases we use rom_ptr() to see if the ROM blob loader
> has any data there.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index ae04884408c..aac78ae6623 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -161,6 +161,72 @@ static void cp_reg_check_reset(gpointer key, gpointer value,  gpointer opaque)
>      assert(oldvalue == newvalue);
>  }
>  
> +#ifndef CONFIG_USER_ONLY
> +typedef struct FindRomCBData {
> +    size_t size; /* Amount of data we want from ROM, in bytes */
> +    MemoryRegion *mr; /* MR at the unaliased guest addr */
> +    hwaddr xlat; /* Offset of addr within mr */
> +    uint8_t *rom; /* Output: rom data pointer, if found */
> +} FindRomCBData;
> +
> +static int find_rom_cb(Int128 start, Int128 len, const MemoryRegion *mr,
> +                       hwaddr offset_in_region, void *opaque)

Return bool maybe?

> +{
> +    FindRomCBData *cbdata = opaque;
> +    hwaddr alias_addr;
> +
> +    if (mr != cbdata->mr) {
> +        return 0;
> +    }
> +
> +    alias_addr = int128_get64(start) + cbdata->xlat - offset_in_region;
> +    cbdata->rom = rom_ptr(alias_addr, cbdata->size);
> +    if (!cbdata->rom) {
> +        return 0;
> +    }
> +    /* Found a match, stop iterating */
> +    return 1;
> +}
> +
> +static uint8_t *find_rom_for_addr(AddressSpace *as, hwaddr addr, size_t size)
> +{
> +    /*
> +     * Find any ROM data for the given guest address range.  If there
> +     * is a ROM blob then return a pointer to the host memory
> +     * corresponding to 'addr'; otherwise return NULL.
> +     *
> +     * This is like rom_ptr(), except that it handles possible aliases
> +     * within the CPU's address space, so that we still find a ROM
> +     * blob even if it was loaded to an address that aliases addr
> +     * rather than to addr itself.
> +     */
> +    FlatView *fv;
> +    uint8_t *rom;
> +    hwaddr len_unused;
> +    FindRomCBData cbdata = {};
> +
> +    /* Easy case: there's data at the actual address */
> +    rom = rom_ptr(addr, size);
> +    if (rom) {
> +        return rom;
> +    }
> +
> +    RCU_READ_LOCK_GUARD();
> +
> +    fv = address_space_to_flatview(as);
> +    cbdata.mr = flatview_translate(fv, addr, &cbdata.xlat, &len_unused,
> +                                   false, MEMTXATTRS_UNSPECIFIED);
> +    if (!cbdata.mr) {
> +        /* Nothing at this address, so there can't be any aliasing */
> +        return NULL;
> +    }
> +
> +    cbdata.size = size;
> +    flatview_for_each_range(fv, find_rom_cb, &cbdata);
> +    return cbdata.rom;
> +}
> +#endif
> +
>  static void arm_cpu_reset(DeviceState *dev)
>  {
>      CPUState *s = CPU(dev);
> @@ -331,7 +397,7 @@ static void arm_cpu_reset(DeviceState *dev)
>  
>          /* Load the initial SP and PC from offset 0 and 4 in the vector table */
>          vecbase = env->v7m.vecbase[env->v7m.secure];
> -        rom = rom_ptr(vecbase, 8);
> +        rom = find_rom_for_addr(s->as, vecbase, 8);
>          if (rom) {
>              /* Address zero is covered by ROM which hasn't yet been
>               * copied into physical memory.
> 

Your solution seems generic, and the problem is not resticted
to Cortex-M:

$ git grep rom_ptr target
target/arm/cpu.c:334:        rom = rom_ptr(vecbase, 8);
target/rx/cpu.c:61:    resetvec = rom_ptr(0xfffffffc, 4);

Some thoughs:
- make find_rom_for_addr() generic ("hw/loader.h" again?)
- poison rom_ptr() under target/ to restrict it to hw/

Regards,

Phil.

On 3/12/21 2:17 PM, Philippe Mathieu-Daudé wrote:
>> +static int find_rom_cb(Int128 start, Int128 len, const MemoryRegion *mr,
>> +                       hwaddr offset_in_region, void *opaque)
> Return bool maybe?
> 

Yes, it might be worth changing with the first patch, while we're changing all 
(one) user of flatview_for_each_range().

But he can't simply change it here -- must match flatview_cb type.


r~

On Fri, 12 Mar 2021 at 20:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Your solution seems generic, and the problem is not resticted
> to Cortex-M:
>
> $ git grep rom_ptr target
> target/arm/cpu.c:334:        rom = rom_ptr(vecbase, 8);
> target/rx/cpu.c:61:    resetvec = rom_ptr(0xfffffffc, 4);
>
> Some thoughs:
> - make find_rom_for_addr() generic ("hw/loader.h" again?)
> - poison rom_ptr() under target/ to restrict it to hw/

This problem isn't specific to code in target/, that's just
the particular case I care about. The uses in hw/ are mostly
in board init code, which (like cpu reset code) runs before
the rom blob reset function, and so wants to directly look
at the blob data. If a board has multiple aliases for a piece
of RAM then rom_ptr() will cause problems for code in hw/ too.
The only advantage that uses of rom_ptr() in hw/ have is that
they can know which board model they're running on and so either
(a) be sure that the address they're looking at doesn't have any
aliases elsewhere in the AddressSpace or (b) manually look at
every alias address in turn.

thanks
-- PMM