Let's be honest, this won't make 6.1, so update the subject to 6.2, as some scan the list for "6.2" in the subject when 6.1 release is out.
On 7/7/21 3:58 PM, Philippe Mathieu-Daudé wrote: > 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; >> >
