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

[David Hildenbrand <david@redhat.com>]

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