[Qemu-devel] [PATCH v2 0/2] memory: Fix up coalesced_io_del not working for KVM

[Qemu-devel] [PATCH v2 0/2] memory: Fix up coalesced_io_del not working for KVM

[Peter Xu]

v2:
- simply migrate has_coalesced_range in patch 1, while I added
  comments in the code because that can be a bit unobvious [Paolo]

v1 is here:

https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg03293.html

Peter Xu (2):
  memory: Inherit has_coalesced_range from the same old FlatRange
  memory: Split zones when do coalesced_io_del()

 memory.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/2] memory: Inherit has_coalesced_range from the same old FlatRange

[Peter Xu]

The previous has_coalesced_range counter has a problem in that it only
works for additions of coalesced mmio ranges but not deletions.  The
reason is that has_coalesced_range information can be lost when the
FlatView updates the topology again when the updated region is not
covering the coalesced regions. When that happens, due to
flatrange_equal() is not checking against has_coalesced_range, the new
FlatRange will be seen as the same one as the old and the new
instance (whose has_coalesced_range will be zero) will replace the old
instance (whose has_coalesced_range _could_ be non-zero).

To fix it, we inherit the has_coalesced_range value from the old
FlatRange to the new one if it's describing the identical range when
updating the topology.

Without this patch, MemoryListener.coalesced_io_del is hardly being
called due to has_coalesced_range will be mostly zero in
flat_range_coalesced_io_del() when topologies frequently change for
the "memory" address space.

