QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4] memory: Directly dispatch alias accesses on origin memory region
@ 2021-04-18  5:57 Philippe Mathieu-Daudé
  2021-04-20 21:08 ` Peter Xu
  0 siblings, 1 reply; 2+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-18  5:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Richard Henderson, Philippe Mathieu-Daudé, Peter Xu

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



^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v4] memory: Directly dispatch alias accesses on origin memory region
  2021-04-18  5:57 [PATCH v4] memory: Directly dispatch alias accesses on origin memory region Philippe Mathieu-Daudé
@ 2021-04-20 21:08 ` Peter Xu
  0 siblings, 0 replies; 2+ messages in thread
From: Peter Xu @ 2021-04-20 21:08 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Paolo Bonzini, Richard Henderson, qemu-devel

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



^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-18  5:57 [PATCH v4] memory: Directly dispatch alias accesses on origin memory region Philippe Mathieu-Daudé
2021-04-20 21:08 ` Peter Xu

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git