[Qemu-devel] [PATCH v3 0/8] Rework vhost memory region updates

[Qemu-devel] [PATCH v3 0/8] 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 updating and trying to merge sections of address
space on each add/remove callback, we wait until the commit phase
and go through and rebuild a list by walking the Flatview of
memory and end up producing an ordered list.
We compare the list to the old list to trigger updates.

v3
  Fix misordered comparison (that is removed later in the series)
  A few spelling fixes
  Now bisect builds, make checks and passes a test with hotlug
  memory on real vhost.

Dave


Dr. David Alan Gilbert (8):
  memory: address_space_iterate
  vhost: Move log_dirty check
  vhost: Simplify ring verification checks
  vhost: New memory update functions
  vhost: update_mem_cb implementation
  vhost: Compare and copy updated region data into device state
  vhost: Remove old vhost_set_memory etc
  vhost: Move mem_sections maintenance into commit/update routines

 hw/virtio/trace-events    |   7 +
 hw/virtio/vhost.c         | 498 ++++++++++++++++------------------------------
 include/exec/memory.h     |  23 +++
 include/hw/virtio/vhost.h |   3 -
 memory.c                  |  22 ++
 5 files changed, 226 insertions(+), 327 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v3 1/8] memory: address_space_iterate

[Dr. David Alan Gilbert (git)]

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

Iterate through an address space calling a function for each
section.  The iteration is done in order.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/exec/memory.h | 23 +++++++++++++++++++++++
 memory.c              | 22 ++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5ed4042f87..f5a9df642e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1987,6 +1987,29 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
     address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED, buf, len);
 }
 
+/**
+ * ASIterateCallback: Function type called by address_space_iterate
+ *
+ * Return 0 on success or a negative error code.
+ *
+ * @mrs: Memory region section for this range
+ * @opaque: The opaque value passed in to the iterator.
+ */
+typedef int (*ASIterateCallback)(MemoryRegionSection *mrs, void *opaque);
+
+/**
+ * address_space_iterate: Call the function for each address range in the
+ *                        AddressSpace, in sorted order.
+ *
+ * Return 0 on success or a negative error code.
+ *
+ * @as: Address space to iterate over
+ * @cb: Function to call.  If the function returns none-0 the iteration will
+ *     stop.
+ * @opaque: Value to pass to the function
+ */
+int
+address_space_iterate(AddressSpace *as, ASIterateCallback cb, void *opaque);
 #endif
 
 #endif
diff --git a/memory.c b/memory.c
index e26e5a3b1d..f45137f25e 100644
--- a/memory.c
+++ b/memory.c
@@ -2810,6 +2810,28 @@ void address_space_destroy(AddressSpace *as)
     call_rcu(as, do_address_space_destroy, rcu);
 }
 
+int address_space_iterate(AddressSpace *as, ASIterateCallback cb,
+                          void *opaque)
+{
+    int res = 0;
+    FlatView *fv = address_space_to_flatview(as);
+    FlatRange *range;
+
+    flatview_ref(fv);
+
+    FOR_EACH_FLAT_RANGE(range, fv) {
+        MemoryRegionSection mrs = section_from_flat_range(range, fv);
+        res = cb(&mrs, opaque);
+        if (res) {
+            break;
+        }
+    }
+
+    flatview_unref(fv);
+
+    return res;
+}
+
 static const char *memory_region_type(MemoryRegion *mr)
 {
     if (memory_region_is_ram_device(mr)) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 1/8] memory: address_space_iterate

[Paolo Bonzini]

On 11/12/2017 20:46, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Iterate through an address space calling a function for each
> section.  The iteration is done in order.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

It seems to me that you can achieve the same effect by implementing the
region_add and region_nop callbacks, and leaving out region_del.  Am I
missing something?

Thanks,

Paolo

