Rework vhost memory region updates

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

vhost_update_mem will replace the existing update mechanism. They make use of the Flatview we have now to make the update simpler. This commit just adds the basic structure.

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

---
 hw/virtio/vhost.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

hw/virtio/vhost.c +3 -0

index 938253d1e8..c959a59fb3 100644
@@ -629,6 +629,43 @@ static void vhost_begin(MemoryListener *listener)
     dev->mem_changed_start_addr = -1;
 }
 
+struct vhost_update_mem_tmp {
+    struct vhost_dev   *dev;
+    uint32_t nregions;

hw/virtio/vhost.c +20 -0

+
+    res = address_space_iterate(&address_space_memory,
+                                vhost_update_mem_cb, &vtmp);
+    if (res) {
+        goto out;
+    }
+
+    /* TODO */
+out:
+    g_free(vtmp.regions);
+    return res;
+}
+
 static void vhost_commit(MemoryListener *listener)
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,

@@ -648,6 +685,10 @@ static void vhost_commit(MemoryListener *listener)
         return;
     }
 
+    if (vhost_update_mem(dev)) {
+        return;
+    }
+
     if (dev->started) {
         start_addr = dev->mem_changed_start_addr;
         size = dev->mem_changed_end_addr - dev->mem_changed_start_addr + 1;

@@ -1523,6 +1564,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         goto fail_features;
     }
 
+    if (vhost_update_mem(hdev)) {
+        goto fail_mem;
+    }
     if (vhost_dev_has_iommu(hdev)) {
         memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
     }

Quoted 5 lines · On Wed, 29 Nov 2017 18:50:22 +0000 "Dr. David Alan Gilbert (git)"…

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

vhost_update_mem will replace the existing update mechanism. They make use of the Flatview we have now to make the update simpler. This commit just adds the basic structure.

see below suggestion on dropping vhost_region_add/vhost_region_del

Quoted 16 lines

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

---
 hw/virtio/vhost.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

hw/virtio/vhost.c +3 -0

index 938253d1e8..c959a59fb3 100644
@@ -629,6 +629,43 @@ static void vhost_begin(MemoryListener *listener)
     dev->mem_changed_start_addr = -1;
 }
 
+struct vhost_update_mem_tmp {
+    struct vhost_dev   *dev;
+    uint32_t nregions;

hw/virtio/vhost.c static void vhost_begin(MemoryListener *listener) · line 644

Quoted 10 lines

hw/virtio/vhost.c +10 -0

+    struct vhost_memory_region *regions;
+};
+
+/* Called for each MRS from vhost_update_mem */
+static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
+{
+    if (!vhost_section(mrs)) {
+        return 0;
+    }
+

it's possible to move section tracking from vhost_region_add() to here

    ++dev->n_mem_sections;                                                       
    dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,          
                                dev->n_mem_sections);                            
    ;[dev->n_mem_sections - 1] = *section;                       
    memory_region_ref(section->mr);  

...

hw/virtio/vhost.c static void vhost_begin(MemoryListener *listener) · line 655

Quoted 7 lines

hw/virtio/vhost.c +7 -0

+static int vhost_update_mem(struct vhost_dev *dev)
+{
+    int res;
+    struct vhost_update_mem_tmp vtmp;
+    vtmp.regions = 0;
+    vtmp.nregions = 0;
+    vtmp.dev = dev;

iterate over dev->mem_sections

    memory_region_unref(section->mr);

g_free(dev->mem_sections)
dev->mem_sections = NULL;

with this you won't need vhost_region_add/del callbacks anymore

Quoted 32 lines

hw/virtio/vhost.c +17 -0

+
+    res = address_space_iterate(&address_space_memory,
+                                vhost_update_mem_cb, &vtmp);
+    if (res) {
+        goto out;
+    }
+
+    /* TODO */
+out:
+    g_free(vtmp.regions);
+    return res;
+}
+
 static void vhost_commit(MemoryListener *listener)
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,

@@ -648,6 +685,10 @@ static void vhost_commit(MemoryListener *listener)
         return;
     }
 
+    if (vhost_update_mem(dev)) {
+        return;
+    }
+
     if (dev->started) {
         start_addr = dev->mem_changed_start_addr;
         size = dev->mem_changed_end_addr - dev->mem_changed_start_addr + 1;

@@ -1523,6 +1564,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         goto fail_features;
     }
 

hw/virtio/vhost.c int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) · line 1572

Quoted 6 lines

hw/virtio/vhost.c +3 -0

+    if (vhost_update_mem(hdev)) {
+        goto fail_mem;
+    }
     if (vhost_dev_has_iommu(hdev)) {
         memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
     }

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

Add the meat of update_mem_cb; this is called for each region, to add a region to our temporary list.
Our temporary list is in order we look to see if this region can be merged with the current head of list.

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

---
 hw/virtio/trace-events |  2 ++
 hw/virtio/vhost.c      | 55 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

hw/virtio/trace-events +2 -0

index 4a493bcd46..92fadec192 100644
@@ -2,6 +2,8 @@
 
 # hw/virtio/vhost.c
 vhost_section(const char *name, int r) "%s:%d"
