[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped() |
Date: |
Tue, 12 Oct 2021 10:53:00 +0200 |
On Tue, 12 Oct 2021 08:50:25 +0200
David Hildenbrand <david@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 <david@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 <david@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.
- [PATCH v1 0/2] memory: Update description of memory_region_is_mapped(), David Hildenbrand, 2021/10/11
- [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), David Hildenbrand, 2021/10/11
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), Richard Henderson, 2021/10/11
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), Philippe Mathieu-Daudé, 2021/10/11
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), David Hildenbrand, 2021/10/12
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(),
Igor Mammedov <=
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), David Hildenbrand, 2021/10/12
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), Igor Mammedov, 2021/10/12
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), David Hildenbrand, 2021/10/12
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), David Hildenbrand, 2021/10/13
- Re: [PATCH v1 2/2] memory: Update description of memory_region_is_mapped(), Igor Mammedov, 2021/10/13
[PATCH v1 1/2] machine: Use host_memory_backend_is_mapped() in machine_consume_memdev(), David Hildenbrand, 2021/10/11