> ---
>  include/exec/memory.h | 23 +++++++++++++++++++++++
>  memory.c              | 22 ++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 5ed4042f87..f5a9df642e 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1987,6 +1987,29 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
>      address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED, buf, len);
>  }
>  
> +/**
> + * ASIterateCallback: Function type called by address_space_iterate
> + *
> + * Return 0 on success or a negative error code.
> + *
> + * @mrs: Memory region section for this range
> + * @opaque: The opaque value passed in to the iterator.
> + */
> +typedef int (*ASIterateCallback)(MemoryRegionSection *mrs, void *opaque);
> +
> +/**
> + * address_space_iterate: Call the function for each address range in the
> + *                        AddressSpace, in sorted order.
> + *
> + * Return 0 on success or a negative error code.
> + *
> + * @as: Address space to iterate over
> + * @cb: Function to call.  If the function returns none-0 the iteration will
> + *     stop.
> + * @opaque: Value to pass to the function
> + */
> +int
> +address_space_iterate(AddressSpace *as, ASIterateCallback cb, void *opaque);
>  #endif
>  
>  #endif
> diff --git a/memory.c b/memory.c
> index e26e5a3b1d..f45137f25e 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2810,6 +2810,28 @@ void address_space_destroy(AddressSpace *as)
>      call_rcu(as, do_address_space_destroy, rcu);
>  }
>  
> +int address_space_iterate(AddressSpace *as, ASIterateCallback cb,
> +                          void *opaque)
> +{
> +    int res = 0;
> +    FlatView *fv = address_space_to_flatview(as);
> +    FlatRange *range;
> +
> +    flatview_ref(fv);
> +
> +    FOR_EACH_FLAT_RANGE(range, fv) {
> +        MemoryRegionSection mrs = section_from_flat_range(range, fv);
> +        res = cb(&mrs, opaque);
> +        if (res) {
> +            break;
> +        }
> +    }
> +
> +    flatview_unref(fv);
> +
> +    return res;
> +}
> +
>  static const char *memory_region_type(MemoryRegion *mr)
>  {
>      if (memory_region_is_ram_device(mr)) {
> 

Re: [Qemu-devel] [PATCH v3 1/8] memory: address_space_iterate

[Igor Mammedov]

On Tue, 12 Dec 2017 00:50:51 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 11/12/2017 20:46, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Iterate through an address space calling a function for each
> > section.  The iteration is done in order.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>  
> 
> It seems to me that you can achieve the same effect by implementing the
> region_add and region_nop callbacks, and leaving out region_del.  Am I
> missing something?
region_del would also be need to track references correctly.

Initially I've also suggested to reuse region_add|del but
compared to David's approach it will be multiple calls for
a memory transaction vs a pass over flatview at commit time.

> 
> Thanks,
> 
> Paolo
> 
> > ---
> >  include/exec/memory.h | 23 +++++++++++++++++++++++
> >  memory.c              | 22 ++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 5ed4042f87..f5a9df642e 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1987,6 +1987,29 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
> >      address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED, buf, len);
> >  }
> >  
> > +/**
> > + * ASIterateCallback: Function type called by address_space_iterate
> > + *
> > + * Return 0 on success or a negative error code.
> > + *
> > + * @mrs: Memory region section for this range
> > + * @opaque: The opaque value passed in to the iterator.
> > + */
> > +typedef int (*ASIterateCallback)(MemoryRegionSection *mrs, void *opaque);
> > +
> > +/**
> > + * address_space_iterate: Call the function for each address range in the
> > + *                        AddressSpace, in sorted order.
> > + *
> > + * Return 0 on success or a negative error code.
> > + *
> > + * @as: Address space to iterate over
> > + * @cb: Function to call.  If the function returns none-0 the iteration will
> > + *     stop.
> > + * @opaque: Value to pass to the function
> > + */
> > +int
> > +address_space_iterate(AddressSpace *as, ASIterateCallback cb, void *opaque);
> >  #endif
> >  
> >  #endif
> > diff --git a/memory.c b/memory.c
> > index e26e5a3b1d..f45137f25e 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -2810,6 +2810,28 @@ void address_space_destroy(AddressSpace *as)
> >      call_rcu(as, do_address_space_destroy, rcu);
> >  }
> >  
> > +int address_space_iterate(AddressSpace *as, ASIterateCallback cb,
> > +                          void *opaque)
> > +{
> > +    int res = 0;
> > +    FlatView *fv = address_space_to_flatview(as);
> > +    FlatRange *range;
> > +
> > +    flatview_ref(fv);
> > +
> > +    FOR_EACH_FLAT_RANGE(range, fv) {
> > +        MemoryRegionSection mrs = section_from_flat_range(range, fv);
> > +        res = cb(&mrs, opaque);
> > +        if (res) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    flatview_unref(fv);
> > +
> > +    return res;
> > +}
> > +
> >  static const char *memory_region_type(MemoryRegion *mr)
> >  {
> >      if (memory_region_is_ram_device(mr)) {
> >   
> 

Re: [Qemu-devel] [PATCH v3 1/8] memory: address_space_iterate

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 11/12/2017 20:46, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Iterate through an address space calling a function for each
> > section.  The iteration is done in order.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> It seems to me that you can achieve the same effect by implementing the
> region_add and region_nop callbacks, and leaving out region_del.  Am I
> missing something?

What's the semantics of region_nop (and for that matter region_add/del)?
Th nice thing we have here is we get a full walk of the physical memory
in order; keeping it in order makes our data structure easy for merging.

Dave


> Thanks,
> 
> Paolo
> 
> > ---
> >  include/exec/memory.h | 23 +++++++++++++++++++++++
> >  memory.c              | 22 ++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 5ed4042f87..f5a9df642e 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1987,6 +1987,29 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
> >      address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED, buf, len);
> >  }
> >  
> > +/**
> > + * ASIterateCallback: Function type called by address_space_iterate
> > + *
> > + * Return 0 on success or a negative error code.
> > + *
> > + * @mrs: Memory region section for this range
> > + * @opaque: The opaque value passed in to the iterator.
> > + */
> > +typedef int (*ASIterateCallback)(MemoryRegionSection *mrs, void *opaque);
> > +
> > +/**
> > + * address_space_iterate: Call the function for each address range in the
> > + *                        AddressSpace, in sorted order.
> > + *
> > + * Return 0 on success or a negative error code.
> > + *
> > + * @as: Address space to iterate over
> > + * @cb: Function to call.  If the function returns none-0 the iteration will
> > + *     stop.
> > + * @opaque: Value to pass to the function
> > + */
> > +int
> > +address_space_iterate(AddressSpace *as, ASIterateCallback cb, void *opaque);
> >  #endif
> >  
> >  #endif
> > diff --git a/memory.c b/memory.c
> > index e26e5a3b1d..f45137f25e 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -2810,6 +2810,28 @@ void address_space_destroy(AddressSpace *as)
> >      call_rcu(as, do_address_space_destroy, rcu);
> >  }
> >  
> > +int address_space_iterate(AddressSpace *as, ASIterateCallback cb,
> > +                          void *opaque)
> > +{
> > +    int res = 0;
> > +    FlatView *fv = address_space_to_flatview(as);
> > +    FlatRange *range;
> > +
> > +    flatview_ref(fv);
> > +
> > +    FOR_EACH_FLAT_RANGE(range, fv) {
> > +        MemoryRegionSection mrs = section_from_flat_range(range, fv);
> > +        res = cb(&mrs, opaque);
> > +        if (res) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    flatview_unref(fv);
> > +
> > +    return res;
> > +}
> > +
> >  static const char *memory_region_type(MemoryRegion *mr)
> >  {
> >      if (memory_region_is_ram_device(mr)) {
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/8] memory: address_space_iterate

[Paolo Bonzini]

On 12/12/2017 11:31, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 11/12/2017 20:46, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> Iterate through an address space calling a function for each
>>> section.  The iteration is done in order.
>>>
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>
>> It seems to me that you can achieve the same effect by implementing the
>> region_add and region_nop callbacks, and leaving out region_del.  Am I
>> missing something?
> 
> What's the semantics of region_nop (and for that matter region_add/del)?

nop means that attributes (readonly, romd_mode, mr+offset_in_region)
haven't changed; nop is optionally followed by log_start or log_stop.
If any of them changes, you get del+add (del is always before add).

> Th nice thing we have here is we get a full walk of the physical memory
> in order; keeping it in order makes our data structure easy for merging.

That's the same that you get with region_del/add.

Thanks,

Paolo

> Dave
> 
> 
>> Thanks,
>>
>> Paolo
>>
>>> ---
>>>  include/exec/memory.h | 23 +++++++++++++++++++++++
>>>  memory.c              | 22 ++++++++++++++++++++++
>>>  2 files changed, 45 insertions(+)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index 5ed4042f87..f5a9df642e 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -1987,6 +1987,29 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
>>>      address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED, buf, len);
>>>  }
>>>  
>>> +/**
>>> + * ASIterateCallback: Function type called by address_space_iterate
>>> + *
>>> + * Return 0 on success or a negative error code.
>>> + *
>>> + * @mrs: Memory region section for this range
>>> + * @opaque: The opaque value passed in to the iterator.
>>> + */
>>> +typedef int (*ASIterateCallback)(MemoryRegionSection *mrs, void *opaque);
>>> +
>>> +/**
>>> + * address_space_iterate: Call the function for each address range in the
>>> + *                        AddressSpace, in sorted order.
>>> + *
>>> + * Return 0 on success or a negative error code.
>>> + *
>>> + * @as: Address space to iterate over
>>> + * @cb: Function to call.  If the function returns none-0 the iteration will
>>> + *     stop.
>>> + * @opaque: Value to pass to the function
>>> + */
>>> +int
>>> +address_space_iterate(AddressSpace *as, ASIterateCallback cb, void *opaque);
>>>  #endif
>>>  
>>>  #endif
>>> diff --git a/memory.c b/memory.c
>>> index e26e5a3b1d..f45137f25e 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -2810,6 +2810,28 @@ void address_space_destroy(AddressSpace *as)
>>>      call_rcu(as, do_address_space_destroy, rcu);
>>>  }
>>>  
>>> +int address_space_iterate(AddressSpace *as, ASIterateCallback cb,
>>> +                          void *opaque)
>>> +{
>>> +    int res = 0;
>>> +    FlatView *fv = address_space_to_flatview(as);
>>> +    FlatRange *range;
>>> +
>>> +    flatview_ref(fv);
>>> +
>>> +    FOR_EACH_FLAT_RANGE(range, fv) {
>>> +        MemoryRegionSection mrs = section_from_flat_range(range, fv);
>>> +        res = cb(&mrs, opaque);
>>> +        if (res) {
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    flatview_unref(fv);
>>> +
>>> +    return res;
>>> +}
>>> +
>>>  static const char *memory_region_type(MemoryRegion *mr)
>>>  {
>>>      if (memory_region_is_ram_device(mr)) {
>>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [PATCH v3 1/8] memory: address_space_iterate

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 12/12/2017 11:31, Dr. David Alan Gilbert wrote:
> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
> >> On 11/12/2017 20:46, Dr. David Alan Gilbert (git) wrote:
> >>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>
> >>> Iterate through an address space calling a function for each
> >>> section.  The iteration is done in order.
> >>>
> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>
> >> It seems to me that you can achieve the same effect by implementing the
> >> region_add and region_nop callbacks, and leaving out region_del.  Am I
> >> missing something?
> > 
> > What's the semantics of region_nop (and for that matter region_add/del)?
> 
> nop means that attributes (readonly, romd_mode, mr+offset_in_region)
> haven't changed; nop is optionally followed by log_start or log_stop.
> If any of them changes, you get del+add (del is always before add).
> 
> > Th nice thing we have here is we get a full walk of the physical memory
> > in order; keeping it in order makes our data structure easy for merging.
> 
> That's the same that you get with region_del/add.

Hmm it would be good if that was documented somewhere.
I thought you'd only get an _add/_del and then a commit.

However, what do I do in vhost_dev_start?  At the moment I'm forcfully
regenerating it using the same code.

Dave

> Thanks,
> 
> Paolo
> 
> > Dave
> > 
> > 
> >> Thanks,
> >>
> >> Paolo
> >>
> >>> ---
> >>>  include/exec/memory.h | 23 +++++++++++++++++++++++
> >>>  memory.c              | 22 ++++++++++++++++++++++
> >>>  2 files changed, 45 insertions(+)
> >>>
> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h
> >>> index 5ed4042f87..f5a9df642e 100644
> >>> --- a/include/exec/memory.h
> >>> +++ b/include/exec/memory.h
> >>> @@ -1987,6 +1987,29 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
> >>>      address_space_write(cache->as, cache->xlat + addr, MEMTXATTRS_UNSPECIFIED, buf, len);
> >>>  }
> >>>  
> >>> +/**
> >>> + * ASIterateCallback: Function type called by address_space_iterate
> >>> + *
> >>> + * Return 0 on success or a negative error code.
> >>> + *
> >>> + * @mrs: Memory region section for this range
> >>> + * @opaque: The opaque value passed in to the iterator.
> >>> + */
> >>> +typedef int (*ASIterateCallback)(MemoryRegionSection *mrs, void *opaque);
> >>> +
> >>> +/**
> >>> + * address_space_iterate: Call the function for each address range in the
> >>> + *                        AddressSpace, in sorted order.
> >>> + *
> >>> + * Return 0 on success or a negative error code.
> >>> + *
> >>> + * @as: Address space to iterate over
> >>> + * @cb: Function to call.  If the function returns none-0 the iteration will
> >>> + *     stop.
> >>> + * @opaque: Value to pass to the function
> >>> + */
> >>> +int
> >>> +address_space_iterate(AddressSpace *as, ASIterateCallback cb, void *opaque);
> >>>  #endif
> >>>  
> >>>  #endif
> >>> diff --git a/memory.c b/memory.c
> >>> index e26e5a3b1d..f45137f25e 100644
> >>> --- a/memory.c
> >>> +++ b/memory.c
> >>> @@ -2810,6 +2810,28 @@ void address_space_destroy(AddressSpace *as)
> >>>      call_rcu(as, do_address_space_destroy, rcu);
> >>>  }
> >>>  
> >>> +int address_space_iterate(AddressSpace *as, ASIterateCallback cb,
> >>> +                          void *opaque)
> >>> +{
> >>> +    int res = 0;
> >>> +    FlatView *fv = address_space_to_flatview(as);
> >>> +    FlatRange *range;
> >>> +
> >>> +    flatview_ref(fv);
> >>> +
> >>> +    FOR_EACH_FLAT_RANGE(range, fv) {
> >>> +        MemoryRegionSection mrs = section_from_flat_range(range, fv);
> >>> +        res = cb(&mrs, opaque);
> >>> +        if (res) {
> >>> +            break;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    flatview_unref(fv);
> >>> +
> >>> +    return res;
> >>> +}
> >>> +
> >>>  static const char *memory_region_type(MemoryRegion *mr)
> >>>  {
> >>>      if (memory_region_is_ram_device(mr)) {
> >>>
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 1/8] memory: address_space_iterate

[Paolo Bonzini]

On 12/12/2017 12:03, Dr. David Alan Gilbert wrote:
>> nop means that attributes (readonly, romd_mode, mr+offset_in_region)
>> haven't changed; nop is optionally followed by log_start or log_stop.
>> If any of them changes, you get del+add (del is always before add).
>>
>>> Th nice thing we have here is we get a full walk of the physical memory
>>> in order; keeping it in order makes our data structure easy for merging.
>> That's the same that you get with region_del/add.
> Hmm it would be good if that was documented somewhere.
> I thought you'd only get an _add/_del and then a commit.
> 
> However, what do I do in vhost_dev_start?  At the moment I'm forcfully
> regenerating it using the same code.

When you add a listener, you get adds and a commit for the current state
of the AddressSpace.

Paolo

[Qemu-devel] [PATCH v3 2/8] 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 e4290ce93d..e923219e63 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 v3 3/8] 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 | 74 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e923219e63..c7ce7baf9b 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 -EBUSY;
     }
-    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 -ENOMEM;
     }
-    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;
         }
@@ -633,8 +638,6 @@ 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;
     uint64_t log_size;
     int r;
 
@@ -649,11 +652,16 @@ static void vhost_commit(MemoryListener *listener)
     }
 
     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);
