Re: [PATCH v2 04/16] kvm: Return number of free memslots
On 06.09.23 16:14, David Hildenbrand wrote:
On 29.08.23 00:26, Philippe Mathieu-Daudé wrote:
On 25/8/23 15:21, David Hildenbrand wrote:
Let's return the number of free slots instead of only checking if there
is a free slot. While at it, check all address spaces, which will also
consider SMM under x86 correctly.
Make the stub return UINT_MAX, such that we can call the function
unconditionally.
This is a preparation for memory devices that consume multiple memslots.
Signed-off-by: David Hildenbrand
---
accel/kvm/kvm-all.c | 33 -
accel/stubs/kvm-stub.c | 4 ++--
hw/mem/memory-device.c | 2 +-
include/sysemu/kvm.h | 2 +-
include/sysemu/kvm_int.h | 1 +
5 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 235dc661bc..f39997d86e 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -109,9 +109,9 @@ int kvm_irqchip_remove_irqfd_notifier_gsi(KVMState *s,
EventNotifier *n,
return -ENOSYS;
}
-bool kvm_has_free_slot(MachineState *ms)
+unsigned int kvm_get_free_memslots(void)
{
-return false;
+return UINT_MAX;
Isn't it clearer returning 0 here and keeping kvm_enabled() below?
I tried doing it similarly to vhost_has_free_slot().
I'll leave the kvm_enabled() check in place, looks cleaner.
--
Cheers,
David / dhildenb
Re: [PATCH v2 04/16] kvm: Return number of free memslots
On 29.08.23 00:26, Philippe Mathieu-Daudé wrote:
On 25/8/23 15:21, David Hildenbrand wrote:
Let's return the number of free slots instead of only checking if there
is a free slot. While at it, check all address spaces, which will also
consider SMM under x86 correctly.
Make the stub return UINT_MAX, such that we can call the function
unconditionally.
This is a preparation for memory devices that consume multiple memslots.
Signed-off-by: David Hildenbrand
---
accel/kvm/kvm-all.c | 33 -
accel/stubs/kvm-stub.c | 4 ++--
hw/mem/memory-device.c | 2 +-
include/sysemu/kvm.h | 2 +-
include/sysemu/kvm_int.h | 1 +
5 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 235dc661bc..f39997d86e 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -109,9 +109,9 @@ int kvm_irqchip_remove_irqfd_notifier_gsi(KVMState *s,
EventNotifier *n,
return -ENOSYS;
}
-bool kvm_has_free_slot(MachineState *ms)
+unsigned int kvm_get_free_memslots(void)
{
-return false;
+return UINT_MAX;
Isn't it clearer returning 0 here and keeping kvm_enabled() below?
I tried doing it similarly to vhost_has_free_slot().
Also simplifies patch #12 :)
No strong opinion, though.
diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index 511b42bde5..8b09e78b12 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -40,6 +40,7 @@ typedef struct KVMMemoryUpdate {
typedef struct KVMMemoryListener {
MemoryListener listener;
KVMSlot *slots;
+int nr_used_slots;
Preferably using 'unsigned' here:
Sure, that should work.
Reviewed-by: Philippe Mathieu-Daudé
Thanks!
--
Cheers,
David / dhildenb
Re: [PATCH v2 04/16] kvm: Return number of free memslots
On 25/8/23 15:21, David Hildenbrand wrote:
Let's return the number of free slots instead of only checking if there
is a free slot. While at it, check all address spaces, which will also
consider SMM under x86 correctly.
Make the stub return UINT_MAX, such that we can call the function
unconditionally.
This is a preparation for memory devices that consume multiple memslots.
Signed-off-by: David Hildenbrand
---
accel/kvm/kvm-all.c | 33 -
accel/stubs/kvm-stub.c | 4 ++--
hw/mem/memory-device.c | 2 +-
include/sysemu/kvm.h | 2 +-
include/sysemu/kvm_int.h | 1 +
5 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 235dc661bc..f39997d86e 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -109,9 +109,9 @@ int kvm_irqchip_remove_irqfd_notifier_gsi(KVMState *s,
EventNotifier *n,
return -ENOSYS;
}
-bool kvm_has_free_slot(MachineState *ms)
+unsigned int kvm_get_free_memslots(void)
{
-return false;
+return UINT_MAX;
Isn't it clearer returning 0 here and keeping kvm_enabled() below?
}
void kvm_init_cpu_signals(CPUState *cpu)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 667d56bd29..7c24685796 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -59,7 +59,7 @@ static void memory_device_check_addable(MachineState *ms,
MemoryRegion *mr,
const uint64_t size = memory_region_size(mr);
/* we will need a new memory slot for kvm and vhost */
-if (kvm_enabled() && !kvm_has_free_slot(ms)) {
+if (!kvm_get_free_memslots()) {
(here)
error_setg(errp, "hypervisor has no free memory slots left");
return;
}
diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index 511b42bde5..8b09e78b12 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -40,6 +40,7 @@ typedef struct KVMMemoryUpdate {
typedef struct KVMMemoryListener {
MemoryListener listener;
KVMSlot *slots;
+int nr_used_slots;
Preferably using 'unsigned' here:
Reviewed-by: Philippe Mathieu-Daudé
[PATCH v2 04/16] kvm: Return number of free memslots
Let's return the number of free slots instead of only checking if there
is a free slot. While at it, check all address spaces, which will also
consider SMM under x86 correctly.
Make the stub return UINT_MAX, such that we can call the function
unconditionally.
This is a preparation for memory devices that consume multiple memslots.
Signed-off-by: David Hildenbrand
---
accel/kvm/kvm-all.c | 33 -
accel/stubs/kvm-stub.c | 4 ++--
hw/mem/memory-device.c | 2 +-
include/sysemu/kvm.h | 2 +-
include/sysemu/kvm_int.h | 1 +
5 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d07f1ecbd3..0fcea923a1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -181,6 +181,24 @@ int kvm_get_max_memslots(void)
return s->nr_slots;
}
+unsigned int kvm_get_free_memslots(void)
+{
+unsigned int used_slots = 0;
+KVMState *s = kvm_state;
+int i;
+
+kvm_slots_lock();
+for (i = 0; i < s->nr_as; i++) {
+if (!s->as[i].ml) {
+continue;
+}
+used_slots = MAX(used_slots, s->as[i].ml->nr_used_slots);
+}
+kvm_slots_unlock();
+
+return s->nr_slots - used_slots;
+}
+
/* Called with KVMMemoryListener.slots_lock held */
static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
{
@@ -196,19 +214,6 @@ static KVMSlot *kvm_get_free_slot(KVMMemoryListener *kml)
return NULL;
}
-bool kvm_has_free_slot(MachineState *ms)
-{
-KVMState *s = KVM_STATE(ms->accelerator);
-bool result;
-KVMMemoryListener *kml = >memory_listener;
-
-kvm_slots_lock();
-result = !!kvm_get_free_slot(kml);
-kvm_slots_unlock();
-
-return result;
-}
-
/* Called with KVMMemoryListener.slots_lock held */
static KVMSlot *kvm_alloc_slot(KVMMemoryListener *kml)
{
@@ -1387,6 +1392,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
}
start_addr += slot_size;
size -= slot_size;
+kml->nr_used_slots--;
} while (size);
return;
}
@@ -1412,6 +1418,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
ram_start_offset += slot_size;
ram += slot_size;
size -= slot_size;
+kml->nr_used_slots++;
} while (size);
}
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 235dc661bc..f39997d86e 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -109,9 +109,9 @@ int kvm_irqchip_remove_irqfd_notifier_gsi(KVMState *s,
EventNotifier *n,
return -ENOSYS;
}
-bool kvm_has_free_slot(MachineState *ms)
+unsigned int kvm_get_free_memslots(void)
{
-return false;
+return UINT_MAX;
}
void kvm_init_cpu_signals(CPUState *cpu)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 667d56bd29..7c24685796 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -59,7 +59,7 @@ static void memory_device_check_addable(MachineState *ms,
MemoryRegion *mr,
const uint64_t size = memory_region_size(mr);
/* we will need a new memory slot for kvm and vhost */
-if (kvm_enabled() && !kvm_has_free_slot(ms)) {
+if (!kvm_get_free_memslots()) {
error_setg(errp, "hypervisor has no free memory slots left");
return;
}
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index ccaf55caf7..321427a543 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -216,7 +216,7 @@ typedef struct KVMRouteChange {
/* external API */
-bool kvm_has_free_slot(MachineState *ms);
+unsigned int kvm_get_free_memslots(void);
bool kvm_has_sync_mmu(void);
int kvm_has_vcpu_events(void);
int kvm_has_robust_singlestep(void);
diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index 511b42bde5..8b09e78b12 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -40,6 +40,7 @@ typedef struct KVMMemoryUpdate {
typedef struct KVMMemoryListener {
MemoryListener listener;
KVMSlot *slots;
+int nr_used_slots;
int as_id;
QSIMPLEQ_HEAD(, KVMMemoryUpdate) transaction_add;
QSIMPLEQ_HEAD(, KVMMemoryUpdate) transaction_del;
--
2.41.0
