[Qemu-devel] [PATCH v5 0/7] Rework vhost memory region updates

[Qemu-devel] [PATCH v5 0/7] Rework vhost memory region updates

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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

Its intention is to simplify a lot of the update code,
and to make it easier for the postcopy+shared code to
do the hugepage alignments that are needed.

Instead of inserting/removing each section during the add/del
callbacks of the listener, we start afresh and build a list
from the add and nop callbacks, then at the end compare the list
we've built with the exisiting list.

v5
  Solve the unref race found by Igor with a new 1st patch
  Now we've got a temporary section list rework the rest of the set
   around that.

Dr. David Alan Gilbert (7):
  vhost: Build temporary section list and deref after commit
  vhost: Move log_dirty check
  vhost: Simplify ring verification checks
  vhost: Merge sections added to temporary list
  vhost: Regenerate region list from changed sections list
  vhost: Clean out old vhost_set_memory and friends
  vhost: Merge and delete unused callbacks

 hw/virtio/trace-events    |   6 +
 hw/virtio/vhost.c         | 490 ++++++++++++++++------------------------------
 include/hw/virtio/vhost.h |   5 +-
 3 files changed, 174 insertions(+), 327 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v5 1/7] vhost: Build temporary section list and deref after commit

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Igor spotted that there's a race, where a region that's unref'd
in a _del callback might be free'd before the set_mem_table call in
the _commit callback, and thus the vhost might end up using free memory.

Fix this by building a complete temporary sections list, ref'ing every
section (during add and nop) and then unref'ing the whole list right
at the end of commit.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/vhost.c         | 73 ++++++++++++++++++++++++++++++-----------------
 include/hw/virtio/vhost.h |  2 ++
 2 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce93d..610bfe6945 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener)
                                          memory_listener);
     dev->mem_changed_end_addr = 0;
     dev->mem_changed_start_addr = -1;
+    dev->tmp_sections = NULL;
+    dev->n_tmp_sections = 0;
 }
 
 static void vhost_commit(MemoryListener *listener)
@@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener)
                                          memory_listener);
     hwaddr start_addr = 0;
     ram_addr_t size = 0;
+    MemoryRegionSection *old_sections;
+    int n_old_sections;
+
     uint64_t log_size;
     int r;
 
+    old_sections = dev->mem_sections;
+    n_old_sections = dev->n_mem_sections;
+    dev->mem_sections = dev->tmp_sections;
+    dev->n_mem_sections = dev->n_tmp_sections;
+
     if (!dev->memory_changed) {
-        return;
+        goto out;
     }
     if (!dev->started) {
-        return;
+        goto out;
     }
     if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
-        return;
+        goto out;
     }
 
     if (dev->started) {
@@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener)
             VHOST_OPS_DEBUG("vhost_set_mem_table failed");
         }
         dev->memory_changed = false;
-        return;
+        goto out;
     }
     log_size = vhost_get_log_size(dev);
     /* We allocate an extra 4K bytes to log,
@@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener)
         vhost_dev_log_resize(dev, log_size);
     }
     dev->memory_changed = false;
+
+out:
+    /* Deref the old list of sections, this must happen _after_ the
+     * vhost_set_mem_table to ensure the client isn't still using the
+     * section we're about to unref.
+     */
+    while (n_old_sections--) {
+        memory_region_unref(old_sections[n_old_sections].mr);
+    }
+    g_free(old_sections);
+    return;
+}
+
+static void vhost_add_section(struct vhost_dev *dev,
+                              MemoryRegionSection *section)
+{
+    ++dev->n_tmp_sections;
+    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
+                                dev->n_tmp_sections);
+    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
+    memory_region_ref(section->mr);
 }
 
 static void vhost_region_add(MemoryListener *listener,
@@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener,
         return;
     }
 
-    ++dev->n_mem_sections;
-    dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
-                                dev->n_mem_sections);
-    dev->mem_sections[dev->n_mem_sections - 1] = *section;
-    memory_region_ref(section->mr);
+    vhost_add_section(dev, section);
     vhost_set_memory(listener, section, true);
 }
 
-static void vhost_region_del(MemoryListener *listener,
+static void vhost_region_nop(MemoryListener *listener,
                              MemoryRegionSection *section)
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,
                                          memory_listener);
-    int i;
 
     if (!vhost_section(section)) {
         return;
     }
 
-    vhost_set_memory(listener, section, false);
-    memory_region_unref(section->mr);
-    for (i = 0; i < dev->n_mem_sections; ++i) {
-        if (dev->mem_sections[i].offset_within_address_space
-            == section->offset_within_address_space) {
-            --dev->n_mem_sections;
-            memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
-                    (dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
-            break;
-        }
+    vhost_add_section(dev, section);
+}
+
+static void vhost_region_del(MemoryListener *listener,
+                             MemoryRegionSection *section)
+{
+    if (!vhost_section(section)) {
+        return;
     }
+
+    vhost_set_memory(listener, section, false);
 }
 
 static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
@@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener,
     }
 }
 
-static void vhost_region_nop(MemoryListener *listener,
-                             MemoryRegionSection *section)
-{
-}
-
 static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
                                     struct vhost_virtqueue *vq,
                                     unsigned idx, bool enable_log)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 467dc7794b..21f3022499 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -54,6 +54,8 @@ struct vhost_dev {
     struct vhost_memory *mem;
     int n_mem_sections;
     MemoryRegionSection *mem_sections;
+    int n_tmp_sections;
+    MemoryRegionSection *tmp_sections;
     struct vhost_virtqueue *vqs;
     int nvqs;
     /* the first virtqueue which would be used by this vhost dev */
-- 
2.14.3

Re: [Qemu-devel] [PATCH v5 1/7] vhost: Build temporary section list and deref after commit

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:34 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Igor spotted that there's a race, where a region that's unref'd
> in a _del callback might be free'd before the set_mem_table call in
> the _commit callback, and thus the vhost might end up using free memory.
> 
> Fix this by building a complete temporary sections list, ref'ing every
> section (during add and nop) and then unref'ing the whole list right
> at the end of commit.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

As Michael've suggested it should go into stable (CCed)

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/virtio/vhost.c         | 73 ++++++++++++++++++++++++++++++-----------------
>  include/hw/virtio/vhost.h |  2 ++
>  2 files changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e4290ce93d..610bfe6945 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener)
>                                           memory_listener);
>      dev->mem_changed_end_addr = 0;
>      dev->mem_changed_start_addr = -1;
> +    dev->tmp_sections = NULL;
> +    dev->n_tmp_sections = 0;
>  }
>  
>  static void vhost_commit(MemoryListener *listener)
> @@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener)
>                                           memory_listener);
>      hwaddr start_addr = 0;
>      ram_addr_t size = 0;
> +    MemoryRegionSection *old_sections;
> +    int n_old_sections;
> +
>      uint64_t log_size;
>      int r;
>  
> +    old_sections = dev->mem_sections;
> +    n_old_sections = dev->n_mem_sections;
> +    dev->mem_sections = dev->tmp_sections;
> +    dev->n_mem_sections = dev->n_tmp_sections;
> +
>      if (!dev->memory_changed) {
> -        return;
> +        goto out;
>      }
>      if (!dev->started) {
> -        return;
> +        goto out;
>      }
>      if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
> -        return;
> +        goto out;
>      }
>  
>      if (dev->started) {
> @@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener)
>              VHOST_OPS_DEBUG("vhost_set_mem_table failed");
>          }
>          dev->memory_changed = false;
> -        return;
> +        goto out;
>      }
>      log_size = vhost_get_log_size(dev);
>      /* We allocate an extra 4K bytes to log,
> @@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener)
>          vhost_dev_log_resize(dev, log_size);
>      }
>      dev->memory_changed = false;
> +
> +out:
> +    /* Deref the old list of sections, this must happen _after_ the
> +     * vhost_set_mem_table to ensure the client isn't still using the
> +     * section we're about to unref.
> +     */
> +    while (n_old_sections--) {
> +        memory_region_unref(old_sections[n_old_sections].mr);
> +    }
> +    g_free(old_sections);
> +    return;
> +}
> +
> +static void vhost_add_section(struct vhost_dev *dev,
> +                              MemoryRegionSection *section)
> +{
> +    ++dev->n_tmp_sections;
> +    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> +                                dev->n_tmp_sections);
> +    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> +    memory_region_ref(section->mr);
>  }
>  
>  static void vhost_region_add(MemoryListener *listener,
> @@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener,
>          return;
>      }
>  
> -    ++dev->n_mem_sections;
> -    dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
> -                                dev->n_mem_sections);
> -    dev->mem_sections[dev->n_mem_sections - 1] = *section;
> -    memory_region_ref(section->mr);
> +    vhost_add_section(dev, section);
>      vhost_set_memory(listener, section, true);
>  }
>  
> -static void vhost_region_del(MemoryListener *listener,
> +static void vhost_region_nop(MemoryListener *listener,
>                               MemoryRegionSection *section)
>  {
>      struct vhost_dev *dev = container_of(listener, struct vhost_dev,
>                                           memory_listener);
> -    int i;
>  
>      if (!vhost_section(section)) {
>          return;
>      }
>  
> -    vhost_set_memory(listener, section, false);
> -    memory_region_unref(section->mr);
> -    for (i = 0; i < dev->n_mem_sections; ++i) {
> -        if (dev->mem_sections[i].offset_within_address_space
> -            == section->offset_within_address_space) {
> -            --dev->n_mem_sections;
> -            memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
> -                    (dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
> -            break;
> -        }
> +    vhost_add_section(dev, section);
> +}
> +
> +static void vhost_region_del(MemoryListener *listener,
> +                             MemoryRegionSection *section)
> +{
> +    if (!vhost_section(section)) {
> +        return;
>      }
> +
> +    vhost_set_memory(listener, section, false);
>  }
>  
>  static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> @@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener,
>      }
>  }
>  
> -static void vhost_region_nop(MemoryListener *listener,
> -                             MemoryRegionSection *section)
> -{
> -}
> -
>  static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
>                                      struct vhost_virtqueue *vq,
>                                      unsigned idx, bool enable_log)
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 467dc7794b..21f3022499 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -54,6 +54,8 @@ struct vhost_dev {
>      struct vhost_memory *mem;
>      int n_mem_sections;
>      MemoryRegionSection *mem_sections;
> +    int n_tmp_sections;
> +    MemoryRegionSection *tmp_sections;
>      struct vhost_virtqueue *vqs;
>      int nvqs;
>      /* the first virtqueue which would be used by this vhost dev */

Re: [Qemu-devel] [PATCH v5 1/7] vhost: Build temporary section list and deref after commit

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Mon, 18 Dec 2017 20:13:34 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Igor spotted that there's a race, where a region that's unref'd
> > in a _del callback might be free'd before the set_mem_table call in
> > the _commit callback, and thus the vhost might end up using free memory.
> > 
> > Fix this by building a complete temporary sections list, ref'ing every
> > section (during add and nop) and then unref'ing the whole list right
> > at the end of commit.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> As Michael've suggested it should go into stable (CCed)

OK,

> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Thanks!

Dave

> 
> > ---
> >  hw/virtio/vhost.c         | 73 ++++++++++++++++++++++++++++++-----------------
> >  include/hw/virtio/vhost.h |  2 ++
> >  2 files changed, 49 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index e4290ce93d..610bfe6945 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener)
> >                                           memory_listener);
> >      dev->mem_changed_end_addr = 0;
> >      dev->mem_changed_start_addr = -1;
> > +    dev->tmp_sections = NULL;
> > +    dev->n_tmp_sections = 0;
> >  }
> >  
> >  static void vhost_commit(MemoryListener *listener)
> > @@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener)
> >                                           memory_listener);
> >      hwaddr start_addr = 0;
> >      ram_addr_t size = 0;
> > +    MemoryRegionSection *old_sections;
> > +    int n_old_sections;
> > +
> >      uint64_t log_size;
> >      int r;
> >  
> > +    old_sections = dev->mem_sections;
> > +    n_old_sections = dev->n_mem_sections;
> > +    dev->mem_sections = dev->tmp_sections;
> > +    dev->n_mem_sections = dev->n_tmp_sections;
> > +
> >      if (!dev->memory_changed) {
> > -        return;
> > +        goto out;
> >      }
> >      if (!dev->started) {
> > -        return;
> > +        goto out;
> >      }
> >      if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
> > -        return;
> > +        goto out;
> >      }
> >  
> >      if (dev->started) {
> > @@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener)
> >              VHOST_OPS_DEBUG("vhost_set_mem_table failed");
> >          }
> >          dev->memory_changed = false;
> > -        return;
> > +        goto out;
> >      }
> >      log_size = vhost_get_log_size(dev);
> >      /* We allocate an extra 4K bytes to log,
> > @@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener)
> >          vhost_dev_log_resize(dev, log_size);
> >      }
> >      dev->memory_changed = false;
> > +
> > +out:
> > +    /* Deref the old list of sections, this must happen _after_ the
> > +     * vhost_set_mem_table to ensure the client isn't still using the
> > +     * section we're about to unref.
> > +     */
> > +    while (n_old_sections--) {
> > +        memory_region_unref(old_sections[n_old_sections].mr);
> > +    }
> > +    g_free(old_sections);
> > +    return;
> > +}
> > +
> > +static void vhost_add_section(struct vhost_dev *dev,
> > +                              MemoryRegionSection *section)
> > +{
> > +    ++dev->n_tmp_sections;
> > +    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> > +                                dev->n_tmp_sections);
> > +    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> > +    memory_region_ref(section->mr);
> >  }
> >  
> >  static void vhost_region_add(MemoryListener *listener,
> > @@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener,
> >          return;
> >      }
> >  
> > -    ++dev->n_mem_sections;
> > -    dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
> > -                                dev->n_mem_sections);
> > -    dev->mem_sections[dev->n_mem_sections - 1] = *section;
> > -    memory_region_ref(section->mr);
> > +    vhost_add_section(dev, section);
> >      vhost_set_memory(listener, section, true);
> >  }
> >  
> > -static void vhost_region_del(MemoryListener *listener,
> > +static void vhost_region_nop(MemoryListener *listener,
> >                               MemoryRegionSection *section)
> >  {
> >      struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> >                                           memory_listener);
> > -    int i;
> >  
> >      if (!vhost_section(section)) {
> >          return;
> >      }
> >  
> > -    vhost_set_memory(listener, section, false);
> > -    memory_region_unref(section->mr);
> > -    for (i = 0; i < dev->n_mem_sections; ++i) {
> > -        if (dev->mem_sections[i].offset_within_address_space
> > -            == section->offset_within_address_space) {
> > -            --dev->n_mem_sections;
> > -            memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
> > -                    (dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
> > -            break;
> > -        }
> > +    vhost_add_section(dev, section);
> > +}
> > +
> > +static void vhost_region_del(MemoryListener *listener,
> > +                             MemoryRegionSection *section)
> > +{
> > +    if (!vhost_section(section)) {
> > +        return;
> >      }
> > +
> > +    vhost_set_memory(listener, section, false);
> >  }
> >  
> >  static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > @@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener,
> >      }
> >  }
> >  
> > -static void vhost_region_nop(MemoryListener *listener,
> > -                             MemoryRegionSection *section)
> > -{
> > -}
> > -
> >  static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
> >                                      struct vhost_virtqueue *vq,
> >                                      unsigned idx, bool enable_log)
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 467dc7794b..21f3022499 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -54,6 +54,8 @@ struct vhost_dev {
> >      struct vhost_memory *mem;
> >      int n_mem_sections;
> >      MemoryRegionSection *mem_sections;
> > +    int n_tmp_sections;
> > +    MemoryRegionSection *tmp_sections;
> >      struct vhost_virtqueue *vqs;
> >      int nvqs;
> >      /* the first virtqueue which would be used by this vhost dev */
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v5 2/7] vhost: Move log_dirty check

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Move the log_dirty check into vhost_section.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/trace-events |  3 +++
 hw/virtio/vhost.c      | 20 +++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 775461ae98..4a493bcd46 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -1,5 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/virtio/vhost.c