+        int i;
+        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

[Qemu-devel] [PATCH v3 4/8] vhost: New memory update functions

[Dr. David Alan Gilbert (git)]

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

vhost_update_mem will replace the existing update mechanism.
They make use of the Flatview we have now to make the update simpler.
This commit just adds the basic structure.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c7ce7baf9b..6d720036db 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -634,11 +634,51 @@ static void vhost_begin(MemoryListener *listener)
     dev->mem_changed_start_addr = -1;
 }
 
+struct vhost_update_mem_tmp {
+    struct vhost_dev   *dev;
+    uint32_t nregions;
+    struct vhost_memory_region *regions;
+};
+
+/* Called for each MRS from vhost_update_mem */
+static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
+{
+    if (!vhost_section(mrs)) {
+        return 0;
+    }
+
+    /* TODO */
+    return 0;
+}
+
+static int vhost_update_mem(struct vhost_dev *dev, bool *changed)
+{
+    int res;
+    struct vhost_update_mem_tmp vtmp;
+    vtmp.regions = 0;
+    vtmp.nregions = 0;
+    vtmp.dev = dev;
+
+    *changed = false;
+    res = address_space_iterate(&address_space_memory,
+                                vhost_update_mem_cb, &vtmp);
+    if (res) {
+        goto out;
+    }
+
+    /* TODO */
+    *changed = dev->mem_changed_start_addr < dev->mem_changed_end_addr;
+out:
+    g_free(vtmp.regions);
+    return res;
+}
+
 static void vhost_commit(MemoryListener *listener)
 {
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,
                                          memory_listener);
     uint64_t log_size;
