If we have request without lock and hit unlocked or invalid
entry during the search, we remap it immediately,
even if we have matching entry in next entries in bucket.
This leads to duplication of mappings of the same size,
and to possibility of selecting the wrong element
during invalidation and underflow it's entry->lock counter

Signed-off-by: Aleksandr Partanen <alex.pentagrid@gmail.com>
---
 hw/xen/xen-mapcache.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
index 698b5c53ed..2c8f861fdb 100644
--- a/hw/xen/xen-mapcache.c
+++ b/hw/xen/xen-mapcache.c
@@ -376,12 +376,12 @@ tryagain:
 
     entry = &mc->entry[address_index % mc->nr_buckets];
 
-    while (entry && (lock || entry->lock) && entry->vaddr_base &&
-            (entry->paddr_index != address_index || entry->size != cache_size ||
+    while (entry && (!entry->vaddr_base ||
+            entry->paddr_index != address_index || entry->size != cache_size ||
              !test_bits(address_offset >> XC_PAGE_SHIFT,
                  test_bit_size >> XC_PAGE_SHIFT,
                  entry->valid_mapping))) {
-        if (!free_entry && !entry->lock) {
+        if (!free_entry && (!entry->lock || !entry->vaddr_base)) {
             free_entry = entry;
             free_pentry = pentry;
         }
-- 
2.39.5

On Thu, 10 Apr 2025, Aleksandr Partanen wrote:
> If we have request without lock and hit unlocked or invalid
> entry during the search, we remap it immediately,
> even if we have matching entry in next entries in bucket.
> This leads to duplication of mappings of the same size,
> and to possibility of selecting the wrong element
> during invalidation and underflow it's entry->lock counter
> 
> Signed-off-by: Aleksandr Partanen <alex.pentagrid@gmail.com>

Hi Aleksandr, thanks for the patch, it looks correct to me.

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


Edgar, would you be able to give it a look as well to make sure?


> ---
>  hw/xen/xen-mapcache.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> index 698b5c53ed..2c8f861fdb 100644
> --- a/hw/xen/xen-mapcache.c
> +++ b/hw/xen/xen-mapcache.c
> @@ -376,12 +376,12 @@ tryagain:
>  
>      entry = &mc->entry[address_index % mc->nr_buckets];
>  
> -    while (entry && (lock || entry->lock) && entry->vaddr_base &&
> -            (entry->paddr_index != address_index || entry->size != cache_size ||
> +    while (entry && (!entry->vaddr_base ||
> +            entry->paddr_index != address_index || entry->size != cache_size ||
>               !test_bits(address_offset >> XC_PAGE_SHIFT,
>                   test_bit_size >> XC_PAGE_SHIFT,
>                   entry->valid_mapping))) {
> -        if (!free_entry && !entry->lock) {
> +        if (!free_entry && (!entry->lock || !entry->vaddr_base)) {
>              free_entry = entry;
>              free_pentry = pentry;
>          }
> -- 
> 2.39.5
>

On Fri, Apr 18, 2025 at 04:39:04PM -0700, Stefano Stabellini wrote:
> On Thu, 10 Apr 2025, Aleksandr Partanen wrote:
> > If we have request without lock and hit unlocked or invalid
> > entry during the search, we remap it immediately,
> > even if we have matching entry in next entries in bucket.
> > This leads to duplication of mappings of the same size,
> > and to possibility of selecting the wrong element
> > during invalidation and underflow it's entry->lock counter
> > 
> > Signed-off-by: Aleksandr Partanen <alex.pentagrid@gmail.com>
> 
> Hi Aleksandr, thanks for the patch, it looks correct to me.
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
> Edgar, would you be able to give it a look as well to make sure?


Looks good to me too, thanks!

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@amd.com>




> 
> 
> > ---
> >  hw/xen/xen-mapcache.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/xen/xen-mapcache.c b/hw/xen/xen-mapcache.c
> > index 698b5c53ed..2c8f861fdb 100644
> > --- a/hw/xen/xen-mapcache.c
> > +++ b/hw/xen/xen-mapcache.c
> > @@ -376,12 +376,12 @@ tryagain:
> >  
> >      entry = &mc->entry[address_index % mc->nr_buckets];
> >  
> > -    while (entry && (lock || entry->lock) && entry->vaddr_base &&
> > -            (entry->paddr_index != address_index || entry->size != cache_size ||
> > +    while (entry && (!entry->vaddr_base ||
> > +            entry->paddr_index != address_index || entry->size != cache_size ||
> >               !test_bits(address_offset >> XC_PAGE_SHIFT,
> >                   test_bit_size >> XC_PAGE_SHIFT,
> >                   entry->valid_mapping))) {
> > -        if (!free_entry && !entry->lock) {
> > +        if (!free_entry && (!entry->lock || !entry->vaddr_base)) {
> >              free_entry = entry;
> >              free_pentry = pentry;
> >          }
> > -- 
> > 2.39.5
> >