[PATCH RFC 0/4] kvm: Implement atomic memory region resizes

[PATCH RFC 0/4] kvm: Implement atomic memory region resizes

[David Hildenbrand]

Currently, when doing a
    memory_region_ram_resize() -> memory_region_set_size()

the old KVM slot will first get removed and the new, resized one, will be
re-added. This is fine as long as no IOCTL is currently using any data from
such a memory slot (e.g., when building ACPI tables). However, if e.g., a
VCPU is in KVM_RUN and tries to access any data on such a slot while we're
growing it, we will get wrong faults while the slot is temporarily removed.

Let's allow to resize memory regions while the guest is running and might
be using the regions. Inhibit any KVM ioctl while we are replacing the
memory slot(s).

This is a preparation for virtio-mem (initially, x86-64 only), which wants
to resize (esp. grow) ram memory regions while the guest is running via
memory_region_ram_resize().

Using pause_all_vcpus()/resume_all_vcpus() is not possible, as it will
temporarily drop the BQL - something most callers can't handle (esp.
when called from vcpu context e.g., in virtio code).

Once we can handle resizes in the kernel (e.g., via
KVM_SET_USER_MEMORY_REGION), we can make inhibiting optional at runtime.

Instead of inhibiting during the region_resize(), we could inhibit for the
hole memory transaction (from begin() to commit()). This could be nice,
because also splitting of memory regions would be atomic (I remember there
was a BUG report regarding that), however, I am not sure if that might
impact any RT users.

Tested so far with x86-64 KVM only. Thoughts? Anything important I am
missing? Any alternatives that don't require kernel changes?

David Hildenbrand (4):
  openpic_kvm: Use kvm_device_ioctl() instead of ioctl()
  intc/s390_flic_kvm.c: Use kvm_device_ioctl() instead of ioctl()
  memory: Add region_resize() callback to memory notifier
  kvm: Implement atomic memory region resizes via region_resize()

 accel/kvm/kvm-all.c     | 121 +++++++++++++++++++++++++++++++++++++---
 hw/intc/openpic_kvm.c   |   8 +--
 hw/intc/s390_flic_kvm.c |  22 ++++----
 include/exec/memory.h   |  18 ++++++
 include/hw/core/cpu.h   |   3 +
 memory.c                |  72 ++++++++++++++++++++++--
 6 files changed, 217 insertions(+), 27 deletions(-)

-- 
2.24.1

[PATCH RFC 1/4] openpic_kvm: Use kvm_device_ioctl() instead of ioctl()

[David Hildenbrand]

Let's use the official variant, which will e.g., trace the call.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/intc/openpic_kvm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index e4bf47d885..0d8abc1604 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -70,7 +70,7 @@ static void kvm_openpic_write(void *opaque, hwaddr addr, uint64_t val,
     attr.attr = addr;
     attr.addr = (uint64_t)(unsigned long)&val32;
 
-    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
+    ret = kvm_device_ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
     if (ret < 0) {
         qemu_log_mask(LOG_UNIMP, "%s: %s %" PRIx64 "\n", __func__,
                       strerror(errno), attr.attr);
@@ -96,7 +96,7 @@ static uint64_t kvm_openpic_read(void *opaque, hwaddr addr, unsigned size)
     attr.attr = addr;
     attr.addr = (uint64_t)(unsigned long)&val;
 
-    ret = ioctl(opp->fd, KVM_GET_DEVICE_ATTR, &attr);
+    ret = kvm_device_ioctl(opp->fd, KVM_GET_DEVICE_ATTR, &attr);
     if (ret < 0) {
         qemu_log_mask(LOG_UNIMP, "%s: %s %" PRIx64 "\n", __func__,
                       strerror(errno), attr.attr);
@@ -145,7 +145,7 @@ static void kvm_openpic_region_add(MemoryListener *listener,
     attr.attr = KVM_DEV_MPIC_BASE_ADDR;
     attr.addr = (uint64_t)(unsigned long)&reg_base;
 
-    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
+    ret = kvm_device_ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
     if (ret < 0) {
         fprintf(stderr, "%s: %s %" PRIx64 "\n", __func__,
                 strerror(errno), reg_base);
@@ -179,7 +179,7 @@ static void kvm_openpic_region_del(MemoryListener *listener,
     attr.attr = KVM_DEV_MPIC_BASE_ADDR;
     attr.addr = (uint64_t)(unsigned long)&reg_base;
 
-    ret = ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
+    ret = kvm_device_ioctl(opp->fd, KVM_SET_DEVICE_ATTR, &attr);
     if (ret < 0) {
         fprintf(stderr, "%s: %s %" PRIx64 "\n", __func__,
                 strerror(errno), reg_base);
-- 
2.24.1

[PATCH RFC 2/4] intc/s390_flic_kvm.c: Use kvm_device_ioctl() instead of ioctl()

[David Hildenbrand]

Let's use the official variant, which will e.g., trace the call.

Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: qemu-s390x@nongnu.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/intc/s390_flic_kvm.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index a306b26faa..5151582ba0 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -68,7 +68,7 @@ static int flic_get_all_irqs(KVMS390FLICState *flic,
     };
     int rc;
 
-    rc = ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr);
+    rc = kvm_device_ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr);
 
     return rc == -1 ? -errno : rc;
 }
@@ -80,7 +80,7 @@ static void flic_enable_pfault(KVMS390FLICState *flic)
     };
     int rc;
 
-    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
+    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
 
     if (rc) {
         fprintf(stderr, "flic: couldn't enable pfault\n");
@@ -94,7 +94,7 @@ static void flic_disable_wait_pfault(KVMS390FLICState *flic)
     };
     int rc;
 
-    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
+    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
 
     if (rc) {
         fprintf(stderr, "flic: couldn't disable pfault\n");
@@ -118,7 +118,7 @@ static int flic_enqueue_irqs(void *buf, uint64_t len,
         .attr = len,
     };
 
-    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
+    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
 
     return rc ? -errno : 0;
 }
@@ -197,7 +197,7 @@ static int kvm_s390_clear_io_flic(S390FLICState *fs, uint16_t subchannel_id,
     if (unlikely(!flic->clear_io_supported)) {
         return -ENOSYS;
     }
-    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
+    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
     return rc ? -errno : 0;
 }
 
@@ -218,7 +218,7 @@ static int kvm_s390_modify_ais_mode(S390FLICState *fs, uint8_t isc,
         return -ENOSYS;
     }
 
-    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
+    return kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
 }
 
 static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
@@ -235,7 +235,7 @@ static int kvm_s390_inject_airq(S390FLICState *fs, uint8_t type,
         return -ENOSYS;
     }
 
-    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
+    return kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
 }
 
 /**
@@ -296,7 +296,7 @@ static int kvm_s390_register_io_adapter(S390FLICState *fs, uint32_t id,
         return 0;
     }
 
-    r = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
+    r = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
 
     return r ? -errno : 0;
 }
@@ -321,7 +321,7 @@ static int kvm_s390_io_adapter_map(S390FLICState *fs, uint32_t id,
         return 0;
     }
 
-    r = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
+    r = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
     return r ? -errno : 0;
 }
 
@@ -519,7 +519,7 @@ static int kvm_flic_ais_post_load(void *opaque, int version_id)
         return -ENOSYS;
     }
 
-    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
+    return kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
 }
 
 static const VMStateDescription kvm_s390_flic_ais_tmp = {
@@ -636,7 +636,7 @@ static void kvm_s390_flic_reset(DeviceState *dev)
         }
     }
 
-    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
+    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
     if (rc) {
         trace_flic_reset_failed(errno);
     }
-- 
2.24.1

[PATCH RFC 3/4] memory: Add region_resize() callback to memory notifier

[David Hildenbrand]

Let's provide a way for memory notifiers to get notified about a resize.
If the region_resize() callback is not implemented by a notifier, we
mimic the old behavior by removing the old section and adding the
new, resized section.

This callback is helpful when backends (like KVM) want to implement
atomic resizes of memory sections (e.g., resize while VCPUs are running and
using the section).

For now, we won't bother about a resize() callback for coalesced MMIO
regions. The main future use case is for virtio-mem to resize ram memory
regions while the guest is running.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/memory.h | 18 +++++++++++
 memory.c              | 72 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 85 insertions(+), 5 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 74805dd448..704eb64d75 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -492,6 +492,24 @@ struct MemoryListener {
      */
     void (*region_nop)(MemoryListener *listener, MemoryRegionSection *section);
 
+    /**
+     * @region_resize:
+     *
+     * Called during an address space update transaction,
+     * for a section of the address space that is in the same place in the
+     * address space as in the last transaction, however, the size changed.
+     * Dirty memory logging can change as well.
+     *
+     * Note: If this callback is not implemented, the resize is communicated
+     *       via a region_del(), followed by a region_add() instead.
+     *
+     * @listener: The #MemoryListener.
+     * @section: The old #MemoryRegionSection.
+     * @new: The new size.
+     */
+    void (*region_resize)(MemoryListener *listener,
+                          MemoryRegionSection *section, Int128 new);
+
     /**
      * @log_start:
      *
diff --git a/memory.c b/memory.c
index 09be40edd2..097b0db0e4 100644
--- a/memory.c
+++ b/memory.c
@@ -246,6 +246,17 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b)
         && a->nonvolatile == b->nonvolatile;
 }
 
+static bool flatrange_resized(FlatRange *a, FlatRange *b)
+{
+    return a->mr == b->mr
+        && int128_eq(a->addr.start, b->addr.start)
+        && int128_ne(a->addr.size, b->addr.size)
+        && a->offset_in_region == b->offset_in_region
+        && a->romd_mode == b->romd_mode
+        && a->readonly == b->readonly
+        && a->nonvolatile == b->nonvolatile;
+}
+
 static FlatView *flatview_new(MemoryRegion *mr_root)
 {
     FlatView *view;
@@ -887,6 +898,45 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
     }
 }
 
+static void memory_listener_resize_region(FlatRange *fr, AddressSpace *as,
+                                          Int128 new, bool adding)
+{
+    FlatView *as_view = address_space_to_flatview(as);
+    MemoryRegionSection old_mrs = section_from_flat_range(fr, as_view);
+    MemoryRegionSection new_mrs = old_mrs;
+    MemoryListener *listener;
+
+    new_mrs.size = new;
+
+    if (adding) {
+        QTAILQ_FOREACH(listener, &as->listeners, link_as) {
+            if (listener->region_resize) {
+                /* Grow in the adding phase. */
+                if (int128_lt(fr->addr.size, new)) {
+                    listener->region_resize(listener, &old_mrs, new);
+                }
+                continue;
+            }
+            if (listener->region_add) {
+                listener->region_add(listener, &new_mrs);
+            }
+        }
+    } else {
+        QTAILQ_FOREACH_REVERSE(listener, &as->listeners, link_as) {
+            if (listener->region_resize) {
+                /* Shrink in the removal phase. */
+                if (int128_gt(fr->addr.size, new)) {
+                    listener->region_resize(listener, &old_mrs, new);
+                }
+                continue;
+            }
+            if (listener->region_del) {
+                listener->region_del(listener, &old_mrs);
+            }
+        }
+    }
+}
+
 static void address_space_update_topology_pass(AddressSpace *as,
                                                const FlatView *old_view,
                                                const FlatView *new_view,
@@ -911,11 +961,23 @@ static void address_space_update_topology_pass(AddressSpace *as,
             frnew = NULL;
         }
 
-        if (frold
-            && (!frnew
-                || int128_lt(frold->addr.start, frnew->addr.start)
-                || (int128_eq(frold->addr.start, frnew->addr.start)
-                    && !flatrange_equal(frold, frnew)))) {
+        if (frold && frnew && flatrange_resized(frold, frnew)) {
+            /* In both and only the size (+ eventually logging) changed. */
+
+            memory_listener_resize_region(frold, as, frnew->addr.size,
+                                          adding);
+            if (adding) {
+                flat_range_coalesced_io_add(frnew, as);
+            } else {
+                flat_range_coalesced_io_del(frold, as);
+            }
+
+            ++iold;
+            ++inew;
+        } else if (frold && (!frnew
+                             || int128_lt(frold->addr.start, frnew->addr.start)
+                             || (int128_eq(frold->addr.start, frnew->addr.start)
+                                 && !flatrange_equal(frold, frnew)))) {
             /* In old but not in new, or in both but attributes changed. */
 
             if (!adding) {
-- 
2.24.1

[PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

virtio-mem wants to resize (esp. grow) ram memory regions while the guest
is already aware of them and makes use of them. Resizing a KVM slot can
only currently be done by removing it and re-adding it. While the kvm slot
is temporarily removed, VCPUs that try to read from these slots will fault.
But also, other ioctls might depend on all slots being in place.

Let's inhibit most KVM ioctls while performing the resize. Once we have an
ioctl that can perform atomic resizes (e.g., KVM_SET_USER_MEMORY_REGION
extensions), we can make inhibiting optional at runtime.

Also, make sure to hold the kvm_slots_lock while performing both
actions (removing+re-adding).

Note: Resizes of memory regions currently seems to happen during bootup
only, so I don't think any existing RT users should be affected.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c   | 121 +++++++++++++++++++++++++++++++++++++++---
 include/hw/core/cpu.h |   3 ++
 2 files changed, 117 insertions(+), 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 439a4efe52..bba58db098 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -149,6 +149,21 @@ bool kvm_msi_use_devid;
 static bool kvm_immediate_exit;
 static hwaddr kvm_max_slot_size = ~0;
 
+/*
+ * While holding this lock in write, no new KVM ioctls can be started, but
+ * kvm ioctl inhibitors will have to wait for existing ones to finish
+ * (indicated by cpu->in_ioctl and kvm_in_ioctl, both updated with this lock
+ * held in read when entering the ioctl).
+ */
+pthread_rwlock_t kvm_ioctl_lock;
+/*
+ * Atomic counter of active KVM ioctls except
+ * - The KVM ioctl inhibitor is doing an ioctl
+ * - kvm_ioctl(): Harmless and not interesting for inhibitors.
+ * - kvm_vcpu_ioctl(): Tracked via cpu->in_ioctl.
+ */
+static int kvm_in_ioctl;
+
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
     KVM_CAP_INFO(DESTROY_MEMORY_REGION_WORKS),
@@ -1023,6 +1038,7 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size)
     kvm_max_slot_size = max_slot_size;
 }
 
+/* Called with KVMMemoryListener.slots_lock held */
 static void kvm_set_phys_mem(KVMMemoryListener *kml,
                              MemoryRegionSection *section, bool add)
 {
@@ -1052,14 +1068,12 @@ 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);
 
-    kvm_slots_lock(kml);
-
     if (!add) {
         do {
             slot_size = MIN(kvm_max_slot_size, size);
             mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
             if (!mem) {
-                goto out;
+                return;
             }
             if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
                 kvm_physical_sync_dirty_bitmap(kml, section);
@@ -1079,7 +1093,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
             start_addr += slot_size;
             size -= slot_size;
         } while (size);
-        goto out;
+        return;
     }
 
     /* register the new slot */
@@ -1108,9 +1122,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         ram += slot_size;
         size -= slot_size;
     } while (size);
-
-out:
-    kvm_slots_unlock(kml);
 }
 
 static void kvm_region_add(MemoryListener *listener,
@@ -1119,7 +1130,9 @@ static void kvm_region_add(MemoryListener *listener,
     KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
 
     memory_region_ref(section->mr);
+    kvm_slots_lock(kml);
     kvm_set_phys_mem(kml, section, true);
+    kvm_slots_unlock(kml);
 }
 
 static void kvm_region_del(MemoryListener *listener,
@@ -1127,10 +1140,68 @@ static void kvm_region_del(MemoryListener *listener,
 {
     KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
 
+    kvm_slots_lock(kml);
     kvm_set_phys_mem(kml, section, false);
+    kvm_slots_unlock(kml);
     memory_region_unref(section->mr);
 }
 
+/*
+ * Certain updates (e.g., resizing memory regions) require temporarily removing
+ * kvm memory slots. Make sure any ioctl sees a consistent memory slot state.
+ */
+static void kvm_ioctl_inhibit_begin(void)
+{
+    CPUState *cpu;
+
+    /*
+     * We allow to inhibit only when holding the BQL, so we can identify
+     * when an inhibitor wants to issue an ioctl easily.
+     */
+    g_assert(qemu_mutex_iothread_locked());
+
+    pthread_rwlock_wrlock(&kvm_ioctl_lock);
+
+    /* Inhibiting happens rarely, we can keep things simple and spin here. */
+    while (true) {
+        bool any_cpu_in_ioctl = false;
+
+        CPU_FOREACH(cpu) {
+            if (atomic_read(&cpu->in_ioctl)) {
+                any_cpu_in_ioctl = true;
+                qemu_cpu_kick(cpu);
+            }
+        }
+        if (!any_cpu_in_ioctl && !atomic_read(&kvm_in_ioctl)) {
+            break;
+        }
+        g_usleep(100);
+    }
+}
+
+static void kvm_ioctl_inhibit_end(void)
+{
+    pthread_rwlock_unlock(&kvm_ioctl_lock);
+}
+
+static void kvm_region_resize(MemoryListener *listener,
+                              MemoryRegionSection *section, Int128 new)
+{
+    KVMMemoryListener *kml = container_of(listener, KVMMemoryListener,
+                                          listener);
+    MemoryRegionSection new_section = *section;
+
+    new_section.size = new;
+
+    kvm_slots_lock(kml);
+    /* Inhibit KVM ioctls while temporarily removing slots. */
+    kvm_ioctl_inhibit_begin();
+    kvm_set_phys_mem(kml, section, false);
+    kvm_set_phys_mem(kml, &new_section, true);
+    kvm_ioctl_inhibit_end();
+    kvm_slots_unlock(kml);
+}
+
 static void kvm_log_sync(MemoryListener *listener,
                          MemoryRegionSection *section)
 {
@@ -1249,6 +1320,7 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
 
     kml->listener.region_add = kvm_region_add;
     kml->listener.region_del = kvm_region_del;
+    kml->listener.region_resize = kvm_region_resize;
     kml->listener.log_start = kvm_log_start;
     kml->listener.log_stop = kvm_log_stop;
     kml->listener.log_sync = kvm_log_sync;
@@ -1894,6 +1966,7 @@ static int kvm_init(MachineState *ms)
     assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
 
     s->sigmask_len = 8;
+    pthread_rwlock_init(&kvm_ioctl_lock, NULL);
 
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     QTAILQ_INIT(&s->kvm_sw_breakpoints);
@@ -2304,6 +2377,34 @@ static void kvm_eat_signals(CPUState *cpu)
     } while (sigismember(&chkset, SIG_IPI));
 }
 
+static void kvm_cpu_set_in_ioctl(CPUState *cpu, bool in_ioctl)
+{
+    if (unlikely(qemu_mutex_iothread_locked())) {
+        return;
+    }
+    if (in_ioctl) {
+        pthread_rwlock_rdlock(&kvm_ioctl_lock);
+        atomic_set(&cpu->in_ioctl, true);
+        pthread_rwlock_unlock(&kvm_ioctl_lock);
+    } else {
+        atomic_set(&cpu->in_ioctl, false);
+    }
+}
+
+static void kvm_set_in_ioctl(bool in_ioctl)
+{
+    if (likely(qemu_mutex_iothread_locked())) {
+        return;
+    }
+    if (in_ioctl) {
+        pthread_rwlock_rdlock(&kvm_ioctl_lock);
+        atomic_inc(&kvm_in_ioctl);
+        pthread_rwlock_unlock(&kvm_ioctl_lock);
+    } else {
+        atomic_dec(&kvm_in_ioctl);
+    }
+}
+
 int kvm_cpu_exec(CPUState *cpu)
 {
     struct kvm_run *run = cpu->kvm_run;
@@ -2488,7 +2589,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
     va_end(ap);
 
     trace_kvm_vm_ioctl(type, arg);
+    kvm_set_in_ioctl(true);
     ret = ioctl(s->vmfd, type, arg);
+    kvm_set_in_ioctl(false);
     if (ret == -1) {
         ret = -errno;
     }
@@ -2506,7 +2609,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
     va_end(ap);
 
     trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
+    kvm_cpu_set_in_ioctl(cpu, true);
     ret = ioctl(cpu->kvm_fd, type, arg);
+    kvm_cpu_set_in_ioctl(cpu, false);
     if (ret == -1) {
         ret = -errno;
     }
@@ -2524,7 +2629,9 @@ int kvm_device_ioctl(int fd, int type, ...)
     va_end(ap);
 
     trace_kvm_device_ioctl(fd, type, arg);
+    kvm_set_in_ioctl(true);
     ret = ioctl(fd, type, arg);
+    kvm_set_in_ioctl(false);
     if (ret == -1) {
         ret = -errno;
     }
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 73e9a869a4..4fbff6f3d7 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -431,6 +431,9 @@ struct CPUState {
     /* shared by kvm, hax and hvf */
     bool vcpu_dirty;
 
+    /* kvm only for now: CPU is in kvm_vcpu_ioctl() (esp. KVM_RUN) */
+    bool in_ioctl;
+
     /* Used to keep track of an outstanding cpu throttle thread for migration
      * autoconverge
      */
-- 
2.24.1

[PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

virtio-mem wants to resize (esp. grow) ram memory regions while the guest
is already aware of them and makes use of them. Resizing a KVM slot can
only currently be done by removing it and re-adding it. While the kvm slot
is temporarily removed, VCPUs that try to read from these slots will fault.
But also, other ioctls might depend on all slots being in place.

Let's inhibit most KVM ioctls while performing the resize. Once we have an
ioctl that can perform atomic resizes (e.g., KVM_SET_USER_MEMORY_REGION
extensions), we can make inhibiting optional at runtime.

Also, make sure to hold the kvm_slots_lock while performing both
actions (removing+re-adding).

Note: Resizes of memory regions currently seems to happen during bootup
only, so I don't think any existing RT users should be affected.

Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/kvm/kvm-all.c   | 121 +++++++++++++++++++++++++++++++++++++++---
 include/hw/core/cpu.h |   3 ++
 2 files changed, 117 insertions(+), 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 439a4efe52..bba58db098 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -149,6 +149,21 @@ bool kvm_msi_use_devid;
 static bool kvm_immediate_exit;
 static hwaddr kvm_max_slot_size = ~0;
 
+/*
+ * While holding this lock in write, no new KVM ioctls can be started, but
+ * kvm ioctl inhibitors will have to wait for existing ones to finish
+ * (indicated by cpu->in_ioctl and kvm_in_ioctl, both updated with this lock
+ * held in read when entering the ioctl).
+ */
+pthread_rwlock_t kvm_ioctl_lock;
+/*
+ * Atomic counter of active KVM ioctls except
+ * - The KVM ioctl inhibitor is doing an ioctl
+ * - kvm_ioctl(): Harmless and not interesting for inhibitors.
+ * - kvm_vcpu_ioctl(): Tracked via cpu->in_ioctl.
+ */
+static int kvm_in_ioctl;
+
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
     KVM_CAP_INFO(DESTROY_MEMORY_REGION_WORKS),
@@ -1023,6 +1038,7 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size)
     kvm_max_slot_size = max_slot_size;
 }
 
+/* Called with KVMMemoryListener.slots_lock held */
 static void kvm_set_phys_mem(KVMMemoryListener *kml,
                              MemoryRegionSection *section, bool add)
 {
@@ -1052,14 +1068,12 @@ 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);
 
-    kvm_slots_lock(kml);
-
     if (!add) {
         do {
             slot_size = MIN(kvm_max_slot_size, size);
             mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
             if (!mem) {
-                goto out;
+                return;
             }
             if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
                 kvm_physical_sync_dirty_bitmap(kml, section);
@@ -1079,7 +1093,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
             start_addr += slot_size;
             size -= slot_size;
         } while (size);
-        goto out;
+        return;
     }
 
     /* register the new slot */
@@ -1108,9 +1122,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
         ram += slot_size;
         size -= slot_size;
     } while (size);
-
-out:
-    kvm_slots_unlock(kml);
 }
 
 static void kvm_region_add(MemoryListener *listener,
@@ -1119,7 +1130,9 @@ static void kvm_region_add(MemoryListener *listener,
     KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
 
     memory_region_ref(section->mr);
+    kvm_slots_lock(kml);
     kvm_set_phys_mem(kml, section, true);
+    kvm_slots_unlock(kml);
 }
 
 static void kvm_region_del(MemoryListener *listener,
@@ -1127,10 +1140,68 @@ static void kvm_region_del(MemoryListener *listener,
 {
     KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
 
+    kvm_slots_lock(kml);
     kvm_set_phys_mem(kml, section, false);
+    kvm_slots_unlock(kml);
     memory_region_unref(section->mr);
 }
 
+/*
+ * Certain updates (e.g., resizing memory regions) require temporarily removing
+ * kvm memory slots. Make sure any ioctl sees a consistent memory slot state.
+ */
+static void kvm_ioctl_inhibit_begin(void)
+{
+    CPUState *cpu;
+
+    /*
+     * We allow to inhibit only when holding the BQL, so we can identify
+     * when an inhibitor wants to issue an ioctl easily.
+     */
+    g_assert(qemu_mutex_iothread_locked());
+
+    pthread_rwlock_wrlock(&kvm_ioctl_lock);
+
+    /* Inhibiting happens rarely, we can keep things simple and spin here. */
+    while (true) {
+        bool any_cpu_in_ioctl = false;
+
+        CPU_FOREACH(cpu) {
+            if (atomic_read(&cpu->in_ioctl)) {
+                any_cpu_in_ioctl = true;
+                qemu_cpu_kick(cpu);
+            }
+        }
+        if (!any_cpu_in_ioctl && !atomic_read(&kvm_in_ioctl)) {
+            break;
+        }
+        g_usleep(100);
+    }
+}
+
+static void kvm_ioctl_inhibit_end(void)
+{
+    pthread_rwlock_unlock(&kvm_ioctl_lock);
+}
+
+static void kvm_region_resize(MemoryListener *listener,
+                              MemoryRegionSection *section, Int128 new)
+{
+    KVMMemoryListener *kml = container_of(listener, KVMMemoryListener,
+                                          listener);
+    MemoryRegionSection new_section = *section;
+
+    new_section.size = new;
+
+    kvm_slots_lock(kml);
+    /* Inhibit KVM ioctls while temporarily removing slots. */
+    kvm_ioctl_inhibit_begin();
+    kvm_set_phys_mem(kml, section, false);
+    kvm_set_phys_mem(kml, &new_section, true);
+    kvm_ioctl_inhibit_end();
+    kvm_slots_unlock(kml);
+}
+
 static void kvm_log_sync(MemoryListener *listener,
                          MemoryRegionSection *section)
 {
@@ -1249,6 +1320,7 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
 
     kml->listener.region_add = kvm_region_add;
     kml->listener.region_del = kvm_region_del;
+    kml->listener.region_resize = kvm_region_resize;
     kml->listener.log_start = kvm_log_start;
     kml->listener.log_stop = kvm_log_stop;
     kml->listener.log_sync = kvm_log_sync;
@@ -1894,6 +1966,7 @@ static int kvm_init(MachineState *ms)
     assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
 
     s->sigmask_len = 8;
+    pthread_rwlock_init(&kvm_ioctl_lock, NULL);
 
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     QTAILQ_INIT(&s->kvm_sw_breakpoints);
@@ -2304,6 +2377,34 @@ static void kvm_eat_signals(CPUState *cpu)
     } while (sigismember(&chkset, SIG_IPI));
 }
 
+static void kvm_cpu_set_in_ioctl(CPUState *cpu, bool in_ioctl)
+{
+    if (unlikely(qemu_mutex_iothread_locked())) {
+        return;
+    }
+    if (in_ioctl) {
+        pthread_rwlock_rdlock(&kvm_ioctl_lock);
+        atomic_set(&cpu->in_ioctl, true);
+        pthread_rwlock_unlock(&kvm_ioctl_lock);
+    } else {
+        atomic_set(&cpu->in_ioctl, false);
+    }
+}
+
+static void kvm_set_in_ioctl(bool in_ioctl)
+{
+    if (likely(qemu_mutex_iothread_locked())) {
+        return;
+    }
+    if (in_ioctl) {
+        pthread_rwlock_rdlock(&kvm_ioctl_lock);
+        atomic_inc(&kvm_in_ioctl);
+        pthread_rwlock_unlock(&kvm_ioctl_lock);
+    } else {
+        atomic_dec(&kvm_in_ioctl);
+    }
+}
+
 int kvm_cpu_exec(CPUState *cpu)
 {
     struct kvm_run *run = cpu->kvm_run;
@@ -2488,7 +2589,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
     va_end(ap);
 
     trace_kvm_vm_ioctl(type, arg);
+    kvm_set_in_ioctl(true);
     ret = ioctl(s->vmfd, type, arg);
+    kvm_set_in_ioctl(false);
     if (ret == -1) {
         ret = -errno;
     }
@@ -2506,7 +2609,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
     va_end(ap);
 
     trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
+    kvm_cpu_set_in_ioctl(cpu, true);
     ret = ioctl(cpu->kvm_fd, type, arg);
+    kvm_cpu_set_in_ioctl(cpu, false);
     if (ret == -1) {
         ret = -errno;
     }
@@ -2524,7 +2629,9 @@ int kvm_device_ioctl(int fd, int type, ...)
     va_end(ap);
 
     trace_kvm_device_ioctl(fd, type, arg);
+    kvm_set_in_ioctl(true);
     ret = ioctl(fd, type, arg);
+    kvm_set_in_ioctl(false);
     if (ret == -1) {
         ret = -errno;
     }
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 73e9a869a4..4fbff6f3d7 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -431,6 +431,9 @@ struct CPUState {
     /* shared by kvm, hax and hvf */
     bool vcpu_dirty;
 
+    /* kvm only for now: CPU is in kvm_vcpu_ioctl() (esp. KVM_RUN) */
+    bool in_ioctl;
+
     /* Used to keep track of an outstanding cpu throttle thread for migration
      * autoconverge
      */
-- 
2.24.1

Re: [PATCH RFC 2/4] intc/s390_flic_kvm.c: Use kvm_device_ioctl() instead of ioctl()

[Christian Borntraeger]

On 03.03.20 15:19, David Hildenbrand wrote:
> Let's use the official variant, which will e.g., trace the call.
> 
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: qemu-s390x@nongnu.org
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/intc/s390_flic_kvm.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index a306b26faa..5151582ba0 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -68,7 +68,7 @@ static int flic_get_all_irqs(KVMS390FLICState *flic,
>      };
>      int rc;
>  
> -    rc = ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr);
> +    rc = kvm_device_ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr);
>  
>      return rc == -1 ? -errno : rc;
>  }
> @@ -80,7 +80,7 @@ static void flic_enable_pfault(KVMS390FLICState *flic)
>      };
>      int rc;
>  
> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>  
>      if (rc) {
>          fprintf(stderr, "flic: couldn't enable pfault\n");
> @@ -94,7 +94,7 @@ static void flic_disable_wait_pfault(KVMS390FLICState *flic)
>      };
>      int rc;
>  
> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>  
>      if (rc) {
>          fprintf(stderr, "flic: couldn't disable pfault\n");
> @@ -118,7 +118,7 @@ static int flic_enqueue_irqs(void *buf, uint64_t len,
>          .attr = len,
>      };
>  
> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> +    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>  
>      return rc ? -errno : 0;

This is no longer necessary as this is done by kvm_device_ioctl, no?

Same for other location.s

Re: [PATCH RFC 2/4] intc/s390_flic_kvm.c: Use kvm_device_ioctl() instead of ioctl()

[David Hildenbrand]

On 04.03.20 09:22, Christian Borntraeger wrote:
> 
> 
> On 03.03.20 15:19, David Hildenbrand wrote:
>> Let's use the official variant, which will e.g., trace the call.
>>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: Halil Pasic <pasic@linux.ibm.com>
>> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> Cc: qemu-s390x@nongnu.org
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/intc/s390_flic_kvm.c | 22 +++++++++++-----------
>>  1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
>> index a306b26faa..5151582ba0 100644
>> --- a/hw/intc/s390_flic_kvm.c
>> +++ b/hw/intc/s390_flic_kvm.c
>> @@ -68,7 +68,7 @@ static int flic_get_all_irqs(KVMS390FLICState *flic,
>>      };
>>      int rc;
>>  
>> -    rc = ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr);
>> +    rc = kvm_device_ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr);
>>  
>>      return rc == -1 ? -errno : rc;
>>  }
>> @@ -80,7 +80,7 @@ static void flic_enable_pfault(KVMS390FLICState *flic)
>>      };
>>      int rc;
>>  
>> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>> +    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>>  
>>      if (rc) {
>>          fprintf(stderr, "flic: couldn't enable pfault\n");
>> @@ -94,7 +94,7 @@ static void flic_disable_wait_pfault(KVMS390FLICState *flic)
>>      };
>>      int rc;
>>  
>> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>> +    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>>  
>>      if (rc) {
>>          fprintf(stderr, "flic: couldn't disable pfault\n");
>> @@ -118,7 +118,7 @@ static int flic_enqueue_irqs(void *buf, uint64_t len,
>>          .attr = len,
>>      };
>>  
>> -    rc = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>> +    rc = kvm_device_ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
>>  
>>      return rc ? -errno : 0;
> 
> This is no longer necessary as this is done by kvm_device_ioctl, no?
> 
> Same for other location.s
> 

Right, missed that. Thanks for pointing that out!

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[Paolo Bonzini]

On 03/03/20 15:19, David Hildenbrand wrote:
> virtio-mem wants to resize (esp. grow) ram memory regions while the guest
> is already aware of them and makes use of them. Resizing a KVM slot can
> only currently be done by removing it and re-adding it. While the kvm slot
> is temporarily removed, VCPUs that try to read from these slots will fault.

Only fetches I think?  Data reads and write would be treated as MMIO
accesses and they should just work (using either the old or new FlatView).

> But also, other ioctls might depend on all slots being in place.
> 
> Let's inhibit most KVM ioctls while performing the resize. Once we have an
> ioctl that can perform atomic resizes (e.g., KVM_SET_USER_MEMORY_REGION
> extensions), we can make inhibiting optional at runtime.
> 
> Also, make sure to hold the kvm_slots_lock while performing both
> actions (removing+re-adding).
>
> Note: Resizes of memory regions currently seems to happen during bootup
> only, so I don't think any existing RT users should be affected.

rwlocks are not efficient, they cause cache line contention.  For
MMIO-heavy workloads the impact will be very large (well, not that large
because right now they all take the BQL, but one can always hope).

I would very much prefer to add a KVM_SET_USER_MEMORY_REGION extension
right away.

Paolo

> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/kvm/kvm-all.c   | 121 +++++++++++++++++++++++++++++++++++++++---
>  include/hw/core/cpu.h |   3 ++
>  2 files changed, 117 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 439a4efe52..bba58db098 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -149,6 +149,21 @@ bool kvm_msi_use_devid;
>  static bool kvm_immediate_exit;
>  static hwaddr kvm_max_slot_size = ~0;
>  
> +/*
> + * While holding this lock in write, no new KVM ioctls can be started, but
> + * kvm ioctl inhibitors will have to wait for existing ones to finish
> + * (indicated by cpu->in_ioctl and kvm_in_ioctl, both updated with this lock
> + * held in read when entering the ioctl).
> + */
> +pthread_rwlock_t kvm_ioctl_lock;
> +/*
> + * Atomic counter of active KVM ioctls except
> + * - The KVM ioctl inhibitor is doing an ioctl
> + * - kvm_ioctl(): Harmless and not interesting for inhibitors.
> + * - kvm_vcpu_ioctl(): Tracked via cpu->in_ioctl.
> + */
> +static int kvm_in_ioctl;
> +
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
>      KVM_CAP_INFO(DESTROY_MEMORY_REGION_WORKS),
> @@ -1023,6 +1038,7 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size)
>      kvm_max_slot_size = max_slot_size;
>  }
>  
> +/* Called with KVMMemoryListener.slots_lock held */
>  static void kvm_set_phys_mem(KVMMemoryListener *kml,
>                               MemoryRegionSection *section, bool add)
>  {
> @@ -1052,14 +1068,12 @@ 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);
>  
> -    kvm_slots_lock(kml);
> -
>      if (!add) {
>          do {
>              slot_size = MIN(kvm_max_slot_size, size);
>              mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
>              if (!mem) {
> -                goto out;
> +                return;
>              }
>              if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
>                  kvm_physical_sync_dirty_bitmap(kml, section);
> @@ -1079,7 +1093,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
>              start_addr += slot_size;
>              size -= slot_size;
>          } while (size);
> -        goto out;
> +        return;
>      }
>  
>      /* register the new slot */
> @@ -1108,9 +1122,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
>          ram += slot_size;
>          size -= slot_size;
>      } while (size);
> -
> -out:
> -    kvm_slots_unlock(kml);
>  }
>  
>  static void kvm_region_add(MemoryListener *listener,
> @@ -1119,7 +1130,9 @@ static void kvm_region_add(MemoryListener *listener,
>      KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
>  
>      memory_region_ref(section->mr);
> +    kvm_slots_lock(kml);
>      kvm_set_phys_mem(kml, section, true);
> +    kvm_slots_unlock(kml);
>  }
>  
>  static void kvm_region_del(MemoryListener *listener,
> @@ -1127,10 +1140,68 @@ static void kvm_region_del(MemoryListener *listener,
>  {
>      KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
>  
> +    kvm_slots_lock(kml);
>      kvm_set_phys_mem(kml, section, false);
> +    kvm_slots_unlock(kml);
>      memory_region_unref(section->mr);
>  }
>  
> +/*
> + * Certain updates (e.g., resizing memory regions) require temporarily removing
> + * kvm memory slots. Make sure any ioctl sees a consistent memory slot state.
> + */
> +static void kvm_ioctl_inhibit_begin(void)
> +{
> +    CPUState *cpu;
> +
> +    /*
> +     * We allow to inhibit only when holding the BQL, so we can identify
> +     * when an inhibitor wants to issue an ioctl easily.
> +     */
> +    g_assert(qemu_mutex_iothread_locked());
> +
> +    pthread_rwlock_wrlock(&kvm_ioctl_lock);
> +
> +    /* Inhibiting happens rarely, we can keep things simple and spin here. */
> +    while (true) {
> +        bool any_cpu_in_ioctl = false;
> +
> +        CPU_FOREACH(cpu) {
> +            if (atomic_read(&cpu->in_ioctl)) {
> +                any_cpu_in_ioctl = true;
> +                qemu_cpu_kick(cpu);
> +            }
> +        }
> +        if (!any_cpu_in_ioctl && !atomic_read(&kvm_in_ioctl)) {
> +            break;
> +        }
> +        g_usleep(100);
> +    }
> +}
> +
> +static void kvm_ioctl_inhibit_end(void)
> +{
> +    pthread_rwlock_unlock(&kvm_ioctl_lock);
> +}
> +
> +static void kvm_region_resize(MemoryListener *listener,
> +                              MemoryRegionSection *section, Int128 new)
> +{
> +    KVMMemoryListener *kml = container_of(listener, KVMMemoryListener,
> +                                          listener);
> +    MemoryRegionSection new_section = *section;
> +
> +    new_section.size = new;
> +
> +    kvm_slots_lock(kml);
> +    /* Inhibit KVM ioctls while temporarily removing slots. */
> +    kvm_ioctl_inhibit_begin();
> +    kvm_set_phys_mem(kml, section, false);
> +    kvm_set_phys_mem(kml, &new_section, true);
> +    kvm_ioctl_inhibit_end();
> +    kvm_slots_unlock(kml);
> +}
> +
>  static void kvm_log_sync(MemoryListener *listener,
>                           MemoryRegionSection *section)
>  {
> @@ -1249,6 +1320,7 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
>  
>      kml->listener.region_add = kvm_region_add;
>      kml->listener.region_del = kvm_region_del;
> +    kml->listener.region_resize = kvm_region_resize;
>      kml->listener.log_start = kvm_log_start;
>      kml->listener.log_stop = kvm_log_stop;
>      kml->listener.log_sync = kvm_log_sync;
> @@ -1894,6 +1966,7 @@ static int kvm_init(MachineState *ms)
>      assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>  
>      s->sigmask_len = 8;
> +    pthread_rwlock_init(&kvm_ioctl_lock, NULL);
>  
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
>      QTAILQ_INIT(&s->kvm_sw_breakpoints);
> @@ -2304,6 +2377,34 @@ static void kvm_eat_signals(CPUState *cpu)
>      } while (sigismember(&chkset, SIG_IPI));
>  }
>  
> +static void kvm_cpu_set_in_ioctl(CPUState *cpu, bool in_ioctl)
> +{
> +    if (unlikely(qemu_mutex_iothread_locked())) {
> +        return;
> +    }
> +    if (in_ioctl) {
> +        pthread_rwlock_rdlock(&kvm_ioctl_lock);
> +        atomic_set(&cpu->in_ioctl, true);
> +        pthread_rwlock_unlock(&kvm_ioctl_lock);
> +    } else {
> +        atomic_set(&cpu->in_ioctl, false);
> +    }
> +}
> +
> +static void kvm_set_in_ioctl(bool in_ioctl)
> +{
> +    if (likely(qemu_mutex_iothread_locked())) {
> +        return;
> +    }
> +    if (in_ioctl) {
> +        pthread_rwlock_rdlock(&kvm_ioctl_lock);
> +        atomic_inc(&kvm_in_ioctl);
> +        pthread_rwlock_unlock(&kvm_ioctl_lock);
> +    } else {
> +        atomic_dec(&kvm_in_ioctl);
> +    }
> +}
> +
>  int kvm_cpu_exec(CPUState *cpu)
>  {
>      struct kvm_run *run = cpu->kvm_run;
> @@ -2488,7 +2589,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
>      va_end(ap);
>  
>      trace_kvm_vm_ioctl(type, arg);
> +    kvm_set_in_ioctl(true);
>      ret = ioctl(s->vmfd, type, arg);
> +    kvm_set_in_ioctl(false);
>      if (ret == -1) {
>          ret = -errno;
>      }
> @@ -2506,7 +2609,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
>      va_end(ap);
>  
>      trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
> +    kvm_cpu_set_in_ioctl(cpu, true);
>      ret = ioctl(cpu->kvm_fd, type, arg);
> +    kvm_cpu_set_in_ioctl(cpu, false);
>      if (ret == -1) {
>          ret = -errno;
>      }
> @@ -2524,7 +2629,9 @@ int kvm_device_ioctl(int fd, int type, ...)
>      va_end(ap);
>  
>      trace_kvm_device_ioctl(fd, type, arg);
> +    kvm_set_in_ioctl(true);
>      ret = ioctl(fd, type, arg);
> +    kvm_set_in_ioctl(false);
>      if (ret == -1) {
>          ret = -errno;
>      }
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 73e9a869a4..4fbff6f3d7 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -431,6 +431,9 @@ struct CPUState {
>      /* shared by kvm, hax and hvf */
>      bool vcpu_dirty;
>  
> +    /* kvm only for now: CPU is in kvm_vcpu_ioctl() (esp. KVM_RUN) */
> +    bool in_ioctl;
> +
>      /* Used to keep track of an outstanding cpu throttle thread for migration
>       * autoconverge
>       */
> 

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[Paolo Bonzini]

On 03/03/20 15:19, David Hildenbrand wrote:
> virtio-mem wants to resize (esp. grow) ram memory regions while the guest
> is already aware of them and makes use of them. Resizing a KVM slot can
> only currently be done by removing it and re-adding it. While the kvm slot
> is temporarily removed, VCPUs that try to read from these slots will fault.

Only fetches I think?  Data reads and write would be treated as MMIO
accesses and they should just work (using either the old or new FlatView).

> But also, other ioctls might depend on all slots being in place.
> 
> Let's inhibit most KVM ioctls while performing the resize. Once we have an
> ioctl that can perform atomic resizes (e.g., KVM_SET_USER_MEMORY_REGION
> extensions), we can make inhibiting optional at runtime.
> 
> Also, make sure to hold the kvm_slots_lock while performing both
> actions (removing+re-adding).
>
> Note: Resizes of memory regions currently seems to happen during bootup
> only, so I don't think any existing RT users should be affected.

rwlocks are not efficient, they cause cache line contention.  For
MMIO-heavy workloads the impact will be very large (well, not that large
because right now they all take the BQL, but one can always hope).

I would very much prefer to add a KVM_SET_USER_MEMORY_REGION extension
right away.

Paolo

> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/kvm/kvm-all.c   | 121 +++++++++++++++++++++++++++++++++++++++---
>  include/hw/core/cpu.h |   3 ++
>  2 files changed, 117 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 439a4efe52..bba58db098 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -149,6 +149,21 @@ bool kvm_msi_use_devid;
>  static bool kvm_immediate_exit;
>  static hwaddr kvm_max_slot_size = ~0;
>  
> +/*
> + * While holding this lock in write, no new KVM ioctls can be started, but
> + * kvm ioctl inhibitors will have to wait for existing ones to finish
> + * (indicated by cpu->in_ioctl and kvm_in_ioctl, both updated with this lock
> + * held in read when entering the ioctl).
> + */
> +pthread_rwlock_t kvm_ioctl_lock;
> +/*
> + * Atomic counter of active KVM ioctls except
> + * - The KVM ioctl inhibitor is doing an ioctl
> + * - kvm_ioctl(): Harmless and not interesting for inhibitors.
> + * - kvm_vcpu_ioctl(): Tracked via cpu->in_ioctl.
> + */
> +static int kvm_in_ioctl;
> +
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
>      KVM_CAP_INFO(DESTROY_MEMORY_REGION_WORKS),
> @@ -1023,6 +1038,7 @@ void kvm_set_max_memslot_size(hwaddr max_slot_size)
>      kvm_max_slot_size = max_slot_size;
>  }
>  
> +/* Called with KVMMemoryListener.slots_lock held */
>  static void kvm_set_phys_mem(KVMMemoryListener *kml,
>                               MemoryRegionSection *section, bool add)
>  {
> @@ -1052,14 +1068,12 @@ 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);
>  
> -    kvm_slots_lock(kml);
> -
>      if (!add) {
>          do {
>              slot_size = MIN(kvm_max_slot_size, size);
>              mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
>              if (!mem) {
> -                goto out;
> +                return;
>              }
>              if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
>                  kvm_physical_sync_dirty_bitmap(kml, section);
> @@ -1079,7 +1093,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
>              start_addr += slot_size;
>              size -= slot_size;
>          } while (size);
> -        goto out;
> +        return;
>      }
>  
>      /* register the new slot */
> @@ -1108,9 +1122,6 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
>          ram += slot_size;
>          size -= slot_size;
>      } while (size);
> -
> -out:
> -    kvm_slots_unlock(kml);
>  }
>  
>  static void kvm_region_add(MemoryListener *listener,
> @@ -1119,7 +1130,9 @@ static void kvm_region_add(MemoryListener *listener,
>      KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
>  
>      memory_region_ref(section->mr);
> +    kvm_slots_lock(kml);
>      kvm_set_phys_mem(kml, section, true);
> +    kvm_slots_unlock(kml);
>  }
>  
>  static void kvm_region_del(MemoryListener *listener,
> @@ -1127,10 +1140,68 @@ static void kvm_region_del(MemoryListener *listener,
>  {
>      KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, listener);
>  
> +    kvm_slots_lock(kml);
>      kvm_set_phys_mem(kml, section, false);
> +    kvm_slots_unlock(kml);
>      memory_region_unref(section->mr);
>  }
>  
> +/*
> + * Certain updates (e.g., resizing memory regions) require temporarily removing
> + * kvm memory slots. Make sure any ioctl sees a consistent memory slot state.
> + */
> +static void kvm_ioctl_inhibit_begin(void)
> +{
> +    CPUState *cpu;
> +
> +    /*
> +     * We allow to inhibit only when holding the BQL, so we can identify
> +     * when an inhibitor wants to issue an ioctl easily.
> +     */
> +    g_assert(qemu_mutex_iothread_locked());
> +
> +    pthread_rwlock_wrlock(&kvm_ioctl_lock);
> +
> +    /* Inhibiting happens rarely, we can keep things simple and spin here. */
> +    while (true) {
> +        bool any_cpu_in_ioctl = false;
> +
> +        CPU_FOREACH(cpu) {
> +            if (atomic_read(&cpu->in_ioctl)) {
> +                any_cpu_in_ioctl = true;
> +                qemu_cpu_kick(cpu);
> +            }
> +        }
> +        if (!any_cpu_in_ioctl && !atomic_read(&kvm_in_ioctl)) {
> +            break;
> +        }
> +        g_usleep(100);
> +    }
> +}
> +
> +static void kvm_ioctl_inhibit_end(void)
> +{
> +    pthread_rwlock_unlock(&kvm_ioctl_lock);
> +}
> +
> +static void kvm_region_resize(MemoryListener *listener,
> +                              MemoryRegionSection *section, Int128 new)
> +{
> +    KVMMemoryListener *kml = container_of(listener, KVMMemoryListener,
> +                                          listener);
> +    MemoryRegionSection new_section = *section;
> +
> +    new_section.size = new;
> +
> +    kvm_slots_lock(kml);
> +    /* Inhibit KVM ioctls while temporarily removing slots. */
> +    kvm_ioctl_inhibit_begin();
> +    kvm_set_phys_mem(kml, section, false);
> +    kvm_set_phys_mem(kml, &new_section, true);
> +    kvm_ioctl_inhibit_end();
> +    kvm_slots_unlock(kml);
> +}
> +
>  static void kvm_log_sync(MemoryListener *listener,
>                           MemoryRegionSection *section)
>  {
> @@ -1249,6 +1320,7 @@ void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
>  
>      kml->listener.region_add = kvm_region_add;
>      kml->listener.region_del = kvm_region_del;
> +    kml->listener.region_resize = kvm_region_resize;
>      kml->listener.log_start = kvm_log_start;
>      kml->listener.log_stop = kvm_log_stop;
>      kml->listener.log_sync = kvm_log_sync;
> @@ -1894,6 +1966,7 @@ static int kvm_init(MachineState *ms)
>      assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>  
>      s->sigmask_len = 8;
> +    pthread_rwlock_init(&kvm_ioctl_lock, NULL);
>  
>  #ifdef KVM_CAP_SET_GUEST_DEBUG
>      QTAILQ_INIT(&s->kvm_sw_breakpoints);
> @@ -2304,6 +2377,34 @@ static void kvm_eat_signals(CPUState *cpu)
>      } while (sigismember(&chkset, SIG_IPI));
>  }
>  
> +static void kvm_cpu_set_in_ioctl(CPUState *cpu, bool in_ioctl)
> +{
> +    if (unlikely(qemu_mutex_iothread_locked())) {
> +        return;
> +    }
> +    if (in_ioctl) {
> +        pthread_rwlock_rdlock(&kvm_ioctl_lock);
> +        atomic_set(&cpu->in_ioctl, true);
> +        pthread_rwlock_unlock(&kvm_ioctl_lock);
> +    } else {
> +        atomic_set(&cpu->in_ioctl, false);
> +    }
> +}
> +
> +static void kvm_set_in_ioctl(bool in_ioctl)
> +{
> +    if (likely(qemu_mutex_iothread_locked())) {
> +        return;
> +    }
> +    if (in_ioctl) {
> +        pthread_rwlock_rdlock(&kvm_ioctl_lock);
> +        atomic_inc(&kvm_in_ioctl);
> +        pthread_rwlock_unlock(&kvm_ioctl_lock);
> +    } else {
> +        atomic_dec(&kvm_in_ioctl);
> +    }
> +}
> +
>  int kvm_cpu_exec(CPUState *cpu)
>  {
>      struct kvm_run *run = cpu->kvm_run;
> @@ -2488,7 +2589,9 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
>      va_end(ap);
>  
>      trace_kvm_vm_ioctl(type, arg);
> +    kvm_set_in_ioctl(true);
>      ret = ioctl(s->vmfd, type, arg);
> +    kvm_set_in_ioctl(false);
>      if (ret == -1) {
>          ret = -errno;
>      }
> @@ -2506,7 +2609,9 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
>      va_end(ap);
>  
>      trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
> +    kvm_cpu_set_in_ioctl(cpu, true);
>      ret = ioctl(cpu->kvm_fd, type, arg);
> +    kvm_cpu_set_in_ioctl(cpu, false);
>      if (ret == -1) {
>          ret = -errno;
>      }
> @@ -2524,7 +2629,9 @@ int kvm_device_ioctl(int fd, int type, ...)
>      va_end(ap);
>  
>      trace_kvm_device_ioctl(fd, type, arg);
> +    kvm_set_in_ioctl(true);
>      ret = ioctl(fd, type, arg);
> +    kvm_set_in_ioctl(false);
>      if (ret == -1) {
>          ret = -errno;
>      }
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 73e9a869a4..4fbff6f3d7 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -431,6 +431,9 @@ struct CPUState {
>      /* shared by kvm, hax and hvf */
>      bool vcpu_dirty;
>  
> +    /* kvm only for now: CPU is in kvm_vcpu_ioctl() (esp. KVM_RUN) */
> +    bool in_ioctl;
> +
>      /* Used to keep track of an outstanding cpu throttle thread for migration
>       * autoconverge
>       */
> 

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 10:50, Paolo Bonzini wrote:
> On 03/03/20 15:19, David Hildenbrand wrote:
>> virtio-mem wants to resize (esp. grow) ram memory regions while the guest
>> is already aware of them and makes use of them. Resizing a KVM slot can
>> only currently be done by removing it and re-adding it. While the kvm slot
>> is temporarily removed, VCPUs that try to read from these slots will fault.
> 

s/try to read/try to access/

> Only fetches I think?  Data reads and write would be treated as MMIO
> accesses and they should just work (using either the old or new FlatView).

On x86-64, I saw KVM fault printks getting printed (it was about 1-2
years ago, though, when I realized this was a problem). Could be that
these were fetches. At least the guest eventually crashed :)

On other archs (esp. s390x) guests will directly receive a
PGM_ADDRESSING from KVM if they stumble over memory that is not covered
by a kvm slot.

> 
>> But also, other ioctls might depend on all slots being in place.
>>
>> Let's inhibit most KVM ioctls while performing the resize. Once we have an
>> ioctl that can perform atomic resizes (e.g., KVM_SET_USER_MEMORY_REGION
>> extensions), we can make inhibiting optional at runtime.
>>
>> Also, make sure to hold the kvm_slots_lock while performing both
>> actions (removing+re-adding).
>>
>> Note: Resizes of memory regions currently seems to happen during bootup
>> only, so I don't think any existing RT users should be affected.
> 
> rwlocks are not efficient, they cause cache line contention.  For
> MMIO-heavy workloads the impact will be very large (well, not that large
> because right now they all take the BQL, but one can always hope).

Yeah, rwlocks are not optimal and I am still looking for better
alternatives (suggestions welcome :) ). Using RCU might not work,
because the rcu_read region might be too big (esp. while in KVM_RUN).

I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
was quite elaborate and buggy.

(I assume only going into KVM_RUN is really affected, and I do wonder if
it will be noticeable at all. Doing an ioctl is always already an
expensive operation.)

I can look into per-cpu locks instead of the rwlock.

> 
> I would very much prefer to add a KVM_SET_USER_MEMORY_REGION extension
> right away.
> 

I really want to avoid dependencies on kernel features to at least make
it work for now. Especially, resizing memory slots in KVM (especially
while dirty bitmaps, rmaps, etc. are active) is non-trivial.

Thanks Paolo!

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 10:50, Paolo Bonzini wrote:
> On 03/03/20 15:19, David Hildenbrand wrote:
>> virtio-mem wants to resize (esp. grow) ram memory regions while the guest
>> is already aware of them and makes use of them. Resizing a KVM slot can
>> only currently be done by removing it and re-adding it. While the kvm slot
>> is temporarily removed, VCPUs that try to read from these slots will fault.
> 

s/try to read/try to access/

> Only fetches I think?  Data reads and write would be treated as MMIO
> accesses and they should just work (using either the old or new FlatView).

On x86-64, I saw KVM fault printks getting printed (it was about 1-2
years ago, though, when I realized this was a problem). Could be that
these were fetches. At least the guest eventually crashed :)

On other archs (esp. s390x) guests will directly receive a
PGM_ADDRESSING from KVM if they stumble over memory that is not covered
by a kvm slot.

> 
>> But also, other ioctls might depend on all slots being in place.
>>
>> Let's inhibit most KVM ioctls while performing the resize. Once we have an
>> ioctl that can perform atomic resizes (e.g., KVM_SET_USER_MEMORY_REGION
>> extensions), we can make inhibiting optional at runtime.
>>
>> Also, make sure to hold the kvm_slots_lock while performing both
>> actions (removing+re-adding).
>>
>> Note: Resizes of memory regions currently seems to happen during bootup
>> only, so I don't think any existing RT users should be affected.
> 
> rwlocks are not efficient, they cause cache line contention.  For
> MMIO-heavy workloads the impact will be very large (well, not that large
> because right now they all take the BQL, but one can always hope).

Yeah, rwlocks are not optimal and I am still looking for better
alternatives (suggestions welcome :) ). Using RCU might not work,
because the rcu_read region might be too big (esp. while in KVM_RUN).

I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
was quite elaborate and buggy.

(I assume only going into KVM_RUN is really affected, and I do wonder if
it will be noticeable at all. Doing an ioctl is always already an
expensive operation.)

I can look into per-cpu locks instead of the rwlock.

> 
> I would very much prefer to add a KVM_SET_USER_MEMORY_REGION extension
> right away.
> 

I really want to avoid dependencies on kernel features to at least make
it work for now. Especially, resizing memory slots in KVM (especially
while dirty bitmaps, rmaps, etc. are active) is non-trivial.

Thanks Paolo!

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[Paolo Bonzini]

On 06/03/20 11:20, David Hildenbrand wrote:
> Yeah, rwlocks are not optimal and I am still looking for better
> alternatives (suggestions welcome :) ). Using RCU might not work,
> because the rcu_read region might be too big (esp. while in KVM_RUN).
> 
> I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
> was quite elaborate and buggy.
> 
> (I assume only going into KVM_RUN is really affected, and I do wonder if
> it will be noticeable at all. Doing an ioctl is always already an
> expensive operation.)
> 
> I can look into per-cpu locks instead of the rwlock.

Assuming we're only talking about CPU ioctls (seems like a good
approximation) maybe you could use start_exclusive/end_exclusive?  The
current_cpu->in_exclusive_context assignments can be made conditional on
"if (current_cpu)".

However that means you have to drop the BQL, see
process_queued_cpu_work.  It may be a problem.


Paolo

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[Paolo Bonzini]

On 06/03/20 11:20, David Hildenbrand wrote:
> Yeah, rwlocks are not optimal and I am still looking for better
> alternatives (suggestions welcome :) ). Using RCU might not work,
> because the rcu_read region might be too big (esp. while in KVM_RUN).
> 
> I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
> was quite elaborate and buggy.
> 
> (I assume only going into KVM_RUN is really affected, and I do wonder if
> it will be noticeable at all. Doing an ioctl is always already an
> expensive operation.)
> 
> I can look into per-cpu locks instead of the rwlock.

Assuming we're only talking about CPU ioctls (seems like a good
approximation) maybe you could use start_exclusive/end_exclusive?  The
current_cpu->in_exclusive_context assignments can be made conditional on
"if (current_cpu)".

However that means you have to drop the BQL, see
process_queued_cpu_work.  It may be a problem.


Paolo

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 12:38, Paolo Bonzini wrote:
> On 06/03/20 11:20, David Hildenbrand wrote:
>> Yeah, rwlocks are not optimal and I am still looking for better
>> alternatives (suggestions welcome :) ). Using RCU might not work,
>> because the rcu_read region might be too big (esp. while in KVM_RUN).
>>
>> I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
>> was quite elaborate and buggy.
>>
>> (I assume only going into KVM_RUN is really affected, and I do wonder if
>> it will be noticeable at all. Doing an ioctl is always already an
>> expensive operation.)
>>
>> I can look into per-cpu locks instead of the rwlock.
> 
> Assuming we're only talking about CPU ioctls (seems like a good

Yeah, I guess most !CPU ioctls are done under the BQL.

> approximation) maybe you could use start_exclusive/end_exclusive?  The
> current_cpu->in_exclusive_context assignments can be made conditional on
> "if (current_cpu)".
> 
> However that means you have to drop the BQL, see
> process_queued_cpu_work.  It may be a problem.

Thanks, I'll look into that. I currently have a simple cpu->ioctl_mutex.

Cheers!


-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 12:38, Paolo Bonzini wrote:
> On 06/03/20 11:20, David Hildenbrand wrote:
>> Yeah, rwlocks are not optimal and I am still looking for better
>> alternatives (suggestions welcome :) ). Using RCU might not work,
>> because the rcu_read region might be too big (esp. while in KVM_RUN).
>>
>> I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
>> was quite elaborate and buggy.
>>
>> (I assume only going into KVM_RUN is really affected, and I do wonder if
>> it will be noticeable at all. Doing an ioctl is always already an
>> expensive operation.)
>>
>> I can look into per-cpu locks instead of the rwlock.
> 
> Assuming we're only talking about CPU ioctls (seems like a good

Yeah, I guess most !CPU ioctls are done under the BQL.

> approximation) maybe you could use start_exclusive/end_exclusive?  The
> current_cpu->in_exclusive_context assignments can be made conditional on
> "if (current_cpu)".
> 
> However that means you have to drop the BQL, see
> process_queued_cpu_work.  It may be a problem.

Thanks, I'll look into that. I currently have a simple cpu->ioctl_mutex.

Cheers!


-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 12:38, Paolo Bonzini wrote:
> On 06/03/20 11:20, David Hildenbrand wrote:
>> Yeah, rwlocks are not optimal and I am still looking for better
>> alternatives (suggestions welcome :) ). Using RCU might not work,
>> because the rcu_read region might be too big (esp. while in KVM_RUN).
>>
>> I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
>> was quite elaborate and buggy.
>>
>> (I assume only going into KVM_RUN is really affected, and I do wonder if
>> it will be noticeable at all. Doing an ioctl is always already an
>> expensive operation.)
>>
>> I can look into per-cpu locks instead of the rwlock.
> 
> Assuming we're only talking about CPU ioctls (seems like a good
> approximation) maybe you could use start_exclusive/end_exclusive?  The
> current_cpu->in_exclusive_context assignments can be made conditional on
> "if (current_cpu)".
> 
> However that means you have to drop the BQL, see
> process_queued_cpu_work.  It may be a problem.
> 

Yeah, start_exclusive() is expected to be called without the BQL,
otherwise the other CPUs would not be able to make progress and can
eventually be "caught".

It's essentially the same reason why I can't use high-level
pause_all_vcpus()/resume_all_vcpus(). Will drop the BQL which is very
bad for resizing code.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 12:38, Paolo Bonzini wrote:
> On 06/03/20 11:20, David Hildenbrand wrote:
>> Yeah, rwlocks are not optimal and I am still looking for better
>> alternatives (suggestions welcome :) ). Using RCU might not work,
>> because the rcu_read region might be too big (esp. while in KVM_RUN).
>>
>> I had a prototype which used a bunch of atomics + qemu_cond_wait. But it
>> was quite elaborate and buggy.
>>
>> (I assume only going into KVM_RUN is really affected, and I do wonder if
>> it will be noticeable at all. Doing an ioctl is always already an
>> expensive operation.)
>>
>> I can look into per-cpu locks instead of the rwlock.
> 
> Assuming we're only talking about CPU ioctls (seems like a good
> approximation) maybe you could use start_exclusive/end_exclusive?  The
> current_cpu->in_exclusive_context assignments can be made conditional on
> "if (current_cpu)".
> 
> However that means you have to drop the BQL, see
> process_queued_cpu_work.  It may be a problem.
> 

Yeah, start_exclusive() is expected to be called without the BQL,
otherwise the other CPUs would not be able to make progress and can
eventually be "caught".

It's essentially the same reason why I can't use high-level
pause_all_vcpus()/resume_all_vcpus(). Will drop the BQL which is very
bad for resizing code.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[Paolo Bonzini]

On 06/03/20 15:30, David Hildenbrand wrote:
>> Assuming we're only talking about CPU ioctls (seems like a good
>> approximation) maybe you could use start_exclusive/end_exclusive?  The
>> current_cpu->in_exclusive_context assignments can be made conditional on
>> "if (current_cpu)".
>>
>> However that means you have to drop the BQL, see
>> process_queued_cpu_work.  It may be a problem.
>>
> Yeah, start_exclusive() is expected to be called without the BQL,
> otherwise the other CPUs would not be able to make progress and can
> eventually be "caught".
> 
> It's essentially the same reason why I can't use high-level
> pause_all_vcpus()/resume_all_vcpus(). Will drop the BQL which is very
> bad for resizing code.

But any other synchronization primitive that you do which blocks all
vCPUs will have the same issue, otherwise you get a deadlock.

Paolo

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[Paolo Bonzini]

On 06/03/20 15:30, David Hildenbrand wrote:
>> Assuming we're only talking about CPU ioctls (seems like a good
>> approximation) maybe you could use start_exclusive/end_exclusive?  The
>> current_cpu->in_exclusive_context assignments can be made conditional on
>> "if (current_cpu)".
>>
>> However that means you have to drop the BQL, see
>> process_queued_cpu_work.  It may be a problem.
>>
> Yeah, start_exclusive() is expected to be called without the BQL,
> otherwise the other CPUs would not be able to make progress and can
> eventually be "caught".
> 
> It's essentially the same reason why I can't use high-level
> pause_all_vcpus()/resume_all_vcpus(). Will drop the BQL which is very
> bad for resizing code.

But any other synchronization primitive that you do which blocks all
vCPUs will have the same issue, otherwise you get a deadlock.

Paolo

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 15:39, Paolo Bonzini wrote:
> On 06/03/20 15:30, David Hildenbrand wrote:
>>> Assuming we're only talking about CPU ioctls (seems like a good
>>> approximation) maybe you could use start_exclusive/end_exclusive?  The
>>> current_cpu->in_exclusive_context assignments can be made conditional on
>>> "if (current_cpu)".
>>>
>>> However that means you have to drop the BQL, see
>>> process_queued_cpu_work.  It may be a problem.
>>>
>> Yeah, start_exclusive() is expected to be called without the BQL,
>> otherwise the other CPUs would not be able to make progress and can
>> eventually be "caught".
>>
>> It's essentially the same reason why I can't use high-level
>> pause_all_vcpus()/resume_all_vcpus(). Will drop the BQL which is very
>> bad for resizing code.
> 
> But any other synchronization primitive that you do which blocks all
> vCPUs will have the same issue, otherwise you get a deadlock.

This is essentially what this patch solves.

The lock essentially blocks anybody from entering, but not leaving a KVM
ioctl. An inhibitor only waits for all IOCTLs to be left. No other lock
prohibits that, so I don't think there can ever be a deadlock.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFC 4/4] kvm: Implement atomic memory region resizes via region_resize()

[David Hildenbrand]

On 06.03.20 15:39, Paolo Bonzini wrote:
> On 06/03/20 15:30, David Hildenbrand wrote:
>>> Assuming we're only talking about CPU ioctls (seems like a good
>>> approximation) maybe you could use start_exclusive/end_exclusive?  The
>>> current_cpu->in_exclusive_context assignments can be made conditional on
>>> "if (current_cpu)".
>>>
>>> However that means you have to drop the BQL, see
>>> process_queued_cpu_work.  It may be a problem.
>>>
>> Yeah, start_exclusive() is expected to be called without the BQL,
>> otherwise the other CPUs would not be able to make progress and can
>> eventually be "caught".
>>
>> It's essentially the same reason why I can't use high-level
>> pause_all_vcpus()/resume_all_vcpus(). Will drop the BQL which is very
>> bad for resizing code.
> 
> But any other synchronization primitive that you do which blocks all
> vCPUs will have the same issue, otherwise you get a deadlock.

This is essentially what this patch solves.

The lock essentially blocks anybody from entering, but not leaving a KVM
ioctl. An inhibitor only waits for all IOCTLs to be left. No other lock
prohibits that, so I don't think there can ever be a deadlock.

-- 
Thanks,

David / dhildenb