Peter Xu already reviewed, but Cc'ing Peter Maydell too due to
his last comment on v3:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg800482.html
On 4/18/21 7:57 AM, Philippe Mathieu-Daudé wrote:
> Since commit 2cdfcf272d ("memory: assign MemoryRegionOps to all
> regions"), all newly created regions are assigned with
> unassigned_mem_ops (which might be then overwritten).
>
> When using aliased container regions, and there is no region mapped
> at address 0 in the container, the memory_region_dispatch_read()
> and memory_region_dispatch_write() calls incorrectly return the
> container unassigned_mem_ops, because the alias offset is not used.
>
> Consider the following setup:
>
> +--------------------+ < - - - - - - - - - - - +
> | Container | mr
> | (unassigned_mem) | |
> | |
> | | |
> | | alias_offset
> + + <- - - - - - +----------+---------+
> | +----------------+ | | |
> | | MemoryRegion0 | | | |
> | +----------------+ | | Alias | addr1
> | | MemoryRegion1 | | <~ ~ ~ ~ ~ | | <~~~~~~
> | +----------------+ | | |
> | | +--------------------+
> | |
> | |
> | |
> | |
> | +----------------+ |
> | | MemoryRegionX | |
> | +----------------+ |
> | | MemoryRegionY | |
> | +----------------+ |
> | | MemoryRegionZ | |
> | +----------------+ |
> +--------------------+
>
> The memory_region_init_alias() flow is:
>
> memory_region_init_alias()
> -> memory_region_init()
> -> object_initialize(TYPE_MEMORY_REGION)
> -> memory_region_initfn()
> -> mr->ops = &unassigned_mem_ops;
>
> Later when accessing offset=addr1 via the alias, we expect to hit
> MemoryRegion1. The memory_region_dispatch_read() flow is:
>
> memory_region_dispatch_read(addr1)
> -> memory_region_access_valid(mr) <- addr1 offset is ignored
> -> mr->ops->valid.accepts()
> -> unassigned_mem_accepts()
> <- false
> <- false
> <- MEMTX_DECODE_ERROR
>
> The caller gets a MEMTX_DECODE_ERROR while the access is OK.
>
> Fix by dispatching aliases recursively, accessing its origin region
> after adding the alias offset.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
> v4:
> - added ASCII schema
> v3:
> - reworded, mentioning the "alias to container" case
> - use recursive call instead of while(), because easier when debugging
> therefore reset Richard R-b tag.
> v2:
> - use while()
> ---
> softmmu/memory.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index d4493ef9e43..b899ca6a6b7 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1442,6 +1442,11 @@ MemTxResult memory_region_dispatch_read(MemoryRegion
> *mr,
> unsigned size = memop_size(op);
> MemTxResult r;
>
> + if (mr->alias) {
> + return memory_region_dispatch_read(mr->alias,
> + mr->alias_offset + addr,
> + pval, op, attrs);
> + }
> if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
> *pval = unassigned_mem_read(mr, addr, size);
> return MEMTX_DECODE_ERROR;
> @@ -1486,6 +1491,11 @@ MemTxResult memory_region_dispatch_write(MemoryRegion
> *mr,
> {
> unsigned size = memop_size(op);
>
> + if (mr->alias) {
> + return memory_region_dispatch_write(mr->alias,
> + mr->alias_offset + addr,
> + data, op, attrs);
> + }
> if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
> unassigned_mem_write(mr, addr, data, size);
> return MEMTX_DECODE_ERROR;
>
