Let's return the number of free slots instead of only checking if there
is a free slot. Required to support memory devices that consume multiple
memslots.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/mem/memory-device.c | 2 +-
hw/virtio/vhost-stub.c | 2 +-
hw/virtio/vhost.c | 4 ++--
include/hw/virtio/vhost.h | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index 9045ead33e..7f76a09e57 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -77,7 +77,7 @@ static void memory_device_check_addable(MachineState *ms, uint64_t size,
error_setg(errp, "hypervisor has no free memory slots left");
return;
}
- if (!vhost_has_free_slot()) {
+ if (!vhost_get_free_memslots()) {
error_setg(errp, "a used vhost backend has no free memory slots left");
return;
}
diff --git a/hw/virtio/vhost-stub.c b/hw/virtio/vhost-stub.c
index c175148fce..fe111e5e45 100644
--- a/hw/virtio/vhost-stub.c
+++ b/hw/virtio/vhost-stub.c
@@ -2,7 +2,7 @@
#include "hw/virtio/vhost.h"
#include "hw/virtio/vhost-user.h"
-bool vhost_has_free_slot(void)
+unsigned int vhost_get_free_memslots(void)
{
return true;
}
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 437347ad01..2707972870 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -48,7 +48,7 @@ static unsigned int used_memslots;
static QLIST_HEAD(, vhost_dev) vhost_devices =
QLIST_HEAD_INITIALIZER(vhost_devices);
-bool vhost_has_free_slot(void)
+unsigned int vhost_get_free_memslots(void)
{
unsigned int slots_limit = ~0U;
struct vhost_dev *hdev;
@@ -57,7 +57,7 @@ bool vhost_has_free_slot(void)
unsigned int r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
slots_limit = MIN(slots_limit, r);
}
- return slots_limit > used_memslots;
+ return slots_limit - used_memslots;
}
static void vhost_dev_sync_region(struct vhost_dev *dev,
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 3fa0b554ef..9d59fc1404 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -130,7 +130,7 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
uint64_t features);
-bool vhost_has_free_slot(void);
+unsigned int vhost_get_free_memslots(void);
int vhost_net_set_backend(struct vhost_dev *hdev,
struct vhost_vring_file *file);
--
2.31.1
On 10/27/21 14:45, David Hildenbrand wrote:
> Let's return the number of free slots instead of only checking if there
> is a free slot. Required to support memory devices that consume multiple
> memslots.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> hw/mem/memory-device.c | 2 +-
> hw/virtio/vhost-stub.c | 2 +-
> hw/virtio/vhost.c | 4 ++--
> include/hw/virtio/vhost.h | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
> --- a/hw/virtio/vhost-stub.c
> +++ b/hw/virtio/vhost-stub.c
> @@ -2,7 +2,7 @@
> #include "hw/virtio/vhost.h"
> #include "hw/virtio/vhost-user.h"
>
> -bool vhost_has_free_slot(void)
> +unsigned int vhost_get_free_memslots(void)
> {
> return true;
return 0;
> }
On 27.10.21 15:36, Philippe Mathieu-Daudé wrote:
> On 10/27/21 14:45, David Hildenbrand wrote:
>> Let's return the number of free slots instead of only checking if there
>> is a free slot. Required to support memory devices that consume multiple
>> memslots.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> hw/mem/memory-device.c | 2 +-
>> hw/virtio/vhost-stub.c | 2 +-
>> hw/virtio/vhost.c | 4 ++--
>> include/hw/virtio/vhost.h | 2 +-
>> 4 files changed, 5 insertions(+), 5 deletions(-)
>
>> --- a/hw/virtio/vhost-stub.c
>> +++ b/hw/virtio/vhost-stub.c
>> @@ -2,7 +2,7 @@
>> #include "hw/virtio/vhost.h"
>> #include "hw/virtio/vhost-user.h"
>>
>> -bool vhost_has_free_slot(void)
>> +unsigned int vhost_get_free_memslots(void)
>> {
>> return true;
>
> return 0;
Thanks, nice catch!
--
Thanks,
David / dhildenb
On 27.10.21 15:36, Philippe Mathieu-Daudé wrote:
> On 10/27/21 14:45, David Hildenbrand wrote:
>> Let's return the number of free slots instead of only checking if there
>> is a free slot. Required to support memory devices that consume multiple
>> memslots.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> hw/mem/memory-device.c | 2 +-
>> hw/virtio/vhost-stub.c | 2 +-
>> hw/virtio/vhost.c | 4 ++--
>> include/hw/virtio/vhost.h | 2 +-
>> 4 files changed, 5 insertions(+), 5 deletions(-)
>
>> --- a/hw/virtio/vhost-stub.c
>> +++ b/hw/virtio/vhost-stub.c
>> @@ -2,7 +2,7 @@
>> #include "hw/virtio/vhost.h"
>> #include "hw/virtio/vhost-user.h"
>>
>> -bool vhost_has_free_slot(void)
>> +unsigned int vhost_get_free_memslots(void)
>> {
>> return true;
>
> return 0;
Oh wait, no. This actually has to be
"return ~0U;" (see real vhost_get_free_memslots())
... because there is no vhost and consequently no limit applies.
--
Thanks,
David / dhildenb
On 10/27/21 16:04, David Hildenbrand wrote:
> On 27.10.21 15:36, Philippe Mathieu-Daudé wrote:
>> On 10/27/21 14:45, David Hildenbrand wrote:
>>> Let's return the number of free slots instead of only checking if there
>>> is a free slot. Required to support memory devices that consume multiple
>>> memslots.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> hw/mem/memory-device.c | 2 +-
>>> hw/virtio/vhost-stub.c | 2 +-
>>> hw/virtio/vhost.c | 4 ++--
>>> include/hw/virtio/vhost.h | 2 +-
>>> 4 files changed, 5 insertions(+), 5 deletions(-)
>>> -bool vhost_has_free_slot(void)
>>> +unsigned int vhost_get_free_memslots(void)
>>> {
>>> return true;
>>
>> return 0;
>
> Oh wait, no. This actually has to be
>
> "return ~0U;" (see real vhost_get_free_memslots())
>
> ... because there is no vhost and consequently no limit applies.
Indeed.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Wed, Oct 27, 2021 at 04:11:38PM +0200, Philippe Mathieu-Daudé wrote:
> On 10/27/21 16:04, David Hildenbrand wrote:
> > On 27.10.21 15:36, Philippe Mathieu-Daudé wrote:
> >> On 10/27/21 14:45, David Hildenbrand wrote:
> >>> Let's return the number of free slots instead of only checking if there
> >>> is a free slot. Required to support memory devices that consume multiple
> >>> memslots.
> >>>
> >>> Signed-off-by: David Hildenbrand <david@redhat.com>
> >>> ---
> >>> hw/mem/memory-device.c | 2 +-
> >>> hw/virtio/vhost-stub.c | 2 +-
> >>> hw/virtio/vhost.c | 4 ++--
> >>> include/hw/virtio/vhost.h | 2 +-
> >>> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> >>> -bool vhost_has_free_slot(void)
> >>> +unsigned int vhost_get_free_memslots(void)
> >>> {
> >>> return true;
> >>
> >> return 0;
> >
> > Oh wait, no. This actually has to be
> >
> > "return ~0U;" (see real vhost_get_free_memslots())
> >
> > ... because there is no vhost and consequently no limit applies.
>
> Indeed.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
confused. are you acking the theoretical patch with ~0 here?
On 27.10.21 17:33, Michael S. Tsirkin wrote:
> On Wed, Oct 27, 2021 at 04:11:38PM +0200, Philippe Mathieu-Daudé wrote:
>> On 10/27/21 16:04, David Hildenbrand wrote:
>>> On 27.10.21 15:36, Philippe Mathieu-Daudé wrote:
>>>> On 10/27/21 14:45, David Hildenbrand wrote:
>>>>> Let's return the number of free slots instead of only checking if there
>>>>> is a free slot. Required to support memory devices that consume multiple
>>>>> memslots.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>> hw/mem/memory-device.c | 2 +-
>>>>> hw/virtio/vhost-stub.c | 2 +-
>>>>> hw/virtio/vhost.c | 4 ++--
>>>>> include/hw/virtio/vhost.h | 2 +-
>>>>> 4 files changed, 5 insertions(+), 5 deletions(-)
>>
>>>>> -bool vhost_has_free_slot(void)
>>>>> +unsigned int vhost_get_free_memslots(void)
>>>>> {
>>>>> return true;
>>>>
>>>> return 0;
>>>
>>> Oh wait, no. This actually has to be
>>>
>>> "return ~0U;" (see real vhost_get_free_memslots())
>>>
>>> ... because there is no vhost and consequently no limit applies.
>>
>> Indeed.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> confused. are you acking the theoretical patch with ~0 here?
>
That's how I interpreted it.
--
Thanks,
David / dhildenb
On 10/27/21 17:45, David Hildenbrand wrote:
> On 27.10.21 17:33, Michael S. Tsirkin wrote:
>> On Wed, Oct 27, 2021 at 04:11:38PM +0200, Philippe Mathieu-Daudé wrote:
>>> On 10/27/21 16:04, David Hildenbrand wrote:
>>>> On 27.10.21 15:36, Philippe Mathieu-Daudé wrote:
>>>>> On 10/27/21 14:45, David Hildenbrand wrote:
>>>>>> Let's return the number of free slots instead of only checking if there
>>>>>> is a free slot. Required to support memory devices that consume multiple
>>>>>> memslots.
>>>>>>
>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>> ---
>>>>>> hw/mem/memory-device.c | 2 +-
>>>>>> hw/virtio/vhost-stub.c | 2 +-
>>>>>> hw/virtio/vhost.c | 4 ++--
>>>>>> include/hw/virtio/vhost.h | 2 +-
>>>>>> 4 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>>>>> -bool vhost_has_free_slot(void)
>>>>>> +unsigned int vhost_get_free_memslots(void)
>>>>>> {
>>>>>> return true;
>>>>>
>>>>> return 0;
>>>>
>>>> Oh wait, no. This actually has to be
>>>>
>>>> "return ~0U;" (see real vhost_get_free_memslots())
>>>>
>>>> ... because there is no vhost and consequently no limit applies.
>>>
>>> Indeed.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> confused. are you acking the theoretical patch with ~0 here?
>>
>
> That's how I interpreted it.
~0U doesn't seem harmful when comparing. However I haven't tested
nor looked at the big picture. I wonder if vhost_has_free_slot()
shouldn't take the Error* as argument and each implementation set
the error message ("virtio/vhost support disabled" would be more
explicit in the stub case). But I still don't understand why when
built without virtio/vhost we return vhost_get_free_memslots() > 0.
On 27.10.21 18:11, Philippe Mathieu-Daudé wrote:
> On 10/27/21 17:45, David Hildenbrand wrote:
>> On 27.10.21 17:33, Michael S. Tsirkin wrote:
>>> On Wed, Oct 27, 2021 at 04:11:38PM +0200, Philippe Mathieu-Daudé wrote:
>>>> On 10/27/21 16:04, David Hildenbrand wrote:
>>>>> On 27.10.21 15:36, Philippe Mathieu-Daudé wrote:
>>>>>> On 10/27/21 14:45, David Hildenbrand wrote:
>>>>>>> Let's return the number of free slots instead of only checking if there
>>>>>>> is a free slot. Required to support memory devices that consume multiple
>>>>>>> memslots.
>>>>>>>
>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>> ---
>>>>>>> hw/mem/memory-device.c | 2 +-
>>>>>>> hw/virtio/vhost-stub.c | 2 +-
>>>>>>> hw/virtio/vhost.c | 4 ++--
>>>>>>> include/hw/virtio/vhost.h | 2 +-
>>>>>>> 4 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>>>>> -bool vhost_has_free_slot(void)
>>>>>>> +unsigned int vhost_get_free_memslots(void)
>>>>>>> {
>>>>>>> return true;
>>>>>>
>>>>>> return 0;
>>>>>
>>>>> Oh wait, no. This actually has to be
>>>>>
>>>>> "return ~0U;" (see real vhost_get_free_memslots())
>>>>>
>>>>> ... because there is no vhost and consequently no limit applies.
>>>>
>>>> Indeed.
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>> confused. are you acking the theoretical patch with ~0 here?
>>>
>>
>> That's how I interpreted it.
>
> ~0U doesn't seem harmful when comparing. However I haven't tested
> nor looked at the big picture. I wonder if vhost_has_free_slot()
> shouldn't take the Error* as argument and each implementation set
> the error message ("virtio/vhost support disabled" would be more
> explicit in the stub case). But I still don't understand why when
> built without virtio/vhost we return vhost_get_free_memslots() > 0.
For the same reason we faked infinite slots via
vhost_has_free_slot()->true for now. We call it unconditionally from
memory device code.
Sure, we could add a stub "vhost_available()-> false" (or
vhost_enabled() ?) instead and do
if (vhost_available())
... vhost_get_free_memslots()
similar to how we have
if (kvm_enabled())
... kvm_get_free_memslots()
Not sure if it's worth it, though.
--
Thanks,
David / dhildenb
© 2016 - 2026 Red Hat, Inc.