diff mbox series

[v2] memory: Directly dispatch alias accesses on origin memory region

Message ID 20200822161449.1327517-1-f4bug@amsat.org
State New
Headers show
Series [v2] memory: Directly dispatch alias accesses on origin memory region | expand

Commit Message

Philippe Mathieu-Daudé Aug. 22, 2020, 4:14 p.m. UTC 
There is an issue when accessing an alias memory region via the
memory_region_dispatch_read() / memory_region_dispatch_write()
calls.

Since commit 2cdfcf272d ("memory: assign MemoryRegionOps to all
regions") we assign &unassigned_mem_ops to all MemoryRegions.

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;

When accessing the alias using memory_region_dispatch_read(),
flow is:

  memory_region_dispatch_read()
  -> memory_region_access_valid(mr)
     -> 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 directly dispatching aliases accesses to its origin region.

Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v1:
- reword as not a bugfix (Paolo)
- access the aliases recursively (Paolo)
---
 softmmu/memory.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Paolo Bonzini Aug. 30, 2020, 10:20 a.m. UTC | #1 
On 22/08/20 18:14, Philippe Mathieu-Daudé wrote:
> There is an issue when accessing an alias memory region via the
> memory_region_dispatch_read() / memory_region_dispatch_write()
> calls.
> 
> Since commit 2cdfcf272d ("memory: assign MemoryRegionOps to all
> regions") we assign &unassigned_mem_ops to all MemoryRegions.
> 
> 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;
> 
> When accessing the alias using memory_region_dispatch_read(),
> flow is:
> 
>   memory_region_dispatch_read()
>   -> memory_region_access_valid(mr)
>      -> 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 directly dispatching aliases accesses to its origin region.
> 
> Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

It's still not clear from the commit message what is the case where you
get this.  I can integrate it in the commit message myself, but can you
explain it and/or link to a bug report?

Paolo
diff mbox series

Patch

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 70b93104e8c..3627985bf9f 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1405,6 +1405,10 @@  MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
     unsigned size = memop_size(op);
     MemTxResult r;
 
+    while (mr->alias) {
+        addr += mr->alias_offset;
+        mr = mr->alias;
+    }
     if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
         *pval = unassigned_mem_read(mr, addr, size);
         return MEMTX_DECODE_ERROR;
@@ -1449,6 +1453,10 @@  MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
 {
     unsigned size = memop_size(op);
 
+    while (mr->alias) {
+        addr += mr->alias_offset;
+        mr = mr->alias;
+    }
     if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
         unassigned_mem_write(mr, addr, data, size);
         return MEMTX_DECODE_ERROR;