[PATCH] accel/kvm: add PIO ioeventfds only in case kvm_eventfds_allowed is true

[PATCH] accel/kvm: add PIO ioeventfds only in case kvm_eventfds_allowed is true

[Elena Afanasova]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
---
 accel/kvm/kvm-all.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 9ef5daf4c5..baaa54249d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2239,8 +2239,10 @@ static int kvm_init(MachineState *ms)
 
     kvm_memory_listener_register(s, &s->memory_listener,
                                  &address_space_memory, 0);
-    memory_listener_register(&kvm_io_listener,
-                             &address_space_io);
+    if (kvm_eventfds_allowed) {
+        memory_listener_register(&kvm_io_listener,
+                                 &address_space_io);
+    }
     memory_listener_register(&kvm_coalesced_pio_listener,
                              &address_space_io);
 
-- 
2.25.1

[PATCH] softmmu/memory: fix memory_region_ioeventfd_equal()

[Elena Afanasova]

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] accel/kvm: add PIO ioeventfds only in case kvm_eventfds_allowed is true

[Stefan Hajnoczi]

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

On Sat, Oct 17, 2020 at 02:01:01PM -0700, Elena Afanasova wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---
>  accel/kvm/kvm-all.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Additional information for the commit description:

  MMIO eventfd_add/del are only registered when kvm_eventfds_allowed is
  true but for PIO they are registered unconditionally. This is a bug
  because kvm.ko ioeventfd should not be used if kvm_eventfds_allowed is
  false.

  This issue is a latent bug because ioeventfd is usually available when
  KVM is enabled.

> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 9ef5daf4c5..baaa54249d 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2239,8 +2239,10 @@ static int kvm_init(MachineState *ms)
>  
>      kvm_memory_listener_register(s, &s->memory_listener,
>                                   &address_space_memory, 0);
> -    memory_listener_register(&kvm_io_listener,
> -                             &address_space_io);
> +    if (kvm_eventfds_allowed) {
> +        memory_listener_register(&kvm_io_listener,
> +                                 &address_space_io);
> +    }
>      memory_listener_register(&kvm_coalesced_pio_listener,
>                               &address_space_io);
>  
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] softmmu/memory: fix memory_region_ioeventfd_equal()

[Stefan Hajnoczi]

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] accel/kvm: add PIO ioeventfds only in case kvm_eventfds_allowed is true

[Stefan Hajnoczi]

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

On Sat, Oct 17, 2020 at 02:01:01PM -0700, Elena Afanasova wrote:
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---
>  accel/kvm/kvm-all.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Thanks, applied just this patch to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]