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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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@xxxxxxxxxx>
>>>
>>> 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
