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é <f4bug@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;
--
2.26.3
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é <f4bug@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; >
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é <f4bug@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; >> >
On 4/18/21 07:57, 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é <f4bug@amsat.org> > --- > softmmu/memory.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) Queued via memory-api.
ping? On Sun, Apr 18, 2021 at 7:57 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 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é <f4bug@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; > -- > 2.26.3 >
On Sun, Apr 18, 2021 at 07:57:08AM +0200, 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é <f4bug@amsat.org> Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu
© 2016 - 2024 Red Hat, Inc.