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

[Peter Xu]

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