Patchwork scp during migration with vhost fails

login
register
mail settings
Submitter Michael S. Tsirkin
Date Feb. 26, 2013, 8:44 a.m.
Message ID <20130226084404.GA19888@redhat.com>
Download mbox | patch
Permalink /patch/223145/
State New
Headers show

Comments

Michael S. Tsirkin - Feb. 26, 2013, 8:44 a.m.
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 <mst@redhat.com>
> >>>>>>>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> 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 <mst@redhat.com>
Jason Wang - Feb. 26, 2013, 9:52 a.m.
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 <mst@redhat.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 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 <mst@redhat.com>

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

Patch

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