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

[Igor Mammedov]

On Wed, 13 Oct 2021 09:14:35 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 12.10.21 12:09, David Hildenbrand wrote:
> >>
> >> The less confusing would be one where check works for any memory region
> >> involved.  
> > 
> > Exactly, so for any alias, even in-between another alias and the target.
> >   
> >>      
> >>>>
> >>>>       
> >>>>>         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).
> >>  
> > 
> > Sure, we could also let memory_region_is_mapped() walk all aliases to
> > the leaf. Not sure though, if it really simplifies things. It merely
> > adds another loop and doesn't get rid of the others :) But I don't
> > particularly care.
> >   
> 
> I just realized that this might not be what we want: we could get false 
> positives when a memory region is referenced via multiple alias and only 
> one of them is mapped. memory_region_is_mapped() could return "true" for 
> an alias that isn't actually mapped.
Agreed, that would be inconsistent.