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.