Re: [Qemu-devel] [PATCH for-2.4 v3 1/3] vhost: add vhost_has_free_slot() interface

[Michael S. Tsirkin]

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 <address@hidden>
> ---
> 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.


> 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