Rework vhost memory region updates

From: "Dr. David Alan Gilbert" <[email protected]>

Move the log_dirty check into vhost_section.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/trace-events |  3 +++
 hw/virtio/vhost.c      | 20 +++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

hw/virtio/trace-events +3 -0

index 775461ae98..4a493bcd46 100644
@@ -1,5 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/virtio/vhost.c
+vhost_section(const char *name, int r) "%s:%d"
+
 # hw/virtio/virtio.c
 virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
 virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u"

hw/virtio/vhost.c +1 -7

index e4290ce93d..e923219e63 100644
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1

@@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener,
                                          memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty =
-        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     int s = offsetof(struct vhost_memory, regions) +
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     void *ram;
 
     dev->mem = g_realloc(dev->mem, s);
 
-    if (log_dirty) {
-        add = false;
-    }
-
     assert(size);
 
     /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */

@@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener,
 
 static bool vhost_section(MemoryRegionSection *section)
 {
-    return memory_region_is_ram(section->mr) &&

hw/virtio/vhost.c +3 -0

+
+    trace_vhost_section(section->mr->name, result);
+    return result;
 }
 
 static void vhost_begin(MemoryListener *listener)

Quoted 59 lines · On Wed, 13 Dec 2017 18:08:02 +0000 "Dr. David Alan Gilbert (git)"…

From: "Dr. David Alan Gilbert" <[email protected]>

Move the log_dirty check into vhost_section.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/trace-events |  3 +++
 hw/virtio/vhost.c      | 20 +++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

hw/virtio/trace-events +3 -0

index 775461ae98..4a493bcd46 100644
@@ -1,5 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/virtio/vhost.c
+vhost_section(const char *name, int r) "%s:%d"
+
 # hw/virtio/virtio.c
 virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
 virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u"

hw/virtio/vhost.c +1 -7

index e4290ce93d..e923219e63 100644
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1

@@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener,
                                          memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty =
-        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     int s = offsetof(struct vhost_memory, regions) +
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     void *ram;
 
     dev->mem = g_realloc(dev->mem, s);
 
-    if (log_dirty) {
-        add = false;
-    }
-
     assert(size);
 
     /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */

@@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener,
 
 static bool vhost_section(MemoryRegionSection *section)
 {
-    return memory_region_is_ram(section->mr) &&

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 618

Quoted 10 lines

hw/virtio/vhost.c +9 -0

+    bool result;
+    bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
+                     ~(1 << DIRTY_MEMORY_MIGRATION);
+    result = memory_region_is_ram(section->mr) &&
         !memory_region_is_rom(section->mr);
+
+    /* Vhost doesn't handle any block which is doing dirty-tracking other
+     * than migration; this typically fires on VGA areas.
+     */
+    result &= !log_dirty;

before patch even if log_dirty, vhost_set_memory will still proceed and may remove dirty section from memmap and set memory_changed = true

but with this patch it will just ignore such section,
I'm not sure it's right (it might be right with new approach add/nop but then this patch should go after new code in place and old one if gone).

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 624

Quoted 6 lines

hw/virtio/vhost.c +3 -0

+
+    trace_vhost_section(section->mr->name, result);
+    return result;
 }
 
 static void vhost_begin(MemoryListener *listener)

Quoted 75 lines · * Igor Mammedov ([email protected]) wrote:

Quoted 59 lines · On Wed, 13 Dec 2017 18:08:02 +0000 "Dr. David Alan Gilbert (git)"…

From: "Dr. David Alan Gilbert" <[email protected]>

Move the log_dirty check into vhost_section.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/trace-events |  3 +++
 hw/virtio/vhost.c      | 20 +++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

hw/virtio/trace-events +3 -0

index 775461ae98..4a493bcd46 100644
@@ -1,5 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/virtio/vhost.c
+vhost_section(const char *name, int r) "%s:%d"
+
 # hw/virtio/virtio.c
 virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
 virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u"

hw/virtio/vhost.c +1 -7

index e4290ce93d..e923219e63 100644
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1

@@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener,
                                          memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty =
-        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     int s = offsetof(struct vhost_memory, regions) +
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     void *ram;
 
     dev->mem = g_realloc(dev->mem, s);
 
-    if (log_dirty) {
-        add = false;
-    }
-
     assert(size);
 
     /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */

