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