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 <pet...@redhat.com>
Thanks,
--
Peter Xu