+vhost_update_mem_cb(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
+vhost_update_mem_cb_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 
 # 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"

hw/virtio/vhost.c +7 -0

index c959a59fb3..7e3c6ae032 100644
@@ -638,11 +638,64 @@ struct vhost_update_mem_tmp {
 /* Called for each MRS from vhost_update_mem */
 static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 {
+    struct vhost_update_mem_tmp *vtmp = opaque;
+    struct vhost_memory_region *cur_vmr;
+    bool need_add = true;
+    uint64_t mrs_size;
+    uint64_t mrs_gpa;
+    uintptr_t mrs_host;
+
     if (!vhost_section(mrs)) {
         return 0;

hw/virtio/vhost.c +38 -1

+        /* 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 = vtmp->regions +
+                                               (vtmp->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 &&
+            (!vtmp->dev->vhost_ops->vhost_backend_can_merge ||
+                vtmp->dev->vhost_ops->vhost_backend_can_merge(vtmp->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_update_mem_cb_abut(mrs->mr->name, mrs_size);
+        }
+    }
+
+    if (need_add) {
+        vtmp->nregions++;
+        vtmp->regions = g_realloc_n(vtmp->regions, vtmp->nregions,
+                                    sizeof(vtmp->regions[0]));
+        cur_vmr = &vtmp->regions[vtmp->nregions - 1];
+        cur_vmr->guest_phys_addr = mrs_gpa;
+        cur_vmr->memory_size     = mrs_size;
+        cur_vmr->userspace_addr  = mrs_host;
+        cur_vmr->flags_padding = 0;
+    }
 
-    /* TODO */
     return 0;
 }
 

Quoted 47 lines · On Wed, 29 Nov 2017 18:50:23 +0000 "Dr. David Alan Gilbert (git)"…

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

Add the meat of update_mem_cb; this is called for each region, to add a region to our temporary list.
Our temporary list is in order we look to see if this region can be merged with the current head of list.

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

---
 hw/virtio/trace-events |  2 ++
 hw/virtio/vhost.c      | 55 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

hw/virtio/trace-events +2 -0

index 4a493bcd46..92fadec192 100644
@@ -2,6 +2,8 @@
 
 # hw/virtio/vhost.c
 vhost_section(const char *name, int r) "%s:%d"
+vhost_update_mem_cb(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
+vhost_update_mem_cb_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 
 # 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"

hw/virtio/vhost.c +10 -0

index c959a59fb3..7e3c6ae032 100644
@@ -638,11 +638,64 @@ struct vhost_update_mem_tmp {
 /* Called for each MRS from vhost_update_mem */
 static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 {
+    struct vhost_update_mem_tmp *vtmp = opaque;
+    struct vhost_memory_region *cur_vmr;
+    bool need_add = true;
+    uint64_t mrs_size;
+    uint64_t mrs_gpa;
+    uintptr_t mrs_host;
+
     if (!vhost_section(mrs)) {
         return 0;
     }
+    mrs_size = int128_get64(mrs->size);
+    mrs_gpa  = mrs->offset_within_address_space;
+    mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +

hw/virtio/vhost.c struct vhost_update_mem_tmp { · line 659

Quoted 6 lines

hw/virtio/vhost.c +6 -0

+                         mrs->offset_within_region;
+
+    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size,
+                              (uint64_t)mrs_host);
+
+    if (vtmp->nregions) {

What forces you to maintain helper vhost_memory_region array instead of MemoryRegionSection array?

Quoted 39 lines

hw/virtio/vhost.c +38 -0

+        /* 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 = vtmp->regions +
+                                               (vtmp->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 &&
+            (!vtmp->dev->vhost_ops->vhost_backend_can_merge ||
+                vtmp->dev->vhost_ops->vhost_backend_can_merge(vtmp->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_update_mem_cb_abut(mrs->mr->name, mrs_size);
+        }
+    }
+
+    if (need_add) {
+        vtmp->nregions++;
+        vtmp->regions = g_realloc_n(vtmp->regions, vtmp->nregions,
+                                    sizeof(vtmp->regions[0]));
+        cur_vmr = &vtmp->regions[vtmp->nregions - 1];
+        cur_vmr->guest_phys_addr = mrs_gpa;
+        cur_vmr->memory_size     = mrs_size;
+        cur_vmr->userspace_addr  = mrs_host;
+        cur_vmr->flags_padding = 0;
+    }
 

hw/virtio/vhost.c struct vhost_update_mem_tmp { · line 701

Quoted 4 lines

hw/virtio/vhost.c +0 -1

-    /* TODO */
     return 0;
 }
 

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

Dave

Dr. David Alan Gilbert (7):
memory: address_space_iterate
vhost: Move log_dirty check
vhost: New memory update functions
vhost: update_mem_cb implementation
vhost: Compare new and old memory lists
vhost: Copy updated region data into device state
vhost: Remove vhost_set_memory and children

 hw/virtio/trace-events |   8 +
 hw/virtio/vhost.c      | 424 ++++++++++++++++++++++---------------------------
 include/exec/memory.h  |  23 +++
 memory.c               |  22 +++
 4 files changed, 241 insertions(+), 236 deletions(-)

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Pls, see comments on individual patches.

Quoted 12 lines

Dave

Dr. David Alan Gilbert (7):
memory: address_space_iterate
vhost: Move log_dirty check
vhost: New memory update functions
vhost: update_mem_cb implementation
vhost: Compare new and old memory lists
vhost: Copy updated region data into device state
vhost: Remove vhost_set_memory and children

Quoted 6 lines

 hw/virtio/trace-events |   8 +
 hw/virtio/vhost.c      | 424 ++++++++++++++++++++++---------------------------
 include/exec/memory.h  |  23 +++
 memory.c               |  22 +++
 4 files changed, 241 insertions(+), 236 deletions(-)

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

Quoted 2 lines

Pls, see comments on individual patches.

Thanks; I have fixed a couple of bugs since I posted, so I'm more interested in structure/shape. Any good ideas how to test it are welcome.

Dave

Quoted 12 lines

Quoted 12 lines

Dave

Dr. David Alan Gilbert (7):
memory: address_space_iterate
vhost: Move log_dirty check
vhost: New memory update functions
vhost: update_mem_cb implementation
vhost: Compare new and old memory lists
vhost: Copy updated region data into device state
vhost: Remove vhost_set_memory and children

Quoted 7 lines

Quoted 6 lines

 hw/virtio/trace-events |   8 +
 hw/virtio/vhost.c      | 424 ++++++++++++++++++++++---------------------------
 include/exec/memory.h  |  23 +++
 memory.c               |  22 +++
 4 files changed, 241 insertions(+), 236 deletions(-)

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

Quoted 33 lines · On Thu, 30 Nov 2017 12:08:06 +0000 "Dr. David Alan Gilbert"…

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 8 lines

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

While skimming through patches I've overlooked it.

Anyways,
why memcmp(old_arr, new_arr) is not sufficient
to detect a change in memory map?

Quoted 28 lines

Quoted 2 lines

Pls, see comments on individual patches.

Thanks; I have fixed a couple of bugs since I posted, so I'm more interested in structure/shape. Any good ideas how to test it are welcome.

Dave

Quoted 19 lines

Quoted 18 lines

Dave

Dr. David Alan Gilbert (7):
memory: address_space_iterate
vhost: Move log_dirty check
vhost: New memory update functions
vhost: update_mem_cb implementation
vhost: Compare new and old memory lists
vhost: Copy updated region data into device state
vhost: Remove vhost_set_memory and children

 hw/virtio/trace-events |   8 +
 hw/virtio/vhost.c      | 424 ++++++++++++++++++++++---------------------------
 include/exec/memory.h  |  23 +++
 memory.c               |  22 +++
 4 files changed, 241 insertions(+), 236 deletions(-)

Quoted 2 lines

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

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

Quoted 33 lines · On Thu, 30 Nov 2017 12:08:06 +0000 "Dr. David Alan Gilbert"…

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 8 lines

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

While skimming through patches I've overlooked it.

Quoted 3 lines

Anyways,
why memcmp(old_arr, new_arr) is not sufficient
to detect a change in memory map?

It tells you that you've got a change, but doesn't give the start/end of the range that's changed, and those are used by vhost_commit to limit the work of vhost_verify_ring_mappings.

Dave

Quoted 28 lines

Quoted 28 lines

Quoted 2 lines

Pls, see comments on individual patches.

Thanks; I have fixed a couple of bugs since I posted, so I'm more interested in structure/shape. Any good ideas how to test it are welcome.

Dave

Quoted 19 lines

Quoted 18 lines

Dave

Dr. David Alan Gilbert (7):
memory: address_space_iterate
vhost: Move log_dirty check
vhost: New memory update functions
vhost: update_mem_cb implementation
vhost: Compare new and old memory lists
vhost: Copy updated region data into device state
vhost: Remove vhost_set_memory and children

 hw/virtio/trace-events |   8 +
 hw/virtio/vhost.c      | 424 ++++++++++++++++++++++---------------------------
 include/exec/memory.h  |  23 +++
 memory.c               |  22 +++
 4 files changed, 241 insertions(+), 236 deletions(-)

Quoted 3 lines

Quoted 2 lines

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

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

Quoted 47 lines · On Thu, 30 Nov 2017 12:47:20 +0000 "Dr. David Alan Gilbert"…

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

Quoted 33 lines · On Thu, 30 Nov 2017 12:08:06 +0000 "Dr. David Alan Gilbert"…

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 8 lines

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

While skimming through patches I've overlooked it.

Quoted 8 lines

Quoted 3 lines

Anyways,
why memcmp(old_arr, new_arr) is not sufficient
to detect a change in memory map?

It tells you that you've got a change, but doesn't give the start/end of the range that's changed, and those are used by vhost_commit to limit the work of vhost_verify_ring_mappings.

Isn't memmap list a sorted and
dev->mem_changed_[start|end]_addr are the lowest|highest
addresses of whole map?

If it's, so wouldn't getting values directly from
the 1st/last entries of array be sufficient?

Quoted 33 lines

Dave

Quoted 28 lines

Quoted 28 lines

Quoted 2 lines

Pls, see comments on individual patches.

Thanks; I have fixed a couple of bugs since I posted, so I'm more interested in structure/shape. Any good ideas how to test it are welcome.

Dave

Quoted 19 lines

Quoted 18 lines

Dave

Dr. David Alan Gilbert (7):
memory: address_space_iterate
vhost: Move log_dirty check
vhost: New memory update functions
vhost: update_mem_cb implementation
vhost: Compare new and old memory lists
vhost: Copy updated region data into device state
vhost: Remove vhost_set_memory and children

 hw/virtio/trace-events |   8 +
 hw/virtio/vhost.c      | 424 ++++++++++++++++++++++---------------------------
 include/exec/memory.h  |  23 +++
 memory.c               |  22 +++
 4 files changed, 241 insertions(+), 236 deletions(-)

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

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

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

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

Quoted 47 lines · On Thu, 30 Nov 2017 12:47:20 +0000 "Dr. David Alan Gilbert"…

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

Quoted 33 lines · On Thu, 30 Nov 2017 12:08:06 +0000 "Dr. David Alan Gilbert"…

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 8 lines

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

While skimming through patches I've overlooked it.

Quoted 8 lines

Quoted 3 lines

Anyways,
why memcmp(old_arr, new_arr) is not sufficient
to detect a change in memory map?

It tells you that you've got a change, but doesn't give the start/end of the range that's changed, and those are used by vhost_commit to limit the work of vhost_verify_ring_mappings.

Isn't memmap list a sorted and
dev->mem_changed_[start|end]_addr are the lowest|highest
addresses of whole map?

Quoted 2 lines

If it's, so wouldn't getting values directly from
the 1st/last entries of array be sufficient?

THat wasn't my understanding from the existing code; my understanding was that changed_start_addr is set by vhost_region_add->vhost_set_memory when a new region is added (or removed) and is set to the limit of the section added. But perhaps I'm misunderstanding.

(The logic in vhost_verify_ring_mappings doesn't make sense to me either though; if vhost_verify_ring_part_mapping returns 0
on success, why is it doing if (!r) { break; } surely it
should be if (r) { break; })

Quoted 63 lines · On Thu, 30 Nov 2017 13:06:29 +0000 "Dr. David Alan Gilbert"…

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

Quoted 47 lines · On Thu, 30 Nov 2017 12:47:20 +0000 "Dr. David Alan Gilbert"…

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

Quoted 33 lines · On Thu, 30 Nov 2017 12:08:06 +0000 "Dr. David Alan Gilbert"…

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 8 lines

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

While skimming through patches I've overlooked it.

Quoted 8 lines

Quoted 3 lines

Anyways,
why memcmp(old_arr, new_arr) is not sufficient
to detect a change in memory map?

It tells you that you've got a change, but doesn't give the start/end of the range that's changed, and those are used by vhost_commit to limit the work of vhost_verify_ring_mappings.

Isn't memmap list a sorted and
dev->mem_changed_[start|end]_addr are the lowest|highest
addresses of whole map?

Quoted 8 lines

Quoted 2 lines

If it's, so wouldn't getting values directly from
the 1st/last entries of array be sufficient?

THat wasn't my understanding from the existing code; my understanding was that changed_start_addr is set by vhost_region_add->vhost_set_memory when a new region is added (or removed) and is set to the limit of the section added. But perhaps I'm misunderstanding.

changed_*_addr is actually lower/upper bound of memory transaction and in practice it often includes several memory sections that get mapped during transaction (between begin - commit).

but then again,
- how expensive vhost_verify_ring_mappings() is?
- do we really need optimization here (perhaps finding out changed range is more expensive)?
- how about calling vhost_verify_ring_mappings() unconditionally?

Quoted 5 lines

(The logic in vhost_verify_ring_mappings doesn't make sense to me either though; if vhost_verify_ring_part_mapping returns 0
on success, why is it doing if (!r) { break; } surely it
should be if (r) { break; })

it looks like a bug (CCing Greg)

before (f1f9e6c5 vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout) logic used to be

if changed_*_addr doesn't contain ring
"IGNORE as we don't care"

if changed_*_addr contain ring AND ring can't be mapped at the same place
ABORT

with f1f9e6c5 we have 3 rings so on any of them following could happen
if "IGNORE as we don't care"
break => false success
since it's possible that the remaining rings in vq do overlap and didn't get checked

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

Quoted 63 lines · On Thu, 30 Nov 2017 13:06:29 +0000 "Dr. David Alan Gilbert"…

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

Quoted 47 lines · On Thu, 30 Nov 2017 12:47:20 +0000 "Dr. David Alan Gilbert"…

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

Quoted 33 lines · On Thu, 30 Nov 2017 12:08:06 +0000 "Dr. David Alan Gilbert"…

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 8 lines

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

While skimming through patches I've overlooked it.

Quoted 8 lines

Quoted 3 lines

Anyways,
why memcmp(old_arr, new_arr) is not sufficient
to detect a change in memory map?

It tells you that you've got a change, but doesn't give the start/end of the range that's changed, and those are used by vhost_commit to limit the work of vhost_verify_ring_mappings.

Isn't memmap list a sorted and
dev->mem_changed_[start|end]_addr are the lowest|highest
addresses of whole map?

Quoted 8 lines

Quoted 2 lines

If it's, so wouldn't getting values directly from
the 1st/last entries of array be sufficient?

THat wasn't my understanding from the existing code; my understanding was that changed_start_addr is set by vhost_region_add->vhost_set_memory when a new region is added (or removed) and is set to the limit of the section added. But perhaps I'm misunderstanding.

Quoted 8 lines

changed_*_addr is actually lower/upper bound of memory transaction and in practice it often includes several memory sections that get mapped during transaction (between begin - commit).

but then again,
- how expensive vhost_verify_ring_mappings() is?
- do we really need optimization here (perhaps finding out changed range is more expensive)?
- how about calling vhost_verify_ring_mappings() unconditionally?

My worry is that:
vhost_verify_ring_mappings
vhost_verify_ring_part_mapping
vhost_verify_ring_part_mapping
vhost_memory_map & vhost_memory_unmap
(non-iommu case...) cpu_physical_memory_map & cpu_physical_memory_unmap
address_space_map/address_space_unmap
flatview_translate etc

so it's not cheap at all - I *think* it should end up doing very little after it's gone all the way through that because it should already be mapped; but still it's not trivial.

Dave

Quoted 13 lines

Quoted 5 lines

(The logic in vhost_verify_ring_mappings doesn't make sense to me either though; if vhost_verify_ring_part_mapping returns 0
on success, why is it doing if (!r) { break; } surely it
should be if (r) { break; })

it looks like a bug (CCing Greg)

before (f1f9e6c5 vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout) logic used to be

if changed_*_addr doesn't contain ring
"IGNORE as we don't care"

Quoted 8 lines

if changed_*_addr contain ring AND ring can't be mapped at the same place
ABORT

with f1f9e6c5 we have 3 rings so on any of them following could happen
if "IGNORE as we don't care"
break => false success
since it's possible that the remaining rings in vq do overlap and didn't get checked

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

Quoted 94 lines · On Thu, 30 Nov 2017 15:18:55 +0000 "Dr. David Alan Gilbert"…

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

Quoted 63 lines · On Thu, 30 Nov 2017 13:06:29 +0000 "Dr. David Alan Gilbert"…

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

Quoted 47 lines · On Thu, 30 Nov 2017 12:47:20 +0000 "Dr. David Alan Gilbert"…

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

Quoted 33 lines · On Thu, 30 Nov 2017 12:08:06 +0000 "Dr. David Alan Gilbert"…

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

Quoted 6 lines · On Wed, 29 Nov 2017 18:50:19 +0000 "Dr. David Alan Gilbert (git)"…

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

Hi,
This is an experimental set that reworks the way the vhost
code handles changes in physical address space layout that came from a discussion with Igor.

Thanks for looking into it.

Quoted 10 lines

Instead of updating and trying to merge sections of address space on each add/remove callback, we wait until the commit phase and go through and rebuild a list by walking the Flatview of memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

Note, only very lightly tested so far, I'm just trying to see if it's the right shape.

Igor, is this what you were intending?

I was thinking about a little less intrusive approach

where vhost_region_add/del are modified to maintain sorted by GPA array of mem_sections, vhost_dev::mem is dropped altogether and vhost_memory_region array is build/used/freed on every vhost_commit().
Maintaining sorted array should roughly cost us O(2 log n) if binary search is used.

Quoted 8 lines

Quoted 3 lines

However I like your idea with iterator even more as it have potential to make it even faster O(n) if we get rid of quadratic and relatively complex vhost_update_compare_list().

Note vhost_update_compare_list is complex,
but it is O(n) - it's
got nested loops, but the inner loop moves forward and oldi never gets reset back to zero.

While skimming through patches I've overlooked it.

Quoted 8 lines

Quoted 3 lines

Anyways,
why memcmp(old_arr, new_arr) is not sufficient
to detect a change in memory map?

It tells you that you've got a change, but doesn't give the start/end of the range that's changed, and those are used by vhost_commit to limit the work of vhost_verify_ring_mappings.

Isn't memmap list a sorted and
dev->mem_changed_[start|end]_addr are the lowest|highest
addresses of whole map?

Quoted 8 lines

Quoted 2 lines

If it's, so wouldn't getting values directly from
the 1st/last entries of array be sufficient?

THat wasn't my understanding from the existing code; my understanding was that changed_start_addr is set by vhost_region_add->vhost_set_memory when a new region is added (or removed) and is set to the limit of the section added. But perhaps I'm misunderstanding.

Quoted 8 lines

changed_*_addr is actually lower/upper bound of memory transaction and in practice it often includes several memory sections that get mapped during transaction (between begin - commit).

but then again,
- how expensive vhost_verify_ring_mappings() is?
- do we really need optimization here (perhaps finding out changed range is more expensive)?
- how about calling vhost_verify_ring_mappings() unconditionally?

My worry is that:
vhost_verify_ring_mappings
vhost_verify_ring_part_mapping
vhost_verify_ring_part_mapping
vhost_memory_map & vhost_memory_unmap
(non-iommu case...) cpu_physical_memory_map & cpu_physical_memory_unmap
address_space_map/address_space_unmap
flatview_translate etc

Quoted 3 lines

so it's not cheap at all - I *think* it should end up doing very little after it's gone all the way through that because it should already be mapped; but still it's not trivial.

neither trivial finding out changed range.
How often it will be called and what actual time it takes
for vhost_verify_ring_mappings and vhost_update_compare_list to complete.

note
vhost_memory_map() would be run only on ranges that overlap with rings (typically 1), while vhost_update_compare_list() would go over all ranges.
So question is does optimization really saves us anything?

Quoted 20 lines

Dave

Quoted 16 lines

Quoted 5 lines

(The logic in vhost_verify_ring_mappings doesn't make sense to me either though; if vhost_verify_ring_part_mapping returns 0
on success, why is it doing if (!r) { break; } surely it
should be if (r) { break; })

it looks like a bug (CCing Greg)

before (f1f9e6c5 vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout) logic used to be

if changed_*_addr doesn't contain ring
"IGNORE as we don't care"

if changed_*_addr contain ring AND ring can't be mapped at the same place
ABORT

Quoted 7 lines

Quoted 5 lines

with f1f9e6c5 we have 3 rings so on any of them following could happen
if "IGNORE as we don't care"
break => false success
since it's possible that the remaining rings in vq do overlap and didn't get checked

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

Quoted 4 lines

(The logic in vhost_verify_ring_mappings doesn't make sense to me either though; if vhost_verify_ring_part_mapping returns 0
on success, why is it doing if (!r) { break; } surely it
should be if (r) { break; })

it looks like a bug (CCing Greg)

before (f1f9e6c5 vhost: adapt vhost_verify_ring_mappings() to virtio 1 ring layout) logic used to be

if changed_*_addr doesn't contain ring
"IGNORE as we don't care"

if changed_*_addr contain ring AND ring can't be mapped at the same place
ABORT

with f1f9e6c5 we have 3 rings so on any of them following could happen
if "IGNORE as we don't care"
break => false success
since it's possible that the remaining rings in vq do overlap and didn't get checked

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +7 -9

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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;

hw/virtio/vhost.c +31 -109

-    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;

@@ -338,23 +346,26 @@ 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);
-        if (!r) {
+        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);
-        if (!r) {
+        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);
-        if (!r) {
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
+        if (r) {
             break;
         }
     }

@@ -384,24 +395,18 @@ static bool vhost_section(MemoryRegionSection *section)
     return result;
 }
 
-static void vhost_begin(MemoryListener *listener)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-    dev->mem_changed_end_addr = 0;
-    dev->mem_changed_start_addr = -1;
-}
-
 struct vhost_update_mem_tmp {
     struct vhost_dev   *dev;
     uint32_t nregions;
     struct vhost_memory_region *regions;
 };
 
+
 /* Called for each MRS from vhost_update_mem */
 static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 {
     struct vhost_update_mem_tmp *vtmp = opaque;
+    struct vhost_dev   *dev = vtmp->dev;
     struct vhost_memory_region *cur_vmr;
     bool need_add = true;
     uint64_t mrs_size;

@@ -416,8 +421,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
     mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
                          mrs->offset_within_region;
 
-    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size,
-                              (uint64_t)mrs_host);
+    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size, mrs_host);
 
     if (vtmp->nregions) {
         /* Since we already have at least one region, lets see if

@@ -459,85 +463,19 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
         cur_vmr->flags_padding = 0;
     }
 
-    return 0;
-}
-
-static bool vhost_update_compare_list(struct vhost_dev *dev,
-                                      struct vhost_update_mem_tmp *vtmp,
-                                      hwaddr *change_start, hwaddr *change_end)
-{
-    uint32_t oldi, newi;
-    *change_start = 0;
-    *change_end = 0;
-
-    for (newi = 0, oldi = 0; newi < vtmp->nregions; newi++) {
-        struct vhost_memory_region *newr = &vtmp->regions[newi];
-        hwaddr newr_last = range_get_last(newr->guest_phys_addr,
-                                          newr->memory_size);
-        trace_vhost_update_compare_list_loopn(newi, oldi,
-                                        newr->guest_phys_addr,
-                                        newr->memory_size);
-        bool whole_change = true;
-        while (oldi < dev->mem->nregions) {
-            struct vhost_memory_region *oldr = &dev->mem->regions[oldi];
-            hwaddr oldr_last = range_get_last(oldr->guest_phys_addr,
-                                          oldr->memory_size);
-            trace_vhost_update_compare_list_loopo(newi, oldi,
-                                        oldr->guest_phys_addr,
-                                        oldr->memory_size);
-            if (newr->guest_phys_addr == oldr->guest_phys_addr &&
-                newr->memory_size == oldr->memory_size) {
-                /* Match in GPA and size, but it could be different
-                 * in host address or flags
-                 */
-                whole_change = newr->userspace_addr != oldr->userspace_addr ||
-                               newr->flags_padding != oldr->flags_padding;
-                oldi++;
-                break; /* inner old loop */
-            }
-            /* There's a difference - need to figure out what */
-            if (oldr_last < newr->guest_phys_addr) {
-                /* There used to be a region before us that's gone */
-                *change_start = MIN(*change_start, oldr->guest_phys_addr);
-                *change_end = MAX(*change_end, oldr_last);
-                oldi++;
-                continue; /* inner old loop */
-            }
-            if (oldr->guest_phys_addr > newr_last) {
-                /* We've passed all the old mappings that could have overlapped
-                 * this one
-                 */
-                break; /* Inner old loop */
-            }
-            /* Overlap case */
-            *change_start = MIN(*change_start,
-                                MIN(oldr->guest_phys_addr,
-                                    newr->guest_phys_addr));
-            *change_end = MAX(*change_end,
-                                MAX(oldr_last, newr_last));
-            whole_change = false;
-            /* There might be more old mappings that overlap */
-            oldi++;
-        }
-        if (whole_change) {
-            /* No old region to compare against, this must be a change */
-            *change_start = MIN(*change_start, newr->guest_phys_addr);
-            *change_end = MAX(*change_end, newr_last);
-        }
+    if (dev->started &&
+        vhost_verify_ring_mappings(dev, (void *)mrs_host, mrs_gpa, mrs_size)) {
+        abort();
     }
 
-    return *change_start || *change_end;
+    return 0;
 }
 
 static int vhost_update_mem(struct vhost_dev *dev)
 {
     int res;
-    struct vhost_update_mem_tmp vtmp;
-    hwaddr change_start, change_end;
-    bool need_update;
-    vtmp.regions = 0;
-    vtmp.nregions = 0;
-    vtmp.dev = dev;
+    size_t mem_size;
+    struct vhost_update_mem_tmp vtmp = {.dev = dev};
 
     res = address_space_iterate(&address_space_memory,
                                 vhost_update_mem_cb, &vtmp);

@@ -545,24 +483,17 @@ static int vhost_update_mem(struct vhost_dev *dev)
         goto out;
     }
 
