[PATCH v2 0/3] memory: memory_region_is_mapped() cleanups

[PATCH v2 0/3] memory: memory_region_is_mapped() cleanups

[David Hildenbrand]

This is the follow-up of [1].

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 cleanup/fix the code and documentation of memory_region_is_mapped()
and change one user that really should be checking something else.

[1] https://lkml.kernel.org/r/20211011174522.14351-1-david@redhat.com

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é" <philmd@redhat.com>

David Hildenbrand (3):
  machine: Use host_memory_backend_is_mapped() in
    machine_consume_memdev()
  memory: Make memory_region_is_mapped() succeed when mapped via an
    alias
  memory: Update description of memory_region_is_mapped()

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

-- 
2.31.1

[PATCH v2 1/3] 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.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
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

Re: [PATCH v2 1/3] machine: Use host_memory_backend_is_mapped() in machine_consume_memdev()

[Philippe Mathieu-Daudé]

On 10/26/21 18:06, 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.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/core/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v2 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[David Hildenbrand]

memory_region_is_mapped() currently does not return "true" when a memory
region is mapped via an alias.

Assuming we have:
    alias (A0) -> alias (A1) -> region (R0)
Mapping A0 would currently only make memory_region_is_mapped() succeed
on A0, but not on A1 and R0.

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 to make it
consistent.

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

diff --git a/include/exec/memory.h b/include/exec/memory.h
index a185b6dcb8..35382d9870 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -707,6 +707,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 e5826faa0c..17ca896c38 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2524,8 +2524,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);
 }
@@ -2550,9 +2555,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;
@@ -2649,7 +2659,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
-- 
2.31.1

Re: [PATCH v2 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[Philippe Mathieu-Daudé]

On 10/26/21 18:06, David Hildenbrand wrote:
> memory_region_is_mapped() currently does not return "true" when a memory
> region is mapped via an alias.
> 
> Assuming we have:
>     alias (A0) -> alias (A1) -> region (R0)
> Mapping A0 would currently only make memory_region_is_mapped() succeed
> on A0, but not on A1 and R0.
> 
> 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 to make it
> consistent.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/exec/memory.h |  1 +
>  softmmu/memory.c      | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index a185b6dcb8..35382d9870 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -707,6 +707,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 e5826faa0c..17ca896c38 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2524,8 +2524,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);
>  }
> @@ -2550,9 +2555,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--;

           assert(alias->mapped_via_alias >= 0);

> +    }
>      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>      memory_region_unref(subregion);
>      memory_region_update_pending |= mr->enabled && subregion->enabled;
> @@ -2649,7 +2659,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
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[David Hildenbrand]

On 26.10.21 19:00, Philippe Mathieu-Daudé wrote:
> On 10/26/21 18:06, David Hildenbrand wrote:
>> memory_region_is_mapped() currently does not return "true" when a memory
>> region is mapped via an alias.
>>
>> Assuming we have:
>>     alias (A0) -> alias (A1) -> region (R0)
>> Mapping A0 would currently only make memory_region_is_mapped() succeed
>> on A0, but not on A1 and R0.
>>
>> 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 to make it
>> consistent.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  include/exec/memory.h |  1 +
>>  softmmu/memory.c      | 12 +++++++++++-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index a185b6dcb8..35382d9870 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -707,6 +707,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 e5826faa0c..17ca896c38 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -2524,8 +2524,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);
>>  }
>> @@ -2550,9 +2555,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--;
> 
>            assert(alias->mapped_via_alias >= 0);

Makes sense, I'll respin with that -- thanks!

-- 
Thanks,

David / dhildenb

[PATCH v2 3/3] memory: Update description of memory_region_is_mapped()

[David Hildenbrand]

Let's update the documentation, making it clearer what the semantics
of memory_region_is_mapped() actually are.

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 35382d9870..bab9349b60 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2266,7 +2266,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 memory region, which does not necessarily imply that it is
+ * mapped into an address space.
  *
  * @mr: a #MemoryRegion which should be checked if it's mapped
  */
-- 
2.31.1

Re: [PATCH v2 3/3] memory: Update description of memory_region_is_mapped()

[Philippe Mathieu-Daudé]

