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

[Philippe Mathieu-Daudé]

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.