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

[David Hildenbrand <david@redhat.com>]

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

-- 
Thanks,

David / dhildenb