+vhost_section(const char *name, int r) "%s:%d"
+
 # hw/virtio/virtio.c
 virtqueue_alloc_element(void *elem, size_t sz, unsigned in_num, unsigned out_num) "elem %p size %zd in_num %u out_num %u"
 virtqueue_fill(void *vq, const void *elem, unsigned int len, unsigned int idx) "vq %p elem %p len %u idx %u"
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 610bfe6945..d4710fc05c 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/blocker.h"
 #include "sysemu/dma.h"
+#include "trace.h"
 
 /* enabled until disconnected backend stabilizes */
 #define _VHOST_DEBUG 1
@@ -567,18 +568,12 @@ static void vhost_set_memory(MemoryListener *listener,
                                          memory_listener);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    bool log_dirty =
-        memory_region_get_dirty_log_mask(section->mr) & ~(1 << DIRTY_MEMORY_MIGRATION);
     int s = offsetof(struct vhost_memory, regions) +
         (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
     void *ram;
 
     dev->mem = g_realloc(dev->mem, s);
 
-    if (log_dirty) {
-        add = false;
-    }
-
     assert(size);
 
     /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */
@@ -611,8 +606,19 @@ static void vhost_set_memory(MemoryListener *listener,
 
 static bool vhost_section(MemoryRegionSection *section)
 {
-    return memory_region_is_ram(section->mr) &&
+    bool result;
+    bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
+                     ~(1 << DIRTY_MEMORY_MIGRATION);
+    result = memory_region_is_ram(section->mr) &&
         !memory_region_is_rom(section->mr);
+
+    /* Vhost doesn't handle any block which is doing dirty-tracking other
+     * than migration; this typically fires on VGA areas.
+     */
+    result &= !log_dirty;
+
+    trace_vhost_section(section->mr->name, result);
+    return result;
 }
 
 static void vhost_begin(MemoryListener *listener)
-- 
2.14.3

[Qemu-devel] [PATCH v5 3/7] vhost: Simplify ring verification checks

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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

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

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

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
with modifications by:
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/vhost.c | 75 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 35 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index d4710fc05c..18611f0d40 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
     }
 }
 
-static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
-                                          void *part,
-                                          uint64_t part_addr,
-                                          uint64_t part_size,
-                                          uint64_t start_addr,
-                                          uint64_t size)
+static int vhost_verify_ring_part_mapping(void *ring_hva,
+                                          uint64_t ring_gpa,
+                                          uint64_t ring_size,
+                                          void *reg_hva,
+                                          uint64_t reg_gpa,
+                                          uint64_t reg_size)
 {
-    hwaddr l;
-    void *p;
-    int r = 0;
+    uint64_t hva_ring_offset;
+    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
+    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
 
-    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
+    if (ring_last < reg_gpa || ring_gpa > reg_last) {
         return 0;
     }
-    l = part_size;
-    p = vhost_memory_map(dev, part_addr, &l, 1);
-    if (!p || l != part_size) {
-        r = -ENOMEM;
+    /* check that whole ring's is mapped */
+    if (ring_last > reg_last) {
+        return -ENOMEM;
     }
-    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 -EBUSY;
     }
-    vhost_memory_unmap(dev, p, l, 0, 0);
-    return r;
+
+    return 0;
 }
 
 static int vhost_verify_ring_mappings(struct vhost_dev *dev,
-                                      uint64_t start_addr,
-                                      uint64_t size)
+                                      void *reg_hva,
+                                      uint64_t reg_gpa,
+                                      uint64_t reg_size)
 {
     int i, j;
     int r = 0;
@@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         struct vhost_virtqueue *vq = dev->vqs + i;
 
         j = 0;
-        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
-                                           vq->desc_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }
 
         j++;
-        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
-                                           vq->avail_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }
 
         j++;
-        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
-                                           vq->used_size, start_addr, size);
+        r = vhost_verify_ring_part_mapping(
+                vq->desc, vq->desc_phys, vq->desc_size,
+                reg_hva, reg_gpa, reg_size);
         if (r) {
             break;
         }
@@ -635,13 +640,11 @@ 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;
     MemoryRegionSection *old_sections;
     int n_old_sections;
-
     uint64_t log_size;
     int r;
+    int i;
 
     old_sections = dev->mem_sections;
     n_old_sections = dev->n_mem_sections;
@@ -658,12 +661,14 @@ static void vhost_commit(MemoryListener *listener)
         goto out;
     }
 
