Re: [Qemu-devel] scp during migration with vhost fails

[Michael S. Tsirkin]

On Tue, Feb 26, 2013 at 02:41:03PM +0800, Jason Wang wrote:
> On 02/25/2013 06:01 PM, Michael S. Tsirkin wrote:
> > On Mon, Feb 25, 2013 at 02:11:44PM +0800, Jason Wang wrote:
> >> On 02/25/2013 01:57 PM, Jason Wang wrote:
> >>> On 02/24/2013 05:54 AM, Michael S. Tsirkin wrote:
> >>>> On Sat, Feb 23, 2013 at 10:49:29PM +0200, Michael S. Tsirkin wrote:
> >>>>> On Fri, Feb 22, 2013 at 11:33:53PM +0800, Jason Wang wrote:
> >>>>>> On 02/21/2013 07:23 PM, Michael S. Tsirkin wrote:
> >>>>>>> On Thu, Feb 21, 2013 at 05:57:04PM +0800, Jason Wang wrote:
> >>>>>>>> On 02/21/2013 12:48 AM, Michael S. Tsirkin wrote:
> >>>>>>>>> On Wed, Feb 20, 2013 at 04:23:52PM +0200, Michael S. Tsirkin wrote:
> >>>>>>>>>> On Fri, Feb 01, 2013 at 06:03:32PM +0800, Jason Wang wrote:
> >>>>>>>>>>> Hello all:
> >>>>>>>>>>>
> >>>>>>>>>>> During testing, I find doing scp during migration with vhost 
> >>>>>>>>>>> fails with 
> >>>>>>>>>>> warnings in guest like:
> >>>>>>>>>>>
> >>>>>>>>>>> Corrupted MAC on input.
> >>>>>>>>>>> Disconnecting: Packet corrupt.
> >>>>>>>>>>> lost connection
> >>>>>>>>>>>
> >>>>>>>>>>> Here's the bisect result:
> >>>>>>>>>>>
> >>>>>>>>>>> Commit a01672d3968cf91208666d371784110bfde9d4f8 kvm: convert to 
> >>>>>>>>>>> MemoryListener 
> >>>>>>>>>>> API is the last commit that works well.
> >>>>>>>>>>>
> >>>>>>>>>>> With commit 04097f7c5957273c578f72b9bd603ba6b1d69e33 vhost: 
> >>>>>>>>>>> convert to 
> >>>>>>>>>>> MemoryListener API, guest network is unusable with warning of 
> >>>>>>>>>>> "bad gso type"
> >>>>>>>>>>>
> >>>>>>>>>>> With commit d743c382861eaa1e13f503b05aba5a382a7e7f7c vhost: fix 
> >>>>>>>>>>> incorrect 
> >>>>>>>>>>> userspace address, guest network is available, but scp during 
> >>>>>>>>>>> migration may 
> >>>>>>>>>>> fail.
> >>>>>>>>>>>
> >>>>>>>>>>> Looks like the issue is related to memory api, any thoughts?
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks
> >>>>>>>>>> Tried to reproduce this for a while without success.
> >>>>>>>>>> Which command line was used?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> -- 
> >>>>>>>>>> MST
> >>>>>>>>> Could be we are not syncing all that we should?
> >>>>>>>>> Does the following hack make the problem go away?
> >>>>>>>>>
> >>>>>>>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>>>>>>> index 8d41fdb..a7a0412 100644
> >>>>>>>>> --- a/hw/vhost.c
> >>>>>>>>> +++ b/hw/vhost.c
> >>>>>>>>> @@ -69,6 +69,8 @@ static int vhost_sync_dirty_bitmap(struct 
> >>>>>>>>> vhost_dev *dev,
> >>>>>>>>>                                     hwaddr end_addr)
> >>>>>>>>>  {
> >>>>>>>>>      int i;
> >>>>>>>>> +    start_addr = 0x0;
> >>>>>>>>> +    end_addr = ~0x0ull;
> >>>>>>>>>  
> >>>>>>>>>      if (!dev->log_enabled || !dev->started) {
> >>>>>>>>>          return 0;
> >>>>>>>>>
> >>>>>>>> Still can reproduce with this. From the bisect result, the vhost 
> >>>>>>>> dirty
> >>>>>>>> bitmap sync itself looks ok but something wrong when converting to
> >>>>>>>> memory listener.
> >>>>>>> Reading the code carefully, I found two bugs introduced during
> >>>>>>> this conversion. Patch below, could you please try?
> >>>>>>>
> >>>>>>>     vhost: memory sync fixes
> >>>>>>>     
> >>>>>>>     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 <address@hidden>
> >>>>>>>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>>>>> index 8d41fdb..dbf6b46 100644
> >>>>>>> --- a/hw/vhost.c
> >>>>>>> +++ b/hw/vhost.c
> >>>>>>> @@ -55,7 +55,7 @@ static void vhost_dev_sync_region(struct vhost_dev 
> >>>>>>> *dev,
> >>>>>>>                  ffsll(log) : ffs(log))) {
> >>>>>>>              ram_addr_t ram_addr;
> >>>>>>>              bit -= 1;
> >>>>>>> -            ram_addr = section->offset_within_region + bit * 
> >>>>>>> VHOST_LOG_PAGE;
> >>>>>>> +            ram_addr = section->offset_within_region + addr + bit * 
> >>>>>>> VHOST_LOG_PAGE;
> >>>>>>>              memory_region_set_dirty(section->mr, ram_addr, 
> >>>>>>> VHOST_LOG_PAGE);
> >>>>>>>              log &= ~(0x1ull << bit);
> >>>>>>>          }
> >>>>>>> @@ -94,7 +94,7 @@ 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;
> >>>>>>> +    hwaddr end_addr = start_addr + section->size - 1;
> >>>>>>>  
> >>>>>>>      vhost_sync_dirty_bitmap(dev, section, start_addr, end_addr);
> >>>>>>>  }
> >>>>>>>
> >>>>>> I can still reproduce the issue with this patch.
> >>>>> Yes it's still wrong. We need the following on top.
> >>>>> Could you try please?
> >>>>>
> >>>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>>> index dbf6b46..c324903 100644
> >>>>> --- a/hw/vhost.c
> >>>>> +++ b/hw/vhost.c
> >>>>> @@ -29,7 +29,7 @@ static void vhost_dev_sync_region(struct vhost_dev 
> >>>>> *dev,
> >>>>>      uint64_t end = MIN(mlast, rlast);
> >>>>>      vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
> >>>>>      vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> >>>>> -    uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >>>>> +    uint64_t addr = 0;
> >>>>>  
> >>>>>      if (end < start) {
> >>>>>          return;
> >>>> Sorry, scratch that last one, sorry.
> >>>> This should be the right thing, I think: on top of
> >>>> 'vhost: memory sync fixes'.
> >>>>
> >>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>> index dbf6b46..72c0095 100644
> >>>> --- a/hw/vhost.c
> >>>> +++ b/hw/vhost.c
> >>>> @@ -53,9 +53,10 @@ 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 ram_addr;
> >>>>              bit -= 1;
> >>>> -            ram_addr = section->offset_within_region + addr + bit * 
> >>>> VHOST_LOG_PAGE;
> >>>> +            ram_addr = addr + bit * VHOST_LOG_PAGE -
> >>>> +                section->mr->offset_within_address_space;
> >>> should be section->offset_within_address_space
> >>>>              memory_region_set_dirty(section->mr, ram_addr, 
> >>>> VHOST_LOG_PAGE);
> >>>>              log &= ~(0x1ull << bit);
> >>>>          }
> >>>>
> >>>>
> >>> Still can reproduce. An interesting thing is after I chage the
> >>> section->offset_within_address_space to section->mr->ram_addr[1]. I
> >>> can't reproduce the issue. I haven't read all the codes, but it looks
> >>> like something is wrong with the valueof
> >>> section->offset_within_address_space? Thanks
> >> It's ok since we need offset inside the region as the second parameter
> >> of memory_region_set_dirty().
> >>> [1]
> >>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>> index 8d41fdb..785e68e 100644
> >>> --- a/hw/vhost.c
> >>> +++ b/hw/vhost.c
> >>> @@ -55,7 +55,7 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
> >>>                  ffsll(log) : ffs(log))) {
> >>>              ram_addr_t ram_addr;
> >>>              bit -= 1;
> >>> -            ram_addr = section->offset_within_region + bit *
> >>> VHOST_LOG_PAGE;
> >>> +            ram_addr = addr + bit * VHOST_LOG_PAGE - 
> >>> section->mr->ram_addr;
> >>>              memory_region_set_dirty(section->mr, ram_addr, 
> >>> VHOST_LOG_PAGE);
> >>>              log &= ~(0x1ull << bit);
> >>>          }
> >>>
> >>>
> >
> > OK I think now I get it.
> > The following should be the real thing.
> >
> >
> > diff --git a/hw/vhost.c b/hw/vhost.c
> > index dbf6b46..2d8ead8 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 + addr + bit * 
> > VHOST_LOG_PAGE;
> > -            memory_region_set_dirty(section->mr, ram_addr, VHOST_LOG_PAGE);
> > +            page_addr = addr + bit * VHOST_LOG_PAGE;
> > +            section_offset = ram_addr - 
> > section->offset_within_address_space;
> 
> Should be page_addr here.
> > +            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;
> >
> >
> >
> I get segfault with this patch, and looks like we need to check whether
> the regions are overlapped in the begining of vhost_dev_sync_region:
> 
>     if (!ranges_overlap(start, end - start + 1,
>                         section->offset_within_address_space,
> section->size)) {
>         return;
>     }
> 
> With this, no segfault and I can not reproduce the issue again.

OK, and I guess this works because sync sections ATM do not cross
tracking sections, but it does not seem to be required by the API, so I
think we really should be more careful and only sync the correct range.

Signed-off-by: Michael S. Tsirkin <address@hidden>

diff --git a/hw/vhost.c b/hw/vhost.c
index dbf6b46..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 + addr + 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 - 1;
+    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);