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

[David Hildenbrand <david@redhat.com>]

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