Small pullreq with some bug fixes to go into rc1.

-- PMM

The following changes since commit 5ca634afcf83215a9a54ca6e66032325b5ffb5f6:

Merge remote-tracking branch 'remotes/philmd/tags/sdmmc-20210322' into staging (2021-03-22 18:50:25 +0000)

https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20210323

for you to fetch changes up to dad90de78e9e9d47cefcbcd30115706b98e6ec87:

target/arm: Set ARMMMUFaultInfo.level in user-only arm_cpu_tlb_fill (2021-03-23 14:07:55 +0000)

* hw/arm/virt: Disable pl011 clock migration if needed

* target/arm: Make M-profile VTOR loads on reset handle memory aliasing

* target/arm: Set ARMMMUFaultInfo.level in user-only arm_cpu_tlb_fill

Gavin Shan (1):

hw/arm/virt: Disable pl011 clock migration if needed

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

Richard Henderson (1):

target/arm: Set ARMMMUFaultInfo.level in user-only arm_cpu_tlb_fill

include/exec/memory.h | 32 +++++++++++++++---

include/hw/char/pl011.h | 1 +

include/hw/loader.h | 31 +++++++++++++++++

hw/char/pl011.c | 9 +++++

hw/core/loader.c | 75 +++++++++++++++++++++++++++++++++++++++++

hw/core/machine.c | 1 +

softmmu/memory.c | 4 ++-

target/arm/cpu.c | 2 +-

target/arm/tlb_helper.c | 1 +

tests/qtest/fuzz/generic_fuzz.c | 11 +++---

10 files changed, 157 insertions(+), 10 deletions(-)

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.

Message-id: 20210318174823.18066-6-peter.maydell@linaro.org

target/arm/cpu.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

@@ -XXX,XX +XXX,XX @@ 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

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.

Message-id: 20210318174823.18066-5-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

--- a/include/hw/loader.h

+++ b/include/hw/loader.h

@@ -XXX,XX +XXX,XX @@ 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

--- a/hw/core/loader.c

+++ b/hw/core/loader.c

@@ -XXX,XX +XXX,XX @@ 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

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.

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

--- a/include/exec/memory.h

+++ b/include/exec/memory.h

@@ -XXX,XX +XXX,XX @@ 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

--- a/tests/qtest/fuzz/generic_fuzz.c

+++ b/tests/qtest/fuzz/generic_fuzz.c

@@ -XXX,XX +XXX,XX @@ 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

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.

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

--- a/include/exec/memory.h

+++ b/include/exec/memory.h

@@ -XXX,XX +XXX,XX @@ 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.

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/softmmu/memory.c

+++ b/softmmu/memory.c

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/tests/qtest/fuzz/generic_fuzz.c

+++ b/tests/qtest/fuzz/generic_fuzz.c

@@ -XXX,XX +XXX,XX @@ 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

From: Gavin Shan <gshan@redhat.com>

A clock is added by commit aac63e0e6ea3 ("hw/char/pl011: add a clock

input") since v5.2.0 which corresponds to virt-5.2 machine type. It

causes backwards migration failure from upstream to downstream (v5.1.0)

when the machine type is specified with virt-5.1.

This fixes the issue by following instructions from section "Connecting

subsections to properties" in docs/devel/migration.rst. With this applied,

the PL011 clock is migrated based on the machine type.

virt-5.2 or newer: migration

virt-5.1 or older: non-migration

Cc: qemu-stable@nongnu.org # v5.2.0+

Fixes: aac63e0e6ea3 ("hw/char/pl011: add a clock input")

Suggested-by: Andrew Jones <drjones@redhat.com>

Signed-off-by: Gavin Shan <gshan@redhat.com>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Message-id: 20210318023801.18287-1-gshan@redhat.com

include/hw/char/pl011.h | 1 +

hw/char/pl011.c | 9 +++++++++

hw/core/machine.c | 1 +

3 files changed, 11 insertions(+)

diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h

--- a/include/hw/char/pl011.h

+++ b/include/hw/char/pl011.h

@@ -XXX,XX +XXX,XX @@ struct PL011State {

CharBackend chr;

qemu_irq irq[6];

Clock *clk;

+ bool migrate_clk;

const unsigned char *id;

diff --git a/hw/char/pl011.c b/hw/char/pl011.c

--- a/hw/char/pl011.c

+++ b/hw/char/pl011.c

@@ -XXX,XX +XXX,XX @@ static const MemoryRegionOps pl011_ops = {

.endianness = DEVICE_NATIVE_ENDIAN,

+static bool pl011_clock_needed(void *opaque)

+{

+ PL011State *s = PL011(opaque);

+

+ return s->migrate_clk;

+}

+

static const VMStateDescription vmstate_pl011_clock = {

.name = "pl011/clock",

.version_id = 1,

.minimum_version_id = 1,