-    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);
+    for (i = 0; i < dev->mem->nregions; i++) {
+        if (vhost_verify_ring_mappings(dev,
+                           (void *)dev->mem->regions[i].userspace_addr,
+                           dev->mem->regions[i].guest_phys_addr,
+                           dev->mem->regions[i].memory_size)) {
+            error_report("Verify ring failure on region %d", i);
+            abort();
+        }
     }
 
     if (!dev->log_enabled) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH v5 3/7] vhost: Simplify ring verification checks

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:36 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> vhost_verify_ring_mappings() were used to verify that
> rings are still accessible and related memory hasn't
> been moved after flatview is updated.
> 
> It was doing checks by mapping ring's GPA+len and
> checking that HVA hadn't changed with new memory map.
> To avoid maybe expensive mapping call, we were
> identifying address range that changed and were doing
> mapping only if ring was in changed range.
> 
> However it's not neccessary to perform ring's GPA
> mapping as we already have its current HVA and all
> we need is to verify that ring's GPA translates to
> the same HVA in updated flatview.
> 
> This will allow the following patches to simplify the range
> comparison that was previously needed to avoid expensive
> verify_ring_mapping calls.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> with modifications by:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/virtio/vhost.c | 75 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 40 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index d4710fc05c..18611f0d40 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
>      }
>  }
>  
> -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> -                                          void *part,
> -                                          uint64_t part_addr,
> -                                          uint64_t part_size,
> -                                          uint64_t start_addr,
> -                                          uint64_t size)
> +static int vhost_verify_ring_part_mapping(void *ring_hva,
> +                                          uint64_t ring_gpa,
> +                                          uint64_t ring_size,
> +                                          void *reg_hva,
> +                                          uint64_t reg_gpa,
> +                                          uint64_t reg_size)
>  {
> -    hwaddr l;
> -    void *p;
> -    int r = 0;
> +    uint64_t hva_ring_offset;
> +    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
> +    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
>  
> -    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
> +    if (ring_last < reg_gpa || ring_gpa > reg_last) {
>          return 0;
>      }
> -    l = part_size;
> -    p = vhost_memory_map(dev, part_addr, &l, 1);
> -    if (!p || l != part_size) {
> -        r = -ENOMEM;
> +    /* check that whole ring's is mapped */
> +    if (ring_last > reg_last) {
> +        return -ENOMEM;
>      }
> -    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 -EBUSY;
>      }
> -    vhost_memory_unmap(dev, p, l, 0, 0);
> -    return r;
> +
> +    return 0;
>  }
>  
>  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> -                                      uint64_t start_addr,
> -                                      uint64_t size)
> +                                      void *reg_hva,
> +                                      uint64_t reg_gpa,
> +                                      uint64_t reg_size)
>  {
>      int i, j;
>      int r = 0;
> @@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
>          struct vhost_virtqueue *vq = dev->vqs + i;
>  
>          j = 0;
> -        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
> -                                           vq->desc_size, start_addr, size);
> +        r = vhost_verify_ring_part_mapping(
> +                vq->desc, vq->desc_phys, vq->desc_size,
> +                reg_hva, reg_gpa, reg_size);
>          if (r) {
>              break;
>          }
>  
>          j++;
> -        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
> -                                           vq->avail_size, start_addr, size);
> +        r = vhost_verify_ring_part_mapping(
> +                vq->desc, vq->desc_phys, vq->desc_size,
> +                reg_hva, reg_gpa, reg_size);
>          if (r) {
>              break;
>          }
>  
>          j++;
> -        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
> -                                           vq->used_size, start_addr, size);
> +        r = vhost_verify_ring_part_mapping(
> +                vq->desc, vq->desc_phys, vq->desc_size,
> +                reg_hva, reg_gpa, reg_size);
>          if (r) {
>              break;
>          }
> @@ -635,13 +640,11 @@ 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;
>      MemoryRegionSection *old_sections;
>      int n_old_sections;
> -
>      uint64_t log_size;
>      int r;
> +    int i;
>  
>      old_sections = dev->mem_sections;
>      n_old_sections = dev->n_mem_sections;
> @@ -658,12 +661,14 @@ static void vhost_commit(MemoryListener *listener)
>          goto out;
>      }
>  
> -    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);
> +    for (i = 0; i < dev->mem->nregions; i++) {
> +        if (vhost_verify_ring_mappings(dev,
> +                           (void *)dev->mem->regions[i].userspace_addr,
> +                           dev->mem->regions[i].guest_phys_addr,
> +                           dev->mem->regions[i].memory_size)) {
> +            error_report("Verify ring failure on region %d", i);
> +            abort();
> +        }
>      }
>  
>      if (!dev->log_enabled) {

Re: [Qemu-devel] [PATCH v5 3/7] vhost: Simplify ring verification checks

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:36 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> vhost_verify_ring_mappings() were used to verify that
> rings are still accessible and related memory hasn't
> been moved after flatview is updated.
> 
> It was doing checks by mapping ring's GPA+len and
> checking that HVA hadn't changed with new memory map.
> To avoid maybe expensive mapping call, we were
> identifying address range that changed and were doing
> mapping only if ring was in changed range.
> 
> However it's not neccessary to perform ring's GPA
> mapping as we already have its current HVA and all
> we need is to verify that ring's GPA translates to
> the same HVA in updated flatview.
> 
> This will allow the following patches to simplify the range
> comparison that was previously needed to avoid expensive
> verify_ring_mapping calls.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> with modifications by:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

an additional question,

in iommu case ring_hva == ring_gpa if we look in to vhost_memory_map()
have you check if iommu case is working with new code?


> ---
>  hw/virtio/vhost.c | 75 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 40 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index d4710fc05c..18611f0d40 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
>      }
>  }
>  
> -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> -                                          void *part,
> -                                          uint64_t part_addr,
> -                                          uint64_t part_size,
> -                                          uint64_t start_addr,
> -                                          uint64_t size)
> +static int vhost_verify_ring_part_mapping(void *ring_hva,
> +                                          uint64_t ring_gpa,
> +                                          uint64_t ring_size,
> +                                          void *reg_hva,
> +                                          uint64_t reg_gpa,
> +                                          uint64_t reg_size)
>  {
> -    hwaddr l;
> -    void *p;
> -    int r = 0;
> +    uint64_t hva_ring_offset;
> +    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
> +    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
>  
> -    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
> +    if (ring_last < reg_gpa || ring_gpa > reg_last) {
>          return 0;
>      }
> -    l = part_size;
> -    p = vhost_memory_map(dev, part_addr, &l, 1);
> -    if (!p || l != part_size) {
> -        r = -ENOMEM;
> +    /* check that whole ring's is mapped */
> +    if (ring_last > reg_last) {
> +        return -ENOMEM;
>      }
> -    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 -EBUSY;
>      }
> -    vhost_memory_unmap(dev, p, l, 0, 0);
> -    return r;
> +
> +    return 0;
>  }
>  
>  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> -                                      uint64_t start_addr,
> -                                      uint64_t size)
> +                                      void *reg_hva,
> +                                      uint64_t reg_gpa,
> +                                      uint64_t reg_size)
>  {
>      int i, j;
>      int r = 0;
> @@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
>          struct vhost_virtqueue *vq = dev->vqs + i;
>  
>          j = 0;
> -        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
> -                                           vq->desc_size, start_addr, size);
> +        r = vhost_verify_ring_part_mapping(
> +                vq->desc, vq->desc_phys, vq->desc_size,
> +                reg_hva, reg_gpa, reg_size);
>          if (r) {
>              break;
>          }
>  
>          j++;
> -        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
> -                                           vq->avail_size, start_addr, size);
> +        r = vhost_verify_ring_part_mapping(
> +                vq->desc, vq->desc_phys, vq->desc_size,
> +                reg_hva, reg_gpa, reg_size);
>          if (r) {
>              break;
>          }
>  
>          j++;
> -        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
> -                                           vq->used_size, start_addr, size);
> +        r = vhost_verify_ring_part_mapping(
> +                vq->desc, vq->desc_phys, vq->desc_size,
> +                reg_hva, reg_gpa, reg_size);
>          if (r) {
>              break;
>          }
> @@ -635,13 +640,11 @@ 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;
>      MemoryRegionSection *old_sections;
>      int n_old_sections;
> -
>      uint64_t log_size;
>      int r;
> +    int i;
>  
>      old_sections = dev->mem_sections;
>      n_old_sections = dev->n_mem_sections;
> @@ -658,12 +661,14 @@ static void vhost_commit(MemoryListener *listener)
>          goto out;
>      }
>  
> -    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);
> +    for (i = 0; i < dev->mem->nregions; i++) {
> +        if (vhost_verify_ring_mappings(dev,
> +                           (void *)dev->mem->regions[i].userspace_addr,
> +                           dev->mem->regions[i].guest_phys_addr,
> +                           dev->mem->regions[i].memory_size)) {
> +            error_report("Verify ring failure on region %d", i);
> +            abort();
> +        }
>      }
>  
>      if (!dev->log_enabled) {

Re: [Qemu-devel] [PATCH v5 3/7] vhost: Simplify ring verification checks

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Mon, 18 Dec 2017 20:13:36 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > vhost_verify_ring_mappings() were used to verify that
> > rings are still accessible and related memory hasn't
> > been moved after flatview is updated.
> > 
> > It was doing checks by mapping ring's GPA+len and
> > checking that HVA hadn't changed with new memory map.
> > To avoid maybe expensive mapping call, we were
> > identifying address range that changed and were doing
> > mapping only if ring was in changed range.
> > 
> > However it's not neccessary to perform ring's GPA
> > mapping as we already have its current HVA and all
> > we need is to verify that ring's GPA translates to
> > the same HVA in updated flatview.
> > 
> > This will allow the following patches to simplify the range
> > comparison that was previously needed to avoid expensive
> > verify_ring_mapping calls.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > with modifications by:
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> an additional question,
> 
> in iommu case ring_hva == ring_gpa if we look in to vhost_memory_map()
> have you check if iommu case is working with new code?

No I've not; do you have a suggested way of testing it?

Dave

> 
> > ---
> >  hw/virtio/vhost.c | 75 +++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 40 insertions(+), 35 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index d4710fc05c..18611f0d40 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
> >      }
> >  }
> >  
> > -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> > -                                          void *part,
> > -                                          uint64_t part_addr,
> > -                                          uint64_t part_size,
> > -                                          uint64_t start_addr,
> > -                                          uint64_t size)
> > +static int vhost_verify_ring_part_mapping(void *ring_hva,
> > +                                          uint64_t ring_gpa,
> > +                                          uint64_t ring_size,
> > +                                          void *reg_hva,
> > +                                          uint64_t reg_gpa,
> > +                                          uint64_t reg_size)
> >  {
> > -    hwaddr l;
> > -    void *p;
> > -    int r = 0;
> > +    uint64_t hva_ring_offset;
> > +    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
> > +    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
> >  
> > -    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
> > +    if (ring_last < reg_gpa || ring_gpa > reg_last) {
> >          return 0;
> >      }
> > -    l = part_size;
> > -    p = vhost_memory_map(dev, part_addr, &l, 1);
> > -    if (!p || l != part_size) {
> > -        r = -ENOMEM;
> > +    /* check that whole ring's is mapped */
> > +    if (ring_last > reg_last) {
> > +        return -ENOMEM;
> >      }
> > -    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 -EBUSY;
> >      }
> > -    vhost_memory_unmap(dev, p, l, 0, 0);
> > -    return r;
> > +
> > +    return 0;
> >  }
> >  
> >  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> > -                                      uint64_t start_addr,
> > -                                      uint64_t size)
> > +                                      void *reg_hva,
> > +                                      uint64_t reg_gpa,
> > +                                      uint64_t reg_size)
> >  {
> >      int i, j;
> >      int r = 0;
> > @@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> >          struct vhost_virtqueue *vq = dev->vqs + i;
> >  
> >          j = 0;
> > -        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
> > -                                           vq->desc_size, start_addr, size);
> > +        r = vhost_verify_ring_part_mapping(
> > +                vq->desc, vq->desc_phys, vq->desc_size,
> > +                reg_hva, reg_gpa, reg_size);
> >          if (r) {
> >              break;
> >          }
> >  
> >          j++;
> > -        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
> > -                                           vq->avail_size, start_addr, size);
> > +        r = vhost_verify_ring_part_mapping(
> > +                vq->desc, vq->desc_phys, vq->desc_size,
> > +                reg_hva, reg_gpa, reg_size);
> >          if (r) {
> >              break;
> >          }
> >  
> >          j++;
> > -        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
> > -                                           vq->used_size, start_addr, size);
> > +        r = vhost_verify_ring_part_mapping(
> > +                vq->desc, vq->desc_phys, vq->desc_size,
> > +                reg_hva, reg_gpa, reg_size);
> >          if (r) {
> >              break;
> >          }
> > @@ -635,13 +640,11 @@ 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;
> >      MemoryRegionSection *old_sections;
> >      int n_old_sections;
> > -
> >      uint64_t log_size;
> >      int r;
> > +    int i;
> >  
> >      old_sections = dev->mem_sections;
> >      n_old_sections = dev->n_mem_sections;
> > @@ -658,12 +661,14 @@ static void vhost_commit(MemoryListener *listener)
> >          goto out;
> >      }
> >  
> > -    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);
> > +    for (i = 0; i < dev->mem->nregions; i++) {
> > +        if (vhost_verify_ring_mappings(dev,
> > +                           (void *)dev->mem->regions[i].userspace_addr,
> > +                           dev->mem->regions[i].guest_phys_addr,
> > +                           dev->mem->regions[i].memory_size)) {
> > +            error_report("Verify ring failure on region %d", i);
> > +            abort();
> > +        }
> >      }
> >  
> >      if (!dev->log_enabled) {
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v5 3/7] vhost: Simplify ring verification checks

[Igor Mammedov]

