[PATCH v1 0/2] xen: mapcache: Fix unmapping of first the entry in a bucket

[PATCH v1 0/2] xen: mapcache: Fix unmapping of first the entry in a bucket

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

This fixes the unmapping of the first mapping in a bucket of a mapcache.

We also add error handling to qemu_ram_block_from_host() to bail out when
xen_ram_addr_from_mapcache() doesn't find an existing mapping.

Cheers,
Edgar

Edgar E. Iglesias (2):
  physmem: Bail out qemu_ram_block_from_host() for invalid ram addrs
  xen: mapcache: Fix unmapping of first entries in buckets

 hw/xen/xen-mapcache.c | 12 +++++++++++-
 system/physmem.c      |  4 ++++
 2 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v1 1/2] physmem: Bail out qemu_ram_block_from_host() for invalid ram addrs

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

Bail out in qemu_ram_block_from_host() when
xen_ram_addr_from_mapcache() does not find an existing
mapping.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 system/physmem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/system/physmem.c b/system/physmem.c
index 33d09f7571..59d1576c2b 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
         ram_addr_t ram_addr;
         RCU_READ_LOCK_GUARD();
         ram_addr = xen_ram_addr_from_mapcache(ptr);
+        if (ram_addr == RAM_ADDR_INVALID) {
+            return NULL;
+        }
+
         block = qemu_get_ram_block(ram_addr);
         if (block) {
             *offset = ram_addr - block->offset;
-- 
2.43.0

Re: [PATCH v1 1/2] physmem: Bail out qemu_ram_block_from_host() for invalid ram addrs

[Alex Bennée]

"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>
> Bail out in qemu_ram_block_from_host() when
> xen_ram_addr_from_mapcache() does not find an existing
> mapping.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  system/physmem.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 33d09f7571..59d1576c2b 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
>          ram_addr_t ram_addr;
>          RCU_READ_LOCK_GUARD();
>          ram_addr = xen_ram_addr_from_mapcache(ptr);
> +        if (ram_addr == RAM_ADDR_INVALID) {
> +            return NULL;
> +        }
> +

Isn't this indicative of a failure? Should there at least be a trace
point for failed mappings?

>          block = qemu_get_ram_block(ram_addr);
>          if (block) {
>              *offset = ram_addr - block->offset;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v1 1/2] physmem: Bail out qemu_ram_block_from_host() for invalid ram addrs

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On Thu, Jul 4, 2024 at 1:26 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> "Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:
>
> > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> >
> > Bail out in qemu_ram_block_from_host() when
> > xen_ram_addr_from_mapcache() does not find an existing
> > mapping.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > ---
> >  system/physmem.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/system/physmem.c b/system/physmem.c
> > index 33d09f7571..59d1576c2b 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr,
> bool round_offset,
> >          ram_addr_t ram_addr;
> >          RCU_READ_LOCK_GUARD();
> >          ram_addr = xen_ram_addr_from_mapcache(ptr);
> > +        if (ram_addr == RAM_ADDR_INVALID) {
> > +            return NULL;
> > +        }
> > +
>
> Isn't this indicative of a failure? Should there at least be a trace
> point for failed mappings?
>
>
Yes but there are already trace points for the failure cases inside
xen_ram_addr_from_mapcache().
Do those address your concerns or do you think we need additional trace
points?

Cheers,
Edgar


> >          block = qemu_get_ram_block(ram_addr);
> >          if (block) {
> >              *offset = ram_addr - block->offset;
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

[-- Attachment #2: Type: text/html, Size: 2357 bytes --]

Re: [PATCH v1 1/2] physmem: Bail out qemu_ram_block_from_host() for invalid ram addrs

[Alex Bennée]

"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> On Thu, Jul 4, 2024 at 1:26 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  "Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:
>
>  > From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>  >
>  > Bail out in qemu_ram_block_from_host() when
>  > xen_ram_addr_from_mapcache() does not find an existing
>  > mapping.
>  >
>  > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>  > ---
>  >  system/physmem.c | 4 ++++
>  >  1 file changed, 4 insertions(+)
>  >
>  > diff --git a/system/physmem.c b/system/physmem.c
>  > index 33d09f7571..59d1576c2b 100644
>  > --- a/system/physmem.c
>  > +++ b/system/physmem.c
>  > @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
>  >          ram_addr_t ram_addr;
>  >          RCU_READ_LOCK_GUARD();
>  >          ram_addr = xen_ram_addr_from_mapcache(ptr);
>  > +        if (ram_addr == RAM_ADDR_INVALID) {
>  > +            return NULL;
>  > +        }
>  > +
>
>  Isn't this indicative of a failure? Should there at least be a trace
>  point for failed mappings?
>
> Yes but there are already trace points for the failure cases inside xen_ram_addr_from_mapcache().
> Do those address your concerns or do you think we need additional
> trace points?

Ahh that will do.

I am curious for the reasons why we might not have an entry in the
mapcache. I guess the trace_xen_map_cache() covers all insertions into
the cache although you need to check trace_xen_map_cache_return() to see
if anything failed.

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v1 1/2] physmem: Bail out qemu_ram_block_from_host() for invalid ram addrs

[Stefano Stabellini]

On Tue, 2 Jul 2024, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> Bail out in qemu_ram_block_from_host() when
> xen_ram_addr_from_mapcache() does not find an existing
> mapping.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  system/physmem.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 33d09f7571..59d1576c2b 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -2277,6 +2277,10 @@ RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
>          ram_addr_t ram_addr;
>          RCU_READ_LOCK_GUARD();
>          ram_addr = xen_ram_addr_from_mapcache(ptr);
> +        if (ram_addr == RAM_ADDR_INVALID) {
> +            return NULL;
> +        }
> +
>          block = qemu_get_ram_block(ram_addr);
>          if (block) {
>              *offset = ram_addr - block->offset;
> -- 
> 2.43.0
> 

[PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>

This fixes the clobbering of the entry->next pointer when
unmapping the first entry in a bucket of a mapcache.

Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
Reported-by: Anthony PERARD <anthony.perard@vates.tech>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 hw/xen/xen-mapcache.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
index 5f23b0adbe..18ba7b1d8f 100644
--- a/hw/xen/xen-mapcache.c
+++ b/hw/xen/xen-mapcache.c
@@ -597,7 +597,17 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
         pentry->next = entry->next;
         g_free(entry);
     } else {
-        memset(entry, 0, sizeof *entry);
+        /*
+         * Invalidate mapping but keep entry->next pointing to the rest
+         * of the list.
+         *
+         * Note that lock is already zero here, otherwise we don't unmap.
+         */
+        entry->paddr_index = 0;
+        entry->vaddr_base = NULL;
+        entry->valid_mapping = NULL;
+        entry->flags = 0;
+        entry->size = 0;
     }
 }
 
-- 
2.43.0

Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Anthony PERARD]

On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> This fixes the clobbering of the entry->next pointer when
> unmapping the first entry in a bucket of a mapcache.
> 
> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
> Reported-by: Anthony PERARD <anthony.perard@vates.tech>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  hw/xen/xen-mapcache.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> index 5f23b0adbe..18ba7b1d8f 100644
> --- a/hw/xen/xen-mapcache.c
> +++ b/hw/xen/xen-mapcache.c
> @@ -597,7 +597,17 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
>          pentry->next = entry->next;
>          g_free(entry);
>      } else {
> -        memset(entry, 0, sizeof *entry);
> +        /*
> +         * Invalidate mapping but keep entry->next pointing to the rest
> +         * of the list.
> +         *
> +         * Note that lock is already zero here, otherwise we don't unmap.
> +         */
> +        entry->paddr_index = 0;
> +        entry->vaddr_base = NULL;
> +        entry->valid_mapping = NULL;
> +        entry->flags = 0;
> +        entry->size = 0;

This kind of feels like mc->entry should be an array of pointer rather
than an array of MapCacheEntry but that seems to work well enough and
not the first time entries are been cleared like that.

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Alex Bennée]

