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

[David Hildenbrand <david@redhat.com>]

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.

-- 
Thanks,

David / dhildenb