[Qemu-devel] [PATCH v1 0/7] kvm: slot handling fixes (and small cleanups)

[Qemu-devel] [PATCH v1 0/7] kvm: slot handling fixes (and small cleanups)

[David Hildenbrand]

Hopefully this is the last bunch of fixes for the cleanup series.

1. log_start() was called before region_add(), which is wrong.
2. I messed up the calculation of the delta when aligning the ram address
   (maybe we should simply also use ROUND_UP there ...).
3. I think we could get log_start/log_stop/log_sync when we actually
   haven't registered a slot (due to trapping).
4. Some further cleanups

Available on: https://github.com/davidhildenbrand/qemu/commits/kvm_slot

@Joe, if you could retest, this would be great! I updated the branch.

David Hildenbrand (7):
  memory: call log_start after region_add
  kvm: fix alignment of ram address
  kvm: tolerate non-existing slot for log_start/log_stop/log_sync
  kvm: fix error message when failing to unregister slot
  kvm: region_add and region_del is not called on updates
  kvm: simplify kvm_align_section()
  memory: reuse section_from_flat_range()

 accel/kvm/kvm-all.c | 39 ++++++++++++++-------------------------
 memory.c            | 16 +++++-----------
 2 files changed, 19 insertions(+), 36 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v1 1/7] memory: call log_start after region_add

[David Hildenbrand]

It might be confusing for some listener implementations that implement
both, region_add and log_start (e.g. KVM) if we call log_start before an
actual region was added using region_add.

This makes current KVM code trigger an assertion
("kvm_section_update_flags: error finding slot"). So let's just reverse
the order instead of tolerating log_start on yet unknown regions.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/memory.c b/memory.c
index 5e6351a6c1..f39b8592bf 100644
--- a/memory.c
+++ b/memory.c
@@ -2607,12 +2607,12 @@ static void listener_add_address_space(MemoryListener *listener,
             .offset_within_address_space = int128_get64(fr->addr.start),
             .readonly = fr->readonly,
         };
-        if (fr->dirty_log_mask && listener->log_start) {
-            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
-        }
         if (listener->region_add) {
             listener->region_add(listener, &section);
         }
+        if (fr->dirty_log_mask && listener->log_start) {
+            listener->log_start(listener, &section, 0, fr->dirty_log_mask);
+        }
     }
     if (listener->commit) {
         listener->commit(listener);
-- 
2.13.5

[Qemu-devel] [PATCH v1 2/7] kvm: fix alignment of ram address

[David Hildenbrand]

Fix the wrong calculation of the delta, used to align the ram address.

This only strikes if alignment has to be done.

Reported-by: Joe Clifford <joeclifford@gmail.com>
Fixes: 5ea69c2e3614 ("kvm: factor out alignment of memory section")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 90c88b517d..fae1eca983 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -717,8 +717,9 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         return;
     }
 
+    /* use aligned delta to align the ram address */
     ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
