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

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

[Philippe Mathieu-Daudé]

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;
-- 
2.26.3

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

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

[Philippe Mathieu-Daudé]

Hi Mark,

On 4/19/21 10:13 PM, Mark Cave-Ayland wrote:
> 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").

So if 2 contributors hit similar issues, there is something wrong with
the API. I don't see your use case or mine as forbidded by the
documentation in "exec/memory.h".

My patch might not be the proper fix, but we need to figure out how
to avoid others to hit the same problem, as it is very hard to debug.

At least an assertion and a comment.

> 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.

I figured out the AddressSpace API make these cases simpler, but IIRC
there is some overhead, some function tries to resolve / update
something and iterate over a list. While from the MemoryRegion API we
already know which region we want to access.

I Cc'ed Peter considering him expert in this area, but don't know else
who to ask for help on this topic...

> ATB,
> 
> Mark.
> 
> [1]
> https://github.com/mcayland/qemu/commit/56f8639fbecb8a8e323ce486e20cbe309e807419
> 
> 
> [2]
> https://github.com/mcayland/qemu/commit/c1fa32da188bb2ce23faf1728228c1714672270d
> 
> 

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

[Philippe Mathieu-Daudé]

On Mon, Apr 19, 2021 at 10:58 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 4/19/21 10:13 PM, Mark Cave-Ayland wrote:
> > 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").
>
> So if 2 contributors hit similar issues, there is something wrong with
> the API. I don't see your use case or mine as forbidded by the
> documentation in "exec/memory.h".
>
> My patch might not be the proper fix, but we need to figure out how
> to avoid others to hit the same problem, as it is very hard to debug.
>
> At least an assertion and a comment.

Something like:

-- >8 --
diff --git a/softmmu/memory.c b/softmmu/memory.c
index d4493ef9e43..e031ac6e074 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1442,6 +1442,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
    unsigned size = memop_size(op);
    MemTxResult r;

+    assert(!(mr->alias && !mr>alias_offset)); /* Use AddressSpace API
instead */
    if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
        *pval = unassigned_mem_read(mr, addr, size);
        return MEMTX_DECODE_ERROR;

---

> > 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.
>
> I figured out the AddressSpace API make these cases simpler, but IIRC
> there is some overhead, some function tries to resolve / update
> something and iterate over a list. While from the MemoryRegion API we
> already know which region we want to access.
>
> I Cc'ed Peter considering him expert in this area, but don't know else
> who to ask for help on this topic...
>
> > ATB,
> >
> > Mark.
> >
> > [1]
> > https://github.com/mcayland/qemu/commit/56f8639fbecb8a8e323ce486e20cbe309e807419
> >
> >
> > [2]
> > https://github.com/mcayland/qemu/commit/c1fa32da188bb2ce23faf1728228c1714672270d
> >
> >

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

[Mark Cave-Ayland]

On 19/04/2021 21:58, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 4/19/21 10:13 PM, Mark Cave-Ayland wrote:
>> 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").
> 
> So if 2 contributors hit similar issues, there is something wrong with
> the API. I don't see your use case or mine as forbidded by the
> documentation in "exec/memory.h".
> 
> My patch might not be the proper fix, but we need to figure out how
> to avoid others to hit the same problem, as it is very hard to debug.

I agree with this sentiment: it has taken me a while to figure out what was 
happening, and that was only because I spotted accesses being rejected with -d 
guest_errors.

 From my perspective the names memory_region_dispatch_read() and 
memory_region_dispatch_write() were the misleading part, although I remember thinking 
it odd whilst trying to use them that I had to start delving into sections etc. just 
to recurse a memory access.

> At least an assertion and a comment.
> 
>> 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.
> 
> I figured out the AddressSpace API make these cases simpler, but IIRC
> there is some overhead, some function tries to resolve / update
> something and iterate over a list. While from the MemoryRegion API we
> already know which region we want to access.
> 
> I Cc'ed Peter considering him expert in this area, but don't know else
> who to ask for help on this topic...

Yeah possibly Paolo, otherwise it's a dig through the git history of memory.c :/


ATB,

Mark.

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

[Mark Cave-Ayland]

On 19/04/2021 22:11, Philippe Mathieu-Daudé wrote:

>> My patch might not be the proper fix, but we need to figure out how
>> to avoid others to hit the same problem, as it is very hard to debug.
>>
>> At least an assertion and a comment.
> 
> Something like:
> 
> -- >8 --
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index d4493ef9e43..e031ac6e074 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1442,6 +1442,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
>      unsigned size = memop_size(op);
>      MemTxResult r;
> 
> +    assert(!(mr->alias && !mr>alias_offset)); /* Use AddressSpace API
> instead */
>      if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
>          *pval = unassigned_mem_read(mr, addr, size);
>          return MEMTX_DECODE_ERROR;

AFAICT the dispatch functions don't handle rom/ram or aliases so you might need to go 
a bit stronger. The check is further complicated by the fact that you can have 
rom/ram devices which do define the underlying mr->ops.

