Re: [PATCH v2 00/11] memory: Forbid mapping AddressSpace root MemoryRegion

[Peter Xu <peterx@redhat.com>]

On Sat, Apr 17, 2021 at 12:30:17PM +0200, Philippe Mathieu-Daudé wrote:
> AddressSpace are physical address view and shouldn't be using
> non-zero base address. The correct way to map a MR used as AS
> root is to use a MR alias.

Today when I rethink this, I figured another way (maybe easier?) to fix the
issue.

The major problem so far we had is that mr->addr can be anything for a root mr
if it's added as subregion of another mr.

E.g. in current implementation of mtree_print_mr() MR.addr is constantly used
as an offset value:

    cur_start = base + mr->addr;

However afaict mr->addr is defined as "relative offset of this mr to its
container", as in memory_region_add_subregion_common().  Say, mr->addr is
undefined from that pov if mr->container==NULL, as this MR belongs to nobody.
And if it's defined, it's only meaning is in its container's context (or say,
address space) only.

That said, when we do mtree_print_mr(), another solution could be as simple as,
not referencing mr->addr if we _know_ we're working on the root mr, as this is
definitely _not_ in the context of the mr's container, even if it has one
container after all:

---8<---
diff --git a/softmmu/memory.c b/softmmu/memory.c
index d4493ef9e43..d71fb8ecc89 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2940,7 +2940,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
         return;
     }
 
-    cur_start = base + mr->addr;
+    cur_start = base + (level == 1) ? 0 : mr->addr;
     cur_end = cur_start + MR_SIZE(mr->size);
 
     /*
---8<---

Phil, do you think it'll work too to fix the strange offset value dumped in
"info mtree"?

I don't know (even if it works, perhaps I've missed something) which is better,
as current series seems cleaner, then any mr will either belong to a AS or a MR
(never both!), but just raise it up.

Thanks,

--
Peter Xu