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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 12 Oct 2021 08:50:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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.

this needs a clarification,
is memory_region_is_mapped() used on aliased memory region or on alias?


>      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++;

it it necessary to update mapped_via_alias for intermediate aliases?
Why not just update on counter only on leaf (aliased region)?

> +    }
>       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.