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

[Mark Cave-Ayland]

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_alias()
>    -> 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_dispatch_read(offset)
>    -> memory_region_access_valid(mr)   <- 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 recusirvely, accessing its origin region
> after adding the alias offset.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 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..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 
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg03190.html ("memory: 
Initialize MemoryRegionOps for RAM memory regions").

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.


ATB,

Mark.

[1] 
https://github.com/mcayland/qemu/commit/56f8639fbecb8a8e323ce486e20cbe309e807419

[2] 
https://github.com/mcayland/qemu/commit/c1fa32da188bb2ce23faf1728228c1714672270d