On Tue, 12 Oct 2021 08:50:25 +0200 David Hildenbrand <da...@redhat.com> wrote:
> On 12.10.21 00:17, Philippe Mathieu-Daudé wrote: > > On 10/11/21 23:21, Richard Henderson wrote: > >> On 10/11/21 10:45 AM, David Hildenbrand wrote: > >>> /** > >>> * memory_region_is_mapped: returns true if #MemoryRegion is mapped > >>> - * into any address space. > >>> + * into another #MemoryRegion directly. Will return false if the > >>> + * #MemoryRegion is mapped indirectly via an alias. > >> > >> Hmm. I guess. It kinda sorta sounds like a bug, but I don't know the > >> interface well enough to tell. > > > > I tend to agree there is a generic issue with aliases, see: > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg732527.html > > then > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg799622.html > > "memory: Directly dispatch alias accesses on origin memory region" > > > > The API description looks OK to me, I'd rather change the > > implementation... Maybe we need a MR_ALIAS_FOREACH() macro? > > > > The API description regarding "address spaces" is certainly not > correct. > > The question is if we care about aliases for > memory_region_is_mapped() for aliases. Anything that relies on ->container > is problematic when the target region is mapped via aliases -- see the cover > letter. > > Before sending this patch, I had > > commit 71d15e90d513327c90d346ef73865d2db749fbba > Author: David Hildenbrand <da...@redhat.com> > Date: Thu Oct 7 11:25:18 2021 +0200 > > memory: make memory_region_is_mapped() succeed when mapped via an alias > > memory_region_is_mapped() currently does not return "true" when a memory > region is mapped via an alias. Let's fix that by adding a > "mapped_via_alias" counter to memory regions and updating it accordingly > when an alias gets (un)mapped. this needs a clarification, is memory_region_is_mapped() used on aliased memory region or on alias? > I am not aware of actual issues, this is rather a cleanup. > > Signed-off-by: David Hildenbrand <da...@redhat.com> > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 75b4f600e3..93d0190202 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -728,6 +728,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 3bcfc3899b..1168a00819 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -2535,8 +2535,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++; it it necessary to update mapped_via_alias for intermediate aliases? Why not just update on counter only on leaf (aliased region)? > + } > subregion->addr = offset; > memory_region_update_container_subregions(subregion); > } > @@ -2561,9 +2566,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; > @@ -2660,7 +2670,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 > > > > But then, I do wonder if we should even care.
