[PATCH v2 04/16] kvm: Return number of free memslots

David Hildenbrand posted 16 patches 1 year, 3 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Xiao Guangrong <xiaoguangrong.eric@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Peter Xu <peterx@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>
There is a newer version of this series
[PATCH v2 04/16] kvm: Return number of free memslots
Posted by David Hildenbrand 1 year, 3 months ago
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 <david@redhat.com>
---
 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 = &s->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
Re: [PATCH v2 04/16] kvm: Return number of free memslots
Posted by Philippe Mathieu-Daudé 1 year, 3 months ago
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 <david@redhat.com>
> ---
>   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é <philmd@linaro.org>



Re: [PATCH v2 04/16] kvm: Return number of free memslots
Posted by David Hildenbrand 1 year, 2 months ago
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 <david@redhat.com>
>> ---
>>    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é <philmd@linaro.org>

Thanks!

-- 
Cheers,

David / dhildenb


Re: [PATCH v2 04/16] kvm: Return number of free memslots
Posted by David Hildenbrand 1 year, 2 months ago
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 <david@redhat.com>
>>> ---
>>>     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