Re: [PATCH for-6.0 v2 4/5] hw/core/loader: Add new function rom_ptr_for_as()

[Richard Henderson <richard.henderson@linaro.org>]

On 3/18/21 11:48 AM, Peter Maydell wrote:
> For accesses to rom blob data before or during reset, we have a
> function rom_ptr() which looks for a rom blob that would be loaded to
> the specified address, and returns a pointer into the rom blob data
> corresponding to that address.  This allows board or CPU code to say
> "what is the data that is going to be loaded to this address?".
> 
> However, this function does not take account of memory region
> aliases.  If for instance a machine model has RAM at address
> 0x0000_0000 which is aliased to also appear at 0x1000_0000, a
> rom_ptr() query for address 0x0000_0000 will only return a match if
> the guest image provided by the user was loaded at 0x0000_0000 and
> not if it was loaded at 0x1000_0000, even though they are the same
> RAM and a run-time guest CPU read of 0x0000_0000 will read the data
> loaded to 0x1000_0000.
> 
> Provide a new function rom_ptr_for_as() which takes an AddressSpace
> argument, so that it can check whether the MemoryRegion corresponding
> to the address is also mapped anywhere else in the AddressSpace and
> look for rom blobs that loaded to that alias.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/loader.h | 31 +++++++++++++++++++
>   hw/core/loader.c    | 75 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 106 insertions(+)
> 
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index a9eeea39521..cbfc1848737 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -290,6 +290,37 @@ void rom_transaction_end(bool commit);
>   
>   int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
>   void *rom_ptr(hwaddr addr, size_t size);
> +/**
> + * rom_ptr_for_as: Return a pointer to ROM blob data for the address
> + * @as: AddressSpace to look for the ROM blob in
> + * @addr: Address within @as
> + * @size: size of data required in bytes
> + *
> + * Returns: pointer into the data which backs the matching ROM blob,
> + * or NULL if no blob covers the address range.
> + *
> + * This function looks for a ROM blob which covers the specified range
> + * of bytes of length @size starting at @addr within the address space
> + * @as. This is useful for code which runs as part of board
> + * initialization or CPU reset which wants to read data that is part
> + * of a user-supplied guest image or other guest memory contents, but
> + * which runs before the ROM loader's reset function has copied the
> + * blobs into guest memory.
> + *
> + * rom_ptr_for_as() will look not just for blobs loaded directly to
> + * the specified address, but also for blobs which were loaded to an
> + * alias of the region at a different location in the AddressSpace.
> + * In other words, if a machine model has RAM at address 0x0000_0000
> + * which is aliased to also appear at 0x1000_0000, rom_ptr_for_as()
> + * will return the correct data whether the guest image was linked and
> + * loaded at 0x0000_0000 or 0x1000_0000.  Contrast rom_ptr(), which
> + * will only return data if the image load address is an exact match
> + * with the queried address.
> + *
> + * New code should prefer to use rom_ptr_for_as() instead of
> + * rom_ptr().
> + */
> +void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size);
>   void hmp_info_roms(Monitor *mon, const QDict *qdict);
>   
>   #define rom_add_file_fixed(_f, _a, _i)          \
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 9feca32de98..d3e5f3b423f 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1383,6 +1383,81 @@ void *rom_ptr(hwaddr addr, size_t size)
>       return rom->data + (addr - rom->addr);
>   }
>   
> +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 */
> +    void *rom; /* Output: rom data pointer, if found */
> +} FindRomCBData;
> +
> +static bool find_rom_cb(Int128 start, Int128 len, const MemoryRegion *mr,
> +                        hwaddr offset_in_region, void *opaque)
> +{
> +    FindRomCBData *cbdata = opaque;
> +    hwaddr alias_addr;
> +
> +    if (mr != cbdata->mr) {
> +        return false;
> +    }
> +
> +    alias_addr = int128_get64(start) + cbdata->xlat - offset_in_region;
> +    cbdata->rom = rom_ptr(alias_addr, cbdata->size);
> +    if (!cbdata->rom) {
> +        return false;
> +    }
> +    /* Found a match, stop iterating */
> +    return true;
> +}
> +
> +void *rom_ptr_for_as(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.
> +     *
> +     * We look not only for ROM blobs that were loaded directly to
> +     * addr, but also for ROM blobs that were loaded to aliases of
> +     * that memory at other addresses within the AddressSpace.
> +     *
> +     * Note that we do not check @as against the 'as' member in the
> +     * 'struct Rom' returned by rom_ptr(). The Rom::as is the
> +     * AddressSpace which the rom blob should be written to, whereas
> +     * our @as argument is the AddressSpace which we are (effectively)
> +     * reading from, and the same underlying RAM will often be visible
> +     * in multiple AddressSpaces. (A common example is a ROM blob
> +     * written to the 'system' address space but then read back via a
> +     * CPU's cpu->as pointer.) This does mean we might potentially
> +     * return a false-positive match if a ROM blob was loaded into an
> +     * AS which is entirely separate and distinct from the one we're
> +     * querying, but this issue exists also for rom_ptr() and hasn't
> +     * caused any problems in practice.
> +     */
> +    FlatView *fv;
> +    void *rom;
> +    hwaddr len_unused;
> +    FindRomCBData cbdata = {};
> +
> +    /* Easy case: there's data at the actual address */
> +    rom = rom_ptr(addr, size);
> +    if (rom) {
> +        return rom;
> +    }

Should you really have this special case?  Nowhere is this going to verify that 
@addr is in @as.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


> +
> +    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;
> +}
> +
>   void hmp_info_roms(Monitor *mon, const QDict *qdict)
>   {
>       Rom *rom;
>