Fixes: 3ac7d43a6fbb5d4a3
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/memory.c b/memory.c
index 8141486832..c53dcfc092 100644
--- a/memory.c
+++ b/memory.c
@@ -939,6 +939,15 @@ static void address_space_update_topology_pass(AddressSpace *as,
             /* In both and unchanged (except logging may have changed) */
 
             if (adding) {
+                /*
+                 * We must inherit the has_coalesced_range information
+                 * if the new FlatRange is exactly the same as the old
+                 * one, because it'll be used to conditionally call
+                 * the coalesced mmio deletion listeners correctly in
+                 * flat_range_coalesced_io_del() when the FlatRange
+                 * needs to really go away.
+                 */
+                frnew->has_coalesced_range = frold->has_coalesced_range;
                 MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, region_nop);
                 if (frnew->dirty_log_mask & ~frold->dirty_log_mask) {
                     MEMORY_LISTENER_UPDATE_REGION(frnew, as, Forward, log_start,
-- 
2.21.0

[Qemu-devel] [PATCH v2 2/2] memory: Split zones when do coalesced_io_del()

[Peter Xu]

It is a workaround of current KVM's KVM_UNREGISTER_COALESCED_MMIO
interface.  The kernel interface only allows to unregister an mmio
device with exactly the zone size when registered, or any smaller zone
that is included in the device mmio zone.  It does not support the
userspace to specify a very large zone to remove all the small mmio
devices within the zone covered.

Logically speaking it would be nicer to fix this from KVM side, though
in all cases we still need to coop with old kernels so let's do this.

This patch has nothing to do with 3ac7d43a6fbb5d4a3 because this is
probably broken from the very beginning when the
KVM_UNREGISTER_COALESCED_MMIO interface is introduced in kernel.
However to make the backport to stables easier, I'm still using the
commit 3ac7d43a6fbb5d4a3 to track this problem because this will
depend on that otherwise even additions of mmio devices won't work.

Fixes: 3ac7d43a6fbb5d4a3
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/memory.c b/memory.c
index c53dcfc092..7684b423f8 100644
--- a/memory.c
+++ b/memory.c
@@ -857,6 +857,9 @@ static void address_space_update_ioeventfds(AddressSpace *as)
 
 static void flat_range_coalesced_io_del(FlatRange *fr, AddressSpace *as)
 {
+    CoalescedMemoryRange *cmr;
+    AddrRange tmp;
+
     if (!fr->has_coalesced_range) {
         return;
     }
@@ -865,9 +868,30 @@ static void flat_range_coalesced_io_del(FlatRange *fr, AddressSpace *as)
         return;
     }
 
-    MEMORY_LISTENER_UPDATE_REGION(fr, as, Reverse, coalesced_io_del,
-                                  int128_get64(fr->addr.start),
-                                  int128_get64(fr->addr.size));
+    /*
+     * We split the big region into smaller ones to satisfy KVM's
+     * KVM_UNREGISTER_COALESCED_MMIO interface, where it does not
+     * allow to specify a large region to unregister all the devices
+     * under that zone instead it only accepts exact zones or even a
+     * smaller zone of previously registered mmio device.  Logically
+     * speaking we should better fix KVM to allow the userspace to
+     * unregister multiple mmio devices within a large requested zone,
+     * but in all cases we'll still need to live with old kernels.  So
+     * let's simply break the zones into exactly the small pieces when
+     * we do coalesced_io_add().
+     */
+    QTAILQ_FOREACH(cmr, &fr->mr->coalesced, link) {
+        tmp = addrrange_shift(cmr->addr,
+                              int128_sub(fr->addr.start,
+                                         int128_make64(fr->offset_in_region)));
+        if (!addrrange_intersects(tmp, fr->addr)) {
+            continue;
+        }
+        tmp = addrrange_intersection(tmp, fr->addr);
+        MEMORY_LISTENER_UPDATE_REGION(fr, as, Reverse, coalesced_io_del,
+                                      int128_get64(tmp.start),
+                                      int128_get64(tmp.size));
+    }
 }
 
 static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 2/2] memory: Split zones when do coalesced_io_del()

[Paolo Bonzini]

On 20/08/19 07:16, Peter Xu wrote:
> It is a workaround of current KVM's KVM_UNREGISTER_COALESCED_MMIO
> interface.  The kernel interface only allows to unregister an mmio
> device with exactly the zone size when registered, or any smaller zone
> that is included in the device mmio zone.  It does not support the
> userspace to specify a very large zone to remove all the small mmio
> devices within the zone covered.
> 
> Logically speaking it would be nicer to fix this from KVM side, though
> in all cases we still need to coop with old kernels so let's do this.
> 
> This patch has nothing to do with 3ac7d43a6fbb5d4a3 because this is
> probably broken from the very beginning when the
> KVM_UNREGISTER_COALESCED_MMIO interface is introduced in kernel.
> However to make the backport to stables easier, I'm still using the
> commit 3ac7d43a6fbb5d4a3 to track this problem because this will
> depend on that otherwise even additions of mmio devices won't work.
> 
> Fixes: 3ac7d43a6fbb5d4a3
> Signed-off-by: Peter Xu <peterx@redhat.com>

What about my comments to this patch? :)

Paolo

> ---
>  memory.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index c53dcfc092..7684b423f8 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -857,6 +857,9 @@ static void address_space_update_ioeventfds(AddressSpace *as)
>  
>  static void flat_range_coalesced_io_del(FlatRange *fr, AddressSpace *as)
>  {
> +    CoalescedMemoryRange *cmr;
> +    AddrRange tmp;
> +
>      if (!fr->has_coalesced_range) {
>          return;
>      }
> @@ -865,9 +868,30 @@ static void flat_range_coalesced_io_del(FlatRange *fr, AddressSpace *as)
>          return;
>      }
>  
> -    MEMORY_LISTENER_UPDATE_REGION(fr, as, Reverse, coalesced_io_del,
> -                                  int128_get64(fr->addr.start),
> -                                  int128_get64(fr->addr.size));
> +    /*
> +     * We split the big region into smaller ones to satisfy KVM's
> +     * KVM_UNREGISTER_COALESCED_MMIO interface, where it does not
> +     * allow to specify a large region to unregister all the devices
> +     * under that zone instead it only accepts exact zones or even a
> +     * smaller zone of previously registered mmio device.  Logically
> +     * speaking we should better fix KVM to allow the userspace to
> +     * unregister multiple mmio devices within a large requested zone,
> +     * but in all cases we'll still need to live with old kernels.  So
> +     * let's simply break the zones into exactly the small pieces when
> +     * we do coalesced_io_add().
> +     */
> +    QTAILQ_FOREACH(cmr, &fr->mr->coalesced, link) {
> +        tmp = addrrange_shift(cmr->addr,
> +                              int128_sub(fr->addr.start,
> +                                         int128_make64(fr->offset_in_region)));
> +        if (!addrrange_intersects(tmp, fr->addr)) {
> +            continue;
> +        }
> +        tmp = addrrange_intersection(tmp, fr->addr);
> +        MEMORY_LISTENER_UPDATE_REGION(fr, as, Reverse, coalesced_io_del,
> +                                      int128_get64(tmp.start),
> +                                      int128_get64(tmp.size));
> +    }
>  }
>  
>  static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
> 

