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

[Jason Wang]

On 02/26/2013 04:44 PM, Michael S. Tsirkin wrote:
> 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>

The patch works well.

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