@@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener,
 
 static bool vhost_section(MemoryRegionSection *section)
 {
-    return memory_region_is_ram(section->mr) &&

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 618

Quoted 10 lines

hw/virtio/vhost.c +9 -0

+    bool result;
+    bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
+                     ~(1 << DIRTY_MEMORY_MIGRATION);
+    result = memory_region_is_ram(section->mr) &&
         !memory_region_is_rom(section->mr);
+
+    /* Vhost doesn't handle any block which is doing dirty-tracking other
+     * than migration; this typically fires on VGA areas.
+     */
+    result &= !log_dirty;  

before patch even if log_dirty, vhost_set_memory will still proceed and may remove dirty section from memmap and set memory_changed = true

Quoted 4 lines

but with this patch it will just ignore such section,
I'm not sure it's right (it might be right with new approach add/nop but then this patch should go after new code in place and old one if gone).

I thought about that, but then I came to the conclusion that the whole idea is that we're supposed to be ignoring these regions - so why should they cause any change to the behaviour of vhost at all? Thus this way seems safer.

Dave

Quoted 7 lines

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 624

Quoted 6 lines

hw/virtio/vhost.c +3 -0

+
+    trace_vhost_section(section->mr->name, result);
+    return result;
 }
 
 static void vhost_begin(MemoryListener *listener)  

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 75 lines · * Igor Mammedov ([email protected]) wrote:

Quoted 59 lines · On Wed, 13 Dec 2017 18:08:02 +0000 "Dr. David Alan Gilbert (git)"…

From: "Dr. David Alan Gilbert" <[email protected]>

Move the log_dirty check into vhost_section.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/trace-events |  3 +++
 hw/virtio/vhost.c      | 20 +++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

hw/virtio/trace-events +3 -0

index 775461ae98..4a493bcd46 100644
@@ -1,5 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/virtio/vhost.c
+vhost_section(const char *name, int r) "%s:%d"
+
 # hw/virtio/virtio.c
 virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
 virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u"

hw/virtio/vhost.c +1 -7

index e4290ce93d..e923219e63 100644
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1

@@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener,
                                          memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty =
-        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     int s = offsetof(struct vhost_memory, regions) +
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     void *ram;
 
     dev->mem = g_realloc(dev->mem, s);
 
-    if (log_dirty) {
-        add = false;
-    }
-
     assert(size);
 
     /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */

@@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener,
 
 static bool vhost_section(MemoryRegionSection *section)
 {
-    return memory_region_is_ram(section->mr) &&

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 618

Quoted 10 lines

hw/virtio/vhost.c +9 -0

+    bool result;
+    bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
+                     ~(1 << DIRTY_MEMORY_MIGRATION);
+    result = memory_region_is_ram(section->mr) &&
         !memory_region_is_rom(section->mr);
+
+    /* Vhost doesn't handle any block which is doing dirty-tracking other
+     * than migration; this typically fires on VGA areas.
+     */
+    result &= !log_dirty;

before patch even if log_dirty, vhost_set_memory will still proceed and may remove dirty section from memmap and set memory_changed = true

Quoted 4 lines

but with this patch it will just ignore such section,
I'm not sure it's right (it might be right with new approach add/nop but then this patch should go after new code in place and old one if gone).

I thought about that, but then I came to the conclusion that the whole idea is that we're supposed to be ignoring these regions - so why should they cause any change to the behaviour of vhost at all? Thus this way seems safer.

Dave

Quoted 7 lines

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 624

Quoted 6 lines

hw/virtio/vhost.c +3 -0

+
+    trace_vhost_section(section->mr->name, result);
+    return result;
 }
 
 static void vhost_begin(MemoryListener *listener)

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 81 lines · On Thu, 14 Dec 2017 15:20:10 +0000 "Dr. David Alan Gilbert"…

Quoted 75 lines · * Igor Mammedov ([email protected]) wrote:

Quoted 59 lines · On Wed, 13 Dec 2017 18:08:02 +0000 "Dr. David Alan Gilbert (git)"…

From: "Dr. David Alan Gilbert" <[email protected]>

Move the log_dirty check into vhost_section.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/trace-events |  3 +++
 hw/virtio/vhost.c      | 20 +++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

hw/virtio/trace-events +3 -0

index 775461ae98..4a493bcd46 100644
@@ -1,5 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/virtio/vhost.c
+vhost_section(const char *name, int r) "%s:%d"
+
 # hw/virtio/virtio.c
 virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
 virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u"

hw/virtio/vhost.c +1 -7

index e4290ce93d..e923219e63 100644
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1

@@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener,
                                          memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty =
-        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     int s = offsetof(struct vhost_memory, regions) +
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     void *ram;
 
     dev->mem = g_realloc(dev->mem, s);
 
-    if (log_dirty) {
-        add = false;
-    }
-
     assert(size);
 
     /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */

@@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener,
 
 static bool vhost_section(MemoryRegionSection *section)
 {
-    return memory_region_is_ram(section->mr) &&

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 618

Quoted 10 lines

hw/virtio/vhost.c +9 -0

+    bool result;
+    bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
+                     ~(1 << DIRTY_MEMORY_MIGRATION);
+    result = memory_region_is_ram(section->mr) &&
         !memory_region_is_rom(section->mr);
+
+    /* Vhost doesn't handle any block which is doing dirty-tracking other
+     * than migration; this typically fires on VGA areas.
+     */
+    result &= !log_dirty;

before patch even if log_dirty, vhost_set_memory will still proceed and may remove dirty section from memmap and set memory_changed = true

Quoted 4 lines

but with this patch it will just ignore such section,
I'm not sure it's right (it might be right with new approach add/nop but then this patch should go after new code in place and old one if gone).

Quoted 4 lines

I thought about that, but then I came to the conclusion that the whole idea is that we're supposed to be ignoring these regions - so why should they cause any change to the behaviour of vhost at all? Thus this way seems safer.

the thing is that with originally with old code backend will use memap with not yet dirty region, then after region becomes dirty current code will REMOVE dirty region from map and NOTIFY backend.

While this patch is fine for new approach as memmap is build from scratch, it doesn't look correct for current code since region that just became dirty will not be removed from memap and might be used backend. Whether it would really break something or not I'm not sure, maybe Michael may Ack this patch ii using dirty region is fine.

Quoted 8 lines

Dave

Quoted 5 lines

Quoted 5 lines

hw/virtio/vhost.c +3 -0

+
+    trace_vhost_section(section->mr->name, result);
+    return result;
 }
 

Quoted 4 lines

Quoted 2 lines

hw/virtio/vhost.c static void vhost_set_memory(MemoryListener *listener, · line 624

Quoted 1 line

hw/virtio/vhost.c +0 -0

 static void vhost_begin(MemoryListener *listener)

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

From: "Dr. David Alan Gilbert" <[email protected]>

vhost_verify_ring_mappings() were used to verify that rings are still accessible and related memory hasn't been moved after flatview is updated.

It was doing checks by mapping ring's GPA+len and checking that HVA hadn't changed with new memory map. To avoid maybe expensive mapping call, we were identifying address range that changed and were doing mapping only if ring was in changed range.

However it's not neccessary to perform ring's GPA mapping as we already have its current HVA and all we need is to verify that ring's GPA translates to the same HVA in updated flatview.

This will allow the following patches to simplify the range comparison that was previously needed to avoid expensive verify_ring_mapping calls.

Signed-off-by Igor Mammedov <[email protected]>

with modifications by:

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/vhost.c | 74 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

hw/virtio/vhost.c +9 -10

index e923219e63..c7ce7baf9b 100644
@@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
     }
 }
 
-static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
-                                          void *part,
-                                          uint64_t part_addr,
-                                          uint64_t part_size,
-                                          uint64_t start_addr,
-                                          uint64_t size)
+static int vhost_verify_ring_part_mapping(void *ring_hva,
+                                          uint64_t ring_gpa,
+                                          uint64_t ring_size,
+                                          void *reg_hva,
+                                          uint64_t reg_gpa,
+                                          uint64_t reg_size)
 {
-    hwaddr l;
-    void *p;
-    int r = 0;
+    uint64_t hva_ring_offset;
+    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
+    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
 
-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {

hw/virtio/vhost.c +14 -12

     }
-    vhost_memory_unmap(dev, p, l, 0, 0);
-    return r;
+
+    return 0;
 }
 
 static int vhost_verify_ring_mappings(struct vhost_dev *dev,
-                                      uint64_t start_addr,
-                                      uint64_t size)
+                                      void *reg_hva,
+                                      uint64_t reg_gpa,
+                                      uint64_t reg_size)
 {
     int i, j;
     int r = 0;

@@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         struct vhost_virtqueue *vq = dev->vqs + i;
 
         j = 0;
-        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
-                                           vq->desc_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }
 
         j++;
-        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
-                                           vq->avail_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }
 
         j++;
-        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
-                                           vq->used_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }

@@ -633,8 +638,6 @@ static void vhost_commit(MemoryListener *listener)
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,
                                          memory_listener);
-    hwaddr start_addr = 0;
-    ram_addr_t size = 0;
     uint64_t log_size;
     int r;
 

hw/virtio/vhost.c +10 -5

