[PATCH for-6.0 v2 0/5] arm: Make M-profile VTOR loads on reset handle memory aliasin

[PATCH for-6.0 v2 0/5] arm: Make M-profile VTOR loads on reset handle memory aliasin

[Peter Maydell]

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.

This series handles 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.

Changes from v1:
 * do a little bit more cleanup on flatview_for_each_range():
   - switch return type to bool
   - document it
 * put the "rom_ptr() but handle aliases" functionality into
   a generally-available function rom_ptr_for_as()

We discussed the idea of just making rom_ptr() itself handle the
aliasing, but that would require looking at all the callsites to
identify a good address space to use; it's also a bit more invasive to
other platforms than I would like at this point in the release
cycle. So I opted for "provide a new function" as a safer and simpler
compromise. In many cases callers of rom_ptr() probably should be
changed to use rom_ptr_for_as() at some point, though.

I realised that although if we can get reset ordering sorted out
we can remove this use of rom_ptr()/rom_ptr_from_as() from the
Arm CPU reset function, we will still have the same "need to read
the blob data directly" problem for board init functions which
are the bulk of the callers of rom_ptr(). I suppose in theory
we could rewrite those to postpone their accessing of the data
until reset, but that sounds like it could get complicated. Anyway,
that means that rom_ptr_for_as() might have a fairly long life.

thanks
-- PMM

Peter Maydell (5):
  memory: Make flatview_cb return bool, not int
  memory: Document flatview_for_each_range()
  memory: Add offset_in_region to flatview_cb arguments
  hw/core/loader: Add new function rom_ptr_for_as()
  target/arm: Make M-profile VTOR loads on reset handle memory aliasing

 include/exec/memory.h           | 32 ++++++++++++--
 include/hw/loader.h             | 31 ++++++++++++++
 hw/core/loader.c                | 75 +++++++++++++++++++++++++++++++++
 softmmu/memory.c                |  4 +-
 target/arm/cpu.c                |  2 +-
 tests/qtest/fuzz/generic_fuzz.c | 11 +++--
 6 files changed, 145 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH for-6.0 v2 1/5] memory: Make flatview_cb return bool, not int

[Peter Maydell]

The return value of the flatview_cb callback passed to the
flatview_for_each_range() function is zero if the iteration through
the ranges should continue, or non-zero to break out of it.  Use a
bool for this rather than int.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory.h           | 6 +++---
 tests/qtest/fuzz/generic_fuzz.c | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 54ccf1a5f09..22c10b8496a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -776,9 +776,9 @@ static inline FlatView *address_space_to_flatview(AddressSpace *as)
     return qatomic_rcu_read(&as->current_map);
 }
 
-typedef int (*flatview_cb)(Int128 start,
-                           Int128 len,
-                           const MemoryRegion*, void*);
+typedef bool (*flatview_cb)(Int128 start,
+                            Int128 len,
+                            const MemoryRegion*, void*);
 
 void flatview_for_each_range(FlatView *fv, flatview_cb cb , void *opaque);
 
diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index b5fe27aae18..b6af4cbb18b 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -98,19 +98,19 @@ struct get_io_cb_info {
     address_range result;
 };
 
-static int get_io_address_cb(Int128 start, Int128 size,
-                          const MemoryRegion *mr, void *opaque) {
+static bool get_io_address_cb(Int128 start, Int128 size,
+                              const MemoryRegion *mr, void *opaque) {
     struct get_io_cb_info *info = opaque;
     if (g_hash_table_lookup(fuzzable_memoryregions, mr)) {
         if (info->index == 0) {
             info->result.addr = (ram_addr_t)start;
             info->result.size = (ram_addr_t)size;
             info->found = 1;
-            return 1;
+            return true;
         }
         info->index--;
     }
-    return 0;
+    return false;
 }
 
 /*
-- 
2.20.1

[PATCH for-6.0 v2 2/5] memory: Document flatview_for_each_range()

[Peter Maydell]

Add a documentation comment describing flatview_for_each_range().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory.h | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 22c10b8496a..71a1841943e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -776,11 +776,33 @@ static inline FlatView *address_space_to_flatview(AddressSpace *as)
     return qatomic_rcu_read(&as->current_map);
 }
 
+/**
+ * typedef flatview_cb: callback for flatview_for_each_range()
+ *
+ * @start: start address of the range within the FlatView
+ * @len: length of the range in bytes
+ * @mr: MemoryRegion covering this range
+ * @opaque: data pointer passed to flatview_for_each_range()
+ *
+ * Returns: true to stop the iteration, false to keep going.
+ */
 typedef bool (*flatview_cb)(Int128 start,
                             Int128 len,
-                            const MemoryRegion*, void*);
+                            const MemoryRegion *mr,
+                            void *opaque);
 
