| Message ID | 20211027124531.57561-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-mem: Expose device memory via multiple memslots | expand |
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!
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.
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.
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.
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);
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(-)