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