-    need_update = vhost_update_compare_list(dev, &vtmp,
-                                            &change_start, &change_end);
-    trace_vhost_update_mem_comparison(need_update,
-                                      (uint64_t)change_start,
-                                      (uint64_t)change_end);
-    if (need_update) {
-        /* Update the main regions list from our tmp */
-        size_t mem_size = offsetof(struct vhost_memory, regions) +
-            (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
+    mem_size = offsetof(struct vhost_memory, regions) +
+               (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
 
+    if(vtmp.nregions != dev->mem->nregions ||
+       memcmp(vtmp.regions, dev->mem->regions, mem_size)) {
+        /* Update the main regions list from our tmp */
         dev->mem = g_realloc(dev->mem, mem_size);
         dev->mem->nregions = vtmp.nregions;
         memcpy(dev->mem->regions, vtmp.regions,
                vtmp.nregions * sizeof dev->mem->regions[0]);
         used_memslots = vtmp.nregions;
-
-        dev->mem_changed_start_addr = change_start;
-        dev->mem_changed_end_addr = change_end;
     }
 
 out:

@@ -574,8 +505,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 +0 -10

     if (vhost_update_mem(dev)) {
         return;
     }
 
-    if (dev->started) {
-        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);
-    }
-
     if (!dev->log_enabled) {
         r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
         if (r < 0) {

@@ -1235,7 +1153,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->features = features;
 
     hdev->memory_listener = (MemoryListener) {
-        .begin = vhost_begin,
         .commit = vhost_commit,
         .region_add = vhost_region_add,
         .region_del = vhost_region_del,

@@ -1458,7 +1375,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    hdev->started = true;
     hdev->vdev = vdev;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);

hw/virtio/vhost.c +0 -0

     if (vhost_dev_has_iommu(hdev)) {
         memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
     }

Signature · 1 line

2.7.4

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

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

Quoted 7 lines

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

There are a few things about this I don't understand; however if it's right I agree that it means we can kill off my comparison code.

First, can I check what constraints 'verify_ring' needs to check; if I'm understanding the original code it's checking that :
a) If a queue falls within a region, that the whole queue can be mapped

b) And the start of the queue corresponds to where we thought it used to be (in GPA?)

so that doesn't have any constraint on the ordering of the regions or whether the region is the same size or anything.
Also I don't think it would spot if there was a qeueue that had no region associated with it at all.

Now, comments on your code below:

Quoted 51 lines

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +9 -9

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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 315

Quoted 7 lines

hw/virtio/vhost.c +4 -1

-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    /* start check from the end so that the rest of checks
+     * are done when whole ring is in merged sections range
+     */
+    if (ring_last < reg_last || ring_gpa > reg_last) {
         return 0;
     }  

so does that mean if our region never grows to be as big as the ring
we wont spot the problem?

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

Quoted 11 lines

hw/virtio/vhost.c +5 -4

-    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 (range_get_last(ring_gpa, ring_size) >
+        range_get_last(reg_gpa, reg_size)) {
+        return -EBUSY;
     }  

can't that be:

       if (ring_last > reg_last) {
           return -EBUSY;
       }

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

Quoted 8 lines

hw/virtio/vhost.c +5 -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;
     }  

Is that the same as:

   if (ring_gpa - reg_gpa != ring_hva - reg_hva) ?

which seems more symmetrical if true.

Quoted 219 lines

hw/virtio/vhost.c +31 -109

-    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;

@@ -338,23 +346,26 @@ 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);
-        if (!r) {
+        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);
-        if (!r) {
+        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);
-        if (!r) {
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
+        if (r) {
             break;
         }
     }

