On Thu, 30 Jul 2015 18:28:05 +0300
"Michael S. Tsirkin" <m...@redhat.com> wrote:
> On Thu, Jul 30, 2015 at 12:11:57PM +0200, Igor Mammedov wrote:
> > it will allow for other parts of QEMU check if it's safe
> > to map memory region during hotplug/runtime.
> > That way hotplug path will have a chance to cancel
> > hotplug operation instead of crashing in vhost_commit().
> >
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > ---
> > v3:
> > * add refcountin of limit by # vhost devices,
> > and make vhost_has_free_slot() return true if no vhost devices
> > are present at current moment
> > * move limit initialization to vhost_dev_init()
> > v2:
> > * replace probbing with checking for
> > /sys/module/vhost/parameters/max_mem_regions and
> > if it's missing has non wrong value return
> > hardcoded legacy limit (64 slots).
> > ---
> > hw/virtio/vhost-backend.c | 21 ++++++++++++++++++++-
> > hw/virtio/vhost-user.c | 8 +++++++-
> > hw/virtio/vhost.c | 23 +++++++++++++++++++++++
> > include/hw/virtio/vhost-backend.h | 2 ++
> > include/hw/virtio/vhost.h | 1 +
> > stubs/Makefile.objs | 1 +
> > stubs/vhost.c | 6 ++++++
> > 7 files changed, 60 insertions(+), 2 deletions(-)
> > create mode 100644 stubs/vhost.c
> >
> > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > index 4d68a27..11ec669 100644
> > --- a/hw/virtio/vhost-backend.c
> > +++ b/hw/virtio/vhost-backend.c
> > @@ -11,6 +11,7 @@
> > #include "hw/virtio/vhost.h"
> > #include "hw/virtio/vhost-backend.h"
> > #include "qemu/error-report.h"
> > +#include "linux/vhost.h"
> >
> > #include <sys/ioctl.h>
> >
> > @@ -42,11 +43,29 @@ static int vhost_kernel_cleanup(struct vhost_dev *dev)
> > return close(fd);
> > }
> >
> > +static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
> > +{
> > + int limit = 64;
> > + char *s;
> > +
> > + if (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
> > + &s, NULL, NULL)) {
> > + uint64_t val = g_ascii_strtoull(s, NULL, 10);
> > + if (!((val == G_MAXUINT64 || !val) && errno)) {
> > + return val;
> > + }
> > + error_report("ignoring invalid max_mem_regions value in vhost
> > module:"
> > + " %s", s);
> > + }
> > + return limit;
> > +}
> > +
> > static const VhostOps kernel_ops = {
> > .backend_type = VHOST_BACKEND_TYPE_KERNEL,
> > .vhost_call = vhost_kernel_call,
> > .vhost_backend_init = vhost_kernel_init,
> > - .vhost_backend_cleanup = vhost_kernel_cleanup
> > + .vhost_backend_cleanup = vhost_kernel_cleanup,
> > + .vhost_backend_memslots_limit = vhost_kernel_memslots_limit
> > };
> >
> > int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType
> > backend_type)
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index e7ab829..acdfd04 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -343,9 +343,15 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
> > return 0;
> > }
> >
> > +static int vhost_user_memslots_limit(struct vhost_dev *dev)
> > +{
> > + return VHOST_MEMORY_MAX_NREGIONS;
> > +}
> > +
> > const VhostOps user_ops = {
> > .backend_type = VHOST_BACKEND_TYPE_USER,
> > .vhost_call = vhost_user_call,
> > .vhost_backend_init = vhost_user_init,
> > - .vhost_backend_cleanup = vhost_user_cleanup
> > + .vhost_backend_cleanup = vhost_user_cleanup,
> > + .vhost_backend_memslots_limit = vhost_user_memslots_limit
> > };
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 2712c6f..bcbad48 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -26,6 +26,18 @@
> >
> > static struct vhost_log *vhost_log;
> >
> > +static struct {
> > + int used_memslots;
> > + int memslots_limit;
> > + int refcount;
> > +} slots_limit;
> > +
> > +bool vhost_has_free_slot(void)
> > +{
> > + return slots_limit.refcount ?
> > + slots_limit.memslots_limit > slots_limit.used_memslots : true;
> > +}
> > +
> > static void vhost_dev_sync_region(struct vhost_dev *dev,
> > MemoryRegionSection *section,
> > uint64_t mfirst, uint64_t mlast,
> > @@ -457,6 +469,7 @@ static void vhost_set_memory(MemoryListener *listener,
> > dev->mem_changed_start_addr = MIN(dev->mem_changed_start_addr,
> > start_addr);
> > dev->mem_changed_end_addr = MAX(dev->mem_changed_end_addr, start_addr
> > + size - 1);
> > dev->memory_changed = true;
> > + slots_limit.used_memslots = dev->mem->nregions;
> > }
> >
> > static bool vhost_section(MemoryRegionSection *section)
> > @@ -916,6 +929,14 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> > *opaque,
> > return -errno;
> > }
> >
> > + r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
> > + if (slots_limit.refcount > 0) {
> > + slots_limit.memslots_limit = MIN(slots_limit.memslots_limit, r);
> > + } else {
> > + slots_limit.memslots_limit = r;
> > + }
> > + slots_limit.refcount++;
> > +
> > r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_OWNER, NULL);
> > if (r < 0) {
> > goto fail;
> > @@ -972,6 +993,7 @@ fail_vq:
> > fail:
> > r = -errno;
> > hdev->vhost_ops->vhost_backend_cleanup(hdev);
> > + slots_limit.refcount--;
> > return r;
> > }
> >
> > @@ -989,6 +1011,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> > g_free(hdev->mem);
> > g_free(hdev->mem_sections);
> > hdev->vhost_ops->vhost_backend_cleanup(hdev);
> > + slots_limit.refcount--;
> > }
> >
> > /* Stop processing guest IO notifications in qemu.
>
> This is not enough in case one is mixing vhost user and vhost kernel devices.
> One has to scan all devices.
why it's not enough?
vhost-user as vhost-kernel backends call
- vhost_dev_init() on init path so slots_limit.refcount++
should count all added backends
- vhost_dev_cleanup() on deinit path so slots_limit.refcount--
should take care of unplugged device counting
>
>
> > diff --git a/include/hw/virtio/vhost-backend.h
> > b/include/hw/virtio/vhost-backend.h
> > index e472f29..28b6714 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -24,12 +24,14 @@ typedef int (*vhost_call)(struct vhost_dev *dev,
> > unsigned long int request,
> > void *arg);
> > typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> > typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> > +typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
> >
> > typedef struct VhostOps {
> > VhostBackendType backend_type;
> > vhost_call vhost_call;
> > vhost_backend_init vhost_backend_init;
> > vhost_backend_cleanup vhost_backend_cleanup;
> > + vhost_backend_memslots_limit vhost_backend_memslots_limit;
> > } VhostOps;
> >
> > extern const VhostOps user_ops;
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index dd51050..17ff7b6 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -81,4 +81,5 @@ 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);
> > #endif
> > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> > index 9937a12..d2f1b21 100644
> > --- a/stubs/Makefile.objs
> > +++ b/stubs/Makefile.objs
> > @@ -38,3 +38,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
> > stub-obj-y += cpus.o
> > stub-obj-y += kvm.o
> > stub-obj-y += qmp_pc_dimm_device_list.o
> > +stub-obj-y += vhost.o
> > diff --git a/stubs/vhost.c b/stubs/vhost.c
> > new file mode 100644
> > index 0000000..d346b85
> > --- /dev/null
> > +++ b/stubs/vhost.c
> > @@ -0,0 +1,6 @@
> > +#include "hw/virtio/vhost.h"
> > +
> > +bool vhost_has_free_slot(void)
> > +{
> > + return true;
> > +}
> > --
> > 1.8.3.1