-        start_addr = dev->mem_changed_start_addr;
-        size = dev->mem_changed_end_addr - dev->mem_changed_start_addr + 1;
-
-        r = vhost_verify_ring_mappings(dev, start_addr, size);
-        assert(r >= 0);
+        int i;
+        for (i = 0; i < dev->mem->nregions; i++) {
+            if (vhost_verify_ring_mappings(dev,
+                               (void *)dev->mem->regions[i].userspace_addr,
+                               dev->mem->regions[i].guest_phys_addr,
+                               dev->mem->regions[i].memory_size)) {
+                error_report("Verify ring failure on region %d", i);
+                abort();
+            }
+        }
     }
 
     if (!dev->log_enabled) {

Quoted 56 lines · On Wed, 13 Dec 2017 18:08:03 +0000 "Dr. David Alan Gilbert (git)"…

From: "Dr. David Alan Gilbert" <[email protected]>

vhost_verify_ring_mappings() were used to verify that rings are still accessible and related memory hasn't been moved after flatview is updated.

It was doing checks by mapping ring's GPA+len and checking that HVA hadn't changed with new memory map. To avoid maybe expensive mapping call, we were identifying address range that changed and were doing mapping only if ring was in changed range.

However it's not neccessary to perform ring's GPA mapping as we already have its current HVA and all we need is to verify that ring's GPA translates to the same HVA in updated flatview.

This will allow the following patches to simplify the range comparison that was previously needed to avoid expensive verify_ring_mapping calls.

Signed-off-by Igor Mammedov <[email protected]>

with modifications by:

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/vhost.c | 74 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

hw/virtio/vhost.c +9 -9

index e923219e63..c7ce7baf9b 100644
@@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
     }
 }
 
-static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
-                                          void *part,
-                                          uint64_t part_addr,
-                                          uint64_t part_size,
-                                          uint64_t start_addr,
-                                          uint64_t size)
+static int vhost_verify_ring_part_mapping(void *ring_hva,
+                                          uint64_t ring_gpa,
+                                          uint64_t ring_size,
+                                          void *reg_hva,
+                                          uint64_t reg_gpa,
+                                          uint64_t reg_size)
 {
-    hwaddr l;
-    void *p;
-    int r = 0;
+    uint64_t hva_ring_offset;
+    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
+    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
 

hw/virtio/vhost.c static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer, · line 469

Quoted 11 lines

hw/virtio/vhost.c +4 -5

-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    if (ring_last < reg_gpa || ring_gpa > reg_last) {
         return 0;
     }
-    l = part_size;
-    p = vhost_memory_map(dev, part_addr, &l, 1);
-    if (!p || l != part_size) {
-        r = -ENOMEM;
+    /* check that whole ring's is mapped */
+    if (ring_last > reg_last) {
+        return -EBUSY;

ENOMEM

hw/virtio/vhost.c static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer, · line 474

Quoted 7 lines

hw/virtio/vhost.c +4 -2

     }
-    if (p != part) {
-        r = -EBUSY;
+    /* check that ring's MemoryRegion wasn't replaced */
+    hva_ring_offset = ring_gpa - reg_gpa;
+    if (ring_hva != reg_hva + hva_ring_offset) {
+        return -ENOMEM;

EBUSY

Quoted 58 lines

hw/virtio/vhost.c +14 -12

     }
-    vhost_memory_unmap(dev, p, l, 0, 0);
-    return r;
+
+    return 0;
 }
 
 static int vhost_verify_ring_mappings(struct vhost_dev *dev,
-                                      uint64_t start_addr,
-                                      uint64_t size)
+                                      void *reg_hva,
+                                      uint64_t reg_gpa,
+                                      uint64_t reg_size)
 {
     int i, j;
     int r = 0;

@@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         struct vhost_virtqueue *vq = dev->vqs + i;
 
         j = 0;
-        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
-                                           vq->desc_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }
 
         j++;
-        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
-                                           vq->avail_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }
 
         j++;
-        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
-                                           vq->used_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }

@@ -633,8 +638,6 @@ static void vhost_commit(MemoryListener *listener)
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,
                                          memory_listener);
-    hwaddr start_addr = 0;
-    ram_addr_t size = 0;
     uint64_t log_size;
     int r;
 

hw/virtio/vhost.c static void vhost_commit(MemoryListener *listener) · line 654

Quoted 4 lines

hw/virtio/vhost.c +0 -0