Anthony PERARD <anthony.perard@vates.tech> writes:

> On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote:
>> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>> 
>> This fixes the clobbering of the entry->next pointer when
>> unmapping the first entry in a bucket of a mapcache.
>> 
>> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
>> Reported-by: Anthony PERARD <anthony.perard@vates.tech>
>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>> ---
>>  hw/xen/xen-mapcache.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
>> index 5f23b0adbe..18ba7b1d8f 100644
>> --- a/hw/xen/xen-mapcache.c
>> +++ b/hw/xen/xen-mapcache.c
>> @@ -597,7 +597,17 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
>>          pentry->next = entry->next;
>>          g_free(entry);
>>      } else {
>> -        memset(entry, 0, sizeof *entry);
>> +        /*
>> +         * Invalidate mapping but keep entry->next pointing to the rest
>> +         * of the list.
>> +         *
>> +         * Note that lock is already zero here, otherwise we don't unmap.
>> +         */
>> +        entry->paddr_index = 0;
>> +        entry->vaddr_base = NULL;
>> +        entry->valid_mapping = NULL;
>> +        entry->flags = 0;
>> +        entry->size = 0;
>
> This kind of feels like mc->entry should be an array of pointer rather
> than an array of MapCacheEntry but that seems to work well enough and
> not the first time entries are been cleared like that.

The use of a hand rolled list is a bit of a concern considering QEMU and
Glib both provide various abstractions used around the rest of the code
base. The original patch that introduces the mapcache doesn't tell me
much about access patterns for the cache, just that it is trying to
solve memory exhaustion issues with lots of dynamic small mappings.

Maybe a simpler structure is desirable?

We also have an interval tree implementation ("qemu/interval-tree.h") if
what we really want is a sorted tree of memory that can be iterated
locklessly.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Edgar E. Iglesias via]

