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

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

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

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