Re: [Qemu-devel] [PATCH v2 2/2] memory: Split zones when do coalesced_io_del()

[Peter Xu]

On Tue, Aug 20, 2019 at 08:24:12AM +0200, Paolo Bonzini wrote:
> On 20/08/19 07:16, Peter Xu wrote:
> > It is a workaround of current KVM's KVM_UNREGISTER_COALESCED_MMIO
> > interface.  The kernel interface only allows to unregister an mmio
> > device with exactly the zone size when registered, or any smaller zone
> > that is included in the device mmio zone.  It does not support the
> > userspace to specify a very large zone to remove all the small mmio
> > devices within the zone covered.
> > 
> > Logically speaking it would be nicer to fix this from KVM side, though
> > in all cases we still need to coop with old kernels so let's do this.
> > 
> > This patch has nothing to do with 3ac7d43a6fbb5d4a3 because this is
> > probably broken from the very beginning when the
> > KVM_UNREGISTER_COALESCED_MMIO interface is introduced in kernel.
> > However to make the backport to stables easier, I'm still using the
> > commit 3ac7d43a6fbb5d4a3 to track this problem because this will
> > depend on that otherwise even additions of mmio devices won't work.
> > 
> > Fixes: 3ac7d43a6fbb5d4a3
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> What about my comments to this patch? :)

I thought that comment was for patch 1 but wrongly replied to this
patch 2... :)

Isn't that comment only valid if we still use the old patch 1?  With
your suggestion, is that still needed to keep mr->coalesced untouched?

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/2] memory: Split zones when do coalesced_io_del()

[Peter Xu]

On Tue, Aug 20, 2019 at 02:35:15PM +0800, Peter Xu wrote:
> On Tue, Aug 20, 2019 at 08:24:12AM +0200, Paolo Bonzini wrote:
> > On 20/08/19 07:16, Peter Xu wrote:
> > > It is a workaround of current KVM's KVM_UNREGISTER_COALESCED_MMIO
> > > interface.  The kernel interface only allows to unregister an mmio
> > > device with exactly the zone size when registered, or any smaller zone
> > > that is included in the device mmio zone.  It does not support the
> > > userspace to specify a very large zone to remove all the small mmio
> > > devices within the zone covered.
> > > 
> > > Logically speaking it would be nicer to fix this from KVM side, though
> > > in all cases we still need to coop with old kernels so let's do this.
> > > 
> > > This patch has nothing to do with 3ac7d43a6fbb5d4a3 because this is
> > > probably broken from the very beginning when the
> > > KVM_UNREGISTER_COALESCED_MMIO interface is introduced in kernel.
> > > However to make the backport to stables easier, I'm still using the
> > > commit 3ac7d43a6fbb5d4a3 to track this problem because this will
> > > depend on that otherwise even additions of mmio devices won't work.
> > > 
> > > Fixes: 3ac7d43a6fbb5d4a3
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > What about my comments to this patch? :)
> 
> I thought that comment was for patch 1 but wrongly replied to this
> patch 2... :)
> 
> Isn't that comment only valid if we still use the old patch 1?  With
> your suggestion, is that still needed to keep mr->coalesced untouched?

Oops, ignore this reply...  It's needed of course!  So it also
affecting this patch...

I'll prepare a v3 soon.

Regards,

-- 
Peter Xu