diff mbox

[PATCHv2,for-1.4] vhost: memory sync fixes

Message ID 20130226101053.GA22129@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Feb. 26, 2013, 10:11 a.m. UTC
This fixes two bugs related to memory sync during
migration:
    - ram address calculation was missing the chunk
      address, so the wrong page was dirtied
    - one after last was used instead of the
      end address of a region, which might overflow to 0
      and cause us to skip the region when the region ends at
      ~0x0ull.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Jason Wang <jasowang@redhat.com>
---
 hw/vhost.c | 49 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

Comments

Jason Wang Feb. 27, 2013, 2:53 a.m. UTC | #1
On 02/26/2013 06:11 PM, Michael S. Tsirkin wrote:
> This fixes two bugs related to memory sync during
> migration:
>     - ram address calculation was missing the chunk
>       address, so the wrong page was dirtied
>     - one after last was used instead of the
>       end address of a region, which might overflow to 0
>       and cause us to skip the region when the region ends at
>       ~0x0ull.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Jason Wang <jasowang@redhat.com>

Also need for 1.3

Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/vhost.c | 49 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 8d41fdb..37777c2 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -53,10 +53,14 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
>          log = __sync_fetch_and_and(from, 0);
>          while ((bit = sizeof(log) > sizeof(int) ?
>                  ffsll(log) : ffs(log))) {
> -            ram_addr_t ram_addr;
> +            hwaddr page_addr;
> +            hwaddr section_offset;
> +            hwaddr mr_offset;
>              bit -= 1;
> -            ram_addr = section->offset_within_region + bit * VHOST_LOG_PAGE;
> -            memory_region_set_dirty(section->mr, ram_addr, VHOST_LOG_PAGE);
> +            page_addr = addr + bit * VHOST_LOG_PAGE;
> +            section_offset = page_addr - section->offset_within_address_space;
> +            mr_offset = section_offset + section->offset_within_region;
> +            memory_region_set_dirty(section->mr, mr_offset, VHOST_LOG_PAGE);
>              log &= ~(0x1ull << bit);
>          }
>          addr += VHOST_LOG_CHUNK;
> @@ -65,14 +69,21 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
>  
>  static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
>                                     MemoryRegionSection *section,
> -                                   hwaddr start_addr,
> -                                   hwaddr end_addr)
> +                                   hwaddr first,
> +                                   hwaddr last)
>  {
>      int i;
> +    hwaddr start_addr;
> +    hwaddr end_addr;
>  
>      if (!dev->log_enabled || !dev->started) {
>          return 0;
>      }
> +    start_addr = section->offset_within_address_space;
> +    end_addr = range_get_last(start_addr, section->size);
> +    start_addr = MAX(first, start_addr);
> +    end_addr = MIN(last, end_addr);
> +
>      for (i = 0; i < dev->mem->nregions; ++i) {
>          struct vhost_memory_region *reg = dev->mem->regions + i;
>          vhost_dev_sync_region(dev, section, start_addr, end_addr,
> @@ -93,10 +104,18 @@ static void vhost_log_sync(MemoryListener *listener,
>  {
>      struct vhost_dev *dev = container_of(listener, struct vhost_dev,
>                                           memory_listener);
> -    hwaddr start_addr = section->offset_within_address_space;
> -    hwaddr end_addr = start_addr + section->size;
> +    vhost_sync_dirty_bitmap(dev, section, 0x0, ~0x0ULL);
> +}
>  
> -    vhost_sync_dirty_bitmap(dev, section, start_addr, end_addr);
> +static void vhost_log_sync_range(struct vhost_dev *dev,
> +                                 hwaddr first, hwaddr last)
> +{
> +    int i;
> +    /* FIXME: this is N^2 in number of sections */
> +    for (i = 0; i < dev->n_mem_sections; ++i) {
> +        MemoryRegionSection *section = &dev->mem_sections[i];
> +        vhost_sync_dirty_bitmap(dev, section, first, last);
> +    }
>  }
>  
>  /* Assign/unassign. Keep an unsorted array of non-overlapping
> @@ -268,16 +287,15 @@ static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
>  {
>      vhost_log_chunk_t *log;
>      uint64_t log_base;
> -    int r, i;
> +    int r;
>  
>      log = g_malloc0(size * sizeof *log);
>      log_base = (uint64_t)(unsigned long)log;
>      r = ioctl(dev->control, VHOST_SET_LOG_BASE, &log_base);
>      assert(r >= 0);
> -    for (i = 0; i < dev->n_mem_sections; ++i) {
> -        /* Sync only the range covered by the old log */
> -        vhost_sync_dirty_bitmap(dev, &dev->mem_sections[i], 0,
> -                                dev->log_size * VHOST_LOG_CHUNK - 1);
> +    /* Sync only the range covered by the old log */
> +    if (dev->log_size) {
> +        vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
>      }
>      if (dev->log) {
>          g_free(dev->log);
> @@ -1014,10 +1032,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
>                               hdev->vqs + i,
>                               hdev->vq_index + i);
>      }
> -    for (i = 0; i < hdev->n_mem_sections; ++i) {
> -        vhost_sync_dirty_bitmap(hdev, &hdev->mem_sections[i],
> -                                0, (hwaddr)~0x0ull);
> -    }
> +    vhost_log_sync_range(hdev, 0, ~0x0ull);
>  
>      hdev->started = false;
>      g_free(hdev->log);
diff mbox

Patch

diff --git a/hw/vhost.c b/hw/vhost.c
index 8d41fdb..37777c2 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -53,10 +53,14 @@  static void vhost_dev_sync_region(struct vhost_dev *dev,
         log = __sync_fetch_and_and(from, 0);
         while ((bit = sizeof(log) > sizeof(int) ?
                 ffsll(log) : ffs(log))) {
-            ram_addr_t ram_addr;
+            hwaddr page_addr;
+            hwaddr section_offset;
+            hwaddr mr_offset;
             bit -= 1;
-            ram_addr = section->offset_within_region + bit * VHOST_LOG_PAGE;
-            memory_region_set_dirty(section->mr, ram_addr, VHOST_LOG_PAGE);
+            page_addr = addr + bit * VHOST_LOG_PAGE;
+            section_offset = page_addr - section->offset_within_address_space;
+            mr_offset = section_offset + section->offset_within_region;
+            memory_region_set_dirty(section->mr, mr_offset, VHOST_LOG_PAGE);
             log &= ~(0x1ull << bit);
         }
         addr += VHOST_LOG_CHUNK;
@@ -65,14 +69,21 @@  static void vhost_dev_sync_region(struct vhost_dev *dev,
 
 static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
                                    MemoryRegionSection *section,
-                                   hwaddr start_addr,
-                                   hwaddr end_addr)
+                                   hwaddr first,
+                                   hwaddr last)
 {
     int i;
+    hwaddr start_addr;
+    hwaddr end_addr;
 
     if (!dev->log_enabled || !dev->started) {
         return 0;
     }
+    start_addr = section->offset_within_address_space;
+    end_addr = range_get_last(start_addr, section->size);
+    start_addr = MAX(first, start_addr);
+    end_addr = MIN(last, end_addr);
+
     for (i = 0; i < dev->mem->nregions; ++i) {
         struct vhost_memory_region *reg = dev->mem->regions + i;
         vhost_dev_sync_region(dev, section, start_addr, end_addr,
@@ -93,10 +104,18 @@  static void vhost_log_sync(MemoryListener *listener,
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,
                                          memory_listener);
-    hwaddr start_addr = section->offset_within_address_space;
-    hwaddr end_addr = start_addr + section->size;
+    vhost_sync_dirty_bitmap(dev, section, 0x0, ~0x0ULL);
+}
 
-    vhost_sync_dirty_bitmap(dev, section, start_addr, end_addr);
+static void vhost_log_sync_range(struct vhost_dev *dev,
+                                 hwaddr first, hwaddr last)
+{
+    int i;
+    /* FIXME: this is N^2 in number of sections */
+    for (i = 0; i < dev->n_mem_sections; ++i) {
+        MemoryRegionSection *section = &dev->mem_sections[i];
+        vhost_sync_dirty_bitmap(dev, section, first, last);
+    }
 }
 
 /* Assign/unassign. Keep an unsorted array of non-overlapping
@@ -268,16 +287,15 @@  static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
 {
     vhost_log_chunk_t *log;
     uint64_t log_base;
-    int r, i;
+    int r;
 
     log = g_malloc0(size * sizeof *log);
     log_base = (uint64_t)(unsigned long)log;
     r = ioctl(dev->control, VHOST_SET_LOG_BASE, &log_base);
     assert(r >= 0);
-    for (i = 0; i < dev->n_mem_sections; ++i) {
-        /* Sync only the range covered by the old log */
-        vhost_sync_dirty_bitmap(dev, &dev->mem_sections[i], 0,
-                                dev->log_size * VHOST_LOG_CHUNK - 1);
+    /* Sync only the range covered by the old log */
+    if (dev->log_size) {
+        vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
     }
     if (dev->log) {
         g_free(dev->log);
@@ -1014,10 +1032,7 @@  void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
                              hdev->vqs + i,
                              hdev->vq_index + i);
     }
-    for (i = 0; i < hdev->n_mem_sections; ++i) {
-        vhost_sync_dirty_bitmap(hdev, &hdev->mem_sections[i],
-                                0, (hwaddr)~0x0ull);
-    }
+    vhost_log_sync_range(hdev, 0, ~0x0ull);
 
     hdev->started = false;
     g_free(hdev->log);