Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped()

[David Hildenbrand]

On 12.10.21 00:17, Philippe Mathieu-Daudé wrote:
> On 10/11/21 23:21, Richard Henderson wrote:
> > On 10/11/21 10:45 AM, David Hildenbrand wrote:
> > >    /**
> > >     * memory_region_is_mapped: returns true if #MemoryRegion is mapped
> > > - * into any address space.
> > > + * into another #MemoryRegion directly. Will return false if the
> > > + * #MemoryRegion is mapped indirectly via an alias.
> >
> > Hmm.  I guess.  It kinda sorta sounds like a bug, but I don't know the
> > interface well enough to tell.
>
> I tend to agree there is a generic issue with aliases, see:
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg732527.html
> then
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg799622.html
> "memory: Directly dispatch alias accesses on origin memory region"
>
> The API description looks OK to me, I'd rather change the
> implementation... Maybe we need a MR_ALIAS_FOREACH() macro?

The API description regarding "address spaces" is certainly not
correct.

The question is if we care about aliases for
memory_region_is_mapped() for aliases. Anything that relies on ->container
is problematic when the target region is mapped via aliases -- see the cover
letter.

Before sending this patch, I had

commit 71d15e90d513327c90d346ef73865d2db749fbba
Author: David Hildenbrand <david@redhat.com>
Date:   Thu Oct 7 11:25:18 2021 +0200

    memory: make memory_region_is_mapped() succeed when mapped via an alias
    
    memory_region_is_mapped() currently does not return "true" when a memory
    region is mapped via an alias. Let's fix that by adding a
    "mapped_via_alias" counter to memory regions and updating it accordingly
    when an alias gets (un)mapped.
    
    I am not aware of actual issues, this is rather a cleanup.
    
    Signed-off-by: David Hildenbrand <david@redhat.com>

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 75b4f600e3..93d0190202 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -728,6 +728,7 @@ struct MemoryRegion {
     const MemoryRegionOps *ops;
     void *opaque;
     MemoryRegion *container;
+    int mapped_via_alias; /* Mapped via an alias, container might be NULL */
     Int128 size;
     hwaddr addr;
     void (*destructor)(MemoryRegion *mr);
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 3bcfc3899b..1168a00819 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2535,8 +2535,13 @@ static void 
memory_region_add_subregion_common(MemoryRegion *mr,
                                                hwaddr offset,
                                                MemoryRegion *subregion)
 {
+    MemoryRegion *alias;
+
     assert(!subregion->container);
     subregion->container = mr;
+    for (alias = subregion->alias; alias; alias = alias->alias) {
+       alias->mapped_via_alias++;
+    }
     subregion->addr = offset;
     memory_region_update_container_subregions(subregion);
 }
@@ -2561,9 +2566,14 @@ void memory_region_add_subregion_overlap(MemoryRegion 
*mr,
 void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion)
 {
+    MemoryRegion *alias;
+
     memory_region_transaction_begin();
     assert(subregion->container == mr);
     subregion->container = NULL;
+    for (alias = subregion->alias; alias; alias = alias->alias) {
+       alias->mapped_via_alias--;
+    }
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
     memory_region_unref(subregion);
     memory_region_update_pending |= mr->enabled && subregion->enabled;
@@ -2660,7 +2670,7 @@ static FlatRange *flatview_lookup(FlatView *view, 
AddrRange addr)
 
 bool memory_region_is_mapped(MemoryRegion *mr)
 {
-    return mr->container ? true : false;
+    return !!mr->container || mr->mapped_via_alias;
 }
 
 /* Same as memory_region_find, but it does not add a reference to the



But then, I do wonder if we should even care.

--
Thanks,

David / dhildenb