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

[Philippe Mathieu-Daudé]

Let's be honest, this won't make 6.1, so update the subject to 6.2,
as some scan the list for "6.2" in the subject when 6.1 release
is out.

On 7/7/21 3:58 PM, Philippe Mathieu-Daudé wrote:
> Peter Xu already reviewed, but Cc'ing Peter Maydell too due to
> his last comment on v3:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg800482.html
> 
> On 4/18/21 7:57 AM, 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>
>> ---
>> 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;
>>
>