kvm: slot handling fixes (and small cleanups)

PATCH In review David Hildenbrand 3 participants 10 messages Oct 16, 2017 – Oct 17, 2017

lore

Share & tools

lore

David Hildenbrand

author

2017-10-16 14:43 UTC #0 lore raw

cover kvm: slot handling fixes (and small cleanups)

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

Signature · 1 line

2.13.5

David Hildenbrand

author

2017-10-16 14:43 UTC #1 lore raw

PATCH 1/7 memory: call log_start after region_add

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 <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

memory.c +3 -3

index 5e6351a6c1..f39b8592bf 100644
@@ -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);

Signature · 1 line

2.13.5

David Hildenbrand

author

2017-10-16 14:43 UTC #2 lore raw

PATCH 2/7 kvm: fix alignment of ram address

In reply to 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 <[email protected]>

Fixes 5ea69c2e3614 ("kvm: factor out alignment of memory section")

Signed-off-by David Hildenbrand <[email protected]>

---
 accel/kvm/kvm-all.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

accel/kvm/kvm-all.c +2 -1

index 90c88b517d..fae1eca983 100644
@@ -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) {

Signature · 1 line

2.13.5

David Hildenbrand

author

2017-10-16 14:43 UTC #3 lore raw

PATCH 3/7 kvm: tolerate non-existing slot for log_start/log_stop/log_sync

In reply to 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 <[email protected]>

---
 accel/kvm/kvm-all.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

accel/kvm/kvm-all.c +4 -4

index fae1eca983..f5fa3e24bd 100644
@@ -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

Signature · 1 line

2.13.5

David Hildenbrand

author

2017-10-16 14:43 UTC #4 lore raw

PATCH 4/7 kvm: fix error message when failing to unregister slot

In reply to David Hildenbrand

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

Signed-off-by David Hildenbrand <[email protected]>

---
 accel/kvm/kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

accel/kvm/kvm-all.c +1 -1

index f5fa3e24bd..559c544501 100644
@@ -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();
         }

Signature · 1 line

2.13.5

David Hildenbrand

author

2017-10-16 14:43 UTC #5 lore raw

PATCH 5/7 kvm: region_add and region_del is not called on updates

In reply to 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 <[email protected]>

---
 accel/kvm/kvm-all.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

accel/kvm/kvm-all.c +1 -7

index 559c544501..2835bb3801 100644
@@ -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;

Signature · 1 line

2.13.5

David Hildenbrand

author

2017-10-16 14:43 UTC #6 lore raw

PATCH 6/7 kvm: simplify kvm_align_section()

In reply to David Hildenbrand

Use ROUND_UP and simplify the code a bit.

Signed-off-by David Hildenbrand <[email protected]>

---
 accel/kvm/kvm-all.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

accel/kvm/kvm-all.c +6 ~~-12~~

index 2835bb3801..f290f487a5 100644
@@ -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,

Signature · 1 line

2.13.5

David Hildenbrand

author

2017-10-16 14:43 UTC #7 lore raw

PATCH 7/7 memory: reuse section_from_flat_range()

In reply to David Hildenbrand

We can use section_from_flat_range() instead of manually initializing.

Signed-off-by David Hildenbrand <[email protected]>

---
 memory.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

memory.c +2 -8

index f39b8592bf..7c8e5878d6 100644
@@ -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);
         }

Signature · 1 line

2.13.5

Paolo Bonzini

2017-10-16 16:20 UTC #8 lore raw

In reply to David Hildenbrand

Quoted 22 lines · 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()

Quoted 4 lines

 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

Joe Clifford

2017-10-17 00:06 UTC #9 lore raw

Quoted 23 lines · Paolo Bonzini

Quoted 22 lines · 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()

Quoted 9 lines

Quoted 4 lines
  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