On Mon, 8 Jan 2018 17:54:50 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * Igor Mammedov (imammedo@redhat.com) wrote:
> > On Mon, 18 Dec 2017 20:13:36 +0000
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> >   
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > vhost_verify_ring_mappings() were used to verify that
> > > rings are still accessible and related memory hasn't
> > > been moved after flatview is updated.
> > > 
> > > It was doing checks by mapping ring's GPA+len and
> > > checking that HVA hadn't changed with new memory map.
> > > To avoid maybe expensive mapping call, we were
> > > identifying address range that changed and were doing
> > > mapping only if ring was in changed range.
> > > 
> > > However it's not neccessary to perform ring's GPA
> > > mapping as we already have its current HVA and all
> > > we need is to verify that ring's GPA translates to
> > > the same HVA in updated flatview.
> > > 
> > > This will allow the following patches to simplify the range
> > > comparison that was previously needed to avoid expensive
> > > verify_ring_mapping calls.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > with modifications by:
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>  
> > 
> > an additional question,
> > 
> > in iommu case ring_hva == ring_gpa if we look in to vhost_memory_map()
> > have you check if iommu case is working with new code?  
> 
> No I've not; do you have a suggested way of testing it?
Not really,
CCing Jason and Peter who worked on IOMMU stuff there,
maybe the would suggest a test case.


> Dave
> 
> >   
> > > ---
> > >  hw/virtio/vhost.c | 75 +++++++++++++++++++++++++++++--------------------------
> > >  1 file changed, 40 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index d4710fc05c..18611f0d40 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
> > >      }
> > >  }
> > >  
> > > -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> > > -                                          void *part,
> > > -                                          uint64_t part_addr,
> > > -                                          uint64_t part_size,
> > > -                                          uint64_t start_addr,
> > > -                                          uint64_t size)
> > > +static int vhost_verify_ring_part_mapping(void *ring_hva,
> > > +                                          uint64_t ring_gpa,
> > > +                                          uint64_t ring_size,
> > > +                                          void *reg_hva,
> > > +                                          uint64_t reg_gpa,
> > > +                                          uint64_t reg_size)
> > >  {
> > > -    hwaddr l;
> > > -    void *p;
> > > -    int r = 0;
> > > +    uint64_t hva_ring_offset;
> > > +    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
> > > +    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
> > >  
> > > -    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
> > > +    if (ring_last < reg_gpa || ring_gpa > reg_last) {
> > >          return 0;
> > >      }
> > > -    l = part_size;
> > > -    p = vhost_memory_map(dev, part_addr, &l, 1);
> > > -    if (!p || l != part_size) {
> > > -        r = -ENOMEM;
> > > +    /* check that whole ring's is mapped */
> > > +    if (ring_last > reg_last) {
> > > +        return -ENOMEM;
> > >      }
> > > -    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 -EBUSY;
> > >      }
> > > -    vhost_memory_unmap(dev, p, l, 0, 0);
> > > -    return r;
> > > +
> > > +    return 0;
> > >  }
> > >  
> > >  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> > > -                                      uint64_t start_addr,
> > > -                                      uint64_t size)
> > > +                                      void *reg_hva,
> > > +                                      uint64_t reg_gpa,
> > > +                                      uint64_t reg_size)
> > >  {
> > >      int i, j;
> > >      int r = 0;
> > > @@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> > >          struct vhost_virtqueue *vq = dev->vqs + i;
> > >  
> > >          j = 0;
> > > -        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
> > > -                                           vq->desc_size, start_addr, size);
> > > +        r = vhost_verify_ring_part_mapping(
> > > +                vq->desc, vq->desc_phys, vq->desc_size,
> > > +                reg_hva, reg_gpa, reg_size);
> > >          if (r) {
> > >              break;
> > >          }
> > >  
> > >          j++;
> > > -        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
> > > -                                           vq->avail_size, start_addr, size);
> > > +        r = vhost_verify_ring_part_mapping(
> > > +                vq->desc, vq->desc_phys, vq->desc_size,
> > > +                reg_hva, reg_gpa, reg_size);
> > >          if (r) {
> > >              break;
> > >          }
> > >  
> > >          j++;
> > > -        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
> > > -                                           vq->used_size, start_addr, size);
> > > +        r = vhost_verify_ring_part_mapping(
> > > +                vq->desc, vq->desc_phys, vq->desc_size,
> > > +                reg_hva, reg_gpa, reg_size);
> > >          if (r) {
> > >              break;
> > >          }
> > > @@ -635,13 +640,11 @@ 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;
> > >      MemoryRegionSection *old_sections;
> > >      int n_old_sections;
> > > -
> > >      uint64_t log_size;
> > >      int r;
> > > +    int i;
> > >  
> > >      old_sections = dev->mem_sections;
> > >      n_old_sections = dev->n_mem_sections;
> > > @@ -658,12 +661,14 @@ static void vhost_commit(MemoryListener *listener)
> > >          goto out;
> > >      }
> > >  
> > > -    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);
> > > +    for (i = 0; i < dev->mem->nregions; i++) {
> > > +        if (vhost_verify_ring_mappings(dev,
> > > +                           (void *)dev->mem->regions[i].userspace_addr,
> > > +                           dev->mem->regions[i].guest_phys_addr,
> > > +                           dev->mem->regions[i].memory_size)) {
> > > +            error_report("Verify ring failure on region %d", i);
> > > +            abort();
> > > +        }
> > >      }
> > >  
> > >      if (!dev->log_enabled) {  
> >   
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v5 3/7] vhost: Simplify ring verification checks

[Jason Wang]

On 2018年01月10日 18:23, Igor Mammedov wrote:
> On Mon, 8 Jan 2018 17:54:50 +0000
> "Dr. David Alan Gilbert"<dgilbert@redhat.com>  wrote:
>
>> * Igor Mammedov (imammedo@redhat.com) wrote:
>>> On Mon, 18 Dec 2017 20:13:36 +0000
>>> "Dr. David Alan Gilbert (git)"<dgilbert@redhat.com>  wrote:
>>>    
>>>> From: "Dr. David Alan Gilbert"<dgilbert@redhat.com>
>>>>
>>>> vhost_verify_ring_mappings() were used to verify that
>>>> rings are still accessible and related memory hasn't
>>>> been moved after flatview is updated.
>>>>
>>>> It was doing checks by mapping ring's GPA+len and
>>>> checking that HVA hadn't changed with new memory map.
>>>> To avoid maybe expensive mapping call, we were
>>>> identifying address range that changed and were doing
>>>> mapping only if ring was in changed range.
>>>>
>>>> However it's not neccessary to perform ring's GPA
>>>> mapping as we already have its current HVA and all
>>>> we need is to verify that ring's GPA translates to
>>>> the same HVA in updated flatview.
>>>>
>>>> This will allow the following patches to simplify the range
>>>> comparison that was previously needed to avoid expensive
>>>> verify_ring_mapping calls.
>>>>
>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>> with modifications by:
>>>> Signed-off-by: Dr. David Alan Gilbert<dgilbert@redhat.com>   
>>> an additional question,
>>>
>>> in iommu case ring_hva == ring_gpa if we look in to vhost_memory_map()
>>> have you check if iommu case is working with new code?
>> No I've not; do you have a suggested way of testing it?
> Not really,
> CCing Jason and Peter who worked on IOMMU stuff there,
> maybe the would suggest a test case.
>
>

Haven't read the thread but for IOMMU test you need:

1) enable iommu_platform for virtio device
2) enable ats for virtio device
3) boot with intel_iomu device
4) boot a guest with intel_iommu=strict (which makes the flush happens 
immediately)
5) do some network loads e.g netperf TCP_STREAM test

Thanks

Re: [Qemu-devel] [PATCH v5 3/7] vhost: Simplify ring verification checks

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Mon, 18 Dec 2017 20:13:36 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > vhost_verify_ring_mappings() were used to verify that
> > rings are still accessible and related memory hasn't
> > been moved after flatview is updated.
> > 
> > It was doing checks by mapping ring's GPA+len and
> > checking that HVA hadn't changed with new memory map.
> > To avoid maybe expensive mapping call, we were
> > identifying address range that changed and were doing
> > mapping only if ring was in changed range.
> > 
> > However it's not neccessary to perform ring's GPA
> > mapping as we already have its current HVA and all
> > we need is to verify that ring's GPA translates to
> > the same HVA in updated flatview.
> > 
> > This will allow the following patches to simplify the range
> > comparison that was previously needed to avoid expensive
> > verify_ring_mapping calls.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > with modifications by:
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> an additional question,
> 
> in iommu case ring_hva == ring_gpa if we look in to vhost_memory_map()
> have you check if iommu case is working with new code?

It seems to be; I've tested a simple dpdk test that Maxime gave me.

Dave

> 
> > ---
> >  hw/virtio/vhost.c | 75 +++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 40 insertions(+), 35 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index d4710fc05c..18611f0d40 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -450,35 +450,37 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
> >      }
> >  }
> >  
> > -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> > -                                          void *part,
> > -                                          uint64_t part_addr,
> > -                                          uint64_t part_size,
> > -                                          uint64_t start_addr,
> > -                                          uint64_t size)
> > +static int vhost_verify_ring_part_mapping(void *ring_hva,
> > +                                          uint64_t ring_gpa,
> > +                                          uint64_t ring_size,
> > +                                          void *reg_hva,
> > +                                          uint64_t reg_gpa,
> > +                                          uint64_t reg_size)
> >  {
> > -    hwaddr l;
> > -    void *p;
> > -    int r = 0;
> > +    uint64_t hva_ring_offset;
> > +    uint64_t ring_last = range_get_last(ring_gpa, ring_size);
> > +    uint64_t reg_last = range_get_last(reg_gpa, reg_size);
> >  
> > -    if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
> > +    if (ring_last < reg_gpa || ring_gpa > reg_last) {
> >          return 0;
> >      }
> > -    l = part_size;
> > -    p = vhost_memory_map(dev, part_addr, &l, 1);
> > -    if (!p || l != part_size) {
> > -        r = -ENOMEM;
> > +    /* check that whole ring's is mapped */
> > +    if (ring_last > reg_last) {
> > +        return -ENOMEM;
> >      }
> > -    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 -EBUSY;
> >      }
> > -    vhost_memory_unmap(dev, p, l, 0, 0);
> > -    return r;
> > +
> > +    return 0;
> >  }
> >  
> >  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> > -                                      uint64_t start_addr,
> > -                                      uint64_t size)
> > +                                      void *reg_hva,
> > +                                      uint64_t reg_gpa,
> > +                                      uint64_t reg_size)
> >  {
> >      int i, j;
> >      int r = 0;
> > @@ -492,22 +494,25 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> >          struct vhost_virtqueue *vq = dev->vqs + i;
> >  
> >          j = 0;
> > -        r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
> > -                                           vq->desc_size, start_addr, size);
> > +        r = vhost_verify_ring_part_mapping(
> > +                vq->desc, vq->desc_phys, vq->desc_size,
> > +                reg_hva, reg_gpa, reg_size);
> >          if (r) {
> >              break;
> >          }
> >  
> >          j++;
> > -        r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
> > -                                           vq->avail_size, start_addr, size);
> > +        r = vhost_verify_ring_part_mapping(
> > +                vq->desc, vq->desc_phys, vq->desc_size,
> > +                reg_hva, reg_gpa, reg_size);
> >          if (r) {
> >              break;
> >          }
> >  
> >          j++;
> > -        r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
> > -                                           vq->used_size, start_addr, size);
> > +        r = vhost_verify_ring_part_mapping(
> > +                vq->desc, vq->desc_phys, vq->desc_size,
> > +                reg_hva, reg_gpa, reg_size);
> >          if (r) {
> >              break;
> >          }
> > @@ -635,13 +640,11 @@ 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;
> >      MemoryRegionSection *old_sections;
> >      int n_old_sections;
> > -
> >      uint64_t log_size;
> >      int r;
> > +    int i;
> >  
> >      old_sections = dev->mem_sections;
> >      n_old_sections = dev->n_mem_sections;
> > @@ -658,12 +661,14 @@ static void vhost_commit(MemoryListener *listener)
> >          goto out;
> >      }
> >  
> > -    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);
> > +    for (i = 0; i < dev->mem->nregions; i++) {
> > +        if (vhost_verify_ring_mappings(dev,
> > +                           (void *)dev->mem->regions[i].userspace_addr,
> > +                           dev->mem->regions[i].guest_phys_addr,
> > +                           dev->mem->regions[i].memory_size)) {
> > +            error_report("Verify ring failure on region %d", i);
> > +            abort();
> > +        }
> >      }
> >  
> >      if (!dev->log_enabled) {
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v5 4/7] vhost: Merge sections added to temporary list

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