-          (section->offset_within_address_space - start_addr);
+          (start_addr - section->offset_within_address_space);
 
     mem = kvm_lookup_matching_slot(kml, start_addr, size);
     if (!add) {
-- 
2.13.5

[Qemu-devel] [PATCH v1 3/7] kvm: tolerate non-existing slot for log_start/log_stop/log_sync

[David Hildenbrand]

If we want to trap every access to a section, we might not have a
slot. So let's just tolerate if we don't have one.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index fae1eca983..f5fa3e24bd 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -394,8 +394,8 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
 
     mem = kvm_lookup_matching_slot(kml, start_addr, size);
     if (!mem) {
-        fprintf(stderr, "%s: error finding slot\n", __func__);
-        abort();
+        /* We don't have a slot if we want to trap every access. */
+        return 0;
     }
 
     return kvm_slot_update_flags(kml, mem, section->mr);
@@ -470,8 +470,8 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
     if (size) {
         mem = kvm_lookup_matching_slot(kml, start_addr, size);
         if (!mem) {
-            fprintf(stderr, "%s: error finding slot\n", __func__);
-            abort();
+            /* We don't have a slot if we want to trap every access. */
+            return 0;
         }
 
         /* XXX bad kernel interface alert
-- 
2.13.5

[Qemu-devel] [PATCH v1 4/7] kvm: fix error message when failing to unregister slot

[David Hildenbrand]

"overlapping" is a leftover, let's drop it.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f5fa3e24bd..559c544501 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -734,7 +734,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         mem->memory_size = 0;
         err = kvm_set_user_memory_region(kml, mem);
         if (err) {
-            fprintf(stderr, "%s: error unregistering overlapping slot: %s\n",
+            fprintf(stderr, "%s: error unregistering slot: %s\n",
                     __func__, strerror(-err));
             abort();
         }
-- 
2.13.5

[Qemu-devel] [PATCH v1 5/7] kvm: region_add and region_del is not called on updates

[David Hildenbrand]

Attributes are not updated via region_add()/region_del(). Attribute changes
lead to a delete first, followed by a new add.

If this would ever not be the case, we would get an error when trying to
register the new slot.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 559c544501..2835bb3801 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -721,8 +721,8 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
     ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
           (start_addr - section->offset_within_address_space);
 
-    mem = kvm_lookup_matching_slot(kml, start_addr, size);
     if (!add) {
+        mem = kvm_lookup_matching_slot(kml, start_addr, size);
         if (!mem) {
             return;
         }
@@ -741,12 +741,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         return;
     }
 
-    if (mem) {
-        /* update the slot */
-        kvm_slot_update_flags(kml, mem, mr);
-        return;
-    }
-
     /* register the new slot */
     mem = kvm_alloc_slot(kml);
     mem->memory_size = size;
-- 
2.13.5

[Qemu-devel] [PATCH v1 6/7] kvm: simplify kvm_align_section()

[David Hildenbrand]

Use ROUND_UP and simplify the code a bit.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 2835bb3801..f290f487a5 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -197,26 +197,20 @@ static hwaddr kvm_align_section(MemoryRegionSection *section,
                                 hwaddr *start)
 {
     hwaddr size = int128_get64(section->size);
-    hwaddr delta;
-
-    *start = section->offset_within_address_space;
+    hwaddr delta, aligned;
 
     /* kvm works in page size chunks, but the function may be called
        with sub-page size and unaligned start address. Pad the start
        address to next and truncate size to previous page boundary. */
-    delta = qemu_real_host_page_size - (*start & ~qemu_real_host_page_mask);
-    delta &= ~qemu_real_host_page_mask;
-    *start += delta;
+    aligned = ROUND_UP(section->offset_within_address_space,
+                       qemu_real_host_page_size);
+    delta = aligned - section->offset_within_address_space;
+    *start = aligned;
     if (delta > size) {
         return 0;
     }
-    size -= delta;
-    size &= qemu_real_host_page_mask;
-    if (*start & ~qemu_real_host_page_mask) {
-        return 0;
-    }
 
-    return size;
+    return (size - delta) & qemu_real_host_page_mask;
 }
 
 int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
-- 
2.13.5

[Qemu-devel] [PATCH v1 7/7] memory: reuse section_from_flat_range()

[David Hildenbrand]

We can use section_from_flat_range() instead of manually initializing.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 memory.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/memory.c b/memory.c
index f39b8592bf..7c8e5878d6 100644
--- a/memory.c
+++ b/memory.c
@@ -2599,14 +2599,8 @@ static void listener_add_address_space(MemoryListener *listener,
 
     view = address_space_get_flatview(as);
     FOR_EACH_FLAT_RANGE(fr, view) {
-        MemoryRegionSection section = {
-            .mr = fr->mr,
-            .fv = view,
-            .offset_within_region = fr->offset_in_region,
-            .size = fr->addr.size,
-            .offset_within_address_space = int128_get64(fr->addr.start),
-            .readonly = fr->readonly,
-        };
+        MemoryRegionSection section = section_from_flat_range(fr, view);
+
         if (listener->region_add) {
             listener->region_add(listener, &section);
         }
-- 
2.13.5

Re: [Qemu-devel] [PATCH v1 0/7] kvm: slot handling fixes (and small cleanups)

[Paolo Bonzini]

On 16/10/2017 16:42, David Hildenbrand wrote:
> Hopefully this is the last bunch of fixes for the cleanup series.
> 
> 1. log_start() was called before region_add(), which is wrong.
> 2. I messed up the calculation of the delta when aligning the ram address
>    (maybe we should simply also use ROUND_UP there ...).
> 3. I think we could get log_start/log_stop/log_sync when we actually
>    haven't registered a slot (due to trapping).
> 4. Some further cleanups
> 
> Available on: https://github.com/davidhildenbrand/qemu/commits/kvm_slot
> 
> @Joe, if you could retest, this would be great! I updated the branch.
> 
> David Hildenbrand (7):
>   memory: call log_start after region_add
>   kvm: fix alignment of ram address
>   kvm: tolerate non-existing slot for log_start/log_stop/log_sync
>   kvm: fix error message when failing to unregister slot
>   kvm: region_add and region_del is not called on updates
>   kvm: simplify kvm_align_section()
>   memory: reuse section_from_flat_range()
> 
>  accel/kvm/kvm-all.c | 39 ++++++++++++++-------------------------
>  memory.c            | 16 +++++-----------
>  2 files changed, 19 insertions(+), 36 deletions(-)
> 

Looks good!  I'll wait for testing results if they come soon.

Paolo

Re: [Qemu-devel] [PATCH v1 0/7] kvm: slot handling fixes (and small cleanups)

[Joe Clifford]

On 16/10/17 17:20, Paolo Bonzini wrote:
> On 16/10/2017 16:42, David Hildenbrand wrote:
>> Hopefully this is the last bunch of fixes for the cleanup series.
>>
>> 1. log_start() was called before region_add(), which is wrong.
>> 2. I messed up the calculation of the delta when aligning the ram address
>>     (maybe we should simply also use ROUND_UP there ...).
>> 3. I think we could get log_start/log_stop/log_sync when we actually
>>     haven't registered a slot (due to trapping).
>> 4. Some further cleanups
>>
>> Available on: https://github.com/davidhildenbrand/qemu/commits/kvm_slot
>>
>> @Joe, if you could retest, this would be great! I updated the branch.
>>
>> David Hildenbrand (7):
>>    memory: call log_start after region_add
>>    kvm: fix alignment of ram address
>>    kvm: tolerate non-existing slot for log_start/log_stop/log_sync
>>    kvm: fix error message when failing to unregister slot
>>    kvm: region_add and region_del is not called on updates
>>    kvm: simplify kvm_align_section()
>>    memory: reuse section_from_flat_range()
>>
>>   accel/kvm/kvm-all.c | 39 ++++++++++++++-------------------------
>>   memory.c            | 16 +++++-----------
>>   2 files changed, 19 insertions(+), 36 deletions(-)
>>
> 
> Looks good!  I'll wait for testing results if they come soon.
> 
> Paolo
> 

Hi,

I've pulled the updates from David's repo and tested the same branch 
(kvm_slot) and they work beautifully for me. Thanks for fixing this David!

Joe