@@ -384,24 +395,18 @@ static bool vhost_section(MemoryRegionSection *section)
     return result;
 }
 
-static void vhost_begin(MemoryListener *listener)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-    dev->mem_changed_end_addr = 0;
-    dev->mem_changed_start_addr = -1;
-}
-
 struct vhost_update_mem_tmp {
     struct vhost_dev   *dev;
     uint32_t nregions;
     struct vhost_memory_region *regions;
 };
 
+
 /* Called for each MRS from vhost_update_mem */
 static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 {
     struct vhost_update_mem_tmp *vtmp = opaque;
+    struct vhost_dev   *dev = vtmp->dev;
     struct vhost_memory_region *cur_vmr;
     bool need_add = true;
     uint64_t mrs_size;

@@ -416,8 +421,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
     mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
                          mrs->offset_within_region;
 
-    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size,
-                              (uint64_t)mrs_host);
+    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size, mrs_host);
 
     if (vtmp->nregions) {
         /* Since we already have at least one region, lets see if

@@ -459,85 +463,19 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
         cur_vmr->flags_padding = 0;
     }
 
-    return 0;
-}
-
-static bool vhost_update_compare_list(struct vhost_dev *dev,
-                                      struct vhost_update_mem_tmp *vtmp,
-                                      hwaddr *change_start, hwaddr *change_end)
-{
-    uint32_t oldi, newi;
-    *change_start = 0;
-    *change_end = 0;
-
-    for (newi = 0, oldi = 0; newi < vtmp->nregions; newi++) {
-        struct vhost_memory_region *newr = &vtmp->regions[newi];
-        hwaddr newr_last = range_get_last(newr->guest_phys_addr,
-                                          newr->memory_size);
-        trace_vhost_update_compare_list_loopn(newi, oldi,
-                                        newr->guest_phys_addr,
-                                        newr->memory_size);
-        bool whole_change = true;
-        while (oldi < dev->mem->nregions) {
-            struct vhost_memory_region *oldr = &dev->mem->regions[oldi];
-            hwaddr oldr_last = range_get_last(oldr->guest_phys_addr,
-                                          oldr->memory_size);
-            trace_vhost_update_compare_list_loopo(newi, oldi,
-                                        oldr->guest_phys_addr,
-                                        oldr->memory_size);
-            if (newr->guest_phys_addr == oldr->guest_phys_addr &&
-                newr->memory_size == oldr->memory_size) {
-                /* Match in GPA and size, but it could be different
-                 * in host address or flags
-                 */
-                whole_change = newr->userspace_addr != oldr->userspace_addr ||
-                               newr->flags_padding != oldr->flags_padding;
-                oldi++;
-                break; /* inner old loop */
-            }
-            /* There's a difference - need to figure out what */
-            if (oldr_last < newr->guest_phys_addr) {
-                /* There used to be a region before us that's gone */
-                *change_start = MIN(*change_start, oldr->guest_phys_addr);
-                *change_end = MAX(*change_end, oldr_last);
-                oldi++;
-                continue; /* inner old loop */
-            }
-            if (oldr->guest_phys_addr > newr_last) {
-                /* We've passed all the old mappings that could have overlapped
-                 * this one
-                 */
-                break; /* Inner old loop */
-            }
-            /* Overlap case */
-            *change_start = MIN(*change_start,
-                                MIN(oldr->guest_phys_addr,
-                                    newr->guest_phys_addr));
-            *change_end = MAX(*change_end,
-                                MAX(oldr_last, newr_last));
-            whole_change = false;
-            /* There might be more old mappings that overlap */
-            oldi++;
-        }
-        if (whole_change) {
-            /* No old region to compare against, this must be a change */
-            *change_start = MIN(*change_start, newr->guest_phys_addr);
-            *change_end = MAX(*change_end, newr_last);
-        }
+    if (dev->started &&
+        vhost_verify_ring_mappings(dev, (void *)mrs_host, mrs_gpa, mrs_size)) {
+        abort();
     }
 
