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

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

[David Hildenbrand]

Playing with memory_region_is_mapped(), I realized that memory regions
mapped via an alias behave a little bit "differently", as they don't have
their ->container set.
* memory_region_is_mapped() will never succeed for memory regions mapped
  via an alias
* memory_region_to_address_space(), memory_region_find(),
  memory_region_find_rcu(), memory_region_present() won't work, which seems
  okay, because we don't expect such memory regions getting passed to these
  functions.
* memory_region_to_absolute_addr() will result in a wrong address. As
  the result is only used for tracing, that is tolerable.

Let's clarify the documentation of memory_region_is_mapped() and change
one user that really should be checking something else.

Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>

David Hildenbrand (2):
  machine: Use host_memory_backend_is_mapped() in
    machine_consume_memdev()
  memory: Update description of memory_region_is_mapped()

 hw/core/machine.c     | 2 +-
 include/exec/memory.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH v1 1/2] machine: Use host_memory_backend_is_mapped() in machine_consume_memdev()

[David Hildenbrand]

memory_region_is_mapped() is the wrong check, we actually want to check
whether the backend is already marked mapped.

For example, memory regions mapped via an alias, such as NVDIMMs,
currently don't make memory_region_is_mapped() return "true". As the
machine is initialized before any memory devices (and thereby before
NVDIMMs are initialized), this isn't a fix but merely a cleanup.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index b8d95eec32..a1db865939 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1260,7 +1260,7 @@ MemoryRegion *machine_consume_memdev(MachineState *machine,
 {
     MemoryRegion *ret = host_memory_backend_get_memory(backend);
 
-    if (memory_region_is_mapped(ret)) {
+    if (host_memory_backend_is_mapped(backend)) {
         error_report("memory backend %s can't be used multiple times.",
                      object_get_canonical_path_component(OBJECT(backend)));
         exit(EXIT_FAILURE);
-- 
2.31.1

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

[David Hildenbrand]

memory_region_is_mapped() only indicates if the memory region is mapped
into a different memory region, and only if it is mapped directly
(->container), not via an alias.

Update the documentation to make this clearer.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/memory.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index a185b6dcb8..abc17fc3c0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2265,7 +2265,8 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr);
 
 /**
  * memory_region_is_mapped: returns true if #MemoryRegion is mapped
- * into any address space.
+ * into another #MemoryRegion directly. Will return false if the
+ * #MemoryRegion is mapped indirectly via an alias.
  *
  * @mr: a #MemoryRegion which should be checked if it's mapped
  */
-- 
2.31.1

Re: [PATCH v1 1/2] machine: Use host_memory_backend_is_mapped() in machine_consume_memdev()

[Richard Henderson]

On 10/11/21 10:45 AM, David Hildenbrand wrote:
> memory_region_is_mapped() is the wrong check, we actually want to check
> whether the backend is already marked mapped.
> 
> For example, memory regions mapped via an alias, such as NVDIMMs,
> currently don't make memory_region_is_mapped() return "true". As the
> machine is initialized before any memory devices (and thereby before
> NVDIMMs are initialized), this isn't a fix but merely a cleanup.
> 
> Signed-off-by: David Hildenbrand<david@redhat.com>
> ---
>   hw/core/machine.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Richard Henderson]

On 10/11/21 10:45 AM, David Hildenbrand wrote:
>   /**
>    * memory_region_is_mapped: returns true if #MemoryRegion is mapped
> - * into any address space.
> + * into another #MemoryRegion directly. Will return false if the
> + * #MemoryRegion is mapped indirectly via an alias.

Hmm.  I guess.  It kinda sorta sounds like a bug, but I don't know the interface well 
enough to tell.

r~

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

[Philippe Mathieu-Daudé]

On 10/11/21 23:21, Richard Henderson wrote:
> On 10/11/21 10:45 AM, David Hildenbrand wrote:
>>   /**
>>    * memory_region_is_mapped: returns true if #MemoryRegion is mapped
>> - * into any address space.
>> + * into another #MemoryRegion directly. Will return false if the
>> + * #MemoryRegion is mapped indirectly via an alias.
> 
> Hmm.  I guess.  It kinda sorta sounds like a bug, but I don't know the
> interface well enough to tell.

I tend to agree there is a generic issue with aliases, see:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg732527.html
then
https://www.mail-archive.com/qemu-devel@nongnu.org/msg799622.html
"memory: Directly dispatch alias accesses on origin memory region"

The API description looks OK to me, I'd rather change the
implementation... Maybe we need a MR_ALIAS_FOREACH() macro?

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

[David Hildenbrand]

On 12.10.21 00:17, Philippe Mathieu-Daudé wrote:
> On 10/11/21 23:21, Richard Henderson wrote:
>> On 10/11/21 10:45 AM, David Hildenbrand wrote:
>>>    /**
>>>     * memory_region_is_mapped: returns true if #MemoryRegion is mapped
>>> - * into any address space.
>>> + * into another #MemoryRegion directly. Will return false if the
>>> + * #MemoryRegion is mapped indirectly via an alias.
>>
>> Hmm.  I guess.  It kinda sorta sounds like a bug, but I don't know the
>> interface well enough to tell.
> 
> I tend to agree there is a generic issue with aliases, see:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg732527.html
> then
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg799622.html
> "memory: Directly dispatch alias accesses on origin memory region"
> 
> The API description looks OK to me, I'd rather change the
> implementation... Maybe we need a MR_ALIAS_FOREACH() macro?
> 

The API description regarding "address spaces" is certainly not
correct.

The question is if we care about aliases for
memory_region_is_mapped() for aliases. Anything that relies on ->container
is problematic when the target region is mapped via aliases -- see the cover
letter.

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.
     
     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++;
+    }
      subregion->addr = offset;
      memory_region_update_container_subregions(subregion);
  }
@@ -2561,9 +2566,14 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
  void memory_region_del_subregion(MemoryRegion *mr,
                                   MemoryRegion *subregion)
  {
+    MemoryRegion *alias;
+
      memory_region_transaction_begin();
      assert(subregion->container == mr);
      subregion->container = NULL;
+    for (alias = subregion->alias; alias; alias = alias->alias) {
+       alias->mapped_via_alias--;
+    }
      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
      memory_region_unref(subregion);
      memory_region_update_pending |= mr->enabled && subregion->enabled;
@@ -2660,7 +2670,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr)
  
  bool memory_region_is_mapped(MemoryRegion *mr)
  {
-    return mr->container ? true : false;
+    return !!mr->container || mr->mapped_via_alias;
  }
  
  /* Same as memory_region_find, but it does not add a reference to the



But then, I do wonder if we should even care.

-- 
Thanks,

David / dhildenb

Re: [PATCH v1 1/2] machine: Use host_memory_backend_is_mapped() in machine_consume_memdev()

[Igor Mammedov]

On Mon, 11 Oct 2021 19:45:20 +0200
David Hildenbrand <david@redhat.com> wrote:

> memory_region_is_mapped() is the wrong check, we actually want to check
> whether the backend is already marked mapped.
> 
> For example, memory regions mapped via an alias, such as NVDIMMs,
> currently don't make memory_region_is_mapped() return "true". As the
> machine is initialized before any memory devices (and thereby before
> NVDIMMs are initialized), this isn't a fix but merely a cleanup.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/core/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index b8d95eec32..a1db865939 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1260,7 +1260,7 @@ MemoryRegion *machine_consume_memdev(MachineState *machine,
>  {
>      MemoryRegion *ret = host_memory_backend_get_memory(backend);
>  
> -    if (memory_region_is_mapped(ret)) {
> +    if (host_memory_backend_is_mapped(backend)) {
>          error_report("memory backend %s can't be used multiple times.",
>                       object_get_canonical_path_component(OBJECT(backend)));
>          exit(EXIT_FAILURE);

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

[Igor Mammedov]

On Tue, 12 Oct 2021 08:50:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 12.10.21 00:17, Philippe Mathieu-Daudé wrote:
> > On 10/11/21 23:21, Richard Henderson wrote:  
> >> On 10/11/21 10:45 AM, David Hildenbrand wrote:  
> >>>    /**
> >>>     * memory_region_is_mapped: returns true if #MemoryRegion is mapped
> >>> - * into any address space.
> >>> + * into another #MemoryRegion directly. Will return false if the
> >>> + * #MemoryRegion is mapped indirectly via an alias.  
> >>
> >> Hmm.  I guess.  It kinda sorta sounds like a bug, but I don't know the
> >> interface well enough to tell.  
> > 
> > I tend to agree there is a generic issue with aliases, see:
> > 
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg732527.html
> > then
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg799622.html
> > "memory: Directly dispatch alias accesses on origin memory region"
> > 
> > The API description looks OK to me, I'd rather change the
> > implementation... Maybe we need a MR_ALIAS_FOREACH() macro?
> >   
> 
> The API description regarding "address spaces" is certainly not
> correct.
> 
> The question is if we care about aliases for
> memory_region_is_mapped() for aliases. Anything that relies on ->container
> is problematic when the target region is mapped via aliases -- see the cover
> letter.
> 
> 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 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)?

> +    }
>       subregion->addr = offset;
>       memory_region_update_container_subregions(subregion);
>   }
> @@ -2561,9 +2566,14 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>   void memory_region_del_subregion(MemoryRegion *mr,
>                                    MemoryRegion *subregion)
>   {
> +    MemoryRegion *alias;
> +
>       memory_region_transaction_begin();
>       assert(subregion->container == mr);
>       subregion->container = NULL;
> +    for (alias = subregion->alias; alias; alias = alias->alias) {
> +       alias->mapped_via_alias--;
> +    }
>       QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>       memory_region_unref(subregion);
>       memory_region_update_pending |= mr->enabled && subregion->enabled;
> @@ -2660,7 +2670,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr)


>   bool memory_region_is_mapped(MemoryRegion *mr)
>   {
> -    return mr->container ? true : false;
> +    return !!mr->container || mr->mapped_via_alias;
>   }
>   
>   /* Same as memory_region_find, but it does not add a reference to the
> 
> 
> 
> But then, I do wonder if we should even care.

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

[David Hildenbrand]

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

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

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

[David Hildenbrand]

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

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

[David Hildenbrand]

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

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.