On Thu, Jul 04, 2024 at 05:44:52PM +0100, Alex Bennée wrote:
> Anthony PERARD <anthony.perard@vates.tech> writes:
> 
> > On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote:
> >> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> >> 
> >> This fixes the clobbering of the entry->next pointer when
> >> unmapping the first entry in a bucket of a mapcache.
> >> 
> >> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
> >> Reported-by: Anthony PERARD <anthony.perard@vates.tech>
> >> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> >> ---
> >>  hw/xen/xen-mapcache.c | 12 +++++++++++-
> >>  1 file changed, 11 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> >> index 5f23b0adbe..18ba7b1d8f 100644
> >> --- a/hw/xen/xen-mapcache.c
> >> +++ b/hw/xen/xen-mapcache.c
> >> @@ -597,7 +597,17 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
> >>          pentry->next = entry->next;
> >>          g_free(entry);
> >>      } else {
> >> -        memset(entry, 0, sizeof *entry);
> >> +        /*
> >> +         * Invalidate mapping but keep entry->next pointing to the rest
> >> +         * of the list.
> >> +         *
> >> +         * Note that lock is already zero here, otherwise we don't unmap.
> >> +         */
> >> +        entry->paddr_index = 0;
> >> +        entry->vaddr_base = NULL;
> >> +        entry->valid_mapping = NULL;
> >> +        entry->flags = 0;
> >> +        entry->size = 0;
> >
> > This kind of feels like mc->entry should be an array of pointer rather
> > than an array of MapCacheEntry but that seems to work well enough and
> > not the first time entries are been cleared like that.
> 
> The use of a hand rolled list is a bit of a concern considering QEMU and
> Glib both provide various abstractions used around the rest of the code
> base. The original patch that introduces the mapcache doesn't tell me
> much about access patterns for the cache, just that it is trying to
> solve memory exhaustion issues with lots of dynamic small mappings.
> 
> Maybe a simpler structure is desirable?
> 
> We also have an interval tree implementation ("qemu/interval-tree.h") if
> what we really want is a sorted tree of memory that can be iterated
> locklessly.
> 

Yes, it would be interesting to benchmark other options.
I agree that we should at minimum reuse existing lists/hash tables.

We've also had some discussions around removing it partially or alltogether but
there are some concerns around that. We're going to need something to
keep track of grants. For 32-bit hosts, it's a problem to exhaust virtual
address-space if mapping all of the guest (are folks still using 32-bit hosts?).
There may be other issues aswell.

Some benefits are that we'll remove some of the complexity and latency for mapping
and unmapping stuff continously.

Cheers,
Edgar

Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

On Thu, Jul 4, 2024 at 9:48 PM Edgar E. Iglesias <edgar.iglesias@amd.com>
wrote:

> On Thu, Jul 04, 2024 at 05:44:52PM +0100, Alex Bennée wrote:
> > Anthony PERARD <anthony.perard@vates.tech> writes:
> >
> > > On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote:
> > >> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> > >>
> > >> This fixes the clobbering of the entry->next pointer when
> > >> unmapping the first entry in a bucket of a mapcache.
> > >>
> > >> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
> > >> Reported-by: Anthony PERARD <anthony.perard@vates.tech>
> > >> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> > >> ---
> > >>  hw/xen/xen-mapcache.c | 12 +++++++++++-
> > >>  1 file changed, 11 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> > >> index 5f23b0adbe..18ba7b1d8f 100644
> > >> --- a/hw/xen/xen-mapcache.c
> > >> +++ b/hw/xen/xen-mapcache.c
> > >> @@ -597,7 +597,17 @@ static void
> xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
> > >>          pentry->next = entry->next;
> > >>          g_free(entry);
> > >>      } else {
> > >> -        memset(entry, 0, sizeof *entry);
> > >> +        /*
> > >> +         * Invalidate mapping but keep entry->next pointing to the
> rest
> > >> +         * of the list.
> > >> +         *
> > >> +         * Note that lock is already zero here, otherwise we don't
> unmap.
> > >> +         */
> > >> +        entry->paddr_index = 0;
> > >> +        entry->vaddr_base = NULL;
> > >> +        entry->valid_mapping = NULL;
> > >> +        entry->flags = 0;
> > >> +        entry->size = 0;
> > >
> > > This kind of feels like mc->entry should be an array of pointer rather
> > > than an array of MapCacheEntry but that seems to work well enough and
> > > not the first time entries are been cleared like that.
> >
> > The use of a hand rolled list is a bit of a concern considering QEMU and
> > Glib both provide various abstractions used around the rest of the code
> > base. The original patch that introduces the mapcache doesn't tell me
> > much about access patterns for the cache, just that it is trying to
> > solve memory exhaustion issues with lots of dynamic small mappings.
> >
> > Maybe a simpler structure is desirable?
> >
> > We also have an interval tree implementation ("qemu/interval-tree.h") if
> > what we really want is a sorted tree of memory that can be iterated
> > locklessly.
> >
>
> Yes, it would be interesting to benchmark other options.
> I agree that we should at minimum reuse existing lists/hash tables.
>
> We've also had some discussions around removing it partially or
> alltogether but
> there are some concerns around that. We're going to need something to
> keep track of grants. For 32-bit hosts, it's a problem to exhaust virtual
> address-space if mapping all of the guest (are folks still using 32-bit
> hosts?).
> There may be other issues aswell.
>
> Some benefits are that we'll remove some of the complexity and latency for
> mapping
> and unmapping stuff continously.
>
>
One more thing I forgot to add is that IMO, these larger longer term
changes should not block this tiny bugfix...

Cheers,
Edgar

[-- Attachment #2: Type: text/html, Size: 4408 bytes --]

Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Alex Bennée]

"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> On Thu, Jul 4, 2024 at 9:48 PM Edgar E. Iglesias <edgar.iglesias@amd.com> wrote:
>
>  On Thu, Jul 04, 2024 at 05:44:52PM +0100, Alex Bennée wrote:
>  > Anthony PERARD <anthony.perard@vates.tech> writes:
>  > 
>  > > On Tue, Jul 02, 2024 at 12:44:21AM +0200, Edgar E. Iglesias wrote:
>  > >> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
>  > >> 
>  > >> This fixes the clobbering of the entry->next pointer when
>  > >> unmapping the first entry in a bucket of a mapcache.
>  > >> 
>  > >> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
>  > >> Reported-by: Anthony PERARD <anthony.perard@vates.tech>
>  > >> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
>  > >> ---
>  > >>  hw/xen/xen-mapcache.c | 12 +++++++++++-
>  > >>  1 file changed, 11 insertions(+), 1 deletion(-)
>  > >> 
>  > >> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
>  > >> index 5f23b0adbe..18ba7b1d8f 100644
>  > >> --- a/hw/xen/xen-mapcache.c
>  > >> +++ b/hw/xen/xen-mapcache.c
>  > >> @@ -597,7 +597,17 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
>  > >>          pentry->next = entry->next;
>  > >>          g_free(entry);
>  > >>      } else {
>  > >> -        memset(entry, 0, sizeof *entry);
>  > >> +        /*
>  > >> +         * Invalidate mapping but keep entry->next pointing to the rest
>  > >> +         * of the list.
>  > >> +         *
>  > >> +         * Note that lock is already zero here, otherwise we don't unmap.
>  > >> +         */
>  > >> +        entry->paddr_index = 0;
>  > >> +        entry->vaddr_base = NULL;
>  > >> +        entry->valid_mapping = NULL;
>  > >> +        entry->flags = 0;
>  > >> +        entry->size = 0;
>  > >
>  > > This kind of feels like mc->entry should be an array of pointer rather
>  > > than an array of MapCacheEntry but that seems to work well enough and
>  > > not the first time entries are been cleared like that.
>  > 
>  > The use of a hand rolled list is a bit of a concern considering QEMU and
>  > Glib both provide various abstractions used around the rest of the code
>  > base. The original patch that introduces the mapcache doesn't tell me
>  > much about access patterns for the cache, just that it is trying to
>  > solve memory exhaustion issues with lots of dynamic small mappings.
>  > 
>  > Maybe a simpler structure is desirable?
>  > 
>  > We also have an interval tree implementation ("qemu/interval-tree.h") if
>  > what we really want is a sorted tree of memory that can be iterated
>  > locklessly.
>  > 
>
>  Yes, it would be interesting to benchmark other options.
>  I agree that we should at minimum reuse existing lists/hash tables.
>
>  We've also had some discussions around removing it partially or alltogether but
>  there are some concerns around that. We're going to need something to
>  keep track of grants. For 32-bit hosts, it's a problem to exhaust virtual
>  address-space if mapping all of the guest (are folks still using 32-bit hosts?).
>  There may be other issues aswell.
>
>  Some benefits are that we'll remove some of the complexity and latency for mapping
>  and unmapping stuff continously.
>
> One more thing I forgot to add is that IMO, these larger longer term
> changes should not block this tiny bugfix...

Agreed.

>
> Cheers,
> Edgar 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v1 2/2] xen: mapcache: Fix unmapping of first entries in buckets

