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

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

[Peter Xu]

v3:
- dropping patch 1 because I'm going to drop the has_coalesced_ranges
  variable later...
- moving previous patch 2 as patch 1 because I think it's definitely
  solving a standalone issue of KVM, and also it'll introduce a helper
  function that will be used in follow up patches.
- added new patches
  - patch 3: a cleanup and prepares for the next
  - patch 4: fixes another issue when add/clear coalescing ranges that
             Paolo pointed out

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 (4):
  memory: Split zones when do coalesced_io_del()
  memory: Remove has_coalesced_range counter
  memory: Refactor memory_region_clear_coalescing
  memory: Fix up memory_region_{add|del}_coalescing

 memory.c | 99 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 52 insertions(+), 47 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v3 1/4] 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.

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

diff --git a/memory.c b/memory.c
index 8141486832..8173f6be62 100644
--- a/memory.c
+++ b/memory.c
@@ -855,8 +855,39 @@ static void address_space_update_ioeventfds(AddressSpace *as)
     flatview_unref(view);
 }
 
+/*
+ * Notify the memory listeners about the coalesced IO change events of
+ * range `cmr'.  Only the part that has intersection of the specified
+ * FlatRange will be sent.
+ */
+static void flat_range_coalesced_io_notify(FlatRange *fr, AddressSpace *as,
+                                           CoalescedMemoryRange *cmr, bool add)
+{
+    AddrRange tmp;
+
+    tmp = addrrange_shift(cmr->addr,
+                          int128_sub(fr->addr.start,
+                                     int128_make64(fr->offset_in_region)));
+    if (!addrrange_intersects(tmp, fr->addr)) {
+        return;
+    }
+    tmp = addrrange_intersection(tmp, fr->addr);
+
+    if (add) {
+        MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, coalesced_io_add,
+                                      int128_get64(tmp.start),
+                                      int128_get64(tmp.size));
+    } else {
+        MEMORY_LISTENER_UPDATE_REGION(fr, as, Reverse, coalesced_io_del,
+                                      int128_get64(tmp.start),
+                                      int128_get64(tmp.size));
+    }
+}
+
 static void flat_range_coalesced_io_del(FlatRange *fr, AddressSpace *as)
 {
+    CoalescedMemoryRange *cmr;
+
     if (!fr->has_coalesced_range) {
         return;
     }
@@ -865,16 +896,15 @@ 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));
+    QTAILQ_FOREACH(cmr, &fr->mr->coalesced, link) {
+        flat_range_coalesced_io_notify(fr, as, cmr, false);
+    }
 }
 
 static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
 {
     MemoryRegion *mr = fr->mr;
     CoalescedMemoryRange *cmr;
-    AddrRange tmp;
 
     if (QTAILQ_EMPTY(&mr->coalesced)) {
         return;
@@ -885,16 +915,7 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
     }
 
     QTAILQ_FOREACH(cmr, &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, Forward, coalesced_io_add,
-                                      int128_get64(tmp.start),
-                                      int128_get64(tmp.size));
+        flat_range_coalesced_io_notify(fr, as, cmr, true);
     }
 }
 
-- 
2.21.0

[Qemu-devel] [PATCH v3 2/4] memory: Remove has_coalesced_range counter

[Peter Xu]

The has_coalesced_range could potentially be problematic 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).

The counter was originally used to make sure every FlatRange will only
notify once for coalesced_io_{add|del} memory listeners, because each
FlatRange can be used by multiple address spaces, so logically
speaking it could be called multiple times.  However we should not
limit that, because memory listeners should will only be registered
with specific address space rather than multiple address spaces.

So let's fix this up by simply removing the whole has_coalesced_range.

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

diff --git a/memory.c b/memory.c
index 8173f6be62..360e0cfa67 100644
--- a/memory.c
+++ b/memory.c
@@ -217,7 +217,6 @@ struct FlatRange {
     bool romd_mode;
     bool readonly;
     bool nonvolatile;
-    int has_coalesced_range;
 };
 
 #define FOR_EACH_FLAT_RANGE(var, view)          \
