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