On 10/26/21 18:06, David Hildenbrand wrote:
> Let's update the documentation, making it clearer what the semantics
> of memory_region_is_mapped() actually are.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/exec/memory.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 0/3] memory: memory_region_is_mapped() cleanups

[Peter Xu]

On Tue, Oct 26, 2021 at 06:06:46PM +0200, David Hildenbrand wrote:
> This is the follow-up of [1].
> 
> 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.

The patches look ok to me, though I have a few pure questions to ask..

> * memory_region_is_mapped() will never succeed for memory regions mapped
>   via an alias

I think you mentioned that in commit message of patch 2 that it fixes no real
problem so far, so I'm also wondering in which case it'll help.  Say, normally
when there's an alias of another MR and we want to know whether the MR is
mapped, we simply call memory_region_is_mapped() upon the alias .

To verify my thoughts, I did look up a few memory_region_is_mapped() random
callers that used with alias and that's what they did:

Here'sthe dino.c example:

*** hw/hppa/dino.c:
gsc_to_pci_forwarding[151]     if (!memory_region_is_mapped(mem)) {
gsc_to_pci_forwarding[155]     } else if (memory_region_is_mapped(mem)) {

The "mem" points to:

        MemoryRegion *mem = &s->pci_mem_alias[i];

Which is the alias.

Another one:

*** hw/pci-host/pnv_phb3.c:
pnv_phb3_check_m32[121]        if (memory_region_is_mapped(&phb->mr_m32)) {
pnv_phb3_update_regions[1076]  if (memory_region_is_mapped(&phb->mr_m32)) {

Andmr_m32 is the alias MR itself:

    memory_region_init_alias(&phb->mr_m32, OBJECT(phb), "phb3-m32",
                             &phb->pci_mmio, start, size);

I mean, if it should always be very straightforward to fetch the alias mr, then
I'm just afraid patch 2 won't really help in any real use case but pure overhead.

And I hope we won't trigger problem with any use case where
memory_region_is_mapped() returned false previously but then it'll return true
after patch 2, because logically with the old code one can detect explicitly on
"whether this original MR is mapped somewhere, irrelevant of other alias
mappings upon this mr".  Patch 2 blurrs it from that pov.

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

Looks right.

> * memory_region_to_absolute_addr() will result in a wrong address. As
>   the result is only used for tracing, that is tolerable.

memory_region_{read|write}_accessor() seem to be only called from the address
space layer, so it looks fine even for tracing as it'll always fetch the alias,
afaiu.  Phil's patch may change that fact, though, it seems.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 0/3] memory: memory_region_is_mapped() cleanups

[David Hildenbrand]

On 27.10.21 05:53, Peter Xu wrote:
> On Tue, Oct 26, 2021 at 06:06:46PM +0200, David Hildenbrand wrote:
>> This is the follow-up of [1].
>>
>> 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.
> 

Hi Peter,

thanks for your review!

> The patches look ok to me, though I have a few pure questions to ask..
> 
>> * memory_region_is_mapped() will never succeed for memory regions mapped
>>   via an alias
> 
> I think you mentioned that in commit message of patch 2 that it fixes no real
> problem so far, so I'm also wondering in which case it'll help.  Say, normally
> when there's an alias of another MR and we want to know whether the MR is
> mapped, we simply call memory_region_is_mapped() upon the alias .

Just to recap: in v1 I proposed to just document that it doesn't work on
aliases, and folks weren't too happy to see regions mapped via aliases
being special-cased where it might just be avoided.

> 
> To verify my thoughts, I did look up a few memory_region_is_mapped() random
> callers that used with alias and that's what they did:
> 
> Here'sthe dino.c example:
> 
> *** hw/hppa/dino.c:
> gsc_to_pci_forwarding[151]     if (!memory_region_is_mapped(mem)) {
> gsc_to_pci_forwarding[155]     } else if (memory_region_is_mapped(mem)) {
> 
> The "mem" points to:
> 
>         MemoryRegion *mem = &s->pci_mem_alias[i];
> 
> Which is the alias.
> 
> Another one:
> 
> *** hw/pci-host/pnv_phb3.c:
> pnv_phb3_check_m32[121]        if (memory_region_is_mapped(&phb->mr_m32)) {
> pnv_phb3_update_regions[1076]  if (memory_region_is_mapped(&phb->mr_m32)) {
> 
> Andmr_m32 is the alias MR itself:
> 
>     memory_region_init_alias(&phb->mr_m32, OBJECT(phb), "phb3-m32",
>                              &phb->pci_mmio, start, size);
> 
> I mean, if it should always be very straightforward to fetch the alias mr, then
> I'm just afraid patch 2 won't really help in any real use case but pure overhead.

That is true as long as it's not a mapping check in generic code. Say,
we have a RAMBlock and use ->mr. Checking
memory_region_is_mapped(rb->mr) is misleading if the region is mapped
via aliases.

The reason I stumbled over this at all is a sanity check I added in

void memory_region_set_ram_discard_manager(MemoryRegion *mr,
                                           RamDiscardManager *rdm)
{
    g_assert(memory_region_is_ram(mr) && !memory_region_is_mapped(mr));
    g_assert(!rdm || !mr->rdm);
    mr->rdm = rdm;
}

If mr is only mapped via aliases (see the virtio-mem memslot series),
this check is of no value, because even if the mr would be mapped via
aliases, we would not be able to catch it.

Having that said, the check is not 100% correct, because
memory_region_is_mapped() does not indicate that we're actually mapped
into an address space. But at least for memory devices (-> target use
case of RamDiscardManager) with an underlying RAMBlock, it's pretty
reliable -- and there is no easy way to check if we're mapped into an
address space when aliases are involved.

Note that there is also a similar check in memory_region_is_mapped(),
but I'm removing that as part of the virtio-mem memslot series, because
it's not actually helpful in the context of RAMBlock migration (nothing
might be mapped, but migration code would still try migrating such a
RAMBlock and has to consider the RamDiscardManager).


Another example of a generic code check is patch #1: the only reason it
works right now is because NVDIMMs cannot exist before initial memory is
created. But yes, there is a better way of doing it when we have a
memdev at hand.

> 
> And I hope we won't trigger problem with any use case where
> memory_region_is_mapped() returned false previously but then it'll return true
> after patch 2, because logically with the old code one can detect explicitly on
> "whether this original MR is mapped somewhere, irrelevant of other alias
> mappings upon this mr".  Patch 2 blurrs it from that pov.
> 

I hope tests will catch that early. I ran some without surprises.

>> * 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.
> 
> Looks right.
> 
>> * memory_region_to_absolute_addr() will result in a wrong address. As
>>   the result is only used for tracing, that is tolerable.
> 
> memory_region_{read|write}_accessor() seem to be only called from the address
> space layer, so it looks fine even for tracing as it'll always fetch the alias,
> afaiu.  Phil's patch may change that fact, though, it seems.

Unfortunately, not much we can do: a memory region might theoretically
be mapped via aliases into different address spaces and into different
locations: there is no right answer to memory_region_to_absolute_addr()
when only having the memory region at hand without an address space.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 0/3] memory: memory_region_is_mapped() cleanups

[Peter Xu]

On Wed, Oct 27, 2021 at 09:12:08AM +0200, David Hildenbrand wrote:
> On 27.10.21 05:53, Peter Xu wrote:
> > On Tue, Oct 26, 2021 at 06:06:46PM +0200, David Hildenbrand wrote:
> >> This is the follow-up of [1].
> >>
> >> 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.
> > 
> 
> Hi Peter,
> 
> thanks for your review!
> 
> > The patches look ok to me, though I have a few pure questions to ask..
> > 
> >> * memory_region_is_mapped() will never succeed for memory regions mapped
> >>   via an alias
> > 
> > I think you mentioned that in commit message of patch 2 that it fixes no real
> > problem so far, so I'm also wondering in which case it'll help.  Say, normally
> > when there's an alias of another MR and we want to know whether the MR is
> > mapped, we simply call memory_region_is_mapped() upon the alias .
> 
> Just to recap: in v1 I proposed to just document that it doesn't work on
> aliases, and folks weren't too happy to see regions mapped via aliases
> being special-cased where it might just be avoided.
> 
> > 
> > To verify my thoughts, I did look up a few memory_region_is_mapped() random
> > callers that used with alias and that's what they did:
> > 
> > Here'sthe dino.c example:
> > 
> > *** hw/hppa/dino.c:
> > gsc_to_pci_forwarding[151]     if (!memory_region_is_mapped(mem)) {
> > gsc_to_pci_forwarding[155]     } else if (memory_region_is_mapped(mem)) {
> > 
> > The "mem" points to:
> > 
> >         MemoryRegion *mem = &s->pci_mem_alias[i];
> > 
> > Which is the alias.
> > 
> > Another one:
> > 
> > *** hw/pci-host/pnv_phb3.c:
> > pnv_phb3_check_m32[121]        if (memory_region_is_mapped(&phb->mr_m32)) {
> > pnv_phb3_update_regions[1076]  if (memory_region_is_mapped(&phb->mr_m32)) {
> > 
> > Andmr_m32 is the alias MR itself:
> > 
> >     memory_region_init_alias(&phb->mr_m32, OBJECT(phb), "phb3-m32",
> >                              &phb->pci_mmio, start, size);
> > 
> > I mean, if it should always be very straightforward to fetch the alias mr, then
> > I'm just afraid patch 2 won't really help in any real use case but pure overhead.
> 
> That is true as long as it's not a mapping check in generic code. Say,
> we have a RAMBlock and use ->mr. Checking
> memory_region_is_mapped(rb->mr) is misleading if the region is mapped
> via aliases.
> 
> The reason I stumbled over this at all is a sanity check I added in
> 
> void memory_region_set_ram_discard_manager(MemoryRegion *mr,
>                                            RamDiscardManager *rdm)
> {
>     g_assert(memory_region_is_ram(mr) && !memory_region_is_mapped(mr));
>     g_assert(!rdm || !mr->rdm);
>     mr->rdm = rdm;
> }
> 
> If mr is only mapped via aliases (see the virtio-mem memslot series),
> this check is of no value, because even if the mr would be mapped via
> aliases, we would not be able to catch it.

Yeah, this is a sane check to ask for.

> 
> Having that said, the check is not 100% correct, because
> memory_region_is_mapped() does not indicate that we're actually mapped
> into an address space. But at least for memory devices (-> target use
> case of RamDiscardManager) with an underlying RAMBlock, it's pretty
> reliable -- and there is no easy way to check if we're mapped into an
> address space when aliases are involved.
> 
> Note that there is also a similar check in memory_region_is_mapped(),
> but I'm removing that as part of the virtio-mem memslot series, because
> it's not actually helpful in the context of RAMBlock migration (nothing
> might be mapped, but migration code would still try migrating such a
> RAMBlock and has to consider the RamDiscardManager).
> 
> 
> Another example of a generic code check is patch #1: the only reason it
> works right now is because NVDIMMs cannot exist before initial memory is
> created. But yes, there is a better way of doing it when we have a
> memdev at hand.

IMHO patch 1 is actually an example showing that when we want that explicit
meaning we can simply introduce another boolean in parent struct. :)

> 
> > 
> > And I hope we won't trigger problem with any use case where
> > memory_region_is_mapped() returned false previously but then it'll return true
> > after patch 2, because logically with the old code one can detect explicitly on
> > "whether this original MR is mapped somewhere, irrelevant of other alias
> > mappings upon this mr".  Patch 2 blurrs it from that pov.
> > 
> 
> I hope tests will catch that early. I ran some without surprises.
> 
> >> * 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.
> > 
> > Looks right.
> > 
> >> * memory_region_to_absolute_addr() will result in a wrong address. As
> >>   the result is only used for tracing, that is tolerable.
> > 
> > memory_region_{read|write}_accessor() seem to be only called from the address
> > space layer, so it looks fine even for tracing as it'll always fetch the alias,
> > afaiu.  Phil's patch may change that fact, though, it seems.
> 
> Unfortunately, not much we can do: a memory region might theoretically
> be mapped via aliases into different address spaces and into different
> locations: there is no right answer to memory_region_to_absolute_addr()
> when only having the memory region at hand without an address space.

Yes.

Now I think patch 2 is fine too, it'll be nicer imho to be a new API like
memory_region_is_mapped_any() with comment showing it checks the aliases, but
no strong opinion.  If nothing fails with the change, it'll be the same indeed.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu