[PATCH v1 0/6] QEMU: kvm: cleanup kvm_slot handling

[PATCH v1 0/6] QEMU: kvm: cleanup kvm_slot handling

[David Hildenbrand]

We can heavily simplify the kvm_slot code. Flatview will make sure that we
don't have to deal with overlapping slots. E.g. when a memory section is
resized, we are first notified about the removal and then about the new
memory section.

So basically, we can directly always map one memory section to one
kvm slot (if the fixed up size is > 0).


RFC -> v1:
- minor changes to avoid changing indentation, therefore making it easier
  to review


David Hildenbrand (6):
  kvm: require JOIN_MEMORY_REGIONS_WORKS
  kvm: factor out alignment of memory section
  kvm: use start + size for memory ranges
  kvm: we never have overlapping slots in kvm_set_phys_mem()
  kvm: kvm_log_start/stop are only called with known sections
  kvm: kvm_log_sync() is only called with known memory sections

 accel/kvm/kvm-all.c | 235 +++++++++++++++-------------------------------------
 1 file changed, 68 insertions(+), 167 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v1 0/6] QEMU: kvm: cleanup kvm_slot handling

[David Hildenbrand]

We can heavily simplify the kvm_slot code. Flatview will make sure that we
don't have to deal with overlapping slots. E.g. when a memory section is
resized, we are first notified about the removal and then about the new
memory section.

So basically, we can directly always map one memory section to one
kvm slot (if the fixed up size is > 0).


RFC -> v1:
- minor changes to avoid changing indentation, therefore making it easier
  to review


David Hildenbrand (6):
  kvm: require JOIN_MEMORY_REGIONS_WORKS
  kvm: factor out alignment of memory section
  kvm: use start + size for memory ranges
  kvm: we never have overlapping slots in kvm_set_phys_mem()
  kvm: kvm_log_start/stop are only called with known sections
  kvm: kvm_log_sync() is only called with known memory sections

 accel/kvm/kvm-all.c | 235 +++++++++++++++-------------------------------------
 1 file changed, 68 insertions(+), 167 deletions(-)

-- 
2.13.5

[PATCH v1 1/6] kvm: require JOIN_MEMORY_REGIONS_WORKS

[David Hildenbrand]

We already require DESTROY_MEMORY_REGION_WORKS, JOIN_MEMORY_REGIONS_WORKS
was added just half a year later.

In addition, with flatview overlapping memory regions are first
removed before adding the changed one. So we can't really detect joining
memory regions this way.

Let's just get rid of this special handling.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f85553a851..985b179ab6 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -79,7 +79,6 @@ struct KVMState
     int coalesced_mmio;
     struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
     bool coalesced_flush_in_progress;
-    int broken_set_mem_region;
     int vcpu_events;
     int robust_singlestep;
     int debugregs;
@@ -127,6 +126,7 @@ static bool kvm_immediate_exit;
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
     KVM_CAP_INFO(DESTROY_MEMORY_REGION_WORKS),
+    KVM_CAP_INFO(JOIN_MEMORY_REGIONS_WORKS),
     KVM_CAP_LAST_INFO
 };
 
@@ -696,7 +696,6 @@ kvm_check_extension_list(KVMState *s, const KVMCapabilityInfo *list)
 static void kvm_set_phys_mem(KVMMemoryListener *kml,
                              MemoryRegionSection *section, bool add)
 {
-    KVMState *s = kvm_state;
     KVMSlot *mem, old;
     int err;
     MemoryRegion *mr = section->mr;
@@ -763,35 +762,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
             abort();
         }
 
-        /* Workaround for older KVM versions: we can't join slots, even not by
-         * unregistering the previous ones and then registering the larger
-         * slot. We have to maintain the existing fragmentation. Sigh.
-         *
-         * This workaround assumes that the new slot starts at the same
-         * address as the first existing one. If not or if some overlapping
-         * slot comes around later, we will fail (not seen in practice so far)
-         * - and actually require a recent KVM version. */
-        if (s->broken_set_mem_region &&
-            old.start_addr == start_addr && old.memory_size < size && add) {
-            mem = kvm_alloc_slot(kml);
-            mem->memory_size = old.memory_size;
-            mem->start_addr = old.start_addr;
-            mem->ram = old.ram;
-            mem->flags = kvm_mem_flags(mr);
-
-            err = kvm_set_user_memory_region(kml, mem);
-            if (err) {
-                fprintf(stderr, "%s: error updating slot: %s\n", __func__,
-                        strerror(-err));
-                abort();
-            }
-
-            start_addr += old.memory_size;
-            ram += old.memory_size;
-            size -= old.memory_size;
-            continue;
-        }
-
         /* register prefix slot */
         if (old.start_addr < start_addr) {
             mem = kvm_alloc_slot(kml);
@@ -833,10 +803,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         }
     }
 
-    /* in case the KVM bug workaround already "consumed" the new slot */
-    if (!size) {
-        return;
-    }
     if (!add) {
         return;
     }
@@ -1692,12 +1658,6 @@ static int kvm_init(MachineState *ms)
 
     s->coalesced_mmio = kvm_check_extension(s, KVM_CAP_COALESCED_MMIO);
 
-    s->broken_set_mem_region = 1;
-    ret = kvm_check_extension(s, KVM_CAP_JOIN_MEMORY_REGIONS_WORKS);
-    if (ret > 0) {
-        s->broken_set_mem_region = 0;
-    }
-
 #ifdef KVM_CAP_VCPU_EVENTS
     s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
 #endif
-- 
2.13.5

[Qemu-devel] [PATCH v1 1/6] kvm: require JOIN_MEMORY_REGIONS_WORKS

[David Hildenbrand]

We already require DESTROY_MEMORY_REGION_WORKS, JOIN_MEMORY_REGIONS_WORKS
was added just half a year later.

In addition, with flatview overlapping memory regions are first
removed before adding the changed one. So we can't really detect joining
memory regions this way.

Let's just get rid of this special handling.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f85553a851..985b179ab6 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -79,7 +79,6 @@ struct KVMState
     int coalesced_mmio;
     struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
     bool coalesced_flush_in_progress;
-    int broken_set_mem_region;
     int vcpu_events;
     int robust_singlestep;
     int debugregs;
@@ -127,6 +126,7 @@ static bool kvm_immediate_exit;
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
     KVM_CAP_INFO(DESTROY_MEMORY_REGION_WORKS),
+    KVM_CAP_INFO(JOIN_MEMORY_REGIONS_WORKS),
     KVM_CAP_LAST_INFO
 };
 
@@ -696,7 +696,6 @@ kvm_check_extension_list(KVMState *s, const KVMCapabilityInfo *list)
 static void kvm_set_phys_mem(KVMMemoryListener *kml,
                              MemoryRegionSection *section, bool add)
 {
-    KVMState *s = kvm_state;
     KVMSlot *mem, old;
     int err;
     MemoryRegion *mr = section->mr;
@@ -763,35 +762,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
             abort();
         }
 
-        /* Workaround for older KVM versions: we can't join slots, even not by
-         * unregistering the previous ones and then registering the larger
-         * slot. We have to maintain the existing fragmentation. Sigh.
-         *
-         * This workaround assumes that the new slot starts at the same
-         * address as the first existing one. If not or if some overlapping
-         * slot comes around later, we will fail (not seen in practice so far)
-         * - and actually require a recent KVM version. */
-        if (s->broken_set_mem_region &&
-            old.start_addr == start_addr && old.memory_size < size && add) {
-            mem = kvm_alloc_slot(kml);
-            mem->memory_size = old.memory_size;
-            mem->start_addr = old.start_addr;
-            mem->ram = old.ram;
-            mem->flags = kvm_mem_flags(mr);
-
-            err = kvm_set_user_memory_region(kml, mem);
-            if (err) {
-                fprintf(stderr, "%s: error updating slot: %s\n", __func__,
-                        strerror(-err));
-                abort();
-            }
-
-            start_addr += old.memory_size;
-            ram += old.memory_size;
-            size -= old.memory_size;
-            continue;
-        }
-
         /* register prefix slot */
         if (old.start_addr < start_addr) {
             mem = kvm_alloc_slot(kml);
@@ -833,10 +803,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         }
     }
 
-    /* in case the KVM bug workaround already "consumed" the new slot */
-    if (!size) {
-        return;
-    }
     if (!add) {
         return;
     }
@@ -1692,12 +1658,6 @@ static int kvm_init(MachineState *ms)
 
     s->coalesced_mmio = kvm_check_extension(s, KVM_CAP_COALESCED_MMIO);
 
-    s->broken_set_mem_region = 1;
-    ret = kvm_check_extension(s, KVM_CAP_JOIN_MEMORY_REGIONS_WORKS);
-    if (ret > 0) {
-        s->broken_set_mem_region = 0;
-    }
-
 #ifdef KVM_CAP_VCPU_EVENTS
     s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
 #endif
-- 
2.13.5

[PATCH v1 2/6] kvm: factor out alignment of memory section

[David Hildenbrand]

Factor it out, so we can reuse it later.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 985b179ab6..e0d100bd30 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -190,6 +190,36 @@ static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
 }
 
 /*
+ * Calculate and align the start address and the size of the section.
+ * Return the size. If the size is 0, the aligned section is empty.
+ */
+static hwaddr kvm_align_section(MemoryRegionSection *section,
+                                hwaddr *start)
+{
+    hwaddr size = int128_get64(section->size);
+    hwaddr delta;
+
+    *start = section->offset_within_address_space;
+
+    /* 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;
+    if (delta > size) {
+        return 0;
+    }
+    size -= delta;
+    size &= qemu_real_host_page_mask;
+    if (*start & ~qemu_real_host_page_mask) {
+        return 0;
+    }
+
+    return size;
+}
+
+/*
  * Find overlapping slot with lowest start address
  */
 static KVMSlot *kvm_lookup_overlapping_slot(KVMMemoryListener *kml,
@@ -700,25 +730,8 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
     int err;
     MemoryRegion *mr = section->mr;
     bool writeable = !mr->readonly && !mr->rom_device;
-    hwaddr start_addr = section->offset_within_address_space;
-    ram_addr_t size = int128_get64(section->size);
-    void *ram = NULL;
-    unsigned delta;
-
-    /* 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_addr & ~qemu_real_host_page_mask);
-    delta &= ~qemu_real_host_page_mask;
-    if (delta > size) {
-        return;
-    }
-    start_addr += delta;
-    size -= delta;
-    size &= qemu_real_host_page_mask;
-    if (!size || (start_addr & ~qemu_real_host_page_mask)) {
-        return;
-    }
+    hwaddr start_addr, size;
+    void *ram;
 
     if (!memory_region_is_ram(mr)) {
         if (writeable || !kvm_readonly_mem_allowed) {
@@ -730,7 +743,13 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         }
     }
 
-    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region + delta;
+    size = kvm_align_section(section, &start_addr);
+    if (!size) {
+        return;
+    }
+
+    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
+          (section->offset_within_address_space - start_addr);
 
     while (1) {
         mem = kvm_lookup_overlapping_slot(kml, start_addr, start_addr + size);
-- 
2.13.5

[Qemu-devel] [PATCH v1 2/6] kvm: factor out alignment of memory section

[David Hildenbrand]

Factor it out, so we can reuse it later.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 985b179ab6..e0d100bd30 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -190,6 +190,36 @@ static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
 }
 
 /*
+ * Calculate and align the start address and the size of the section.
+ * Return the size. If the size is 0, the aligned section is empty.
+ */
+static hwaddr kvm_align_section(MemoryRegionSection *section,
+                                hwaddr *start)
+{
+    hwaddr size = int128_get64(section->size);
+    hwaddr delta;
+
+    *start = section->offset_within_address_space;
+
+    /* 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;
+    if (delta > size) {
+        return 0;
+    }
+    size -= delta;
+    size &= qemu_real_host_page_mask;
+    if (*start & ~qemu_real_host_page_mask) {
+        return 0;
+    }
+
+    return size;
+}
+
+/*
  * Find overlapping slot with lowest start address
  */
 static KVMSlot *kvm_lookup_overlapping_slot(KVMMemoryListener *kml,
@@ -700,25 +730,8 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
     int err;
     MemoryRegion *mr = section->mr;
     bool writeable = !mr->readonly && !mr->rom_device;
-    hwaddr start_addr = section->offset_within_address_space;
-    ram_addr_t size = int128_get64(section->size);
-    void *ram = NULL;
-    unsigned delta;
-
-    /* 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_addr & ~qemu_real_host_page_mask);
-    delta &= ~qemu_real_host_page_mask;
-    if (delta > size) {
-        return;
-    }
-    start_addr += delta;
-    size -= delta;
-    size &= qemu_real_host_page_mask;
-    if (!size || (start_addr & ~qemu_real_host_page_mask)) {
-        return;
-    }
+    hwaddr start_addr, size;
+    void *ram;
 
     if (!memory_region_is_ram(mr)) {
         if (writeable || !kvm_readonly_mem_allowed) {
@@ -730,7 +743,13 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         }
     }
 
-    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region + delta;
+    size = kvm_align_section(section, &start_addr);
+    if (!size) {
+        return;
+    }
+
+    ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
+          (section->offset_within_address_space - start_addr);
 
     while (1) {
         mem = kvm_lookup_overlapping_slot(kml, start_addr, start_addr + size);
-- 
2.13.5

[PATCH v1 3/6] kvm: use start + size for memory ranges

[David Hildenbrand]

Convert kvm_lookup_matching_slot().

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e0d100bd30..88b0e631bd 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -172,7 +172,7 @@ static KVMSlot *kvm_alloc_slot(KVMMemoryListener *kml)
 
 static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
                                          hwaddr start_addr,
-                                         hwaddr end_addr)
+                                         hwaddr size)
 {
     KVMState *s = kvm_state;
     int i;
@@ -180,8 +180,7 @@ static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
     for (i = 0; i < s->nr_slots; i++) {
         KVMSlot *mem = &kml->slots[i];
 
-        if (start_addr == mem->start_addr &&
-            end_addr == mem->start_addr + mem->memory_size) {
+        if (start_addr == mem->start_addr && size == mem->memory_size) {
             return mem;
         }
     }
@@ -414,7 +413,7 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
 {
     hwaddr phys_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, phys_addr + size);
+    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
 
     if (mem == NULL)  {
         return 0;
-- 
2.13.5

[Qemu-devel] [PATCH v1 3/6] kvm: use start + size for memory ranges

[David Hildenbrand]

Convert kvm_lookup_matching_slot().

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e0d100bd30..88b0e631bd 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -172,7 +172,7 @@ static KVMSlot *kvm_alloc_slot(KVMMemoryListener *kml)
 
 static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
                                          hwaddr start_addr,
-                                         hwaddr end_addr)
+                                         hwaddr size)
 {
     KVMState *s = kvm_state;
     int i;
@@ -180,8 +180,7 @@ static KVMSlot *kvm_lookup_matching_slot(KVMMemoryListener *kml,
     for (i = 0; i < s->nr_slots; i++) {
         KVMSlot *mem = &kml->slots[i];
 
-        if (start_addr == mem->start_addr &&
-            end_addr == mem->start_addr + mem->memory_size) {
+        if (start_addr == mem->start_addr && size == mem->memory_size) {
             return mem;
         }
     }
@@ -414,7 +413,7 @@ static int kvm_section_update_flags(KVMMemoryListener *kml,
 {
     hwaddr phys_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
-    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, phys_addr + size);
+    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
 
     if (mem == NULL)  {
         return 0;
-- 
2.13.5

[PATCH v1 4/6] kvm: we never have overlapping slots in kvm_set_phys_mem()

[David Hildenbrand]

The way flatview handles memory sections, we will never have overlapping
memory sections in kvm.

address_space_update_topology_pass() will make sure that we will only
get called for

a) an existing memory section for which we only update parameters
(log_start, log_stop).
b) an existing memory section we want to delete (region_del)
c) a brand new memory section we want to add (region_add)

We cannot have overlapping memory sections in kvm as we will first remove
the overlapping sections and then add the ones without conflicts.

Therefore we can remove the complexity for handling prefix and suffix
slots.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 88b0e631bd..b677d1b13e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -725,7 +725,7 @@ kvm_check_extension_list(KVMState *s, const KVMCapabilityInfo *list)
 static void kvm_set_phys_mem(KVMMemoryListener *kml,
                              MemoryRegionSection *section, bool add)
 {
-    KVMSlot *mem, old;
+    KVMSlot *mem;
     int err;
     MemoryRegion *mr = section->mr;
     bool writeable = !mr->readonly && !mr->rom_device;
@@ -750,28 +750,17 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
     ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
           (section->offset_within_address_space - start_addr);
 
-    while (1) {
-        mem = kvm_lookup_overlapping_slot(kml, start_addr, start_addr + size);
+    mem = kvm_lookup_matching_slot(kml, start_addr, size);
+    if (!add) {
         if (!mem) {
-            break;
-        }
-
-        if (add && start_addr >= mem->start_addr &&
-            (start_addr + size <= mem->start_addr + mem->memory_size) &&
-            (ram - start_addr == mem->ram - mem->start_addr)) {
-            /* The new slot fits into the existing one and comes with
-             * identical parameters - update flags and done. */
-            kvm_slot_update_flags(kml, mem, mr);
+            g_assert(!memory_region_is_ram(mr) && !writeable && !mr->romd_mode);
             return;
         }
-
-        old = *mem;
-
         if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
             kvm_physical_sync_dirty_bitmap(kml, section);
         }
 
-        /* unregister the overlapping slot */
+        /* unregister the slot */
         mem->memory_size = 0;
         err = kvm_set_user_memory_region(kml, mem);
         if (err) {
@@ -779,51 +768,16 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
                     __func__, strerror(-err));
             abort();
         }
-
-        /* register prefix slot */
-        if (old.start_addr < start_addr) {
-            mem = kvm_alloc_slot(kml);
-            mem->memory_size = start_addr - old.start_addr;
-            mem->start_addr = old.start_addr;
-            mem->ram = old.ram;
-            mem->flags =  kvm_mem_flags(mr);
-
-            err = kvm_set_user_memory_region(kml, mem);
-            if (err) {
-                fprintf(stderr, "%s: error registering prefix slot: %s\n",
-                        __func__, strerror(-err));
-#ifdef TARGET_PPC
-                fprintf(stderr, "%s: This is probably because your kernel's " \
-                                "PAGE_SIZE is too big. Please try to use 4k " \
-                                "PAGE_SIZE!\n", __func__);
-#endif
-                abort();
-            }
-        }
-
-        /* register suffix slot */
-        if (old.start_addr + old.memory_size > start_addr + size) {
-            ram_addr_t size_delta;
-
-            mem = kvm_alloc_slot(kml);
-            mem->start_addr = start_addr + size;
-            size_delta = mem->start_addr - old.start_addr;
-            mem->memory_size = old.memory_size - size_delta;
-            mem->ram = old.ram + size_delta;
-            mem->flags = kvm_mem_flags(mr);
-
-            err = kvm_set_user_memory_region(kml, mem);
-            if (err) {
-                fprintf(stderr, "%s: error registering suffix slot: %s\n",
-                        __func__, strerror(-err));
-                abort();
-            }
-        }
+        return;
     }
 
-    if (!add) {
+    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;
     mem->start_addr = start_addr;
-- 
2.13.5

[Qemu-devel] [PATCH v1 4/6] kvm: we never have overlapping slots in kvm_set_phys_mem()

[David Hildenbrand]

The way flatview handles memory sections, we will never have overlapping
memory sections in kvm.

address_space_update_topology_pass() will make sure that we will only
get called for

a) an existing memory section for which we only update parameters
(log_start, log_stop).
b) an existing memory section we want to delete (region_del)
c) a brand new memory section we want to add (region_add)

We cannot have overlapping memory sections in kvm as we will first remove
the overlapping sections and then add the ones without conflicts.

Therefore we can remove the complexity for handling prefix and suffix
slots.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 88b0e631bd..b677d1b13e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -725,7 +725,7 @@ kvm_check_extension_list(KVMState *s, const KVMCapabilityInfo *list)
 static void kvm_set_phys_mem(KVMMemoryListener *kml,
                              MemoryRegionSection *section, bool add)
 {
-    KVMSlot *mem, old;
+    KVMSlot *mem;
     int err;
     MemoryRegion *mr = section->mr;
     bool writeable = !mr->readonly && !mr->rom_device;
@@ -750,28 +750,17 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
     ram = memory_region_get_ram_ptr(mr) + section->offset_within_region +
           (section->offset_within_address_space - start_addr);
 
-    while (1) {
-        mem = kvm_lookup_overlapping_slot(kml, start_addr, start_addr + size);
+    mem = kvm_lookup_matching_slot(kml, start_addr, size);
+    if (!add) {
         if (!mem) {
-            break;
-        }
-
-        if (add && start_addr >= mem->start_addr &&
-            (start_addr + size <= mem->start_addr + mem->memory_size) &&
-            (ram - start_addr == mem->ram - mem->start_addr)) {
-            /* The new slot fits into the existing one and comes with
-             * identical parameters - update flags and done. */
-            kvm_slot_update_flags(kml, mem, mr);
+            g_assert(!memory_region_is_ram(mr) && !writeable && !mr->romd_mode);
             return;
         }
-
-        old = *mem;
-
         if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
             kvm_physical_sync_dirty_bitmap(kml, section);
         }
 
-        /* unregister the overlapping slot */
+        /* unregister the slot */
         mem->memory_size = 0;
         err = kvm_set_user_memory_region(kml, mem);
         if (err) {
@@ -779,51 +768,16 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
                     __func__, strerror(-err));
             abort();
         }
-
-        /* register prefix slot */
-        if (old.start_addr < start_addr) {
-            mem = kvm_alloc_slot(kml);
-            mem->memory_size = start_addr - old.start_addr;
-            mem->start_addr = old.start_addr;
-            mem->ram = old.ram;
-            mem->flags =  kvm_mem_flags(mr);
-
-            err = kvm_set_user_memory_region(kml, mem);
-            if (err) {
-                fprintf(stderr, "%s: error registering prefix slot: %s\n",
-                        __func__, strerror(-err));
-#ifdef TARGET_PPC
-                fprintf(stderr, "%s: This is probably because your kernel's " \
-                                "PAGE_SIZE is too big. Please try to use 4k " \
-                                "PAGE_SIZE!\n", __func__);
-#endif
-                abort();
-            }
-        }
-
-        /* register suffix slot */
-        if (old.start_addr + old.memory_size > start_addr + size) {
-            ram_addr_t size_delta;
-
-            mem = kvm_alloc_slot(kml);
-            mem->start_addr = start_addr + size;
-            size_delta = mem->start_addr - old.start_addr;
-            mem->memory_size = old.memory_size - size_delta;
-            mem->ram = old.ram + size_delta;
-            mem->flags = kvm_mem_flags(mr);
-
-            err = kvm_set_user_memory_region(kml, mem);
-            if (err) {
-                fprintf(stderr, "%s: error registering suffix slot: %s\n",
-                        __func__, strerror(-err));
-                abort();
-            }
-        }
+        return;
     }
 
-    if (!add) {
+    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;
     mem->start_addr = start_addr;
-- 
2.13.5

[PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections

[David Hildenbrand]

Let's properly align the sections first and bail out if we would ever
get called with a memory section we don't know yet.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b677d1b13e..2ae459453d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
 static int kvm_section_update_flags(KVMMemoryListener *kml,
                                     MemoryRegionSection *section)
 {
-    hwaddr phys_addr = section->offset_within_address_space;
-    ram_addr_t size = int128_get64(section->size);
-    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
+    hwaddr start_addr, size;
+    KVMSlot *mem;
 
-    if (mem == NULL)  {
+    size = kvm_align_section(section, &start_addr);
+    if (!size) {
         return 0;
-    } else {
-        return kvm_slot_update_flags(kml, mem, section->mr);
     }
+
+    mem = kvm_lookup_matching_slot(kml, start_addr, size);
+    if (!mem) {
+        fprintf(stderr, "%s: error finding slot\n", __func__);
+        abort();
+    }
+
+    return kvm_slot_update_flags(kml, mem, section->mr);
 }
 
 static void kvm_log_start(MemoryListener *listener,
-- 
2.13.5

[Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections

[David Hildenbrand]

Let's properly align the sections first and bail out if we would ever
get called with a memory section we don't know yet.

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b677d1b13e..2ae459453d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
 static int kvm_section_update_flags(KVMMemoryListener *kml,
                                     MemoryRegionSection *section)
 {
-    hwaddr phys_addr = section->offset_within_address_space;
-    ram_addr_t size = int128_get64(section->size);
-    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
+    hwaddr start_addr, size;
+    KVMSlot *mem;
 
-    if (mem == NULL)  {
+    size = kvm_align_section(section, &start_addr);
+    if (!size) {
         return 0;
-    } else {
-        return kvm_slot_update_flags(kml, mem, section->mr);
     }
+
+    mem = kvm_lookup_matching_slot(kml, start_addr, size);
+    if (!mem) {
+        fprintf(stderr, "%s: error finding slot\n", __func__);
+        abort();
+    }
+
+    return kvm_slot_update_flags(kml, mem, section->mr);
 }
 
 static void kvm_log_start(MemoryListener *listener,
-- 
2.13.5

[PATCH v1 6/6] kvm: kvm_log_sync() is only called with known memory sections

[David Hildenbrand]

Flatview will make sure that we can only end up in this function with
memory sections that correspond to exactly one slot. So we don't
have to iterate multiple times. There won't be overlapping slots but
only matching slots.

Properly align the section and look up the corresponding slot. This
heavily simplifies this function.

We can now get rid of kvm_lookup_overlapping_slot().

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 2ae459453d..a8083e80be 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -218,34 +218,6 @@ static hwaddr kvm_align_section(MemoryRegionSection *section,
     return size;
 }
 
-/*
- * Find overlapping slot with lowest start address
- */
-static KVMSlot *kvm_lookup_overlapping_slot(KVMMemoryListener *kml,
-                                            hwaddr start_addr,
-                                            hwaddr end_addr)
-{
-    KVMState *s = kvm_state;
-    KVMSlot *found = NULL;
-    int i;
-
-    for (i = 0; i < s->nr_slots; i++) {
-        KVMSlot *mem = &kml->slots[i];
-
-        if (mem->memory_size == 0 ||
-            (found && found->start_addr < mem->start_addr)) {
-            continue;
-        }
-
-        if (end_addr > mem->start_addr &&
-            start_addr < mem->start_addr + mem->memory_size) {
-            found = mem;
-        }
-    }
-
-    return found;
-}
-
 int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
                                        hwaddr *phys_addr)
 {
@@ -489,18 +461,16 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
                                           MemoryRegionSection *section)
 {
     KVMState *s = kvm_state;
-    unsigned long size, allocated_size = 0;
     struct kvm_dirty_log d = {};
     KVMSlot *mem;
-    int ret = 0;
-    hwaddr start_addr = section->offset_within_address_space;
-    hwaddr end_addr = start_addr + int128_get64(section->size);
+    hwaddr start_addr, size;
 
-    d.dirty_bitmap = NULL;
-    while (start_addr < end_addr) {
-        mem = kvm_lookup_overlapping_slot(kml, start_addr, end_addr);
-        if (mem == NULL) {
-            break;
+    size = kvm_align_section(section, &start_addr);
+    if (size) {
+        mem = kvm_lookup_matching_slot(kml, start_addr, size);
+        if (!mem) {
+            fprintf(stderr, "%s: error finding slot\n", __func__);
+            abort();
         }
 
         /* XXX bad kernel interface alert
@@ -517,27 +487,20 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
          */
         size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
                      /*HOST_LONG_BITS*/ 64) / 8;
-        if (!d.dirty_bitmap) {
-            d.dirty_bitmap = g_malloc(size);
-        } else if (size > allocated_size) {
-            d.dirty_bitmap = g_realloc(d.dirty_bitmap, size);
-        }
-        allocated_size = size;
-        memset(d.dirty_bitmap, 0, allocated_size);
+        d.dirty_bitmap = g_malloc0(size);
 
         d.slot = mem->slot | (kml->as_id << 16);
         if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
             DPRINTF("ioctl failed %d\n", errno);
-            ret = -1;
-            break;
+            g_free(d.dirty_bitmap);
+            return -1;
         }
 
         kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
-        start_addr = mem->start_addr + mem->memory_size;
+        g_free(d.dirty_bitmap);
     }
-    g_free(d.dirty_bitmap);
 
-    return ret;
+    return 0;
 }
 
 static void kvm_coalesce_mmio_region(MemoryListener *listener,
-- 
2.13.5

[Qemu-devel] [PATCH v1 6/6] kvm: kvm_log_sync() is only called with known memory sections

[David Hildenbrand]

Flatview will make sure that we can only end up in this function with
memory sections that correspond to exactly one slot. So we don't
have to iterate multiple times. There won't be overlapping slots but
only matching slots.

Properly align the section and look up the corresponding slot. This
heavily simplifies this function.

We can now get rid of kvm_lookup_overlapping_slot().

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

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 2ae459453d..a8083e80be 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -218,34 +218,6 @@ static hwaddr kvm_align_section(MemoryRegionSection *section,
     return size;
 }
 
-/*
- * Find overlapping slot with lowest start address
- */
-static KVMSlot *kvm_lookup_overlapping_slot(KVMMemoryListener *kml,
-                                            hwaddr start_addr,
-                                            hwaddr end_addr)
-{
-    KVMState *s = kvm_state;
-    KVMSlot *found = NULL;
-    int i;
-
-    for (i = 0; i < s->nr_slots; i++) {
-        KVMSlot *mem = &kml->slots[i];
-
-        if (mem->memory_size == 0 ||
-            (found && found->start_addr < mem->start_addr)) {
-            continue;
-        }
-
-        if (end_addr > mem->start_addr &&
-            start_addr < mem->start_addr + mem->memory_size) {
-            found = mem;
-        }
-    }
-
-    return found;
-}
-
 int kvm_physical_memory_addr_from_host(KVMState *s, void *ram,
                                        hwaddr *phys_addr)
 {
@@ -489,18 +461,16 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
                                           MemoryRegionSection *section)
 {
     KVMState *s = kvm_state;
-    unsigned long size, allocated_size = 0;
     struct kvm_dirty_log d = {};
     KVMSlot *mem;
-    int ret = 0;
-    hwaddr start_addr = section->offset_within_address_space;
-    hwaddr end_addr = start_addr + int128_get64(section->size);
+    hwaddr start_addr, size;
 
-    d.dirty_bitmap = NULL;
-    while (start_addr < end_addr) {
-        mem = kvm_lookup_overlapping_slot(kml, start_addr, end_addr);
-        if (mem == NULL) {
-            break;
+    size = kvm_align_section(section, &start_addr);
+    if (size) {
+        mem = kvm_lookup_matching_slot(kml, start_addr, size);
+        if (!mem) {
+            fprintf(stderr, "%s: error finding slot\n", __func__);
+            abort();
         }
 
         /* XXX bad kernel interface alert
@@ -517,27 +487,20 @@ static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
          */
         size = ALIGN(((mem->memory_size) >> TARGET_PAGE_BITS),
                      /*HOST_LONG_BITS*/ 64) / 8;
-        if (!d.dirty_bitmap) {
-            d.dirty_bitmap = g_malloc(size);
-        } else if (size > allocated_size) {
-            d.dirty_bitmap = g_realloc(d.dirty_bitmap, size);
-        }
-        allocated_size = size;
-        memset(d.dirty_bitmap, 0, allocated_size);
+        d.dirty_bitmap = g_malloc0(size);
 
         d.slot = mem->slot | (kml->as_id << 16);
         if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
             DPRINTF("ioctl failed %d\n", errno);
-            ret = -1;
-            break;
+            g_free(d.dirty_bitmap);
+            return -1;
         }
 
         kvm_get_dirty_pages_log_range(section, d.dirty_bitmap);
-        start_addr = mem->start_addr + mem->memory_size;
+        g_free(d.dirty_bitmap);
     }
-    g_free(d.dirty_bitmap);
 
-    return ret;
+    return 0;
 }
 
 static void kvm_coalesce_mmio_region(MemoryListener *listener,
-- 
2.13.5

Re: [PATCH v1 0/6] QEMU: kvm: cleanup kvm_slot handling

[Paolo Bonzini]

On 11/09/2017 19:49, David Hildenbrand wrote:
> We can heavily simplify the kvm_slot code. Flatview will make sure that we
> don't have to deal with overlapping slots. E.g. when a memory section is
> resized, we are first notified about the removal and then about the new
> memory section.
> 
> So basically, we can directly always map one memory section to one
> kvm slot (if the fixed up size is > 0).
> 
> 
> RFC -> v1:
> - minor changes to avoid changing indentation, therefore making it easier
>   to review

Queued, thanks!

Paolo

> 
> David Hildenbrand (6):
>   kvm: require JOIN_MEMORY_REGIONS_WORKS
>   kvm: factor out alignment of memory section
>   kvm: use start + size for memory ranges
>   kvm: we never have overlapping slots in kvm_set_phys_mem()
>   kvm: kvm_log_start/stop are only called with known sections
>   kvm: kvm_log_sync() is only called with known memory sections
> 
>  accel/kvm/kvm-all.c | 235 +++++++++++++++-------------------------------------
>  1 file changed, 68 insertions(+), 167 deletions(-)
> 

Re: [Qemu-devel] [PATCH v1 0/6] QEMU: kvm: cleanup kvm_slot handling

[Paolo Bonzini]

On 11/09/2017 19:49, David Hildenbrand wrote:
> We can heavily simplify the kvm_slot code. Flatview will make sure that we
> don't have to deal with overlapping slots. E.g. when a memory section is
> resized, we are first notified about the removal and then about the new
> memory section.
> 
> So basically, we can directly always map one memory section to one
> kvm slot (if the fixed up size is > 0).
> 
> 
> RFC -> v1:
> - minor changes to avoid changing indentation, therefore making it easier
>   to review

Queued, thanks!

Paolo

> 
> David Hildenbrand (6):
>   kvm: require JOIN_MEMORY_REGIONS_WORKS
>   kvm: factor out alignment of memory section
>   kvm: use start + size for memory ranges
>   kvm: we never have overlapping slots in kvm_set_phys_mem()
>   kvm: kvm_log_start/stop are only called with known sections
>   kvm: kvm_log_sync() is only called with known memory sections
> 
>  accel/kvm/kvm-all.c | 235 +++++++++++++++-------------------------------------
>  1 file changed, 68 insertions(+), 167 deletions(-)
> 

Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections

[Thomas Huth]

On 11.09.2017 19:49, David Hildenbrand wrote:
> Let's properly align the sections first and bail out if we would ever
> get called with a memory section we don't know yet.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b677d1b13e..2ae459453d 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>                                      MemoryRegionSection *section)
>  {
> -    hwaddr phys_addr = section->offset_within_address_space;
> -    ram_addr_t size = int128_get64(section->size);
> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
> +    hwaddr start_addr, size;
> +    KVMSlot *mem;
>  
> -    if (mem == NULL)  {
> +    size = kvm_align_section(section, &start_addr);
> +    if (!size) {
>          return 0;
> -    } else {
> -        return kvm_slot_update_flags(kml, mem, section->mr);
>      }
> +
> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
> +    if (!mem) {
> +        fprintf(stderr, "%s: error finding slot\n", __func__);
> +        abort();
> +    }

FYI, this abort now triggers with the "isa-vga" device:

$ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
kvm_section_update_flags: error finding slot
Aborted (core dumped)

Any ideas how to fix this?

 Thomas

Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections

[Paolo Bonzini]

On 10/10/2017 11:06, Thomas Huth wrote:
> On 11.09.2017 19:49, David Hildenbrand wrote:
>> Let's properly align the sections first and bail out if we would ever
>> get called with a memory section we don't know yet.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index b677d1b13e..2ae459453d 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>                                      MemoryRegionSection *section)
>>  {
>> -    hwaddr phys_addr = section->offset_within_address_space;
>> -    ram_addr_t size = int128_get64(section->size);
>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>> +    hwaddr start_addr, size;
>> +    KVMSlot *mem;
>>  
>> -    if (mem == NULL)  {
>> +    size = kvm_align_section(section, &start_addr);
>> +    if (!size) {
>>          return 0;
>> -    } else {
>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>      }
>> +
>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>> +    if (!mem) {
>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>> +        abort();
>> +    }
> 
> FYI, this abort now triggers with the "isa-vga" device:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
> kvm_section_update_flags: error finding slot
> Aborted (core dumped)
> 
> Any ideas how to fix this?

Reverting, unless David has some time to look into it.

Paolo

Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections

[David Hildenbrand]

On 10.10.2017 15:29, Paolo Bonzini wrote:
> On 10/10/2017 11:06, Thomas Huth wrote:
>> On 11.09.2017 19:49, David Hildenbrand wrote:
>>> Let's properly align the sections first and bail out if we would ever
>>> get called with a memory section we don't know yet.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>> index b677d1b13e..2ae459453d 100644
>>> --- a/accel/kvm/kvm-all.c
>>> +++ b/accel/kvm/kvm-all.c
>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>>                                      MemoryRegionSection *section)
>>>  {
>>> -    hwaddr phys_addr = section->offset_within_address_space;
>>> -    ram_addr_t size = int128_get64(section->size);
>>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>>> +    hwaddr start_addr, size;
>>> +    KVMSlot *mem;
>>>  
>>> -    if (mem == NULL)  {
>>> +    size = kvm_align_section(section, &start_addr);
>>> +    if (!size) {
>>>          return 0;
>>> -    } else {
>>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>>      }
>>> +
>>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>>> +    if (!mem) {
>>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>>> +        abort();
>>> +    }
>>
>> FYI, this abort now triggers with the "isa-vga" device:
>>
>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>> kvm_section_update_flags: error finding slot
>> Aborted (core dumped)
>>
>> Any ideas how to fix this?
> 
> Reverting, unless David has some time to look into it.
> 
> Paolo
> 

Just returned from vacation, I'll have a look this week.

Thanks for that nice reproducer Thomas, that should help a lot!

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections

[Thomas Huth]

On 16.10.2017 09:16, David Hildenbrand wrote:
> On 10.10.2017 15:29, Paolo Bonzini wrote:
>> On 10/10/2017 11:06, Thomas Huth wrote:
>>> On 11.09.2017 19:49, David Hildenbrand wrote:
>>>> Let's properly align the sections first and bail out if we would ever
>>>> get called with a memory section we don't know yet.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>> index b677d1b13e..2ae459453d 100644
>>>> --- a/accel/kvm/kvm-all.c
>>>> +++ b/accel/kvm/kvm-all.c
>>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>>>                                      MemoryRegionSection *section)
>>>>  {
>>>> -    hwaddr phys_addr = section->offset_within_address_space;
>>>> -    ram_addr_t size = int128_get64(section->size);
>>>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>>>> +    hwaddr start_addr, size;
>>>> +    KVMSlot *mem;
>>>>  
>>>> -    if (mem == NULL)  {
>>>> +    size = kvm_align_section(section, &start_addr);
>>>> +    if (!size) {
>>>>          return 0;
>>>> -    } else {
>>>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>>>      }
>>>> +
>>>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>>>> +    if (!mem) {
>>>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>>>> +        abort();
>>>> +    }
>>>
>>> FYI, this abort now triggers with the "isa-vga" device:
>>>
>>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>>> kvm_section_update_flags: error finding slot
>>> Aborted (core dumped)
>>>
>>> Any ideas how to fix this?
>>
>> Reverting, unless David has some time to look into it.
>>
>> Paolo
>>
> 
> Just returned from vacation, I'll have a look this week.
> 
> Thanks for that nice reproducer Thomas, that should help a lot!

FWIW, I've found the problem with the scripts/device-crash-test script,
so you might want to run that, too, before submitting a new version.

 Thomas

Re: [Qemu-devel] [PATCH v1 5/6] kvm: kvm_log_start/stop are only called with known sections

[David Hildenbrand]

On 16.10.2017 10:52, Thomas Huth wrote:
> On 16.10.2017 09:16, David Hildenbrand wrote:
>> On 10.10.2017 15:29, Paolo Bonzini wrote:
>>> On 10/10/2017 11:06, Thomas Huth wrote:
>>>> On 11.09.2017 19:49, David Hildenbrand wrote:
>>>>> Let's properly align the sections first and bail out if we would ever
>>>>> get called with a memory section we don't know yet.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>  accel/kvm/kvm-all.c | 18 ++++++++++++------
>>>>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>>> index b677d1b13e..2ae459453d 100644
>>>>> --- a/accel/kvm/kvm-all.c
>>>>> +++ b/accel/kvm/kvm-all.c
>>>>> @@ -411,15 +411,21 @@ static int kvm_slot_update_flags(KVMMemoryListener *kml, KVMSlot *mem,
>>>>>  static int kvm_section_update_flags(KVMMemoryListener *kml,
>>>>>                                      MemoryRegionSection *section)
>>>>>  {
>>>>> -    hwaddr phys_addr = section->offset_within_address_space;
>>>>> -    ram_addr_t size = int128_get64(section->size);
>>>>> -    KVMSlot *mem = kvm_lookup_matching_slot(kml, phys_addr, size);
>>>>> +    hwaddr start_addr, size;
>>>>> +    KVMSlot *mem;
>>>>>  
>>>>> -    if (mem == NULL)  {
>>>>> +    size = kvm_align_section(section, &start_addr);
>>>>> +    if (!size) {
>>>>>          return 0;
>>>>> -    } else {
>>>>> -        return kvm_slot_update_flags(kml, mem, section->mr);
>>>>>      }
>>>>> +
>>>>> +    mem = kvm_lookup_matching_slot(kml, start_addr, size);
>>>>> +    if (!mem) {
>>>>> +        fprintf(stderr, "%s: error finding slot\n", __func__);
>>>>> +        abort();
>>>>> +    }
>>>>
>>>> FYI, this abort now triggers with the "isa-vga" device:
>>>>
>>>> $ x86_64-softmmu/qemu-system-x86_64 -S -accel kvm -device isa-vga
>>>> kvm_section_update_flags: error finding slot
>>>> Aborted (core dumped)
>>>>
>>>> Any ideas how to fix this?
>>>
>>> Reverting, unless David has some time to look into it.
>>>
>>> Paolo
>>>
>>
>> Just returned from vacation, I'll have a look this week.
>>
>> Thanks for that nice reproducer Thomas, that should help a lot!
> 
> FWIW, I've found the problem with the scripts/device-crash-test script,
> so you might want to run that, too, before submitting a new version.
> 
>  Thomas
> 

This one was easy to fix, there was just one place where the asssumption
"only called with known sections" was wrong.

See "memory: call log_start after region_add"

-- 

Thanks,

David