@@ -649,11 +652,16 @@ static void vhost_commit(MemoryListener *listener)
     }
 
     if (dev->started) {

^^^ not needed as we bail out earlier if it's not true

Quoted 17 lines

hw/virtio/vhost.c +10 -5

-        start_addr = dev->mem_changed_start_addr;
-        size = dev->mem_changed_end_addr - dev->mem_changed_start_addr + 1;
-
-        r = vhost_verify_ring_mappings(dev, start_addr, size);
-        assert(r >= 0);
+        int i;
+        for (i = 0; i < dev->mem->nregions; i++) {
+            if (vhost_verify_ring_mappings(dev,
+                               (void *)dev->mem->regions[i].userspace_addr,
+                               dev->mem->regions[i].guest_phys_addr,
+                               dev->mem->regions[i].memory_size)) {
+                error_report("Verify ring failure on region %d", i);
+                abort();
+            }
+        }
     }
 

hw/virtio/vhost.c static void vhost_commit(MemoryListener *listener) · line 667

Quoted 1 line

hw/virtio/vhost.c +0 -0

     if (!dev->log_enabled) {

From: "Dr. David Alan Gilbert" <[email protected]>

Add a 2nd 'vhost_memory' structure that will be used to build the new version as the listener iterates over the address space.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/vhost.c         | 3 +++
 include/hw/virtio/vhost.h | 1 +
 2 files changed, 4 insertions(+)

hw/virtio/vhost.c +3 -0

index c7ce7baf9b..4523f45587 100644
@@ -632,6 +632,8 @@ static void vhost_begin(MemoryListener *listener)
                                          memory_listener);
     dev->mem_changed_end_addr = 0;
     dev->mem_changed_start_addr = -1;
+
+    dev->tmp_mem = g_malloc0(offsetof(struct vhost_memory, regions));
 }
 
 static void vhost_commit(MemoryListener *listener)

@@ -641,6 +643,7 @@ static void vhost_commit(MemoryListener *listener)
     uint64_t log_size;
     int r;
 
+    g_free(dev->tmp_mem);
     if (!dev->memory_changed) {
         return;
     }

include/hw/virtio/vhost.h +1 -0

index 467dc7794b..41f9e569be 100644
@@ -70,6 +70,7 @@ struct vhost_dev {
     bool memory_changed;
     hwaddr mem_changed_start_addr;
     hwaddr mem_changed_end_addr;
+    struct vhost_memory *tmp_mem;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

Quoted 4 lines · On Wed, 13 Dec 2017 18:08:04 +0000 "Dr. David Alan Gilbert (git)"…

From: "Dr. David Alan Gilbert" <[email protected]>

Add a 2nd 'vhost_memory' structure that will be used to build the new version as the listener iterates over the address space.

I'd suggest to add temporary 'mem_sections' instead and create/use/free temporary 'vhost_memory' structure at commit time, (1 less duplicated data set to keep in memory)

Quoted 24 lines

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/vhost.c         | 3 +++
 include/hw/virtio/vhost.h | 1 +
 2 files changed, 4 insertions(+)

hw/virtio/vhost.c +2 -0

index c7ce7baf9b..4523f45587 100644
@@ -632,6 +632,8 @@ static void vhost_begin(MemoryListener *listener)
                                          memory_listener);
     dev->mem_changed_end_addr = 0;
     dev->mem_changed_start_addr = -1;
+
+    dev->tmp_mem = g_malloc0(offsetof(struct vhost_memory, regions));
 }
 
 static void vhost_commit(MemoryListener *listener)

@@ -641,6 +643,7 @@ static void vhost_commit(MemoryListener *listener)
     uint64_t log_size;
     int r;
 

Quoted 9 lines

hw/virtio/vhost.c +1 -0

+    g_free(dev->tmp_mem);
     if (!dev->memory_changed) {
         return;
     }

include/hw/virtio/vhost.h +0 -0

index 467dc7794b..41f9e569be 100644