I'm wondering for memory_region_dispatch_read() if this would work?

    assert(!mr->alias && !memory_access_is_direct(mr, false));


ATB,

Mark.

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

[Philippe Mathieu-Daudé]

On 4/20/21 9:00 AM, Mark Cave-Ayland wrote:
> On 19/04/2021 21:58, Philippe Mathieu-Daudé wrote:
> 
>> Hi Mark,
>>
>> On 4/19/21 10:13 PM, Mark Cave-Ayland wrote:
>>> 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").
>>
>> So if 2 contributors hit similar issues, there is something wrong with
>> the API. I don't see your use case or mine as forbidded by the
>> documentation in "exec/memory.h".
>>
>> My patch might not be the proper fix, but we need to figure out how
>> to avoid others to hit the same problem, as it is very hard to debug.
> 
> I agree with this sentiment: it has taken me a while to figure out what
> was happening, and that was only because I spotted accesses being
> rejected with -d guest_errors.
> 
> From my perspective the names memory_region_dispatch_read() and
> memory_region_dispatch_write() were the misleading part, although I
> remember thinking it odd whilst trying to use them that I had to start
> delving into sections etc. just to recurse a memory access.
> 
>> At least an assertion and a comment.
>>
>>> 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.
>>
>> I figured out the AddressSpace API make these cases simpler, but IIRC
>> there is some overhead, some function tries to resolve / update
>> something and iterate over a list. While from the MemoryRegion API we
>> already know which region we want to access.
>>
>> I Cc'ed Peter considering him expert in this area, but don't know else
>> who to ask for help on this topic...
> 
> Yeah possibly Paolo, otherwise it's a dig through the git history of
> memory.c :/

Yes, most of the commits in this area are from Paolo.
I Also Cc'ed:

- Michael S. Tsirkin
- Alexey Kardashevskiy
- Igor Mammedov
- David Gibson

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

[Peter Xu]

On Tue, Apr 20, 2021 at 11:10:26AM +0200, Philippe Mathieu-Daudé wrote:
> On 4/20/21 9:00 AM, Mark Cave-Ayland wrote:
> > On 19/04/2021 21:58, Philippe Mathieu-Daudé wrote:
> > 
> >> Hi Mark,
> >>
> >> On 4/19/21 10:13 PM, Mark Cave-Ayland wrote:
> >>> 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").
> >>
> >> So if 2 contributors hit similar issues, there is something wrong with
> >> the API. I don't see your use case or mine as forbidded by the
> >> documentation in "exec/memory.h".
> >>
> >> My patch might not be the proper fix, but we need to figure out how
> >> to avoid others to hit the same problem, as it is very hard to debug.
> > 
> > I agree with this sentiment: it has taken me a while to figure out what
> > was happening, and that was only because I spotted accesses being
> > rejected with -d guest_errors.
> > 
> > From my perspective the names memory_region_dispatch_read() and
> > memory_region_dispatch_write() were the misleading part, although I
> > remember thinking it odd whilst trying to use them that I had to start
> > delving into sections etc. just to recurse a memory access.

I think it should always be a valid request to trigger memory access via the MR
layer, say, what if the caller has no address space context at all? From the
name of memory_region_dispatch_write|read I don't see either on why we should
not take care of alias mrs.  That's also the reason I'd even prefer this patch
rather than an assert.

But of course it would be great to get opinion from Paolo etc..

-- 
Peter Xu

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

[Mark Cave-Ayland]

On 20/04/2021 21:59, Peter Xu wrote:

>>> I agree with this sentiment: it has taken me a while to figure out what
>>> was happening, and that was only because I spotted accesses being
>>> rejected with -d guest_errors.
>>>
>>>  From my perspective the names memory_region_dispatch_read() and
>>> memory_region_dispatch_write() were the misleading part, although I
>>> remember thinking it odd whilst trying to use them that I had to start
>>> delving into sections etc. just to recurse a memory access.

> I think it should always be a valid request to trigger memory access via the MR
> layer, say, what if the caller has no address space context at all?

For these cases you can just use the global default address_space_memory which is the 
solution I used in the second version of my patch e.g.

     val = address_space_ldl_be(&address_space_memory, addr, attrs, &r);

> From the
> name of memory_region_dispatch_write|read I don't see either on why we should
> not take care of alias mrs.  That's also the reason I'd even prefer this patch
> rather than an assert.

The problem I see here is that this patch is breaking the abstraction between 
generating the flatview from the memory topology and dispatching a request to it.

If you look at the existing code then aliased memory regions are de-referenced at 
flatview construction time, so you end up with a flatview where each range points to 
a target (leaf or terminating) memory region plus offset. You can see this if you 
compare the output of "info mtree" with "info mtree -f" in the monitor.

This patch adds a "live" memory region alias de-reference at dispatch time when this 
should already have occurred as the flatview was constructed. I haven't had a chance 
to look at this patch in detail yet but requiring this special case just for 
de-referencing the alias at dispatch time seems wrong.

Given that the related patch "memory: Initialize MemoryRegionOps for RAM memory 
regions" is also changing the default mr->ops for ram devices in a commit originally 
from 2013 then this is another hint that the dispatch API is being used in a way in 
which it wasn't intended.


