* [PATCH] memory: Directly dispatch alias accesses on origin memory region
@ 2020-08-16 17:30 Philippe Mathieu-Daudé
2020-08-17 16:27 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-16 17:30 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Richard Henderson, Philippe Mathieu-Daudé,
Paolo Bonzini
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.
Fix by directly dispatching aliases accesses to its origin region.
Fixes: 2cdfcf272d ("memory: assign MemoryRegionOps to all regions")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
softmmu/memory.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..651705b7d1 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1405,6 +1405,10 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
unsigned size = memop_size(op);
MemTxResult r;
+ if (mr->alias) {
+ addr += mr->alias_offset;
+ mr = mr->alias;
+ }
if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
*pval = unassigned_mem_read(mr, addr, size);
return MEMTX_DECODE_ERROR;
@@ -1449,6 +1453,10 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
{
unsigned size = memop_size(op);
+ if (mr->alias) {
+ addr += mr->alias_offset;
+ mr = mr->alias;
+ }
if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
unassigned_mem_write(mr, addr, data, size);
return MEMTX_DECODE_ERROR;
--
2.21.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] memory: Directly dispatch alias accesses on origin memory region
2020-08-16 17:30 [PATCH] memory: Directly dispatch alias accesses on origin memory region Philippe Mathieu-Daudé
@ 2020-08-17 16:27 ` Paolo Bonzini
2020-08-18 8:01 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2020-08-17 16:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Peter Maydell, Richard Henderson
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?
> 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.
> + if (mr->alias) {
> + addr += mr->alias_offset;
> + mr = mr->alias;
> + }
Also, I think this would have to be a while loop.
Paolo
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] memory: Directly dispatch alias accesses on origin memory region
2020-08-17 16:27 ` Paolo Bonzini
@ 2020-08-18 8:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-18 8:01 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Peter Maydell, Richard Henderson
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-18 8:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-16 17:30 [PATCH] memory: Directly dispatch alias accesses on origin memory region Philippe Mathieu-Daudé
2020-08-17 16:27 ` Paolo Bonzini
2020-08-18 8:01 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).