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

[Peter Maydell]

On Thu, 18 Mar 2021 at 18:44, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/18/21 11:48 AM, Peter Maydell wrote:
> > +
> > +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.

It's the "happens almost all the time" case. Nothing verifies
that @addr is in @as anyway -- see the "Note that" part of the
comment above.

thanks
-- PMM