ATB,

Mark.

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

[Peter Maydell]

On Tue, 20 Apr 2021 at 21:59, Peter Xu <peterx@redhat.com> wrote:
> I think it should always be a valid request to trigger memory access via the MR
> layer, say, what if the caller has no address space context at all? From the
> name of memory_region_dispatch_write|read I don't see either on why we should
> not take care of alias mrs.  That's also the reason I'd even prefer this patch
> rather than an assert.

It's a bit of an odd case to need to do direct accesses on an MR
(so if you think you need to you should probably look for whether there's
a better way to do it), but there are sometimes reasons it's necessary or
expedient -- we have about half a dozen uses which aren't part of the core
memory system using memory_region_dispatch_* as an internal function.
So I think I agree that while we have this as an API exposed to
the rest of the system it would be nice if it Just Worked for
alias MRs.

thanks
-- PMM

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

[Peter Xu]

On Wed, Apr 21, 2021 at 11:33:55AM +0100, Mark Cave-Ayland wrote:
> On 20/04/2021 21:59, Peter Xu wrote:
> 
> > > > I agree with this sentiment: it has taken me a while to figure out what
> > > > was happening, and that was only because I spotted accesses being
> > > > rejected with -d guest_errors.
> > > > 
> > > >  From my perspective the names memory_region_dispatch_read() and
> > > > memory_region_dispatch_write() were the misleading part, although I
> > > > remember thinking it odd whilst trying to use them that I had to start
> > > > delving into sections etc. just to recurse a memory access.
> 
> > I think it should always be a valid request to trigger memory access via the MR
> > layer, say, what if the caller has no address space context at all?
> 
> For these cases you can just use the global default address_space_memory
> which is the solution I used in the second version of my patch e.g.
> 
>     val = address_space_ldl_be(&address_space_memory, addr, attrs, &r);

Yes, but what if it's an MR that does not belong to address_space_memory?  We
can still use the other AS, however that's something we don't actually need to
worry if we can directly operate on MRs.

The other thing is if there're plenty of layers of a deep hidden MR, then we'll
also need to cache all the offsets (e.g., mr A is subregion of mr B, B is
subregion of C, C belong to a AS, then we need to record offset of A+B+C to
finally be able to access this MR from the AS?) which seems an overkill if we
know exactly we want to operate on this mr.

I randomly looked at memory_region_dispatch_write(), and I think most of them
indeed do not have the AS context.  As Peter Maydell mentioned in the other
thread, if we have plenty of users use this interface, maybe there's a reason?
I'm thinking is it possible that they "worked" simply because current users
haven't used alias that much.

> 
> > From the
> > name of memory_region_dispatch_write|read I don't see either on why we should
> > not take care of alias mrs.  That's also the reason I'd even prefer this patch
> > rather than an assert.
> 
> The problem I see here is that this patch is breaking the abstraction
> between generating the flatview from the memory topology and dispatching a
> request to it.
> 
> If you look at the existing code then aliased memory regions are
> de-referenced at flatview construction time, so you end up with a flatview
> where each range points to a target (leaf or terminating) memory region plus
> offset. You can see this if you compare the output of "info mtree" with
> "info mtree -f" in the monitor.

Yes it's a fair point.  However per my understanding, address space is solving
some other problems rather than doing memory access on its own.

Staring from flatview: AS operations uses flatview indeed, and also take care
of translations (e.g. flatview_translate), however all of them are logic to
route a AS memory access to memory region only.  If we have the mr in hand, I
see nothing helping in there but extra (useless) work to resolve the mr..

One thing I noticed that may be tricky is what we did in prepare_mmio_access()
before lands e.g. memory_region_dispatch_write() in flatview_write_continue(),
as there're tricks on taking BQL or flushing MMIO buffers. I'm not sure whether
it means if we're going to have a MR layer memory access API then the user
should be aware of them (e.g., we should have BQL taken before calling MR
APIs?).  Or, we can simply move prepare_mmio_access() into the new memory
region API too?  In all cases, that's still not an obvious reason to not having
the memory region API on its own.

We also calculate size of memory ops (memory_access_size), but what if we know
them before hand?  They could be redundant calculations too.

Then we already lands memory_region_dispatch_write().

So if we see memory_region_dispatch_write() is the point where the MR access
really starts.  I don't know whether it works for RAM, but I think that's not a
major concern either..  Then there's a fair point to export it to work for all
general cases, including aliasing.

My point may not stand solid enough as I didn't really use the mr API so I
could have things overlooked... so I think if AS APIs will work for both of you
then why not. :) However I just wanted to express my understanding that AS APIs
should majorly solve some problems else comparing to (if there's going to have)
the memory region APIs, so if we're sure we don't have those AS problems
(routing to the mr not needed as we have the mr pointer; offsetting not needed
as we even know the direct offset of the mr to write to; we know exactly the
size to operate, and so on..) then it's a valid request to ask for a memory
region layer API.

Thanks,

-- 
Peter Xu