-    return *change_start || *change_end;
+    return 0;
 }
 
 static int vhost_update_mem(struct vhost_dev *dev)
 {
     int res;
-    struct vhost_update_mem_tmp vtmp;
-    hwaddr change_start, change_end;
-    bool need_update;
-    vtmp.regions = 0;
-    vtmp.nregions = 0;
-    vtmp.dev = dev;
+    size_t mem_size;
+    struct vhost_update_mem_tmp vtmp = {.dev = dev};
 
     res = address_space_iterate(&address_space_memory,
                                 vhost_update_mem_cb, &vtmp);

@@ -545,24 +483,17 @@ static int vhost_update_mem(struct vhost_dev *dev)
         goto out;
     }
 
-    need_update = vhost_update_compare_list(dev, &vtmp,
-                                            &change_start, &change_end);
-    trace_vhost_update_mem_comparison(need_update,
-                                      (uint64_t)change_start,
-                                      (uint64_t)change_end);
-    if (need_update) {
-        /* Update the main regions list from our tmp */
-        size_t mem_size = offsetof(struct vhost_memory, regions) +
-            (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
+    mem_size = offsetof(struct vhost_memory, regions) +
+               (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
 
+    if(vtmp.nregions != dev->mem->nregions ||
+       memcmp(vtmp.regions, dev->mem->regions, mem_size)) {
+        /* Update the main regions list from our tmp */
         dev->mem = g_realloc(dev->mem, mem_size);
         dev->mem->nregions = vtmp.nregions;
         memcpy(dev->mem->regions, vtmp.regions,
                vtmp.nregions * sizeof dev->mem->regions[0]);
         used_memslots = vtmp.nregions;
-
-        dev->mem_changed_start_addr = change_start;
-        dev->mem_changed_end_addr = change_end;
     }
 
 out:

@@ -574,8 +505,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 590

Quoted 7 lines

hw/virtio/vhost.c +0 -3

@@ -585,22 +514,11 @@ static void vhost_commit(MemoryListener *listener)
     if (!dev->started) {
         return;
     }
-    if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
-        return;
-    }  

This was in the wrong place in the version I posted; this should have been after the vhost_update_mem; I think we still need something telling us whether vhost_update_mem found a change, because we want to skip the rest of vhost_commit if there's no change at all.

Quoted 33 lines

hw/virtio/vhost.c +0 -10

     if (vhost_update_mem(dev)) {
         return;
     }
 
-    if (dev->started) {
-        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);
-    }
-
     if (!dev->log_enabled) {
         r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
         if (r < 0) {

@@ -1235,7 +1153,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->features = features;
 
     hdev->memory_listener = (MemoryListener) {
-        .begin = vhost_begin,
         .commit = vhost_commit,
         .region_add = vhost_region_add,
         .region_del = vhost_region_del,

@@ -1458,7 +1375,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    hdev->started = true;
     hdev->vdev = vdev;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);

hw/virtio/vhost.c int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) · line 1390

Quoted 6 lines

hw/virtio/vhost.c +3 -0

@@ -1469,6 +1385,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     if (vhost_update_mem(hdev)) {
         goto fail_mem;
     }
+
+    hdev->started = true;
+  

Ah, good spot.

Dave

Quoted 6 lines

hw/virtio/vhost.c +0 -0

     if (vhost_dev_has_iommu(hdev)) {
         memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
     }

Signature · 1 line

2.7.4

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

Quoted 35 lines · On Thu, 7 Dec 2017 18:17:51 +0000 "Dr. David Alan Gilbert"…

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

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

Quoted 7 lines

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

There are a few things about this I don't understand; however if it's right I agree that it means we can kill off my comparison code.

Quoted 4 lines

First, can I check what constraints 'verify_ring' needs to check; if I'm understanding the original code it's checking that :
a) If a queue falls within a region, that the whole queue can be mapped

see vhost_verify_ring_part_mapping():

p = vhost_memory_map(dev, part_addr, &l, 1); if (!p || l != part_size)
error_out

1st: (!p) requested part_addr must be mapped
i.e. be a part of MemorySection in flatview
AND
2nd: (l != part_size) mapped range size must match what we asked for
i.e. mapped as continuous range so that
[GPA, GPA + part_size) could directly correspond to [HVA, HVA + part_size)
and that's is possible only withing 1 MemorySection && 1 MeoryRegion if I read address_space_map() correctly
flatview_translate() -> GPA's MemorySection flatview_extend_translation() -> 1:1 GPA->HVA range size

So answer in case of RAM memory region that we are interested in, would be:
queue falls within a MemorySection and whole queue fits in to it

Quoted 2 lines

b) And the start of the queue corresponds to where we thought it used to be (in GPA?)

         that cached at vq->desc queue HVA hasn't changed after flatview change
            if (p != part)
               error_out

Quoted 59 lines

so that doesn't have any constraint on the ordering of the regions or whether the region is the same size or anything.
Also I don't think it would spot if there was a qeueue that had no region associated with it at all.

Now, comments on your code below:

Quoted 51 lines

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +9 -9

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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);
 

Quoted 10 lines

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

Quoted 7 lines

hw/virtio/vhost.c +4 -1

-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    /* start check from the end so that the rest of checks
+     * are done when whole ring is in merged sections range
+     */
+    if (ring_last < reg_last || ring_gpa > reg_last) {
         return 0;
     }  

so does that mean if our region never grows to be as big as the ring
we wont spot the problem?

I think there is mistake here it should be:

   ring_last < reg_gpa || ring_gpa > reg_last

so if ring is out side of continuous region, we do not care => no change

Quoted 11 lines

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

Quoted 10 lines

hw/virtio/vhost.c +5 -4

-    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 (range_get_last(ring_gpa, ring_size) >
+        range_get_last(reg_gpa, reg_size)) {
+        return -EBUSY;
     }  

Quoted 4 lines

can't that be:

       if (ring_last > reg_last) {
           return -EBUSY;
       }

yep

Quoted 9 lines

Quoted 8 lines

hw/virtio/vhost.c +5 -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;
     }  

Quoted 3 lines

Is that the same as:

   if (ring_gpa - reg_gpa != ring_hva - reg_hva) ?

which seems more symmetrical if true.

it looks more cryptic to me, my more verbose variant should be easier to read when one forgets how it all works and needs to figure it out again.

Quoted 219 lines

Quoted 219 lines

hw/virtio/vhost.c +31 -109

-    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;

@@ -338,23 +346,26 @@ 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);
-        if (!r) {
+        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);
-        if (!r) {
+        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);
-        if (!r) {
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
+        if (r) {
             break;
         }
     }

@@ -384,24 +395,18 @@ static bool vhost_section(MemoryRegionSection *section)
     return result;
 }
 
-static void vhost_begin(MemoryListener *listener)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-    dev->mem_changed_end_addr = 0;
-    dev->mem_changed_start_addr = -1;
-}
-
 struct vhost_update_mem_tmp {
     struct vhost_dev   *dev;
     uint32_t nregions;
     struct vhost_memory_region *regions;
 };
 
