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

[Igor Mammedov]

On Tue, 12 Oct 2021 11:28:56 +0200
David Hildenbrand <david@redhat.com> wrote:

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

The less confusing would be one where check works for any memory region
involved. 
 
> > 
> >   
> >>       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.

as long as add_subregion increments counter on leaf it doesn't matter
how many intermediate aliases are there. Check on every one of them
should end up at the leaf counter (at expense of traversing
chain on every check but less state to track/think about).