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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 8/17/20 6:27 PM, Paolo Bonzini wrote:
> On 16/08/20 19:30, Philippe Mathieu-Daudé wrote:
>> There is an issue when accessing an alias memory region via the
>> memory_region_dispatch_read() / memory_region_dispatch_write()
>> calls:
>>
>> 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()
>>   -> memory_region_access_valid(mr)
>>      -> mr->ops->valid.accepts()
>>         -> unassigned_mem_accepts()
>>         <- false
>>      <- false
>>    <- MEMTX_DECODE_ERROR
>>
>> The caller gets a MEMTX_DECODE_ERROR while the access is OK.
> 
> What is the path that leads to this call?

Using the interleaver from:
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg03680.html

#2  0x8162f7b6 in unassigned_mem_read (opaque=0x82ac7330, addr=0,
size=1) at softmmu/memory.c:1261
#3  0x8162fc2f in memory_region_dispatch_read (mr=0x82ac7330, addr=0,
pval=0x7ffe24b9cc08, op=MO_8, attrs=...) at softmmu/memory.c:1417
#4  0x8175488b in interleaver_read (opaque=0x82b9e530, offset=0,
data=0x7ffe24b9cc08, size=1, attrs=...) at hw/misc/interleaver.c:76
#5  0x8162cbdc in memory_region_read_with_attrs_accessor (mr=0x82b9e850,
addr=0, value=0x7ffe24b9cd90, size=1, shift=0, mask=255, attrs=...) at
softmmu/memory.c:456
#6  0x8162cfab in access_with_adjusted_size (addr=0,
value=0x7ffe24b9cd90, size=4, access_size_min=1, access_size_max=1,
access_fn=
    0x8162cb7c <memory_region_read_with_attrs_accessor>, mr=0x82b9e850,
attrs=...) at softmmu/memory.c:544
#7  0x8162fb98 in memory_region_dispatch_read1 (mr=0x82b9e850, addr=0,
pval=0x7ffe24b9cd90, size=4, attrs=...) at softmmu/memory.c:1395
#8  0x8162fc5a in memory_region_dispatch_read (mr=0x82b9e850, addr=0,
pval=0x7ffe24b9cd90, op=MO_32, attrs=...) at softmmu/memory.c:1421
#9  0x8153012b in flatview_read_continue (fv=0x82bd0d10, addr=320897024,
attrs=..., ptr=0x7ffe24b9cea0, len=4, addr1=0, l=4, mr=0x82b9e850) at
exec.c:3239
#10 0x8153027e in flatview_read (fv=0x82bd0d10, addr=320897024,
attrs=..., buf=0x7ffe24b9cea0, len=4) at exec.c:3278
#11 0x81530307 in address_space_read_full (as=0x81ec1ac0
<address_space_memory>, addr=320897024, attrs=..., buf=0x7ffe24b9cea0,
len=4) at exec.c:3291
#12 0x8163761e in address_space_read (len=4, buf=0x7ffe24b9cea0,
attrs=..., addr=320897024, as=0x81ec1ac0 <address_space_memory>) at
include/exec/memory.h:2420
#13 qtest_process_command (chr=0x81edcd00 <qtest_chr>, words=0x82be2b30)
at softmmu/qtest.c:495
#14 0x8163877b in qtest_process_inbuf (chr=0x81edcd00 <qtest_chr>,
inbuf=0x82a65220) at softmmu/qtest.c:724
#15 0x8163880c in qtest_read (opaque=0x81edcd00 <qtest_chr>,
buf=0x7ffe24b9d1f0 "readl 0x13208000\n 0x76\n", size=17) at
softmmu/qtest.c:736

> 
>> Fix by directly dispatching aliases accesses to its origin region.
>>
>> Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")
> 
> I don't think the "Fixes" is okay because you'd have gotten a different
> bug before.

OK I'll reword.

> 
>> +    if (mr->alias) {
>> +        addr += mr->alias_offset;
>> +        mr = mr->alias;
>> +    }
> 
> Also, I think this would have to be a while loop.

I haven't thought about this case! I'll add a test for it :)

> 
> Paolo
> 
>