+    bool changed;
     int r;
 
     if (!dev->memory_changed) {
@@ -647,7 +687,12 @@ static void vhost_commit(MemoryListener *listener)
     if (!dev->started) {
         return;
     }
-    if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
+    if (vhost_update_mem(dev, &changed)) {
+        return;
+    }
+
+    if (!changed) {
+        /* None of the mappings we care about changed */
         return;
     }
 
@@ -1523,6 +1568,7 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
 int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i, r;
+    bool changed;
 
     /* should only be called after backend is connected */
     assert(hdev->vhost_ops);
@@ -1535,6 +1581,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         goto fail_features;
     }
 
+    if (vhost_update_mem(hdev, &changed)) {
+        goto fail_mem;
+    }
     if (vhost_dev_has_iommu(hdev)) {
         memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
     }
-- 
2.14.3

[Qemu-devel] [PATCH v3 5/8] vhost: update_mem_cb implementation

[Dr. David Alan Gilbert (git)]

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

Add the meat of update_mem_cb;  this is called for each region,
to add a region to our temporary list.
Our temporary list is in order we look to see if this
region can be merged with the current head of list.

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

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 4a493bcd46..92fadec192 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -2,6 +2,8 @@
 
 # hw/virtio/vhost.c
 vhost_section(const char *name, int r) "%s:%d"