include/hw/virtio/vhost.h struct vhost_dev { · line 76

Quoted 7 lines

include/hw/virtio/vhost.h +1 -0

@@ -70,6 +70,7 @@ struct vhost_dev {
     bool memory_changed;
     hwaddr mem_changed_start_addr;
     hwaddr mem_changed_end_addr;
+    struct vhost_memory *tmp_mem;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +18 -0

+    if (dev->tmp_mem->nregions) {
+        /* Since we already have at least one region, lets see if
+         * this extends it; since we're scanning in order, we only
+         * have to look at the last one, and the FlatView that calls
+         * us shouldn't have overlaps.
+         */
+        struct vhost_memory_region *prev_vmr = dev->tmp_mem->regions +
+                                               (dev->tmp_mem->nregions - 1);
+        uint64_t prev_gpa_start = prev_vmr->guest_phys_addr;
+        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start,
+                                                 prev_vmr->memory_size);
+        uint64_t prev_host_start = prev_vmr->userspace_addr;
+        uint64_t prev_host_end   = range_get_last(prev_host_start,
+                                                  prev_vmr->memory_size);
+
+        if (prev_gpa_end + 1 == mrs_gpa &&
+            prev_host_end + 1 == mrs_host &&
+            (!dev->vhost_ops->vhost_backend_can_merge ||

Quoted 18 lines · Dr. David Alan Gilbert (git)

hw/virtio/vhost.c +18 -0

+    if (dev->tmp_mem->nregions) {
+        /* Since we already have at least one region, lets see if
+         * this extends it; since we're scanning in order, we only
+         * have to look at the last one, and the FlatView that calls
+         * us shouldn't have overlaps.
+         */
+        struct vhost_memory_region *prev_vmr = dev->tmp_mem->regions +
+                                               (dev->tmp_mem->nregions - 1);
+        uint64_t prev_gpa_start = prev_vmr->guest_phys_addr;
+        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start,
+                                                 prev_vmr->memory_size);
+        uint64_t prev_host_start = prev_vmr->userspace_addr;
+        uint64_t prev_host_end   = range_get_last(prev_host_start,
+                                                  prev_vmr->memory_size);
+
+        if (prev_gpa_end + 1 == mrs_gpa &&
+            prev_host_end + 1 == mrs_host &&
+            (!dev->vhost_ops->vhost_backend_can_merge ||

hw/virtio/vhost.c static void vhost_commit(MemoryListener *listener) · line 739

Quoted 10 lines

hw/virtio/vhost.c +10 -0

+                dev->vhost_ops->vhost_backend_can_merge(dev,
+                    mrs_host, mrs_size,
+                    prev_host_start, prev_vmr->memory_size))) {
+            /* The two regions abut */
+            need_add = false;
+            mrs_size = mrs_size + prev_vmr->memory_size;
+            prev_vmr->memory_size = mrs_size;
+            trace_vhost_region_add_tmp_abut(section->mr->name, mrs_size);
+        }
+    }

Interesting, in which cases does this actually trigger?

vhost_region_add_tmp_abut /objects/mem: 0xd000

vhost_region_add_tmp_abut /objects/mem: 0x2c000

vhost_region_add_tmp_abut /objects/mem: 0x30000

vhost_region_add_tmp_abut /objects/mem: 0x40000

vhost_region_add_tmp_abut /objects/mem: 0x3ff40000

Paolo

From: "Dr. David Alan Gilbert" <[email protected]>

As regions are reported by the listener to the _nop and _add methods, add them to our new temporary list. Regions that abut can be merged if the backend allows.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/trace-events |  2 ++
 hw/virtio/vhost.c      | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

hw/virtio/trace-events +2 -0

index 4a493bcd46..7de0663652 100644
@@ -1,6 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # hw/virtio/vhost.c
+vhost_region_add_tmp(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
+vhost_region_add_tmp_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 vhost_section(const char *name, int r) "%s:%d"
 
 # hw/virtio/virtio.c

hw/virtio/vhost.c +62 -0

index 4523f45587..2084888aa7 100644
@@ -694,6 +694,67 @@ static void vhost_commit(MemoryListener *listener)
     dev->memory_changed = false;
 }
 
+/* Adds the section data to the tmp_mem structure.
+ * It relies on the listener calling us in memory address order
+ * and for each region (via the _add and _nop methods).
+ */
+static void vhost_region_add_tmp(struct vhost_dev *dev,
+                                 MemoryRegionSection *section)
+{
+    bool need_add = true;
+    uint64_t mrs_size = int128_get64(section->size);
+    uint64_t mrs_gpa = section->offset_within_address_space;
+    uintptr_t mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
+                         section->offset_within_region;
+
+    trace_vhost_region_add_tmp(section->mr->name, mrs_gpa, mrs_size, mrs_host);
+
+    if (dev->tmp_mem->nregions) {
+        /* Since we already have at least one region, lets see if
+         * this extends it; since we're scanning in order, we only
+         * have to look at the last one, and the FlatView that calls
+         * us shouldn't have overlaps.
+         */
+        struct vhost_memory_region *prev_vmr = dev->tmp_mem->regions +
+                                               (dev->tmp_mem->nregions - 1);
+        uint64_t prev_gpa_start = prev_vmr->guest_phys_addr;
+        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start,
+                                                 prev_vmr->memory_size);
+        uint64_t prev_host_start = prev_vmr->userspace_addr;
+        uint64_t prev_host_end   = range_get_last(prev_host_start,
+                                                  prev_vmr->memory_size);
+
+        if (prev_gpa_end + 1 == mrs_gpa &&
+            prev_host_end + 1 == mrs_host &&
+            (!dev->vhost_ops->vhost_backend_can_merge ||
+                dev->vhost_ops->vhost_backend_can_merge(dev,
+                    mrs_host, mrs_size,
+                    prev_host_start, prev_vmr->memory_size))) {
+            /* The two regions abut */
+            need_add = false;
+            mrs_size = mrs_size + prev_vmr->memory_size;
+            prev_vmr->memory_size = mrs_size;
+            trace_vhost_region_add_tmp_abut(section->mr->name, mrs_size);
+        }
+    }
+
+    if (need_add) {
+        uint32_t nregions = dev->tmp_mem->nregions;
+        size_t s = offsetof(struct vhost_memory, regions) +
+                            (nregions + 1) * sizeof dev->tmp_mem->regions[0];
+        dev->tmp_mem = g_realloc(dev->tmp_mem, s);
+        dev->tmp_mem->nregions++;
+        struct vhost_memory_region *cur_vmr = &dev->tmp_mem->regions[nregions];
+
+        cur_vmr->guest_phys_addr = mrs_gpa;
+        cur_vmr->memory_size     = mrs_size;
+        cur_vmr->userspace_addr  = mrs_host;
+        cur_vmr->flags_padding = 0;
+    }
+
+
+}
+
 static void vhost_region_add(MemoryListener *listener,
                              MemoryRegionSection *section)
 {

@@ -703,6 +764,7 @@ static void vhost_region_add(MemoryListener *listener,
     if (!vhost_section(section)) {
         return;
     }
+    vhost_region_add_tmp(dev, section);
 
     ++dev->n_mem_sections;
     dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,

hw/virtio/vhost.c +0 -0

 }
 
 static int vhost_virtqueue_set_addr(struct vhost_dev *dev,

Also it seems that we have a race in current code where region_del() unrefs memory region first and then by the commit time memory region could be gone since old flatview is unreffed before commit callback is called, but guest still uses old memory map until vhost_set_mem_table() is complete. We probably should unref deleted(old) sections after guest gets new memmap.

Quoted 4 lines

}

static int vhost_virtqueue_set_addr(struct vhost_dev *dev,

Quoted 7 lines · Igor Mammedov

Also it seems that we have a race in current code where region_del() unrefs memory region first and then by the commit time memory region could be gone since old flatview is unreffed before commit callback is called, but guest still uses old memory map until vhost_set_mem_table() is complete. We probably should unref deleted(old) sections after guest gets new memmap.

Care trying to post a patch for stable? Might be a good idea to merge before this rework, for the sake of downstreams.

Quoted 4 lines

Quoted 4 lines

}

static int vhost_virtqueue_set_addr(struct vhost_dev *dev,

Quoted 106 lines · On Wed, 13 Dec 2017 18:08:05 +0000 "Dr. David Alan Gilbert (git)"…

From: "Dr. David Alan Gilbert" <[email protected]>

As regions are reported by the listener to the _nop and _add methods, add them to our new temporary list. Regions that abut can be merged if the backend allows.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 hw/virtio/trace-events |  2 ++
 hw/virtio/vhost.c      | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

hw/virtio/trace-events +2 -0

index 4a493bcd46..7de0663652 100644
@@ -1,6 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # hw/virtio/vhost.c
+vhost_region_add_tmp(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
+vhost_region_add_tmp_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 vhost_section(const char *name, int r) "%s:%d"
 
 # hw/virtio/virtio.c

hw/virtio/vhost.c +62 -0

index 4523f45587..2084888aa7 100644
@@ -694,6 +694,67 @@ static void vhost_commit(MemoryListener *listener)
     dev->memory_changed = false;
 }
 
+/* Adds the section data to the tmp_mem structure.
+ * It relies on the listener calling us in memory address order
+ * and for each region (via the _add and _nop methods).
+ */
+static void vhost_region_add_tmp(struct vhost_dev *dev,
+                                 MemoryRegionSection *section)
+{
+    bool need_add = true;
+    uint64_t mrs_size = int128_get64(section->size);
+    uint64_t mrs_gpa = section->offset_within_address_space;
+    uintptr_t mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
+                         section->offset_within_region;
+
+    trace_vhost_region_add_tmp(section->mr->name, mrs_gpa, mrs_size, mrs_host);
+
+    if (dev->tmp_mem->nregions) {
+        /* Since we already have at least one region, lets see if
+         * this extends it; since we're scanning in order, we only
+         * have to look at the last one, and the FlatView that calls
+         * us shouldn't have overlaps.
+         */
+        struct vhost_memory_region *prev_vmr = dev->tmp_mem->regions +
+                                               (dev->tmp_mem->nregions - 1);
+        uint64_t prev_gpa_start = prev_vmr->guest_phys_addr;
+        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start,
+                                                 prev_vmr->memory_size);
+        uint64_t prev_host_start = prev_vmr->userspace_addr;
+        uint64_t prev_host_end   = range_get_last(prev_host_start,
+                                                  prev_vmr->memory_size);
+
+        if (prev_gpa_end + 1 == mrs_gpa &&
+            prev_host_end + 1 == mrs_host &&
+            (!dev->vhost_ops->vhost_backend_can_merge ||
+                dev->vhost_ops->vhost_backend_can_merge(dev,
+                    mrs_host, mrs_size,
+                    prev_host_start, prev_vmr->memory_size))) {
+            /* The two regions abut */
+            need_add = false;
+            mrs_size = mrs_size + prev_vmr->memory_size;
+            prev_vmr->memory_size = mrs_size;
+            trace_vhost_region_add_tmp_abut(section->mr->name, mrs_size);
+        }
+    }
+
+    if (need_add) {
+        uint32_t nregions = dev->tmp_mem->nregions;
+        size_t s = offsetof(struct vhost_memory, regions) +
+                            (nregions + 1) * sizeof dev->tmp_mem->regions[0];
+        dev->tmp_mem = g_realloc(dev->tmp_mem, s);
+        dev->tmp_mem->nregions++;
+        struct vhost_memory_region *cur_vmr = &dev->tmp_mem->regions[nregions];
+
+        cur_vmr->guest_phys_addr = mrs_gpa;
+        cur_vmr->memory_size     = mrs_size;
+        cur_vmr->userspace_addr  = mrs_host;
+        cur_vmr->flags_padding = 0;
+    }
+
+
+}
+
 static void vhost_region_add(MemoryListener *listener,
                              MemoryRegionSection *section)
 {

@@ -703,6 +764,7 @@ static void vhost_region_add(MemoryListener *listener,
     if (!vhost_section(section)) {
         return;
     }
+    vhost_region_add_tmp(dev, section);
 
     ++dev->n_mem_sections;
     dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,

hw/virtio/vhost.c static void vhost_iommu_region_del(MemoryListener *listener, · line 868

Quoted 7 lines

hw/virtio/vhost.c +1 -0

@@ -800,9 +862,17 @@ static void vhost_iommu_region_del(MemoryListener *listener,
     }
 }
 
+/* Called on regions that have not changed */
 static void vhost_region_nop(MemoryListener *listener,
                              MemoryRegionSection *section)
 {

move it close to add/del callbacks

hw/virtio/vhost.c static void vhost_iommu_region_del(MemoryListener *listener, · line 875

Quoted 7 lines

hw/virtio/vhost.c +7 -0

+    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
+                                         memory_listener);
+    if (!vhost_section(section)) {
+        return;
+    }
+
+    vhost_region_add_tmp(dev, section);

if you'd operate on temporary mem_sections,
I'd add memory_region_ref() and get rid of region_del callback with its section lookup, by unreffing old sections at commit time.

Also it seems that we have a race in current code where region_del() unrefs memory region first and then by the commit time memory region could be gone since old flatview is unreffed before commit callback is called, but guest still uses old memory map until vhost_set_mem_table() is complete. We probably should unref deleted(old) sections after guest gets new memmap.

hw/virtio/vhost.c static void vhost_iommu_region_del(MemoryListener *listener, · line 878

Quoted 4 lines

hw/virtio/vhost.c +0 -0

 }
 
 static int vhost_virtqueue_set_addr(struct vhost_dev *dev,

Quoted 7 lines

Also it seems that we have a race in current code where region_del() unrefs memory region first and then by the commit time memory region could be gone since old flatview is unreffed before commit callback is called, but guest still uses old memory map until vhost_set_mem_table() is complete. We probably should unref deleted(old) sections after guest gets new memmap.

Will they really get cleaned up before the commit() returns? There's no rcu like thing guarding it?

Quoted 8 lines · Dr. David Alan Gilbert

Quoted 7 lines

Also it seems that we have a race in current code where region_del() unrefs memory region first and then by the commit time memory region could be gone since old flatview is unreffed before commit callback is called, but guest still uses old memory map until vhost_set_mem_table() is complete. We probably should unref deleted(old) sections after guest gets new memmap.

Quoted 2 lines

Will they really get cleaned up before the commit() returns? There's no rcu like thing guarding it?

The memory subsystem only keeps them alive until before commmit() is invoked.

Paolo