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

[David Hildenbrand]

> > 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 think right now it's barely used with aliases 
(memory_region_is_mapped(alias)), at least I am not aware of users.

What's more likely is that the final memory region will be the target of 
memory_region_is_mapped().

The question is: which semantics do we want to have so we can properly 
document and eventually fix.

> >       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)?

Assume we have alias0 -> alias1 -> region and map alias0.

Once alias0 is mapped it will have ->container set and 
memory_region_is_mapped(alias0) will return "true".

With my patch, both, "alias1" and the region will be marked 
"mapped_via_alias" and memory_region_is_mapped() will succeed on both of 
them. With what you propose, memory_region_is_mapped() would only 
succeed on the region (well, and on alias 0) but not on alias1.

--
Thanks,

David / dhildenb