-void flatview_for_each_range(FlatView *fv, flatview_cb cb , void *opaque);
+/**
+ * flatview_for_each_range: Iterate through a FlatView
+ * @fv: the FlatView to iterate through
+ * @cb: function to call for each range
+ * @opaque: opaque data pointer to pass to @cb
+ *
+ * A FlatView is made up of a list of non-overlapping ranges, each of
+ * which is a slice of a MemoryRegion. This function iterates through
+ * each range in @fv, calling @cb. The callback function can terminate
+ * iteration early by returning 'true'.
+ */
+void flatview_for_each_range(FlatView *fv, flatview_cb cb, void *opaque);
 
 /**
  * struct MemoryRegionSection: describes a fragment of a #MemoryRegion
-- 
2.20.1

[PATCH for-6.0 v2 3/5] memory: Add offset_in_region to flatview_cb arguments

[Peter Maydell]

The function flatview_for_each_range() calls a callback for each
range in a FlatView.  Currently the callback gets the start and
length of the range and the MemoryRegion involved, but not the offset
within the MemoryRegion.  Add this to the callback's arguments; we're
going to want it for a new use in the next commit.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory.h           | 2 ++
 softmmu/memory.c                | 4 +++-
 tests/qtest/fuzz/generic_fuzz.c | 5 ++++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 71a1841943e..529152c6a85 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -782,6 +782,7 @@ static inline FlatView *address_space_to_flatview(AddressSpace *as)
  * @start: start address of the range within the FlatView
  * @len: length of the range in bytes
  * @mr: MemoryRegion covering this range
+ * @offset_in_region: offset of the first byte of the range within @mr
  * @opaque: data pointer passed to flatview_for_each_range()
  *
  * Returns: true to stop the iteration, false to keep going.
@@ -789,6 +790,7 @@ static inline FlatView *address_space_to_flatview(AddressSpace *as)
 typedef bool (*flatview_cb)(Int128 start,
                             Int128 len,
                             const MemoryRegion *mr,
+                            hwaddr offset_in_region,
                             void *opaque);
 
 /**
diff --git a/softmmu/memory.c b/softmmu/memory.c
index c4730ec47ae..d4493ef9e43 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -671,8 +671,10 @@ void flatview_for_each_range(FlatView *fv, flatview_cb cb , void *opaque)
     assert(cb);
 
     FOR_EACH_FLAT_RANGE(fr, fv) {
-        if (cb(fr->addr.start, fr->addr.size, fr->mr, opaque))
+        if (cb(fr->addr.start, fr->addr.size, fr->mr,
+               fr->offset_in_region, opaque)) {
             break;
+        }
     }
 }
 
diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index b6af4cbb18b..ae219540b42 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -99,7 +99,10 @@ struct get_io_cb_info {
 };
 
 static bool get_io_address_cb(Int128 start, Int128 size,
-                              const MemoryRegion *mr, void *opaque) {
+                              const MemoryRegion *mr,
+                              hwaddr offset_in_region,
+                              void *opaque)
+{
     struct get_io_cb_info *info = opaque;
     if (g_hash_table_lookup(fuzzable_memoryregions, mr)) {
         if (info->index == 0) {
-- 
2.20.1

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

[Peter Maydell]

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;
+    }
+
+    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;
-- 
2.20.1

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

[Peter Maydell]

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.

Use the new rom_ptr_for_as() function, which deals with memory
aliasing when locating a relevant ROM blob.

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

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ae04884408c..0dd623e5909 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -331,7 +331,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 = rom_ptr_for_as(s->as, vecbase, 8);
         if (rom) {
             /* Address zero is covered by ROM which hasn't yet been
              * copied into physical memory.
-- 
2.20.1

Re: [PATCH for-6.0 v2 1/5] memory: Make flatview_cb return bool, not int

[Richard Henderson]

On 3/18/21 11:48 AM, Peter Maydell wrote:
> The return value of the flatview_cb callback passed to the
> flatview_for_each_range() function is zero if the iteration through
> the ranges should continue, or non-zero to break out of it.  Use a
> bool for this rather than int.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/exec/memory.h           | 6 +++---
>   tests/qtest/fuzz/generic_fuzz.c | 8 ++++----
>   2 files changed, 7 insertions(+), 7 deletions(-)

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

r~

Re: [PATCH for-6.0 v2 2/5] memory: Document flatview_for_each_range()

[Richard Henderson]

On 3/18/21 11:48 AM, Peter Maydell wrote:
> Add a documentation comment describing flatview_for_each_range().
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/exec/memory.h | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)

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

r~

Re: [PATCH for-6.0 v2 3/5] memory: Add offset_in_region to flatview_cb arguments

[Richard Henderson]

On 3/18/21 11:48 AM, Peter Maydell wrote:
> The function flatview_for_each_range() calls a callback for each
> range in a FlatView.  Currently the callback gets the start and
> length of the range and the MemoryRegion involved, but not the offset
> within the MemoryRegion.  Add this to the callback's arguments; we're
> going to want it for a new use in the next commit.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/exec/memory.h           | 2 ++
>   softmmu/memory.c                | 4 +++-
>   tests/qtest/fuzz/generic_fuzz.c | 5 ++++-
>   3 files changed, 9 insertions(+), 2 deletions(-)

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

r~

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

[Richard Henderson]

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;
> 

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

[Richard Henderson]

On 3/18/21 11:48 AM, 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.
> 
> Use the new rom_ptr_for_as() function, which deals with memory
> aliasing when locating a relevant ROM blob.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

r~

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

Re: [PATCH for-6.0 v2 1/5] memory: Make flatview_cb return bool, not int

[Philippe Mathieu-Daudé]

On 3/18/21 6:48 PM, Peter Maydell wrote:
> The return value of the flatview_cb callback passed to the
> flatview_for_each_range() function is zero if the iteration through
> the ranges should continue, or non-zero to break out of it.  Use a
> bool for this rather than int.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory.h           | 6 +++---
>  tests/qtest/fuzz/generic_fuzz.c | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH for-6.0 v2 2/5] memory: Document flatview_for_each_range()

[Philippe Mathieu-Daudé]

On 3/18/21 6:48 PM, Peter Maydell wrote:
> Add a documentation comment describing flatview_for_each_range().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory.h | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH for-6.0 v2 3/5] memory: Add offset_in_region to flatview_cb arguments

[Philippe Mathieu-Daudé]

On 3/18/21 6:48 PM, Peter Maydell wrote:
> The function flatview_for_each_range() calls a callback for each
> range in a FlatView.  Currently the callback gets the start and
> length of the range and the MemoryRegion involved, but not the offset
> within the MemoryRegion.  Add this to the callback's arguments; we're
> going to want it for a new use in the next commit.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory.h           | 2 ++
>  softmmu/memory.c                | 4 +++-
>  tests/qtest/fuzz/generic_fuzz.c | 5 ++++-
>  3 files changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

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

[Richard Henderson]

On 3/18/21 1:02 PM, Peter Maydell wrote:
>>> +     * 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...
...
>> 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.

The comment explains why we don't examine Rom::as.
But we do check @addr vs @as via @as -> @fv -> flatview_translate.


r~

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 21:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/18/21 1:02 PM, Peter Maydell wrote:
> >>> +     * 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...
> ...
> >> 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.
>
> The comment explains why we don't examine Rom::as.
> But we do check @addr vs @as via @as -> @fv -> flatview_translate.

All we do is look for "every other address in the AS that
maps to the same MR as the address we started with".
Are you asking about the !cbdata.mr exit ? That's for the
implausible corner case that the caller asked us about an
address with no RAM. In that case the early "check rom_ptr()
directly against @address" gets us the "did the user load
an image at that address", and we can skip the "check for
other aliases, because by there aren't going to be any
aliases to no MR at all.

thanks
-- PMM

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

[Richard Henderson]

On 3/18/21 3:28 PM, Peter Maydell wrote:
> On Thu, 18 Mar 2021 at 21:14, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 3/18/21 1:02 PM, Peter Maydell wrote:
>>>>> +     * 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...
>> ...
>>>> 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.
>>
>> The comment explains why we don't examine Rom::as.
>> But we do check @addr vs @as via @as -> @fv -> flatview_translate.
> 
> All we do is look for "every other address in the AS that
> maps to the same MR as the address we started with".
> Are you asking about the !cbdata.mr exit ?

Well, I suppose.  I guess I didn't read the cb properly.

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

r~