As sections are reported by the listener to the _nop and _add
methods, add them to the temporary section list but now merge them
with the previous section if the new one abuts and the backend allows.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/trace-events |  2 ++
 hw/virtio/vhost.c      | 70 +++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 4a493bcd46..0e63c8739d 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -1,6 +1,8 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # hw/virtio/vhost.c
+vhost_region_add_section(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
+vhost_region_add_section_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 vhost_section(const char *name, int r) "%s:%d"
 
 # hw/virtio/virtio.c
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 18611f0d40..57d15acd2b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -709,14 +709,65 @@ out:
     return;
 }
 
-static void vhost_add_section(struct vhost_dev *dev,
-                              MemoryRegionSection *section)
+/* Adds the section data to the tmp_section structure.
+ * It relies on the listener calling us in memory address order
+ * and for each region (via the _add and _nop methods) to
+ * join neighbours.
+ */
+static void vhost_region_add_section(struct vhost_dev *dev,
+                                 MemoryRegionSection *section)
 {
-    ++dev->n_tmp_sections;
-    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
-                                dev->n_tmp_sections);
-    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
-    memory_region_ref(section->mr);
+    bool need_add = true;
+    uint64_t mrs_size = int128_get64(section->size);
+    uint64_t mrs_gpa = section->offset_within_address_space;
+    uintptr_t mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
+                         section->offset_within_region;
+
+    trace_vhost_region_add_section(section->mr->name, mrs_gpa, mrs_size,
+                                   mrs_host);
+
+    if (dev->n_tmp_sections) {
+        /* Since we already have at least one section, 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.
+         */
+        MemoryRegionSection *prev_sec = dev->tmp_sections +
+                                               (dev->n_tmp_sections - 1);
+        uint64_t prev_gpa_start = prev_sec->offset_within_address_space;
+        uint64_t prev_size = int128_get64(prev_sec->size);
+        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start, prev_size);
+        uint64_t prev_host_start =
+                        (uintptr_t)memory_region_get_ram_ptr(prev_sec->mr) +
+                        prev_sec->offset_within_region;
+        uint64_t prev_host_end   = range_get_last(prev_host_start, prev_size);
+
+        if (prev_gpa_end + 1 == mrs_gpa &&
+            prev_host_end + 1 == mrs_host &&
+            section->mr == prev_sec->mr &&
+            (!dev->vhost_ops->vhost_backend_can_merge ||
+                dev->vhost_ops->vhost_backend_can_merge(dev,
+                    mrs_host, mrs_size,
+                    prev_host_start, prev_size))) {
+            /* The two sections abut */
+            need_add = false;
+            prev_sec->size = int128_add(prev_sec->size, section->size);
+            trace_vhost_region_add_section_abut(section->mr->name,
+                                                mrs_size + prev_size);
+        }
+    }
+
+    if (need_add) {
+        ++dev->n_tmp_sections;
+        dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
+                                    dev->n_tmp_sections);
+        dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
+        /* The flatview isn't stable and we don't use it, making it NULL
+         * means we can memcmp the list.
+         */
+        dev->tmp_sections[dev->n_tmp_sections - 1].fv = NULL;
+        memory_region_ref(section->mr);
+    }
 }
 
 static void vhost_region_add(MemoryListener *listener,
@@ -728,11 +779,12 @@ static void vhost_region_add(MemoryListener *listener,
     if (!vhost_section(section)) {
         return;
     }
+    vhost_region_add_section(dev, section);
 
-    vhost_add_section(dev, section);
     vhost_set_memory(listener, section, true);
 }
 
+/* Called on regions that have not changed */
 static void vhost_region_nop(MemoryListener *listener,
                              MemoryRegionSection *section)
 {
@@ -743,7 +795,7 @@ static void vhost_region_nop(MemoryListener *listener,
         return;
     }
 
-    vhost_add_section(dev, section);
+    vhost_region_add_section(dev, section);
 }
 
 static void vhost_region_del(MemoryListener *listener,
-- 
2.14.3

Re: [Qemu-devel] [PATCH v5 4/7] vhost: Merge sections added to temporary list

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:37 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> As sections are reported by the listener to the _nop and _add
> methods, add them to the temporary section list but now merge them
> with the previous section if the new one abuts and the backend allows.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
beside small nit patch looks good to me, so with it fixed

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/virtio/trace-events |  2 ++
>  hw/virtio/vhost.c      | 70 +++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 4a493bcd46..0e63c8739d 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -1,6 +1,8 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>  
>  # hw/virtio/vhost.c
> +vhost_region_add_section(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
> +vhost_region_add_section_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
>  vhost_section(const char *name, int r) "%s:%d"
>  
>  # hw/virtio/virtio.c
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 18611f0d40..57d15acd2b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -709,14 +709,65 @@ out:
>      return;
>  }
>  
> -static void vhost_add_section(struct vhost_dev *dev,
> -                              MemoryRegionSection *section)
> +/* Adds the section data to the tmp_section structure.
> + * It relies on the listener calling us in memory address order
> + * and for each region (via the _add and _nop methods) to
> + * join neighbours.
> + */
> +static void vhost_region_add_section(struct vhost_dev *dev,
> +                                 MemoryRegionSection *section)
wrong alignment, should be on '('

>  {
> -    ++dev->n_tmp_sections;
> -    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> -                                dev->n_tmp_sections);
> -    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> -    memory_region_ref(section->mr);
> +    bool need_add = true;
> +    uint64_t mrs_size = int128_get64(section->size);
> +    uint64_t mrs_gpa = section->offset_within_address_space;
> +    uintptr_t mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
> +                         section->offset_within_region;
> +
> +    trace_vhost_region_add_section(section->mr->name, mrs_gpa, mrs_size,
> +                                   mrs_host);
> +
> +    if (dev->n_tmp_sections) {
> +        /* Since we already have at least one section, 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.
> +         */
> +        MemoryRegionSection *prev_sec = dev->tmp_sections +
> +                                               (dev->n_tmp_sections - 1);
> +        uint64_t prev_gpa_start = prev_sec->offset_within_address_space;
> +        uint64_t prev_size = int128_get64(prev_sec->size);
> +        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start, prev_size);
> +        uint64_t prev_host_start =
> +                        (uintptr_t)memory_region_get_ram_ptr(prev_sec->mr) +
> +                        prev_sec->offset_within_region;
> +        uint64_t prev_host_end   = range_get_last(prev_host_start, prev_size);
> +
> +        if (prev_gpa_end + 1 == mrs_gpa &&
> +            prev_host_end + 1 == mrs_host &&
> +            section->mr == prev_sec->mr &&
> +            (!dev->vhost_ops->vhost_backend_can_merge ||
> +                dev->vhost_ops->vhost_backend_can_merge(dev,
> +                    mrs_host, mrs_size,
> +                    prev_host_start, prev_size))) {
> +            /* The two sections abut */
> +            need_add = false;
> +            prev_sec->size = int128_add(prev_sec->size, section->size);
> +            trace_vhost_region_add_section_abut(section->mr->name,
> +                                                mrs_size + prev_size);
> +        }
> +    }
> +
> +    if (need_add) {
> +        ++dev->n_tmp_sections;
> +        dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> +                                    dev->n_tmp_sections);
> +        dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> +        /* The flatview isn't stable and we don't use it, making it NULL
> +         * means we can memcmp the list.
> +         */
> +        dev->tmp_sections[dev->n_tmp_sections - 1].fv = NULL;
> +        memory_region_ref(section->mr);
> +    }
>  }
>  
>  static void vhost_region_add(MemoryListener *listener,
> @@ -728,11 +779,12 @@ static void vhost_region_add(MemoryListener *listener,
>      if (!vhost_section(section)) {
>          return;
>      }
> +    vhost_region_add_section(dev, section);
>  
> -    vhost_add_section(dev, section);
>      vhost_set_memory(listener, section, true);
>  }
>  
> +/* Called on regions that have not changed */
>  static void vhost_region_nop(MemoryListener *listener,
>                               MemoryRegionSection *section)
>  {
> @@ -743,7 +795,7 @@ static void vhost_region_nop(MemoryListener *listener,
>          return;
>      }
>  
> -    vhost_add_section(dev, section);
> +    vhost_region_add_section(dev, section);
>  }
>  
>  static void vhost_region_del(MemoryListener *listener,

Re: [Qemu-devel] [PATCH v5 4/7] vhost: Merge sections added to temporary list

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Mon, 18 Dec 2017 20:13:37 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > As sections are reported by the listener to the _nop and _add
> > methods, add them to the temporary section list but now merge them
> > with the previous section if the new one abuts and the backend allows.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> beside small nit patch looks good to me, so with it fixed
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> > ---
> >  hw/virtio/trace-events |  2 ++
> >  hw/virtio/vhost.c      | 70 +++++++++++++++++++++++++++++++++++++++++++-------
> >  2 files changed, 63 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > index 4a493bcd46..0e63c8739d 100644
> > --- a/hw/virtio/trace-events
> > +++ b/hw/virtio/trace-events
> > @@ -1,6 +1,8 @@
> >  # See docs/devel/tracing.txt for syntax documentation.
> >  
> >  # hw/virtio/vhost.c
> > +vhost_region_add_section(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
> > +vhost_region_add_section_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
> >  vhost_section(const char *name, int r) "%s:%d"
> >  
> >  # hw/virtio/virtio.c
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 18611f0d40..57d15acd2b 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -709,14 +709,65 @@ out:
> >      return;
> >  }
> >  
> > -static void vhost_add_section(struct vhost_dev *dev,
> > -                              MemoryRegionSection *section)
> > +/* Adds the section data to the tmp_section structure.
> > + * It relies on the listener calling us in memory address order
> > + * and for each region (via the _add and _nop methods) to
> > + * join neighbours.
> > + */
> > +static void vhost_region_add_section(struct vhost_dev *dev,
> > +                                 MemoryRegionSection *section)
> wrong alignment, should be on '('

Fixed, thanks.

Dave

> >  {
> > -    ++dev->n_tmp_sections;
> > -    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> > -                                dev->n_tmp_sections);
> > -    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> > -    memory_region_ref(section->mr);
> > +    bool need_add = true;
> > +    uint64_t mrs_size = int128_get64(section->size);
> > +    uint64_t mrs_gpa = section->offset_within_address_space;
> > +    uintptr_t mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
> > +                         section->offset_within_region;
> > +
> > +    trace_vhost_region_add_section(section->mr->name, mrs_gpa, mrs_size,
> > +                                   mrs_host);
> > +
> > +    if (dev->n_tmp_sections) {
> > +        /* Since we already have at least one section, 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.
> > +         */
> > +        MemoryRegionSection *prev_sec = dev->tmp_sections +
> > +                                               (dev->n_tmp_sections - 1);
> > +        uint64_t prev_gpa_start = prev_sec->offset_within_address_space;
> > +        uint64_t prev_size = int128_get64(prev_sec->size);
> > +        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start, prev_size);
> > +        uint64_t prev_host_start =
> > +                        (uintptr_t)memory_region_get_ram_ptr(prev_sec->mr) +
> > +                        prev_sec->offset_within_region;
> > +        uint64_t prev_host_end   = range_get_last(prev_host_start, prev_size);
> > +
> > +        if (prev_gpa_end + 1 == mrs_gpa &&
> > +            prev_host_end + 1 == mrs_host &&
> > +            section->mr == prev_sec->mr &&
> > +            (!dev->vhost_ops->vhost_backend_can_merge ||
> > +                dev->vhost_ops->vhost_backend_can_merge(dev,
> > +                    mrs_host, mrs_size,
> > +                    prev_host_start, prev_size))) {
> > +            /* The two sections abut */
> > +            need_add = false;
> > +            prev_sec->size = int128_add(prev_sec->size, section->size);
> > +            trace_vhost_region_add_section_abut(section->mr->name,
> > +                                                mrs_size + prev_size);
> > +        }
> > +    }
> > +
> > +    if (need_add) {
> > +        ++dev->n_tmp_sections;
> > +        dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> > +                                    dev->n_tmp_sections);
> > +        dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> > +        /* The flatview isn't stable and we don't use it, making it NULL
> > +         * means we can memcmp the list.
> > +         */
> > +        dev->tmp_sections[dev->n_tmp_sections - 1].fv = NULL;
> > +        memory_region_ref(section->mr);
> > +    }
> >  }
> >  
> >  static void vhost_region_add(MemoryListener *listener,
> > @@ -728,11 +779,12 @@ static void vhost_region_add(MemoryListener *listener,
> >      if (!vhost_section(section)) {
> >          return;
> >      }
> > +    vhost_region_add_section(dev, section);
> >  
> > -    vhost_add_section(dev, section);
> >      vhost_set_memory(listener, section, true);
> >  }
> >  
> > +/* Called on regions that have not changed */
> >  static void vhost_region_nop(MemoryListener *listener,
> >                               MemoryRegionSection *section)
> >  {
> > @@ -743,7 +795,7 @@ static void vhost_region_nop(MemoryListener *listener,
> >          return;
> >      }
> >  
> > -    vhost_add_section(dev, section);
> > +    vhost_region_add_section(dev, section);
> >  }
> >  
> >  static void vhost_region_del(MemoryListener *listener,
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v5 4/7] vhost: Merge sections added to temporary list

[Michael S. Tsirkin]

On Mon, Jan 08, 2018 at 06:07:30PM +0000, Dr. David Alan Gilbert wrote:
> * Igor Mammedov (imammedo@redhat.com) wrote:
> > On Mon, 18 Dec 2017 20:13:37 +0000
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > 
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > As sections are reported by the listener to the _nop and _add
> > > methods, add them to the temporary section list but now merge them
> > > with the previous section if the new one abuts and the backend allows.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > beside small nit patch looks good to me, so with it fixed
> > 
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > > ---
> > >  hw/virtio/trace-events |  2 ++
> > >  hw/virtio/vhost.c      | 70 +++++++++++++++++++++++++++++++++++++++++++-------
> > >  2 files changed, 63 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> > > index 4a493bcd46..0e63c8739d 100644
> > > --- a/hw/virtio/trace-events
> > > +++ b/hw/virtio/trace-events
> > > @@ -1,6 +1,8 @@
> > >  # See docs/devel/tracing.txt for syntax documentation.
> > >  
> > >  # hw/virtio/vhost.c
> > > +vhost_region_add_section(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
> > > +vhost_region_add_section_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
> > >  vhost_section(const char *name, int r) "%s:%d"
> > >  
> > >  # hw/virtio/virtio.c
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 18611f0d40..57d15acd2b 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -709,14 +709,65 @@ out:
> > >      return;
> > >  }
> > >  
> > > -static void vhost_add_section(struct vhost_dev *dev,
> > > -                              MemoryRegionSection *section)
> > > +/* Adds the section data to the tmp_section structure.
> > > + * It relies on the listener calling us in memory address order
> > > + * and for each region (via the _add and _nop methods) to
> > > + * join neighbours.
> > > + */
> > > +static void vhost_region_add_section(struct vhost_dev *dev,
> > > +                                 MemoryRegionSection *section)
> > wrong alignment, should be on '('
> 
> Fixed, thanks.
> 
> Dave

This is too big by now. I'm applying as is and you can fix with
a patch on top.

> > >  {
> > > -    ++dev->n_tmp_sections;
> > > -    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> > > -                                dev->n_tmp_sections);
> > > -    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> > > -    memory_region_ref(section->mr);
> > > +    bool need_add = true;
> > > +    uint64_t mrs_size = int128_get64(section->size);
> > > +    uint64_t mrs_gpa = section->offset_within_address_space;
> > > +    uintptr_t mrs_host = (uintptr_t)memory_region_get_ram_ptr(section->mr) +
> > > +                         section->offset_within_region;
> > > +
> > > +    trace_vhost_region_add_section(section->mr->name, mrs_gpa, mrs_size,
> > > +                                   mrs_host);
> > > +
> > > +    if (dev->n_tmp_sections) {
> > > +        /* Since we already have at least one section, 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.
> > > +         */
> > > +        MemoryRegionSection *prev_sec = dev->tmp_sections +
> > > +                                               (dev->n_tmp_sections - 1);
> > > +        uint64_t prev_gpa_start = prev_sec->offset_within_address_space;
> > > +        uint64_t prev_size = int128_get64(prev_sec->size);
> > > +        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start, prev_size);
> > > +        uint64_t prev_host_start =
> > > +                        (uintptr_t)memory_region_get_ram_ptr(prev_sec->mr) +
> > > +                        prev_sec->offset_within_region;
> > > +        uint64_t prev_host_end   = range_get_last(prev_host_start, prev_size);
> > > +
> > > +        if (prev_gpa_end + 1 == mrs_gpa &&
> > > +            prev_host_end + 1 == mrs_host &&
> > > +            section->mr == prev_sec->mr &&
> > > +            (!dev->vhost_ops->vhost_backend_can_merge ||
> > > +                dev->vhost_ops->vhost_backend_can_merge(dev,
> > > +                    mrs_host, mrs_size,
> > > +                    prev_host_start, prev_size))) {
> > > +            /* The two sections abut */
> > > +            need_add = false;
> > > +            prev_sec->size = int128_add(prev_sec->size, section->size);
> > > +            trace_vhost_region_add_section_abut(section->mr->name,
> > > +                                                mrs_size + prev_size);
> > > +        }
> > > +    }
> > > +
> > > +    if (need_add) {
> > > +        ++dev->n_tmp_sections;
> > > +        dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> > > +                                    dev->n_tmp_sections);
> > > +        dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> > > +        /* The flatview isn't stable and we don't use it, making it NULL
> > > +         * means we can memcmp the list.
> > > +         */
> > > +        dev->tmp_sections[dev->n_tmp_sections - 1].fv = NULL;
> > > +        memory_region_ref(section->mr);
> > > +    }
> > >  }
> > >  
> > >  static void vhost_region_add(MemoryListener *listener,
> > > @@ -728,11 +779,12 @@ static void vhost_region_add(MemoryListener *listener,
> > >      if (!vhost_section(section)) {
> > >          return;
> > >      }
> > > +    vhost_region_add_section(dev, section);
> > >  
> > > -    vhost_add_section(dev, section);
> > >      vhost_set_memory(listener, section, true);
> > >  }
> > >  
> > > +/* Called on regions that have not changed */
> > >  static void vhost_region_nop(MemoryListener *listener,
> > >                               MemoryRegionSection *section)
> > >  {
> > > @@ -743,7 +795,7 @@ static void vhost_region_nop(MemoryListener *listener,
> > >          return;
> > >      }
> > >  
> > > -    vhost_add_section(dev, section);
> > > +    vhost_region_add_section(dev, section);
> > >  }
> > >  
> > >  static void vhost_region_del(MemoryListener *listener,
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v5 5/7] vhost: Regenerate region list from changed sections list

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Compare the sections list that's just been generated, and if it's
different from the old one regenerate the region list.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/trace-events |  1 +
 hw/virtio/vhost.c      | 34 ++++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 0e63c8739d..742ff0f90b 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -1,6 +1,7 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
 # hw/virtio/vhost.c
+vhost_commit(bool started, bool changed) "Started: %d Changed: %d"
 vhost_region_add_section(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
 vhost_region_add_section_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 vhost_section(const char *name, int r) "%s:%d"
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 57d15acd2b..4394ac0275 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -643,21 +643,47 @@ static void vhost_commit(MemoryListener *listener)
     MemoryRegionSection *old_sections;
     int n_old_sections;
     uint64_t log_size;
+    size_t regions_size;
     int r;
     int i;
+    bool changed = false;
 
     old_sections = dev->mem_sections;
     n_old_sections = dev->n_mem_sections;
     dev->mem_sections = dev->tmp_sections;
     dev->n_mem_sections = dev->n_tmp_sections;
 
-    if (!dev->memory_changed) {
-        goto out;
+    if (dev->n_mem_sections != n_old_sections) {
+        changed = true;
+    } else {
+        /* Same size, lets check the contents */
+        changed = memcmp(dev->mem_sections, old_sections,
+                         n_old_sections * sizeof(old_sections[0])) != 0;
     }
-    if (!dev->started) {
+
+    trace_vhost_commit(dev->started, changed);
+    if (!changed) {
         goto out;
     }
-    if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
+
+    /* Rebuild the regions list from the new sections list */
+    regions_size = offsetof(struct vhost_memory, regions) +
+                       dev->n_mem_sections * sizeof dev->mem->regions[0];
+    dev->mem = g_realloc(dev->mem, regions_size);
+    dev->mem->nregions = dev->n_mem_sections;
+    for (i = 0; i < dev->n_mem_sections; i++) {
+        struct vhost_memory_region *cur_vmr = dev->mem->regions + i;
+        struct MemoryRegionSection *mrs = dev->mem_sections + i;
+
+        cur_vmr->guest_phys_addr = mrs->offset_within_address_space;
+        cur_vmr->memory_size     = int128_get64(mrs->size);
+        cur_vmr->userspace_addr  =
+            (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
+            mrs->offset_within_region;
+        cur_vmr->flags_padding   = 0;
+    }
+
+    if (!dev->started) {
         goto out;
     }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v5 5/7] vhost: Regenerate region list from changed sections list

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:38 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Compare the sections list that's just been generated, and if it's
> different from the old one regenerate the region list.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/virtio/trace-events |  1 +
>  hw/virtio/vhost.c      | 34 ++++++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 0e63c8739d..742ff0f90b 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -1,6 +1,7 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>  
>  # hw/virtio/vhost.c
> +vhost_commit(bool started, bool changed) "Started: %d Changed: %d"
>  vhost_region_add_section(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
>  vhost_region_add_section_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
>  vhost_section(const char *name, int r) "%s:%d"
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 57d15acd2b..4394ac0275 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -643,21 +643,47 @@ static void vhost_commit(MemoryListener *listener)
>      MemoryRegionSection *old_sections;
>      int n_old_sections;
>      uint64_t log_size;
> +    size_t regions_size;
>      int r;
>      int i;
> +    bool changed = false;
>  
>      old_sections = dev->mem_sections;
>      n_old_sections = dev->n_mem_sections;
>      dev->mem_sections = dev->tmp_sections;
>      dev->n_mem_sections = dev->n_tmp_sections;
>  
> -    if (!dev->memory_changed) {
> -        goto out;
> +    if (dev->n_mem_sections != n_old_sections) {
> +        changed = true;
> +    } else {
> +        /* Same size, lets check the contents */
> +        changed = memcmp(dev->mem_sections, old_sections,
> +                         n_old_sections * sizeof(old_sections[0])) != 0;
>      }
> -    if (!dev->started) {
> +
> +    trace_vhost_commit(dev->started, changed);
> +    if (!changed) {
>          goto out;
>      }
> -    if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
> +
> +    /* Rebuild the regions list from the new sections list */
> +    regions_size = offsetof(struct vhost_memory, regions) +
> +                       dev->n_mem_sections * sizeof dev->mem->regions[0];
> +    dev->mem = g_realloc(dev->mem, regions_size);
> +    dev->mem->nregions = dev->n_mem_sections;
> +    for (i = 0; i < dev->n_mem_sections; i++) {
> +        struct vhost_memory_region *cur_vmr = dev->mem->regions + i;
> +        struct MemoryRegionSection *mrs = dev->mem_sections + i;
> +
> +        cur_vmr->guest_phys_addr = mrs->offset_within_address_space;
> +        cur_vmr->memory_size     = int128_get64(mrs->size);
> +        cur_vmr->userspace_addr  =
> +            (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
> +            mrs->offset_within_region;
> +        cur_vmr->flags_padding   = 0;
> +    }
is there a reason to keep dev->mem around after vhost_commit executed?
I'd suggest to use temporary local variable and free it on exit from function.

> +    if (!dev->started) {
also why memmap is generated this early and not right before first use?
here we might generate memap but not actually use it.

>          goto out;
>      }
>  

Re: [Qemu-devel] [PATCH v5 5/7] vhost: Regenerate region list from changed sections list

[Igor Mammedov]

On Wed, 27 Dec 2017 14:19:03 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 18 Dec 2017 20:13:38 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Compare the sections list that's just been generated, and if it's
> > different from the old one regenerate the region list.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
[...]
> 
> > +    if (!dev->started) {
> also why memmap is generated this early and not right before first use?
> here we might generate memap but not actually use it.
I retract question, as commit + follow up set_mem_table could happen before
device is started. Having a comment here explaining it would be nice as
the call flow is obvious.


> >          goto out;
> >      }
> >  
> 
> 

Re: [Qemu-devel] [PATCH v5 5/7] vhost: Regenerate region list from changed sections list

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Wed, 27 Dec 2017 14:19:03 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Mon, 18 Dec 2017 20:13:38 +0000
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > 
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > Compare the sections list that's just been generated, and if it's
> > > different from the old one regenerate the region list.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > ---
> [...]
> > 
> > > +    if (!dev->started) {
> > also why memmap is generated this early and not right before first use?
> > here we might generate memap but not actually use it.
> I retract question, as commit + follow up set_mem_table could happen before
> device is started. Having a comment here explaining it would be nice as
> the call flow is obvious.

Yes, and I think I've seen that.  I'll add a comment.

Dave

> 
> > >          goto out;
> > >      }
> > >  
> > 
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v5 6/7] vhost: Clean out old vhost_set_memory and friends

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Remove the old update mechanism, vhost_set_memory, and the functions
and flags it used.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/vhost.c         | 245 ----------------------------------------------
 include/hw/virtio/vhost.h |   3 -
 2 files changed, 248 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 4394ac0275..358ceb3033 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -156,160 +156,6 @@ static void vhost_log_sync_range(struct vhost_dev *dev,
     }
 }
 
-/* Assign/unassign. Keep an unsorted array of non-overlapping
- * memory regions in dev->mem. */
-static void vhost_dev_unassign_memory(struct vhost_dev *dev,
-                                      uint64_t start_addr,
-                                      uint64_t size)
-{
-    int from, to, n = dev->mem->nregions;
-    /* Track overlapping/split regions for sanity checking. */
-    int overlap_start = 0, overlap_end = 0, overlap_middle = 0, split = 0;
-
-    for (from = 0, to = 0; from < n; ++from, ++to) {
-        struct vhost_memory_region *reg = dev->mem->regions + to;
-        uint64_t reglast;
-        uint64_t memlast;
-        uint64_t change;
-
-        /* clone old region */
-        if (to != from) {
-            memcpy(reg, dev->mem->regions + from, sizeof *reg);
-        }
-
-        /* No overlap is simple */
-        if (!ranges_overlap(reg->guest_phys_addr, reg->memory_size,
-                            start_addr, size)) {
-            continue;
-        }
-
-        /* Split only happens if supplied region
-         * is in the middle of an existing one. Thus it can not
-         * overlap with any other existing region. */
-        assert(!split);
-
-        reglast = range_get_last(reg->guest_phys_addr, reg->memory_size);
-        memlast = range_get_last(start_addr, size);
-
-        /* Remove whole region */
-        if (start_addr <= reg->guest_phys_addr && memlast >= reglast) {
-            --dev->mem->nregions;
-            --to;
-            ++overlap_middle;
-            continue;
-        }
-
-        /* Shrink region */
-        if (memlast >= reglast) {
-            reg->memory_size = start_addr - reg->guest_phys_addr;
-            assert(reg->memory_size);
-            assert(!overlap_end);
-            ++overlap_end;
-            continue;
-        }
-
-        /* Shift region */
-        if (start_addr <= reg->guest_phys_addr) {
-            change = memlast + 1 - reg->guest_phys_addr;
-            reg->memory_size -= change;
-            reg->guest_phys_addr += change;
-            reg->userspace_addr += change;
-            assert(reg->memory_size);
-            assert(!overlap_start);
-            ++overlap_start;
-            continue;
-        }
-
-        /* This only happens if supplied region
-         * is in the middle of an existing one. Thus it can not
-         * overlap with any other existing region. */
-        assert(!overlap_start);
-        assert(!overlap_end);
-        assert(!overlap_middle);
-        /* Split region: shrink first part, shift second part. */
-        memcpy(dev->mem->regions + n, reg, sizeof *reg);
-        reg->memory_size = start_addr - reg->guest_phys_addr;
-        assert(reg->memory_size);
-        change = memlast + 1 - reg->guest_phys_addr;
-        reg = dev->mem->regions + n;
-        reg->memory_size -= change;
-        assert(reg->memory_size);
-        reg->guest_phys_addr += change;
-        reg->userspace_addr += change;
-        /* Never add more than 1 region */
-        assert(dev->mem->nregions == n);
-        ++dev->mem->nregions;
-        ++split;
-    }
-}
-
-/* Called after unassign, so no regions overlap the given range. */
-static void vhost_dev_assign_memory(struct vhost_dev *dev,
-                                    uint64_t start_addr,
-                                    uint64_t size,
-                                    uint64_t uaddr)
-{
-    int from, to;
-    struct vhost_memory_region *merged = NULL;
-    for (from = 0, to = 0; from < dev->mem->nregions; ++from, ++to) {
-        struct vhost_memory_region *reg = dev->mem->regions + to;
-        uint64_t prlast, urlast;
-        uint64_t pmlast, umlast;
-        uint64_t s, e, u;
-
-        /* clone old region */
-        if (to != from) {
-            memcpy(reg, dev->mem->regions + from, sizeof *reg);
-        }
-        prlast = range_get_last(reg->guest_phys_addr, reg->memory_size);
-        pmlast = range_get_last(start_addr, size);
-        urlast = range_get_last(reg->userspace_addr, reg->memory_size);
-        umlast = range_get_last(uaddr, size);
-
-        /* check for overlapping regions: should never happen. */
-        assert(prlast < start_addr || pmlast < reg->guest_phys_addr);
-        /* Not an adjacent or overlapping region - do not merge. */
-        if ((prlast + 1 != start_addr || urlast + 1 != uaddr) &&
-            (pmlast + 1 != reg->guest_phys_addr ||
-             umlast + 1 != reg->userspace_addr)) {
-            continue;
-        }
-
-        if (dev->vhost_ops->vhost_backend_can_merge &&
-            !dev->vhost_ops->vhost_backend_can_merge(dev, uaddr, size,
-                                                     reg->userspace_addr,
-                                                     reg->memory_size)) {
-            continue;
-        }
-
-        if (merged) {
-            --to;
-            assert(to >= 0);
-        } else {
-            merged = reg;
-        }
-        u = MIN(uaddr, reg->userspace_addr);
-        s = MIN(start_addr, reg->guest_phys_addr);
-        e = MAX(pmlast, prlast);
-        uaddr = merged->userspace_addr = u;
-        start_addr = merged->guest_phys_addr = s;
-        size = merged->memory_size = e - s + 1;
-        assert(merged->memory_size);
-    }
-
-    if (!merged) {
-        struct vhost_memory_region *reg = dev->mem->regions + to;
-        memset(reg, 0, sizeof *reg);
-        reg->memory_size = size;
-        assert(reg->memory_size);
-        reg->guest_phys_addr = start_addr;
-        reg->userspace_addr = uaddr;
-        ++to;
-    }
-    assert(to <= dev->mem->nregions + 1);
-    dev->mem->nregions = to;
-}
-
 static uint64_t vhost_get_log_size(struct vhost_dev *dev)
 {
     uint64_t log_size = 0;
@@ -526,89 +372,6 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
     return r;
 }
 
-static struct vhost_memory_region *vhost_dev_find_reg(struct vhost_dev *dev,
-						      uint64_t start_addr,
-						      uint64_t size)
-{
-    int i, n = dev->mem->nregions;
-    for (i = 0; i < n; ++i) {
-        struct vhost_memory_region *reg = dev->mem->regions + i;
-        if (ranges_overlap(reg->guest_phys_addr, reg->memory_size,
-                           start_addr, size)) {
-            return reg;
-        }
-    }
-    return NULL;
-}
-
-static bool vhost_dev_cmp_memory(struct vhost_dev *dev,
-                                 uint64_t start_addr,
-                                 uint64_t size,
-                                 uint64_t uaddr)
-{
-    struct vhost_memory_region *reg = vhost_dev_find_reg(dev, start_addr, size);
-    uint64_t reglast;
-    uint64_t memlast;
-
-    if (!reg) {
-        return true;
-    }
-
-    reglast = range_get_last(reg->guest_phys_addr, reg->memory_size);
-    memlast = range_get_last(start_addr, size);
-
-    /* Need to extend region? */
-    if (start_addr < reg->guest_phys_addr || memlast > reglast) {
-        return true;
-    }
-    /* userspace_addr changed? */
-    return uaddr != reg->userspace_addr + start_addr - reg->guest_phys_addr;
-}
-
-static void vhost_set_memory(MemoryListener *listener,
-                             MemoryRegionSection *section,
-                             bool add)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-    hwaddr start_addr = section->offset_within_address_space;
-    ram_addr_t size = int128_get64(section->size);
-    int s = offsetof(struct vhost_memory, regions) +
-        (dev->mem->nregions + 1) * sizeof dev->mem->regions[0];
-    void *ram;
-
-    dev->mem = g_realloc(dev->mem, s);
-
-    assert(size);
-
-    /* Optimize no-change case. At least cirrus_vga does this a lot at this time. */
-    ram = memory_region_get_ram_ptr(section->mr) + section->offset_within_region;
-    if (add) {
-        if (!vhost_dev_cmp_memory(dev, start_addr, size, (uintptr_t)ram)) {
-            /* Region exists with same address. Nothing to do. */
-            return;
-        }
-    } else {
-        if (!vhost_dev_find_reg(dev, start_addr, size)) {
-            /* Removing region that we don't access. Nothing to do. */
-            return;
-        }
-    }
-
-    vhost_dev_unassign_memory(dev, start_addr, size);
-    if (add) {
-        /* Add given mapping, merging adjacent regions if any */
-        vhost_dev_assign_memory(dev, start_addr, size, (uintptr_t)ram);
-    } else {
-        /* Remove old mapping for this memory, if any. */
-        vhost_dev_unassign_memory(dev, start_addr, size);
-    }
-    dev->mem_changed_start_addr = MIN(dev->mem_changed_start_addr, start_addr);
-    dev->mem_changed_end_addr = MAX(dev->mem_changed_end_addr, start_addr + size - 1);
-    dev->memory_changed = true;
-    used_memslots = dev->mem->nregions;
-}
-
 static bool vhost_section(MemoryRegionSection *section)
 {
     bool result;
@@ -630,8 +393,6 @@ 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;
     dev->tmp_sections = NULL;
     dev->n_tmp_sections = 0;
 }
@@ -702,7 +463,6 @@ static void vhost_commit(MemoryListener *listener)
         if (r < 0) {
             VHOST_OPS_DEBUG("vhost_set_mem_table failed");
         }
-        dev->memory_changed = false;
         goto out;
     }
     log_size = vhost_get_log_size(dev);
@@ -721,7 +481,6 @@ static void vhost_commit(MemoryListener *listener)
     if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
         vhost_dev_log_resize(dev, log_size);
     }
-    dev->memory_changed = false;
 
 out:
     /* Deref the old list of sections, this must happen _after_ the
@@ -806,8 +565,6 @@ static void vhost_region_add(MemoryListener *listener,
         return;
     }
     vhost_region_add_section(dev, section);
-
-    vhost_set_memory(listener, section, true);
 }
 
 /* Called on regions that have not changed */
@@ -831,7 +588,6 @@ static void vhost_region_del(MemoryListener *listener,
         return;
     }
 
-    vhost_set_memory(listener, section, false);
 }
 
 static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
@@ -1446,7 +1202,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->log_size = 0;
     hdev->log_enabled = false;
     hdev->started = false;
-    hdev->memory_changed = false;
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
     return 0;
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 21f3022499..b64732420a 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -69,9 +69,6 @@ struct vhost_dev {
     bool log_enabled;
     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;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v5 6/7] vhost: Clean out old vhost_set_memory and friends

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:39 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Remove the old update mechanism, vhost_set_memory, and the functions
> and flags it used.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
[...]

> -
> -static void vhost_set_memory(MemoryListener *listener,
> -                             MemoryRegionSection *section,
> -                             bool add)
> -{
[...]

> -    used_memslots = dev->mem->nregions;
lost sheep?

pls,
see "[PATCH v2 0/2] vhost: two fixes" to get vague idea how used_memslots is used

Re: [Qemu-devel] [PATCH v5 6/7] vhost: Clean out old vhost_set_memory and friends

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Mon, 18 Dec 2017 20:13:39 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Remove the old update mechanism, vhost_set_memory, and the functions
> > and flags it used.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> [...]
> 
> > -
> > -static void vhost_set_memory(MemoryListener *listener,
> > -                             MemoryRegionSection *section,
> > -                             bool add)
> > -{
> [...]
> 
> > -    used_memslots = dev->mem->nregions;
> lost sheep?

Yes, I've moved that into the previous patch, ( vhost: Regenerate region
list from changed sections list ) so it now has:

...
    dev->mem = g_realloc(dev->mem, regions_size);
    dev->mem->nregions = dev->n_mem_sections;
    used_memslots = dev->mem->nregions;
    for (i = 0; i < dev->n_mem_sections; i++) {
...

> pls,
> see "[PATCH v2 0/2] vhost: two fixes" to get vague idea how used_memslots is used

Yes, that series does make it better because what confused me about the
current use of used_memslots is it seems obviously unsafe for multiple
devices.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v5 7/7] vhost: Merge and delete unused callbacks

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Now that the olf vhost_set_memory code is gone, the _nop and _add
callbacks are identical and can be merged.  The _del callback is
no longer needed.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/virtio/vhost.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 358ceb3033..4eaa4f889f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -555,7 +555,8 @@ static void vhost_region_add_section(struct vhost_dev *dev,
     }
 }
 
-static void vhost_region_add(MemoryListener *listener,
+/* Used for both add and nop callbacks */
+static void vhost_region_addnop(MemoryListener *listener,
                              MemoryRegionSection *section)
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,
@@ -567,29 +568,6 @@ static void vhost_region_add(MemoryListener *listener,
     vhost_region_add_section(dev, section);
 }
 
-/* Called on regions that have not changed */
-static void vhost_region_nop(MemoryListener *listener,
-                             MemoryRegionSection *section)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-
-    if (!vhost_section(section)) {
-        return;
-    }
-
-    vhost_region_add_section(dev, section);
-}
-
-static void vhost_region_del(MemoryListener *listener,
-                             MemoryRegionSection *section)
-{
-    if (!vhost_section(section)) {
-        return;
-    }
-
-}
-
 static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
 {
     struct vhost_iommu *iommu = container_of(n, struct vhost_iommu, n);
@@ -1158,9 +1136,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->memory_listener = (MemoryListener) {
         .begin = vhost_begin,
         .commit = vhost_commit,
-        .region_add = vhost_region_add,
-        .region_del = vhost_region_del,
-        .region_nop = vhost_region_nop,
+        .region_add = vhost_region_addnop,
+        .region_nop = vhost_region_addnop,
         .log_start = vhost_log_start,
         .log_stop = vhost_log_stop,
         .log_sync = vhost_log_sync,
-- 
2.14.3

Re: [Qemu-devel] [PATCH v5 7/7] vhost: Merge and delete unused callbacks

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:40 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Now that the olf vhost_set_memory code is gone, the _nop and _add
> callbacks are identical and can be merged.  The _del callback is
> no longer needed.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
With style nit fixed

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/virtio/vhost.c | 31 ++++---------------------------
>  1 file changed, 4 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 358ceb3033..4eaa4f889f 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -555,7 +555,8 @@ static void vhost_region_add_section(struct vhost_dev *dev,
>      }
>  }
>  
> -static void vhost_region_add(MemoryListener *listener,
> +/* Used for both add and nop callbacks */
> +static void vhost_region_addnop(MemoryListener *listener,
>                               MemoryRegionSection *section)
argument alignment should on '('

>  {
>      struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> @@ -567,29 +568,6 @@ static void vhost_region_add(MemoryListener *listener,
>      vhost_region_add_section(dev, section);
>  }
>  
> -/* Called on regions that have not changed */
> -static void vhost_region_nop(MemoryListener *listener,
> -                             MemoryRegionSection *section)
> -{
> -    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> -                                         memory_listener);
> -
> -    if (!vhost_section(section)) {
> -        return;
> -    }
> -
> -    vhost_region_add_section(dev, section);
> -}
> -
> -static void vhost_region_del(MemoryListener *listener,
> -                             MemoryRegionSection *section)
> -{
> -    if (!vhost_section(section)) {
> -        return;
> -    }
> -
> -}
> -
>  static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>  {
>      struct vhost_iommu *iommu = container_of(n, struct vhost_iommu, n);
> @@ -1158,9 +1136,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>      hdev->memory_listener = (MemoryListener) {
>          .begin = vhost_begin,
>          .commit = vhost_commit,
> -        .region_add = vhost_region_add,
> -        .region_del = vhost_region_del,
> -        .region_nop = vhost_region_nop,
> +        .region_add = vhost_region_addnop,
> +        .region_nop = vhost_region_addnop,
>          .log_start = vhost_log_start,
>          .log_stop = vhost_log_stop,
>          .log_sync = vhost_log_sync,

Re: [Qemu-devel] [PATCH v5 7/7] vhost: Merge and delete unused callbacks

[Dr. David Alan Gilbert]

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Mon, 18 Dec 2017 20:13:40 +0000
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Now that the olf vhost_set_memory code is gone, the _nop and _add
> > callbacks are identical and can be merged.  The _del callback is
> > no longer needed.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> With style nit fixed
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> > ---
> >  hw/virtio/vhost.c | 31 ++++---------------------------
> >  1 file changed, 4 insertions(+), 27 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 358ceb3033..4eaa4f889f 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -555,7 +555,8 @@ static void vhost_region_add_section(struct vhost_dev *dev,
> >      }
> >  }
> >  
> > -static void vhost_region_add(MemoryListener *listener,
> > +/* Used for both add and nop callbacks */
> > +static void vhost_region_addnop(MemoryListener *listener,
> >                               MemoryRegionSection *section)
> argument alignment should on '('

Fixed, thanks.

Dave

> >  {
> >      struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> > @@ -567,29 +568,6 @@ static void vhost_region_add(MemoryListener *listener,
> >      vhost_region_add_section(dev, section);
> >  }
> >  
> > -/* Called on regions that have not changed */
> > -static void vhost_region_nop(MemoryListener *listener,
> > -                             MemoryRegionSection *section)
> > -{
> > -    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> > -                                         memory_listener);
> > -
> > -    if (!vhost_section(section)) {
> > -        return;
> > -    }
> > -
> > -    vhost_region_add_section(dev, section);
> > -}
> > -
> > -static void vhost_region_del(MemoryListener *listener,
> > -                             MemoryRegionSection *section)
> > -{
> > -    if (!vhost_section(section)) {
> > -        return;
> > -    }
> > -
> > -}
> > -
> >  static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> >  {
> >      struct vhost_iommu *iommu = container_of(n, struct vhost_iommu, n);
> > @@ -1158,9 +1136,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >      hdev->memory_listener = (MemoryListener) {
> >          .begin = vhost_begin,
> >          .commit = vhost_commit,
> > -        .region_add = vhost_region_add,
> > -        .region_del = vhost_region_del,
> > -        .region_nop = vhost_region_nop,
> > +        .region_add = vhost_region_addnop,
> > +        .region_nop = vhost_region_addnop,
> >          .log_start = vhost_log_start,
> >          .log_stop = vhost_log_stop,
> >          .log_sync = vhost_log_sync,
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v5 0/7] Rework vhost memory region updates

[Igor Mammedov]

On Mon, 18 Dec 2017 20:13:33 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Hi,
>   This patch set reworks the way the vhost code handles changes in
> physical address space layout that came from a discussion with Igor.
> 
> Its intention is to simplify a lot of the update code,
> and to make it easier for the postcopy+shared code to
> do the hugepage alignments that are needed.
> 
> Instead of inserting/removing each section during the add/del
> callbacks of the listener, we start afresh and build a list
> from the add and nop callbacks, then at the end compare the list
> we've built with the exisiting list.
> 
> v5
>   Solve the unref race found by Igor with a new 1st patch
>   Now we've got a temporary section list rework the rest of the set
>    around that.
> 
> Dr. David Alan Gilbert (7):
>   vhost: Build temporary section list and deref after commit
>   vhost: Move log_dirty check
>   vhost: Simplify ring verification checks
>   vhost: Merge sections added to temporary list
>   vhost: Regenerate region list from changed sections list
>   vhost: Clean out old vhost_set_memory and friends
>   vhost: Merge and delete unused callbacks
> 
>  hw/virtio/trace-events    |   6 +
>  hw/virtio/vhost.c         | 490 ++++++++++++++++------------------------------
>  include/hw/virtio/vhost.h |   5 +-
>  3 files changed, 174 insertions(+), 327 deletions(-)
Nice diffstat and more importantly it should be easier to reason about changes to memmap in future patches,
Thanks for making it easier to read/understand how memap updates work.

The series looks almost ready, I've Acked most of it modulo
    "vhost: Move log_dirty check"
      - I'm not convinced that doing it while old code still around is right thing to do as it might break bisectability,
        Maybe Michael may ack it if doesn't really affect old code,
        see discussion at '[PATCH v4 1/6] vhost: Move log_dirty check'

    "vhost: Regenerate region list from changed sections list"
      - suggested an additional cleanup

    "vhost: Clean out old vhost_set_memory and friends"
      - removes update to used_memslots without providing an alternative,
        it could lead to crashes when region count goes above backend's limit
        related thread "[PATCH v2 0/2] vhost: two fixes"

    the rest of comments are just minor issues