+
 /* Called for each MRS from vhost_update_mem */
 static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 {
     struct vhost_update_mem_tmp *vtmp = opaque;
+    struct vhost_dev   *dev = vtmp->dev;
     struct vhost_memory_region *cur_vmr;
     bool need_add = true;
     uint64_t mrs_size;

@@ -416,8 +421,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
     mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
                          mrs->offset_within_region;
 
-    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size,
-                              (uint64_t)mrs_host);
+    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size, mrs_host);
 
     if (vtmp->nregions) {
         /* Since we already have at least one region, lets see if

@@ -459,85 +463,19 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
         cur_vmr->flags_padding = 0;
     }
 
-    return 0;
-}
-
-static bool vhost_update_compare_list(struct vhost_dev *dev,
-                                      struct vhost_update_mem_tmp *vtmp,
-                                      hwaddr *change_start, hwaddr *change_end)
-{
-    uint32_t oldi, newi;
-    *change_start = 0;
-    *change_end = 0;
-
-    for (newi = 0, oldi = 0; newi < vtmp->nregions; newi++) {
-        struct vhost_memory_region *newr = &vtmp->regions[newi];
-        hwaddr newr_last = range_get_last(newr->guest_phys_addr,
-                                          newr->memory_size);
-        trace_vhost_update_compare_list_loopn(newi, oldi,
-                                        newr->guest_phys_addr,
-                                        newr->memory_size);
-        bool whole_change = true;
-        while (oldi < dev->mem->nregions) {
-            struct vhost_memory_region *oldr = &dev->mem->regions[oldi];
-            hwaddr oldr_last = range_get_last(oldr->guest_phys_addr,
-                                          oldr->memory_size);
-            trace_vhost_update_compare_list_loopo(newi, oldi,
-                                        oldr->guest_phys_addr,
-                                        oldr->memory_size);
-            if (newr->guest_phys_addr == oldr->guest_phys_addr &&
-                newr->memory_size == oldr->memory_size) {
-                /* Match in GPA and size, but it could be different
-                 * in host address or flags
-                 */
-                whole_change = newr->userspace_addr != oldr->userspace_addr ||
-                               newr->flags_padding != oldr->flags_padding;
-                oldi++;
-                break; /* inner old loop */
-            }
-            /* There's a difference - need to figure out what */
-            if (oldr_last < newr->guest_phys_addr) {
-                /* There used to be a region before us that's gone */
-                *change_start = MIN(*change_start, oldr->guest_phys_addr);
-                *change_end = MAX(*change_end, oldr_last);
-                oldi++;
-                continue; /* inner old loop */
-            }
-            if (oldr->guest_phys_addr > newr_last) {
-                /* We've passed all the old mappings that could have overlapped
-                 * this one
-                 */
-                break; /* Inner old loop */
-            }
-            /* Overlap case */
-            *change_start = MIN(*change_start,
-                                MIN(oldr->guest_phys_addr,
-                                    newr->guest_phys_addr));
-            *change_end = MAX(*change_end,
-                                MAX(oldr_last, newr_last));
-            whole_change = false;
-            /* There might be more old mappings that overlap */
-            oldi++;
-        }
-        if (whole_change) {
-            /* No old region to compare against, this must be a change */
-            *change_start = MIN(*change_start, newr->guest_phys_addr);
-            *change_end = MAX(*change_end, newr_last);
-        }
+    if (dev->started &&
+        vhost_verify_ring_mappings(dev, (void *)mrs_host, mrs_gpa, mrs_size)) {
+        abort();
     }
 
-    return *change_start || *change_end;
+    return 0;
 }
 
 static int vhost_update_mem(struct vhost_dev *dev)
 {
     int res;
-    struct vhost_update_mem_tmp vtmp;
-    hwaddr change_start, change_end;
-    bool need_update;
-    vtmp.regions = 0;
-    vtmp.nregions = 0;
-    vtmp.dev = dev;
+    size_t mem_size;
+    struct vhost_update_mem_tmp vtmp = {.dev = dev};
 
     res = address_space_iterate(&address_space_memory,
                                 vhost_update_mem_cb, &vtmp);

@@ -545,24 +483,17 @@ static int vhost_update_mem(struct vhost_dev *dev)
         goto out;
     }
 
-    need_update = vhost_update_compare_list(dev, &vtmp,
-                                            &change_start, &change_end);
-    trace_vhost_update_mem_comparison(need_update,
-                                      (uint64_t)change_start,
-                                      (uint64_t)change_end);
-    if (need_update) {
-        /* Update the main regions list from our tmp */
-        size_t mem_size = offsetof(struct vhost_memory, regions) +
-            (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
+    mem_size = offsetof(struct vhost_memory, regions) +
+               (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
 
+    if(vtmp.nregions != dev->mem->nregions ||
+       memcmp(vtmp.regions, dev->mem->regions, mem_size)) {
+        /* Update the main regions list from our tmp */
         dev->mem = g_realloc(dev->mem, mem_size);
         dev->mem->nregions = vtmp.nregions;
         memcpy(dev->mem->regions, vtmp.regions,
                vtmp.nregions * sizeof dev->mem->regions[0]);
         used_memslots = vtmp.nregions;
-
-        dev->mem_changed_start_addr = change_start;
-        dev->mem_changed_end_addr = change_end;
     }
 
 out:

@@ -574,8 +505,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;
 

Quoted 12 lines

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

Quoted 7 lines

hw/virtio/vhost.c +0 -3

@@ -585,22 +514,11 @@ static void vhost_commit(MemoryListener *listener)
     if (!dev->started) {
         return;
     }
-    if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
-        return;
-    }  

This was in the wrong place in the version I posted; this should have been after the vhost_update_mem; I think we still need something telling us whether vhost_update_mem found a change, because we want to skip the rest of vhost_commit if there's no change at all.

maybe cache memcmp result in dev->memory_changed

Quoted 44 lines

Quoted 33 lines

hw/virtio/vhost.c +0 -10

     if (vhost_update_mem(dev)) {
         return;
     }
 
-    if (dev->started) {
-        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);
-    }
-
     if (!dev->log_enabled) {
         r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
         if (r < 0) {

@@ -1235,7 +1153,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->features = features;
 
     hdev->memory_listener = (MemoryListener) {
-        .begin = vhost_begin,
         .commit = vhost_commit,
         .region_add = vhost_region_add,
         .region_del = vhost_region_del,

@@ -1458,7 +1375,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    hdev->started = true;
     hdev->vdev = vdev;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);

hw/virtio/vhost.c int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) · line 1390

Quoted 6 lines

hw/virtio/vhost.c +3 -0

@@ -1469,6 +1385,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     if (vhost_update_mem(hdev)) {
         goto fail_mem;
     }
+
+    hdev->started = true;
+  

Ah, good spot.

Dave

Quoted 8 lines

Quoted 6 lines

hw/virtio/vhost.c +0 -0

     if (vhost_dev_has_iommu(hdev)) {
         memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
     }

Signature · 1 line

2.7.4

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

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

Quoted 35 lines · On Thu, 7 Dec 2017 18:17:51 +0000 "Dr. David Alan Gilbert"…

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

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

Quoted 7 lines

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

There are a few things about this I don't understand; however if it's right I agree that it means we can kill off my comparison code.

Quoted 4 lines

First, can I check what constraints 'verify_ring' needs to check; if I'm understanding the original code it's checking that :
a) If a queue falls within a region, that the whole queue can be mapped

see vhost_verify_ring_part_mapping():

p = vhost_memory_map(dev, part_addr, &l, 1); if (!p || l != part_size)
error_out

1st: (!p) requested part_addr must be mapped
i.e. be a part of MemorySection in flatview
AND
2nd: (l != part_size) mapped range size must match what we asked for
i.e. mapped as continuous range so that
[GPA, GPA + part_size) could directly correspond to [HVA, HVA + part_size)
and that's is possible only withing 1 MemorySection && 1 MeoryRegion if I read address_space_map() correctly
flatview_translate() -> GPA's MemorySection flatview_extend_translation() -> 1:1 GPA->HVA range size

Quoted 2 lines

So answer in case of RAM memory region that we are interested in, would be:
queue falls within a MemorySection and whole queue fits in to it

Yes, OK.

Quoted 5 lines

Quoted 2 lines

b) And the start of the queue corresponds to where we thought it used to be (in GPA?)

         that cached at vq->desc queue HVA hasn't changed after flatview change
            if (p != part)
               error_out  

OK, so it's the HVA that's not changed based on taking the part_addr which is GPA and checking the map?

Quoted 72 lines

Quoted 59 lines

so that doesn't have any constraint on the ordering of the regions or whether the region is the same size or anything.
Also I don't think it would spot if there was a qeueue that had no region associated with it at all.

Now, comments on your code below:

Quoted 51 lines

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +9 -9

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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);
 

Quoted 10 lines

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

Quoted 7 lines

hw/virtio/vhost.c +4 -1

-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    /* start check from the end so that the rest of checks
+     * are done when whole ring is in merged sections range
+     */
+    if (ring_last < reg_last || ring_gpa > reg_last) {
         return 0;
     }    

so does that mean if our region never grows to be as big as the ring
we wont spot the problem?

I think there is mistake here it should be:

   ring_last < reg_gpa || ring_gpa > reg_last

Quoted 1 line

so if ring is out side of continuous region, we do not care => no change

OK, I don't see how that corresponds to your 'start check from the end' comment - I thought it was doing something smarter to deal with this being called from the _cb routine potentially before another part of the range had been joined onto it.
In that case, we can just use ranges_overlap like the original routine did.

Quoted 65 lines · On Fri, 8 Dec 2017 17:51:56 +0000 "Dr. David Alan Gilbert"…

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

Quoted 35 lines · On Thu, 7 Dec 2017 18:17:51 +0000 "Dr. David Alan Gilbert"…

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

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

Quoted 7 lines

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

There are a few things about this I don't understand; however if it's right I agree that it means we can kill off my comparison code.

Quoted 4 lines

First, can I check what constraints 'verify_ring' needs to check; if I'm understanding the original code it's checking that :
a) If a queue falls within a region, that the whole queue can be mapped

see vhost_verify_ring_part_mapping():

p = vhost_memory_map(dev, part_addr, &l, 1); if (!p || l != part_size)
error_out

1st: (!p) requested part_addr must be mapped
i.e. be a part of MemorySection in flatview
AND
2nd: (l != part_size) mapped range size must match what we asked for
i.e. mapped as continuous range so that
[GPA, GPA + part_size) could directly correspond to [HVA, HVA + part_size)
and that's is possible only withing 1 MemorySection && 1 MeoryRegion if I read address_space_map() correctly
flatview_translate() -> GPA's MemorySection flatview_extend_translation() -> 1:1 GPA->HVA range size

Quoted 2 lines

So answer in case of RAM memory region that we are interested in, would be:
queue falls within a MemorySection and whole queue fits in to it

