Re: [PATCH v2 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[Philippe Mathieu-Daudé]

On 10/26/21 18:06, David Hildenbrand wrote:
> memory_region_is_mapped() currently does not return "true" when a memory
> region is mapped via an alias.
> 
> Assuming we have:
>     alias (A0) -> alias (A1) -> region (R0)
> Mapping A0 would currently only make memory_region_is_mapped() succeed
> on A0, but not on A1 and R0.
> 
> 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 to make it
> consistent.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/exec/memory.h |  1 +
>  softmmu/memory.c      | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index a185b6dcb8..35382d9870 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -707,6 +707,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 e5826faa0c..17ca896c38 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2524,8 +2524,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);
>  }
> @@ -2550,9 +2555,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--;

           assert(alias->mapped_via_alias >= 0);

> +    }
>      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>      memory_region_unref(subregion);
>      memory_region_update_pending |= mr->enabled && subregion->enabled;
> @@ -2649,7 +2659,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
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>