+vhost_update_mem_cb(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
+vhost_update_mem_cb_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 
 # 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"
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6d720036db..646a3480c1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -643,11 +643,63 @@ struct vhost_update_mem_tmp {
 /* Called for each MRS from vhost_update_mem */
 static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 {
+    struct vhost_update_mem_tmp *vtmp = opaque;
+    struct vhost_memory_region *cur_vmr;
+    bool need_add = true;
+    uint64_t mrs_size;
+    uint64_t mrs_gpa;
+    uintptr_t mrs_host;
+
     if (!vhost_section(mrs)) {
         return 0;
     }
+    mrs_size = int128_get64(mrs->size);
+    mrs_gpa  = mrs->offset_within_address_space;
+    mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
+                         mrs->offset_within_region;
+
+    trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size, mrs_host);
+
+    if (vtmp->nregions) {
+        /* Since we already have at least one region, 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.
+         */
+        struct vhost_memory_region *prev_vmr = vtmp->regions +
+                                               (vtmp->nregions - 1);
+        uint64_t prev_gpa_start = prev_vmr->guest_phys_addr;
+        uint64_t prev_gpa_end   = range_get_last(prev_gpa_start,
+                                                 prev_vmr->memory_size);
+        uint64_t prev_host_start = prev_vmr->userspace_addr;
+        uint64_t prev_host_end   = range_get_last(prev_host_start,
+                                                  prev_vmr->memory_size);
+
+        if (prev_gpa_end + 1 == mrs_gpa &&
+            prev_host_end + 1 == mrs_host &&
+            (!vtmp->dev->vhost_ops->vhost_backend_can_merge ||
+                vtmp->dev->vhost_ops->vhost_backend_can_merge(vtmp->dev,
+                    mrs_host, mrs_size,
+                    prev_host_start, prev_vmr->memory_size))) {
+            /* The two regions abut */
+            need_add = false;
+            mrs_size = mrs_size + prev_vmr->memory_size;
+            prev_vmr->memory_size = mrs_size;
+            trace_vhost_update_mem_cb_abut(mrs->mr->name, mrs_size);
+        }
+    }
+
+    if (need_add) {
+        vtmp->nregions++;
+        vtmp->regions = g_realloc_n(vtmp->regions, vtmp->nregions,
+                                    sizeof(vtmp->regions[0]));
+        cur_vmr = &vtmp->regions[vtmp->nregions - 1];
+        cur_vmr->guest_phys_addr = mrs_gpa;
+        cur_vmr->memory_size     = mrs_size;
+        cur_vmr->userspace_addr  = mrs_host;
+        cur_vmr->flags_padding = 0;
+    }
 
-    /* TODO */
     return 0;
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH v3 6/8] vhost: Compare and copy updated region data into device state

[Dr. David Alan Gilbert (git)]

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

Compare the temporary region data with the original, and if it's
different update the original in the device state.

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

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 92fadec192..fac89aaba5 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -2,6 +2,8 @@
 
 # hw/virtio/vhost.c
 vhost_section(const char *name, int r) "%s:%d"
+vhost_update_mem(void) ""
+vhost_update_mem_changed(void) ""
 vhost_update_mem_cb(const char *name, uint64_t gpa, uint64_t size, uint64_t host) "%s: 0x%"PRIx64"+0x%"PRIx64" @ 0x%"PRIx64
 vhost_update_mem_cb_abut(const char *name, uint64_t new_size) "%s: 0x%"PRIx64
 
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 646a3480c1..c00d82f96a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -707,10 +707,12 @@ static int vhost_update_mem(struct vhost_dev *dev, bool *changed)
 {
     int res;
     struct vhost_update_mem_tmp vtmp;
+    size_t mem_size;
     vtmp.regions = 0;
     vtmp.nregions = 0;
     vtmp.dev = dev;
 
+    trace_vhost_update_mem();
     *changed = false;
     res = address_space_iterate(&address_space_memory,
                                 vhost_update_mem_cb, &vtmp);
@@ -718,8 +720,21 @@ static int vhost_update_mem(struct vhost_dev *dev, bool *changed)
         goto out;
     }
 
-    /* TODO */
-    *changed = dev->mem_changed_start_addr < dev->mem_changed_end_addr;
+    mem_size = offsetof(struct vhost_memory, regions) +
+               (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
+
+    if (vtmp.nregions != dev->mem->nregions ||
+       memcmp(vtmp.regions, dev->mem->regions, mem_size)) {
+        *changed = true;
+        /* Update the main regions list from our tmp */
+        dev->mem = g_realloc(dev->mem, mem_size);
+        dev->mem->nregions = vtmp.nregions;
+        memcpy(dev->mem->regions, vtmp.regions,
+               vtmp.nregions * sizeof dev->mem->regions[0]);
+        used_memslots = vtmp.nregions;
+        trace_vhost_update_mem_changed();
+    }
+
 out:
     g_free(vtmp.regions);
     return res;
-- 
2.14.3

[Qemu-devel] [PATCH v3 7/8] vhost: Remove old vhost_set_memory etc

[Dr. David Alan Gilbert (git)]

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

Remove the old update mechanism, vhost_set_memory, and the functions
it uses and the memory_changed flags we no longer use.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c00d82f96a..0bf6fb0577 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;
@@ -626,14 +389,6 @@ static bool vhost_section(MemoryRegionSection *section)
     return result;
 }
 
-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;
-}
-
 struct vhost_update_mem_tmp {
     struct vhost_dev   *dev;
     uint32_t nregions;
@@ -748,9 +503,6 @@ static void vhost_commit(MemoryListener *listener)
     bool changed;
     int r;
 
-    if (!dev->memory_changed) {
-        return;
-    }
     if (!dev->started) {
         return;
     }
@@ -781,7 +533,6 @@ static void vhost_commit(MemoryListener *listener)
         if (r < 0) {
             VHOST_OPS_DEBUG("vhost_set_mem_table failed");
         }
-        dev->memory_changed = false;
         return;
     }
     log_size = vhost_get_log_size(dev);