[Stefano Stabellini]

On Tue, 2 Jul 2024, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@amd.com>
> 
> This fixes the clobbering of the entry->next pointer when
> unmapping the first entry in a bucket of a mapcache.
> 
> Fixes: 123acd816d ("xen: mapcache: Unmap first entries in buckets")
> Reported-by: Anthony PERARD <anthony.perard@vates.tech>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  hw/xen/xen-mapcache.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> index 5f23b0adbe..18ba7b1d8f 100644
> --- a/hw/xen/xen-mapcache.c
> +++ b/hw/xen/xen-mapcache.c
> @@ -597,7 +597,17 @@ static void xen_invalidate_map_cache_entry_unlocked(MapCache *mc,
>          pentry->next = entry->next;
>          g_free(entry);
>      } else {
> -        memset(entry, 0, sizeof *entry);
> +        /*
> +         * Invalidate mapping but keep entry->next pointing to the rest
> +         * of the list.
> +         *
> +         * Note that lock is already zero here, otherwise we don't unmap.
> +         */
> +        entry->paddr_index = 0;
> +        entry->vaddr_base = NULL;
> +        entry->valid_mapping = NULL;
> +        entry->flags = 0;
> +        entry->size = 0;
>      }
>  }
>  
> -- 
> 2.43.0
>