Yes, OK.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

b) And the start of the queue corresponds to where we thought it used to be (in GPA?)

         that cached at vq->desc queue HVA hasn't changed after flatview change
            if (p != part)
               error_out  

OK, so it's the HVA that's not changed based on taking the part_addr which is GPA and checking the map?

Yes, I think so.

Quoted 72 lines

Quoted 72 lines

Quoted 59 lines

so that doesn't have any constraint on the ordering of the regions or whether the region is the same size or anything.
Also I don't think it would spot if there was a qeueue that had no region associated with it at all.

Now, comments on your code below:

Quoted 51 lines

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +9 -9

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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);
 

Quoted 10 lines

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

Quoted 7 lines

hw/virtio/vhost.c +4 -1

-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    /* start check from the end so that the rest of checks
+     * are done when whole ring is in merged sections range
+     */
+    if (ring_last < reg_last || ring_gpa > reg_last) {
         return 0;
     }    

so does that mean if our region never grows to be as big as the ring
we wont spot the problem?

I think there is mistake here it should be:

   ring_last < reg_gpa || ring_gpa > reg_last

Quoted 8 lines

Quoted 1 line

so if ring is out side of continuous region, we do not care => no change

OK, I don't see how that corresponds to your 'start check from the end' comment - I thought it was doing something smarter to deal with this being called from the _cb routine potentially before another part of the range had been joined onto it.
In that case, we can just use ranges_overlap like the original routine did.

I suppose range check will do as well, the reason for check from the end was optimization to make less checks than ranges_overlap(), considering we are comparing against vhost_memory_region.

But it seems like a bit an overdoing, since by the commit time flatview would contain 1:1 mapping of MemorySection:vhost_memory_region (modulo sections we are not interested in). It should be unlikely to get 2 MemoryRegions allocated one after another and merge in vhost_memory_region() into one vhost_memory_region. Maybe we would be better off with just copying MemorySections into vhost_memory_region in vhost_update_mem_cb() and drop merging altogether.

I'd even consider 'non deterministic' merging of 2 MemoryRegions harmful to migration, since vhost might see different memory map on target vs source.

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

Quoted 65 lines · On Fri, 8 Dec 2017 17:51:56 +0000 "Dr. David Alan Gilbert"…

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

Quoted 35 lines · On Thu, 7 Dec 2017 18:17:51 +0000 "Dr. David Alan Gilbert"…

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

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

Quoted 7 lines

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

There are a few things about this I don't understand; however if it's right I agree that it means we can kill off my comparison code.

Quoted 4 lines

First, can I check what constraints 'verify_ring' needs to check; if I'm understanding the original code it's checking that :
a) If a queue falls within a region, that the whole queue can be mapped

see vhost_verify_ring_part_mapping():

p = vhost_memory_map(dev, part_addr, &l, 1); if (!p || l != part_size)
error_out

1st: (!p) requested part_addr must be mapped
i.e. be a part of MemorySection in flatview
AND
2nd: (l != part_size) mapped range size must match what we asked for
i.e. mapped as continuous range so that
[GPA, GPA + part_size) could directly correspond to [HVA, HVA + part_size)
and that's is possible only withing 1 MemorySection && 1 MeoryRegion if I read address_space_map() correctly
flatview_translate() -> GPA's MemorySection flatview_extend_translation() -> 1:1 GPA->HVA range size

Quoted 2 lines

So answer in case of RAM memory region that we are interested in, would be:
queue falls within a MemorySection and whole queue fits in to it

Yes, OK.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

b) And the start of the queue corresponds to where we thought it used to be (in GPA?)

         that cached at vq->desc queue HVA hasn't changed after flatview change
            if (p != part)
               error_out    

OK, so it's the HVA that's not changed based on taking the part_addr which is GPA and checking the map?

Yes, I think so.

Quoted 80 lines

Quoted 69 lines

Quoted 59 lines

so that doesn't have any constraint on the ordering of the regions or whether the region is the same size or anything.
Also I don't think it would spot if there was a qeueue that had no region associated with it at all.

Now, comments on your code below:

Quoted 51 lines

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +9 -9

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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);
 

Quoted 10 lines

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

Quoted 7 lines

hw/virtio/vhost.c +4 -1

-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    /* start check from the end so that the rest of checks
+     * are done when whole ring is in merged sections range
+     */
+    if (ring_last < reg_last || ring_gpa > reg_last) {
         return 0;
     }      

so does that mean if our region never grows to be as big as the ring
we wont spot the problem?

Quoted 4 lines

I think there is mistake here it should be:

   ring_last < reg_gpa || ring_gpa > reg_last

so if ring is out side of continuous region, we do not care => no change

OK, I don't see how that corresponds to your 'start check from the end' comment - I thought it was doing something smarter to deal with this being called from the _cb routine potentially before another part of the range had been joined onto it.
In that case, we can just use ranges_overlap like the original routine did.

Quoted 3 lines

I suppose range check will do as well, the reason for check from the end was optimization to make less checks than ranges_overlap(), considering we are comparing against vhost_memory_region.

Have a look at the RFC v2 I've posted; I've reworked it a bit so we call this code aftwards rather than from the _cb.

Quoted 8 lines

But it seems like a bit an overdoing, since by the commit time flatview would contain 1:1 mapping of MemorySection:vhost_memory_region (modulo sections we are not interested in). It should be unlikely to get 2 MemoryRegions allocated one after another and merge in vhost_memory_region() into one vhost_memory_region. Maybe we would be better off with just copying MemorySections into vhost_memory_region in vhost_update_mem_cb() and drop merging altogether.

Except I need the merging for the hugepage stuff that's coming in the postcopy world.

Quoted 2 lines

I'd even consider 'non deterministic' merging of 2 MemoryRegions harmful to migration, since vhost might see different memory map on target vs source.

I think that's come up before and it was decided it's not a problem as long as the regions are still mapped; the only consistency required is between the qemu and the vhost (either the kernel or the vhost-user); it shouldn't be a guest visibile issue if I understand it correctly.

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

Quoted 159 lines · On Mon, 11 Dec 2017 11:03:00 +0000 "Dr. David Alan Gilbert"…

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

Quoted 65 lines · On Fri, 8 Dec 2017 17:51:56 +0000 "Dr. David Alan Gilbert"…

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

Quoted 35 lines · On Thu, 7 Dec 2017 18:17:51 +0000 "Dr. David Alan Gilbert"…

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

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

Quoted 7 lines

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

There are a few things about this I don't understand; however if it's right I agree that it means we can kill off my comparison code.

Quoted 4 lines

First, can I check what constraints 'verify_ring' needs to check; if I'm understanding the original code it's checking that :
a) If a queue falls within a region, that the whole queue can be mapped

see vhost_verify_ring_part_mapping():

p = vhost_memory_map(dev, part_addr, &l, 1); if (!p || l != part_size)
error_out

1st: (!p) requested part_addr must be mapped
i.e. be a part of MemorySection in flatview
AND
2nd: (l != part_size) mapped range size must match what we asked for
i.e. mapped as continuous range so that
[GPA, GPA + part_size) could directly correspond to [HVA, HVA + part_size)
and that's is possible only withing 1 MemorySection && 1 MeoryRegion if I read address_space_map() correctly
flatview_translate() -> GPA's MemorySection flatview_extend_translation() -> 1:1 GPA->HVA range size

Quoted 2 lines

So answer in case of RAM memory region that we are interested in, would be:
queue falls within a MemorySection and whole queue fits in to it

Yes, OK.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

b) And the start of the queue corresponds to where we thought it used to be (in GPA?)

         that cached at vq->desc queue HVA hasn't changed after flatview change
            if (p != part)
               error_out    

OK, so it's the HVA that's not changed based on taking the part_addr which is GPA and checking the map?

Yes, I think so.

Quoted 80 lines

Quoted 69 lines

Quoted 59 lines

so that doesn't have any constraint on the ordering of the regions or whether the region is the same size or anything.
Also I don't think it would spot if there was a qeueue that had no region associated with it at all.

Now, comments on your code below:

Quoted 51 lines

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +9 -9

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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);
 

Quoted 10 lines

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

Quoted 7 lines

hw/virtio/vhost.c +4 -1

-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    /* start check from the end so that the rest of checks
+     * are done when whole ring is in merged sections range
+     */
+    if (ring_last < reg_last || ring_gpa > reg_last) {
         return 0;
     }      

so does that mean if our region never grows to be as big as the ring
we wont spot the problem?

Quoted 4 lines

I think there is mistake here it should be:

   ring_last < reg_gpa || ring_gpa > reg_last

so if ring is out side of continuous region, we do not care => no change

OK, I don't see how that corresponds to your 'start check from the end' comment - I thought it was doing something smarter to deal with this being called from the _cb routine potentially before another part of the range had been joined onto it.
In that case, we can just use ranges_overlap like the original routine did.

Quoted 6 lines

Quoted 3 lines

I suppose range check will do as well, the reason for check from the end was optimization to make less checks than ranges_overlap(), considering we are comparing against vhost_memory_region.

Have a look at the RFC v2 I've posted; I've reworked it a bit so we call this code aftwards rather than from the _cb.

I'll skimmed through it already and it looks good to me on the first glance

/it needs spelling fixes at least/

I'll do line by line review and testing this week

Quoted 12 lines

Quoted 8 lines

But it seems like a bit an overdoing, since by the commit time flatview would contain 1:1 mapping of MemorySection:vhost_memory_region (modulo sections we are not interested in). It should be unlikely to get 2 MemoryRegions allocated one after another and merge in vhost_memory_region() into one vhost_memory_region. Maybe we would be better off with just copying MemorySections into vhost_memory_region in vhost_update_mem_cb() and drop merging altogether.

