[PATCH] softmmu/memory: fix memory_region_ioeventfd_equal()

Elena Afanasova posted 1 patch 3 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201017210102.26036-2-eafanasova@gmail.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
softmmu/memory.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
[PATCH] softmmu/memory: fix memory_region_ioeventfd_equal()
Posted by Elena Afanasova 3 years, 6 months ago
Eventfd can be registered with a zero length when fast_mmio is true.
Handle this case properly when dispatching through QEMU.

Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
---
 softmmu/memory.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 403ff3abc9..3ca2154a64 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -203,10 +203,17 @@ static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
 }
 
 static bool memory_region_ioeventfd_equal(MemoryRegionIoeventfd *a,
-                                          MemoryRegionIoeventfd *b)
-{
-    return !memory_region_ioeventfd_before(a, b)
-        && !memory_region_ioeventfd_before(b, a);
+                                          MemoryRegionIoeventfd *mrb)
+{
+    if (int128_eq(a->addr.start, mrb->addr.start) &&
+            (!int128_nz(mrb->addr.size) ||
+             int128_eq(a->addr.size, mrb->addr.size)) &&
+            (a->match_data == mrb->match_data) &&
+            ((mrb->match_data && (a->data == mrb->data)) || !mrb->match_data) &&
+            (a->e == mrb->e))
+        return true;
+
+    return false;
 }
 
 /* Range of memory in the global map.  Addresses are absolute. */
-- 
2.25.1


Re: [PATCH] softmmu/memory: fix memory_region_ioeventfd_equal()
Posted by Stefan Hajnoczi 3 years, 6 months ago
On Sat, Oct 17, 2020 at 02:01:02PM -0700, Elena Afanasova wrote:
> Eventfd can be registered with a zero length when fast_mmio is true.
> Handle this case properly when dispatching through QEMU.
> 
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---
>  softmmu/memory.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 403ff3abc9..3ca2154a64 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -203,10 +203,17 @@ static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
>  }
>  
>  static bool memory_region_ioeventfd_equal(MemoryRegionIoeventfd *a,
> -                                          MemoryRegionIoeventfd *b)
> -{
> -    return !memory_region_ioeventfd_before(a, b)
> -        && !memory_region_ioeventfd_before(b, a);
> +                                          MemoryRegionIoeventfd *mrb)

Why rename b -> mrb?

> +{
> +    if (int128_eq(a->addr.start, mrb->addr.start) &&
> +            (!int128_nz(mrb->addr.size) ||
> +             int128_eq(a->addr.size, mrb->addr.size)) &&
> +            (a->match_data == mrb->match_data) &&
> +            ((mrb->match_data && (a->data == mrb->data)) || !mrb->match_data) &&
> +            (a->e == mrb->e))
> +        return true;

The kernel behaves differently in that a and b are symmetric:

  static bool
  ioeventfd_check_collision(struct kvm *kvm, struct _ioeventfd *p)
  {
  	struct _ioeventfd *_p;

  	list_for_each_entry(_p, &kvm->ioeventfds, list)
  		if (_p->bus_idx == p->bus_idx &&
  		    _p->addr == p->addr &&
  		    (!_p->length || !p->length ||
  		     (_p->length == p->length &&
  		      (_p->wildcard || p->wildcard ||
  		       _p->datamatch == p->datamatch))))
  			return true;

  	return false;
  }

For example, if a->addr.size == 0 then the ioeventfd matches whereas
this patch only does it for b.

Please match the kernel behavior.

A different approach is setting ioeventfd.addr.size to zero when
mr->ioeventfds[i].addr.size is zero in the
memory_region_dispatch_write_eventfds() loop. That way
memory_region_ioeventfd_equal() doesn't need to be modified.