@@ -800,7 +551,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;
 }
 
 static void vhost_region_add(MemoryListener *listener,
@@ -818,7 +568,6 @@ static void vhost_region_add(MemoryListener *listener,
                                 dev->n_mem_sections);
     dev->mem_sections[dev->n_mem_sections - 1] = *section;
     memory_region_ref(section->mr);
-    vhost_set_memory(listener, section, true);
 }
 
 static void vhost_region_del(MemoryListener *listener,
@@ -832,7 +581,6 @@ static void vhost_region_del(MemoryListener *listener,
         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
@@ -1416,7 +1164,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->features = features;
 
     hdev->memory_listener = (MemoryListener) {
-        .begin = vhost_begin,
         .commit = vhost_commit,
         .region_add = vhost_region_add,
         .region_del = vhost_region_del,
@@ -1462,7 +1209,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 467dc7794b..34ace450ab 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -67,9 +67,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

[Qemu-devel] [PATCH v3 8/8] vhost: Move mem_sections maintenance into commit/update routines

[Dr. David Alan Gilbert (git)]

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

Move the maintenance of mem_sections into the vhost_update_mem routines,
this removes the need for the vhost_region_add/del callbacks.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
  (and mostly written by Igor!)

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0bf6fb0577..5d3f921be5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -408,6 +408,13 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
     if (!vhost_section(mrs)) {
         return 0;
     }
+    ++vtmp->dev->n_mem_sections;
+    vtmp->dev->mem_sections = g_renew(MemoryRegionSection,
+                                      vtmp->dev->mem_sections,
+                                      vtmp->dev->n_mem_sections);
+    vtmp->dev->mem_sections[vtmp->dev->n_mem_sections - 1] = *mrs;
+    memory_region_ref(mrs->mr);
+
     mrs_size = int128_get64(mrs->size);
     mrs_gpa  = mrs->offset_within_address_space;
     mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
@@ -461,6 +468,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
 static int vhost_update_mem(struct vhost_dev *dev, bool *changed)
 {
     int res;
+    unsigned i;
     struct vhost_update_mem_tmp vtmp;
     size_t mem_size;
     vtmp.regions = 0;
@@ -469,6 +477,14 @@ static int vhost_update_mem(struct vhost_dev *dev, bool *changed)
 
     trace_vhost_update_mem();
     *changed = false;
+    /* Clear out the section list, it'll get rebuilt */
+    for (i = 0; i < dev->n_mem_sections; i++) {
+        memory_region_unref(dev->mem_sections[i].mr);
+    }
+    g_free(dev->mem_sections);
+    dev->mem_sections = NULL;
+    dev->n_mem_sections = 0;
+
     res = address_space_iterate(&address_space_memory,
                                 vhost_update_mem_cb, &vtmp);
     if (res) {
@@ -553,46 +569,6 @@ static void vhost_commit(MemoryListener *listener)
     }
 }
 
-static void vhost_region_add(MemoryListener *listener,
-                             MemoryRegionSection *section)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-
-    if (!vhost_section(section)) {
-        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);
-}
-
-static void vhost_region_del(MemoryListener *listener,
-                             MemoryRegionSection *section)
-{
-    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
-                                         memory_listener);
-    int i;
-
-    if (!vhost_section(section)) {
-        return;
-    }
-
-    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;
-        }
-    }
-}
-
 static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
 {
     struct vhost_iommu *iommu = container_of(n, struct vhost_iommu, n);
@@ -1165,8 +1141,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 
     hdev->memory_listener = (MemoryListener) {
         .commit = vhost_commit,
-        .region_add = vhost_region_add,
-        .region_del = vhost_region_del,
         .region_nop = vhost_region_nop,
         .log_start = vhost_log_start,
         .log_stop = vhost_log_stop,
-- 
2.14.3