Except I need the merging for the hugepage stuff that's coming in the postcopy world.

Quoted 7 lines

Quoted 2 lines

I'd even consider 'non deterministic' merging of 2 MemoryRegions harmful to migration, since vhost might see different memory map on target vs source.

I think that's come up before and it was decided it's not a problem as long as the regions are still mapped; the only consistency required is between the qemu and the vhost (either the kernel or the vhost-user); it shouldn't be a guest visibile issue if I understand it correctly.

non consistent merging will cause change in number of memory regions in vhost memmap and even if it's not visible to guests it's still migration blocker in borderline cases where number of regions
is around vhost_backend_memslots_limit(). i.e. it source QEMU starts fine by luck (merging happens) but target fails as it hits limit (merging fails).

Anyways it's problem that exists in current code as well and not related to this series, so we can ponder on it later (perhaps we don't see the issue because merging doesn't happen in practice).

Quoted 5 lines

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

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 were doing checks by mapping ring's GPA+len and checking that HVA hasn'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 were in changed range.

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

That could be done during time when we are building vhost memmap where vhost_update_mem_cb() already maps every RAM MemoryRegionSection to get section HVA. This fact can be used to check that ring belongs to the same MemoryRegion in the same place, like this:

hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection) ring_hva == HVA(MemoryRegionSection) + hva_ring_offset

Doing this would allow us to drop ~100LOC of code which figures out changed memory range and avoid calling not needed reverse vhost_memory_map(is_write=true) which is overkill for the task at hand.

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

---
PS:
should be applied on top of David's series

---
 include/hw/virtio/vhost.h |   2 -
 hw/virtio/vhost.c         | 195 ++++++++++++++--------------------------------
 2 files changed, 57 insertions(+), 140 deletions(-)

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

index 467dc77..fc4af5c 100644
@@ -68,8 +68,6 @@ struct vhost_dev {
     uint64_t log_size;
     Error *migration_blocker;
     bool memory_changed;
-    hwaddr mem_changed_start_addr;
-    hwaddr mem_changed_end_addr;
     const VhostOps *vhost_ops;
     void *opaque;
     struct vhost_log *log;

hw/virtio/vhost.c +57 -138

index 5b9a7d7..026bac3 100644
@@ -296,35 +296,43 @@ 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)) {
+    /* start check from the end so that the rest of checks
+     * are done when whole ring is in merged sections range
+     */
+    if (ring_last < reg_last || 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 (range_get_last(ring_gpa, ring_size) >
+        range_get_last(reg_gpa, reg_size)) {
+        return -EBUSY;
     }
-    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;
     }
-    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;

@@ -338,23 +346,26 @@ 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);
-        if (!r) {
+        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);
-        if (!r) {
+        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);
-        if (!r) {
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
+        if (r) {
             break;
         }
     }

@@ -384,24 +395,18 @@ static bool vhost_section(MemoryRegionSection *section)
     return result;
 }
 
-static void vhost_begin(MemoryListener *listener)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-    dev->mem_changed_end_addr = 0;
-    dev->mem_changed_start_addr = -1;
-}
-
 struct vhost_update_mem_tmp {
     struct vhost_dev   *dev;
     uint32_t nregions;
     struct vhost_memory_region *regions;
 };
 
+
 /* Called for each MRS from vhost_update_mem */
 static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 {
     struct vhost_update_mem_tmp *vtmp = opaque;
+    struct vhost_dev   *dev = vtmp->dev;
     struct vhost_memory_region *cur_vmr;
     bool need_add = true;
     uint64_t mrs_size;

@@ -416,8 +421,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
     mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
                          mrs->offset_within_region;
 
-    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size,
-                              (uint64_t)mrs_host);
+    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size, mrs_host);
 
     if (vtmp->nregions) {
         /* Since we already have at least one region, lets see if

@@ -459,85 +463,19 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
         cur_vmr->flags_padding = 0;
     }
 
-    return 0;
-}
-
-static bool vhost_update_compare_list(struct vhost_dev *dev,
-                                      struct vhost_update_mem_tmp *vtmp,
-                                      hwaddr *change_start, hwaddr *change_end)
-{
-    uint32_t oldi, newi;
-    *change_start = 0;
-    *change_end = 0;
-
-    for (newi = 0, oldi = 0; newi < vtmp->nregions; newi++) {
-        struct vhost_memory_region *newr = &vtmp->regions[newi];
-        hwaddr newr_last = range_get_last(newr->guest_phys_addr,
-                                          newr->memory_size);
-        trace_vhost_update_compare_list_loopn(newi, oldi,
-                                        newr->guest_phys_addr,
-                                        newr->memory_size);
-        bool whole_change = true;
-        while (oldi < dev->mem->nregions) {
-            struct vhost_memory_region *oldr = &dev->mem->regions[oldi];
-            hwaddr oldr_last = range_get_last(oldr->guest_phys_addr,
-                                          oldr->memory_size);
-            trace_vhost_update_compare_list_loopo(newi, oldi,
-                                        oldr->guest_phys_addr,
-                                        oldr->memory_size);
-            if (newr->guest_phys_addr == oldr->guest_phys_addr &&
-                newr->memory_size == oldr->memory_size) {
-                /* Match in GPA and size, but it could be different
-                 * in host address or flags
-                 */
-                whole_change = newr->userspace_addr != oldr->userspace_addr ||
-                               newr->flags_padding != oldr->flags_padding;
-                oldi++;
-                break; /* inner old loop */
-            }
-            /* There's a difference - need to figure out what */
-            if (oldr_last < newr->guest_phys_addr) {
-                /* There used to be a region before us that's gone */
-                *change_start = MIN(*change_start, oldr->guest_phys_addr);
-                *change_end = MAX(*change_end, oldr_last);
-                oldi++;
-                continue; /* inner old loop */
-            }
-            if (oldr->guest_phys_addr > newr_last) {
-                /* We've passed all the old mappings that could have overlapped
-                 * this one
-                 */
-                break; /* Inner old loop */
-            }
-            /* Overlap case */
-            *change_start = MIN(*change_start,
-                                MIN(oldr->guest_phys_addr,
-                                    newr->guest_phys_addr));
-            *change_end = MAX(*change_end,
-                                MAX(oldr_last, newr_last));
-            whole_change = false;
-            /* There might be more old mappings that overlap */
-            oldi++;
-        }
-        if (whole_change) {
-            /* No old region to compare against, this must be a change */
-            *change_start = MIN(*change_start, newr->guest_phys_addr);
-            *change_end = MAX(*change_end, newr_last);
-        }
+    if (dev->started &&
+        vhost_verify_ring_mappings(dev, (void *)mrs_host, mrs_gpa, mrs_size)) {
+        abort();
     }
 
-    return *change_start || *change_end;
+    return 0;
 }
 
 static int vhost_update_mem(struct vhost_dev *dev)
 {
     int res;
-    struct vhost_update_mem_tmp vtmp;
-    hwaddr change_start, change_end;
-    bool need_update;
-    vtmp.regions = 0;
-    vtmp.nregions = 0;
-    vtmp.dev = dev;
+    size_t mem_size;
+    struct vhost_update_mem_tmp vtmp = {.dev = dev};
 
     res = address_space_iterate(&address_space_memory,
                                 vhost_update_mem_cb, &vtmp);

@@ -545,24 +483,17 @@ static int vhost_update_mem(struct vhost_dev *dev)
         goto out;
     }
 
-    need_update = vhost_update_compare_list(dev, &vtmp,
-                                            &change_start, &change_end);
-    trace_vhost_update_mem_comparison(need_update,
-                                      (uint64_t)change_start,
-                                      (uint64_t)change_end);
-    if (need_update) {
-        /* Update the main regions list from our tmp */
-        size_t mem_size = offsetof(struct vhost_memory, regions) +
-            (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
+    mem_size = offsetof(struct vhost_memory, regions) +
+               (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
 
+    if(vtmp.nregions != dev->mem->nregions ||
+       memcmp(vtmp.regions, dev->mem->regions, mem_size)) {
+        /* Update the main regions list from our tmp */
         dev->mem = g_realloc(dev->mem, mem_size);
         dev->mem->nregions = vtmp.nregions;
         memcpy(dev->mem->regions, vtmp.regions,
                vtmp.nregions * sizeof dev->mem->regions[0]);
         used_memslots = vtmp.nregions;
-
-        dev->mem_changed_start_addr = change_start;
-        dev->mem_changed_end_addr = change_end;
     }
 
 out:

@@ -574,8 +505,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;
 

@@ -585,22 +514,11 @@ static void vhost_commit(MemoryListener *listener)
     if (!dev->started) {
         return;
     }
-    if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
-        return;
-    }
 
     if (vhost_update_mem(dev)) {
         return;
     }
 
-    if (dev->started) {
-        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);
-    }
-
     if (!dev->log_enabled) {
         r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
         if (r < 0) {

@@ -1235,7 +1153,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->features = features;
 
     hdev->memory_listener = (MemoryListener) {
-        .begin = vhost_begin,
         .commit = vhost_commit,
         .region_add = vhost_region_add,
         .region_del = vhost_region_del,

@@ -1458,7 +1375,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
 
-    hdev->started = true;
     hdev->vdev = vdev;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);

@@ -1469,6 +1385,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     if (vhost_update_mem(hdev)) {
         goto fail_mem;
     }
+
+    hdev->started = true;
+
     if (vhost_dev_has_iommu(hdev)) {
         memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
     }

Signature · 1 line

2.7.4