[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3] memory: Directly dispatch alias accesses on origin memory

From: Mark Cave-Ayland
Subject: Re: [PATCH v3] memory: Directly dispatch alias accesses on origin memory region
Date: Tue, 20 Apr 2021 08:00:09 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0

On 19/04/2021 21:58, Philippe Mathieu-Daudé wrote:

Hi Mark,

On 4/19/21 10:13 PM, Mark Cave-Ayland wrote:
On 17/04/2021 15:02, 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.

The memory_region_init_alias() flow is:

    -> memory_region_init()
       -> object_initialize(TYPE_MEMORY_REGION)
          -> memory_region_initfn()
             -> mr->ops = &unassigned_mem_ops;

Later when accessing the alias, the memory_region_dispatch_read()
flow is:

    -> memory_region_access_valid(mr)   <- offset is ignored
       -> mr->ops->valid.accepts()
          -> unassigned_mem_accepts()
          <- false
       <- false

The caller gets a MEMTX_DECODE_ERROR while the access is OK.

Fix by dispatching aliases recusirvely, accessing its origin region
after adding the alias offset.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
- reworded, mentioning the "alias to container" case
- use recursive call instead of while(), because easier when debugging
    therefore reset Richard R-b tag.
- use while()
   softmmu/memory.c | 10 ++++++++++
   1 file changed, 10 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index d4493ef9e43..23bdbfac079 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,
+                                           addr + mr->alias_offset,
+                                           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,
+                                            addr + mr->alias_offset,
+                                            data, op, attrs);
+    }
       if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
           unassigned_mem_write(mr, addr, data, size);
           return MEMTX_DECODE_ERROR;

Whilst working on my q800 patches I realised that this was a similar
problem to the one I had with my macio.alias implementation at [1]:
except that in my case the unassigned_mem_ops mr->ops->valid.accepts()
function was being invoked on a ROM memory region instead of an alias. I
think this is exactly the same issue that you are attempting to fix with
your related patch at
("memory: Initialize MemoryRegionOps for RAM memory regions").

So if 2 contributors hit similar issues, there is something wrong with
the API. I don't see your use case or mine as forbidded by the
documentation in "exec/memory.h".

My patch might not be the proper fix, but we need to figure out how
to avoid others to hit the same problem, as it is very hard to debug.

I agree with this sentiment: it has taken me a while to figure out what was happening, and that was only because I spotted accesses being rejected with -d guest_errors.

From my perspective the names memory_region_dispatch_read() and memory_region_dispatch_write() were the misleading part, although I remember thinking it odd whilst trying to use them that I had to start delving into sections etc. just to recurse a memory access.

At least an assertion and a comment.

I eventually realised that I needed functions that could dispatch
reads/writes to both IO memory regions and ROM memory regions, and that
functionality is covered by the address_space_*() access functions.
Using the address_space_*() functions I was then able to come up with
the working implementation at [2] that handles accesses to both IO
memory regions and ROM memory regions correctly.

The reason I initially used the
memory_region_dispatch_read()/memory_region_dispatch_write() functions
was because I could see that was how the virtio devices dispatched
accesses through the proxy. However I'm wondering now if this API can
only be used for terminating IO memory regions, and so the alias_offset
that you're applying above should actually be applied elsewhere instead.

I figured out the AddressSpace API make these cases simpler, but IIRC
there is some overhead, some function tries to resolve / update
something and iterate over a list. While from the MemoryRegion API we
already know which region we want to access.

I Cc'ed Peter considering him expert in this area, but don't know else
who to ask for help on this topic...

Yeah possibly Paolo, otherwise it's a dig through the git history of memory.c :/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]