@@ -654,7 +653,6 @@ static void render_memory_region(FlatView *view,
     fr.romd_mode = mr->romd_mode;
     fr.readonly = readonly;
     fr.nonvolatile = nonvolatile;
-    fr.has_coalesced_range = 0;
 
     /* Render the region itself into any gaps left by the current view. */
     for (i = 0; i < view->nr && int128_nz(remain); ++i) {
@@ -888,14 +886,6 @@ static void flat_range_coalesced_io_del(FlatRange *fr, AddressSpace *as)
 {
     CoalescedMemoryRange *cmr;
 
-    if (!fr->has_coalesced_range) {
-        return;
-    }
-
-    if (--fr->has_coalesced_range > 0) {
-        return;
-    }
-
     QTAILQ_FOREACH(cmr, &fr->mr->coalesced, link) {
         flat_range_coalesced_io_notify(fr, as, cmr, false);
     }
@@ -910,10 +900,6 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
         return;
     }
 
-    if (fr->has_coalesced_range++) {
-        return;
-    }
-
     QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
         flat_range_coalesced_io_notify(fr, as, cmr, true);
     }
-- 
2.21.0

[Qemu-devel] [PATCH v3 3/4] memory: Refactor memory_region_clear_coalescing

[Peter Xu]

Removing the update variable and quit earlier if the memory region has
no coalesced range.  This prepares for the next patch.

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

diff --git a/memory.c b/memory.c
index 360e0cfa67..2f7a67086a 100644
--- a/memory.c
+++ b/memory.c
@@ -2283,7 +2283,10 @@ void memory_region_add_coalescing(MemoryRegion *mr,
 void memory_region_clear_coalescing(MemoryRegion *mr)
 {
     CoalescedMemoryRange *cmr;
-    bool updated = false;
+
+    if (QTAILQ_EMPTY(&mr->coalesced)) {
+        return;
+    }
 
     qemu_flush_coalesced_mmio_buffer();
     mr->flush_coalesced_mmio = false;
@@ -2292,12 +2295,9 @@ void memory_region_clear_coalescing(MemoryRegion *mr)
         cmr = QTAILQ_FIRST(&mr->coalesced);
         QTAILQ_REMOVE(&mr->coalesced, cmr, link);
         g_free(cmr);
-        updated = true;
     }
 
-    if (updated) {
-        memory_region_update_coalesced_range(mr);
-    }
+    memory_region_update_coalesced_range(mr);
 }
 
 void memory_region_set_flush_coalesced(MemoryRegion *mr)
-- 
2.21.0

[Qemu-devel] [PATCH v3 4/4] memory: Fix up memory_region_{add|del}_coalescing

[Peter Xu]

The old memory_region_{add|clear}_coalescing() has some defects
because they both changed mr->coalesced before updating the regions
using memory_region_update_coalesced_range_as().  Then when the
regions were updated in memory_region_update_coalesced_range_as() the
mr->coalesced will always be either one more or one less.  So:

- For memory_region_add_coalescing: it'll always trying to remove the
  newly added coalesced region while it shouldn't, and,

- For memory_region_clear_coalescing: when it calls the update there
  will be no coalesced ranges on mr->coalesced because they were all
  removed before hand so the update will probably do nothing for real.

Let's fix this.  Now we've got flat_range_coalesced_io_notify() to
notify a single CoalescedMemoryRange instance change, so use it in the
existing memory_region_update_coalesced_range() logic by only notify
either an addition or deletion.  Then we hammer both the
memory_region_{add|clear}_coalescing() to use it.

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

diff --git a/memory.c b/memory.c
index 2f7a67086a..5d2194ebcc 100644
--- a/memory.c
+++ b/memory.c
@@ -2238,27 +2238,26 @@ void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error **errp
     qemu_ram_resize(mr->ram_block, newsize, errp);
 }
 
-static void memory_region_update_coalesced_range_as(MemoryRegion *mr, AddressSpace *as)
+/*
+ * Call proper memory listeners about the change on the newly
+ * added/removed CoalescedMemoryRange.
+ */
+static void memory_region_update_coalesced_range(MemoryRegion *mr,
+                                                 CoalescedMemoryRange *cmr,
+                                                 bool add)
 {
+    AddressSpace *as;
     FlatView *view;
     FlatRange *fr;
 
-    view = address_space_get_flatview(as);
-    FOR_EACH_FLAT_RANGE(fr, view) {
-        if (fr->mr == mr) {
-            flat_range_coalesced_io_del(fr, as);
-            flat_range_coalesced_io_add(fr, as);
-        }
-    }
-    flatview_unref(view);
-}
-
-static void memory_region_update_coalesced_range(MemoryRegion *mr)
-{
-    AddressSpace *as;
-
     QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
-        memory_region_update_coalesced_range_as(mr, as);
+        view = address_space_get_flatview(as);
+        FOR_EACH_FLAT_RANGE(fr, view) {
+            if (fr->mr == mr) {
+                flat_range_coalesced_io_notify(fr, as, cmr, add);
+            }
+        }
+        flatview_unref(view);
     }
 }
 
@@ -2276,7 +2275,7 @@ void memory_region_add_coalescing(MemoryRegion *mr,
 
     cmr->addr = addrrange_make(int128_make64(offset), int128_make64(size));
     QTAILQ_INSERT_TAIL(&mr->coalesced, cmr, link);
-    memory_region_update_coalesced_range(mr);
+    memory_region_update_coalesced_range(mr, cmr, true);
     memory_region_set_flush_coalesced(mr);
 }
 
@@ -2294,10 +2293,9 @@ void memory_region_clear_coalescing(MemoryRegion *mr)
     while (!QTAILQ_EMPTY(&mr->coalesced)) {
         cmr = QTAILQ_FIRST(&mr->coalesced);
         QTAILQ_REMOVE(&mr->coalesced, cmr, link);
+        memory_region_update_coalesced_range(mr, cmr, false);
         g_free(cmr);
     }
-
-    memory_region_update_coalesced_range(mr);
 }
 
 void memory_region_set_flush_coalesced(MemoryRegion *mr)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 0/4] memory: Fix up coalesced_io_del not working for KVM

[Paolo Bonzini]

Looks great, thanks!

I'm even squashing together 1/3/4 since it's actually quite readable and
the intermediate states aren't really bisectable.

Paolo

On 20/08/19 16:13, Peter Xu wrote:
> v3:
> - dropping patch 1 because I'm going to drop the has_coalesced_ranges
>   variable later...
> - moving previous patch 2 as patch 1 because I think it's definitely
>   solving a standalone issue of KVM, and also it'll introduce a helper
>   function that will be used in follow up patches.
> - added new patches
>   - patch 3: a cleanup and prepares for the next
>   - patch 4: fixes another issue when add/clear coalescing ranges that
>              Paolo pointed out
> 
> 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 (4):
>   memory: Split zones when do coalesced_io_del()
>   memory: Remove has_coalesced_range counter
>   memory: Refactor memory_region_clear_coalescing
>   memory: Fix up memory_region_{add|del}_coalescing
> 
>  memory.c | 99 +++++++++++++++++++++++++++++---------------------------
>  1 file changed, 52 insertions(+), 47 deletions(-)
>