virtio: split device initialization to separate functions

Previously we used to manually set
vdev->max_queues and called virtio_dev_restart
to go through all virtio init states, negotiate
features and allocate virtqueues. This is,
however, insufficient for Virtio-Blk, where we
e.g. need to check against negotiated multiqueue
flag before deciding how many queues we can use
(reading num_queues field from device config is
forbidden unless VIRTIO_BLK_F_MQ is negotiated).

This patch refactors queue-num related code
and also removes various restrictions. If device
supports less queues than requested, a warning
will be printed during initialization, but
the device will now continue to init normally.

The queue-num negotiation for virtio-user should
be eventually moved to upper layers, but that is
not necessary for now.

Change-Id: I418b56fa62c17b547243422ea077f0d76555bd13
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/393087
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>

include/spdk_internal/virtio.h

@@ -67,7 +67,10 @@ struct virtio_dev {
 	/** Name of this virtio dev set by backend */
 	char		*name;
 
-	/** Max number of queues the host supports. */
+	/** Fixed number of backend-specific non-I/O virtqueues. */
+	uint16_t	fixed_queues_num;
+
+	/** Max number of virtqueues the host supports. */
 	uint16_t	max_queues;
 
 	/** Common device & guest features. */
@@ -256,20 +259,36 @@ int virtio_dev_construct(struct virtio_dev *vdev, const struct virtio_dev_ops *o
 			 void *ctx);
 
 /**
- * Notify the host to start processing this virtio device.  This is
+ * Reset the device and prepare it to be `virtio_dev_start`ed.  This call
- * a blocking call that won't return until the host has started.
+ * will also renegotiate feature flags.
- * This call will also allocate virtqueues and renegotiate feature flags.
  *
  * \param vdev virtio device
  * \param req_features features this driver supports. A VIRTIO_F_VERSION_1
  * flag will be automatically appended, as legacy devices are not supported.
  */
-int virtio_dev_restart(struct virtio_dev *vdev, uint64_t req_features);
+int virtio_dev_reset(struct virtio_dev *vdev, uint64_t req_features);
+
+/**
+ * Notify the host to start processing this virtio device.  This is
+ * a blocking call that won't return until the host has started.
+ * This will also allocate virtqueues.
+ *
+ * \param vdev virtio device
+ * \param max_queues number of queues to allocate. The max number of
+ * usable I/O queues is also limited by the host device. `vdev` will be
+ * started successfully even if the host supports less queues than requested.
+ * \param fixed_queue_num number of queues preceeding the first
+ * request queue. For Virtio-SCSI this is equal to 2, as there are
+ * additional event and control queues.
+ */
+int virtio_dev_start(struct virtio_dev *vdev, uint16_t max_queues,
+		     uint16_t fixed_queues_num);
 
 /**
  * Stop the host from processing the device.  This is a blocking call
  * that won't return until all outstanding I/O has been processed on
- * the host (virtio device) side.
+ * the host (virtio device) side. In order to re-start the device, it
+ * has to be `virtio_dev_reset` first.
  *
  * \param vdev virtio device
  */
@@ -426,23 +445,14 @@ int virtio_pci_scsi_dev_enumerate(virtio_pci_create_cb enum_cb);
  * \param vdev preallocated vhost device struct to operate on
  * \param name name of this virtio device
  * \param path path to the Unix domain socket of the vhost-user device
- * \param requested_queues maximum number of request queues that this
- * device will support
  * \param queue_size size of each of the queues
- * \param fixed_queue_num number of queues preceeding the first
- * request queue. For Virtio-SCSI this is equal to 2, as there are
- * additional event and control queues.
  * \return virtio device
  */
 int virtio_user_dev_init(struct virtio_dev *vdev, const char *name, const char *path,
-			 uint16_t requested_queues, uint32_t queue_size,
+			 uint32_t queue_size);
-			 uint16_t fixed_queue_num);
 
 /**
- * Initialize a virtio_dev for the given PCI device.
+ * Initialize virtio_dev for a given PCI device.
- * The virtio_dev will try to use \c SPDK_VIRTIO_MAX_VIRTQUEUES queues by
- * default and might fail to start. It is advised to overwrite the
- * `virtio_dev->max_queues` field manually starting the device.
  * The virtio_dev has to be freed with \c virtio_dev_destruct.
  *
  * \param vdev preallocated vhost device struct to operate on

lib/bdev/virtio/bdev_virtio_scsi.c

@@ -199,7 +199,7 @@ virtio_scsi_dev_send_eventq_io(struct virtqueue *vq, struct virtio_scsi_eventq_i
 }
 
 static int
-virtio_scsi_dev_init(struct virtio_scsi_dev *svdev)
+virtio_scsi_dev_init(struct virtio_scsi_dev *svdev, uint16_t max_queues)
 {
 	struct virtio_dev *vdev = &svdev->vdev;
 	struct spdk_ring *ctrlq_ring;
@@ -208,7 +208,12 @@ virtio_scsi_dev_init(struct virtio_scsi_dev *svdev)
 	uint16_t i, num_events;
 	int rc;
 
-	rc = virtio_dev_restart(vdev, VIRTIO_SCSI_DEV_SUPPORTED_FEATURES);
+	rc = virtio_dev_reset(vdev, VIRTIO_SCSI_DEV_SUPPORTED_FEATURES);
+	if (rc != 0) {
+		return rc;
+	}
+
+	rc = virtio_dev_start(vdev, max_queues, SPDK_VIRTIO_SCSI_QUEUE_NUM_FIXED);
 	if (rc != 0) {
 		return rc;
 	}
@@ -305,9 +310,8 @@ virtio_pci_scsi_dev_create_cb(struct virtio_pci_ctx *pci_ctx)
 
 	virtio_dev_read_dev_config(vdev, offsetof(struct virtio_scsi_config, num_queues),
 				   &num_queues, sizeof(num_queues));
-	vdev->max_queues = SPDK_VIRTIO_SCSI_QUEUE_NUM_FIXED + num_queues;
 
-	rc = virtio_scsi_dev_init(svdev);
+	rc = virtio_scsi_dev_init(svdev, num_queues);
 	if (rc != 0) {
 		virtio_dev_destruct(vdev);
 		free(svdev);
@@ -331,15 +335,14 @@ virtio_user_scsi_dev_create(const char *name, const char *path,
 	}
 
 	vdev = &svdev->vdev;
-	rc = virtio_user_dev_init(vdev, name, path, num_queues, queue_size,
+	rc = virtio_user_dev_init(vdev, name, path, queue_size);
-				  SPDK_VIRTIO_SCSI_QUEUE_NUM_FIXED);
 	if (rc != 0) {
 		SPDK_ERRLOG("Failed to create virito device %s: %s\n", name, path);
 		free(svdev);
 		return NULL;
 	}
 
-	rc = virtio_scsi_dev_init(svdev);
+	rc = virtio_scsi_dev_init(svdev, num_queues);
 	if (rc != 0) {
 		virtio_dev_destruct(vdev);
 		free(svdev);

lib/virtio/virtio.c

@@ -261,16 +261,19 @@ virtio_free_queues(struct virtio_dev *dev)
 }
 
 static int
-virtio_alloc_queues(struct virtio_dev *dev)
+virtio_alloc_queues(struct virtio_dev *dev, uint16_t request_vq_num, uint16_t fixed_vq_num)
 {
-	uint16_t nr_vq = dev->max_queues;
+	uint16_t nr_vq;
 	uint16_t i;
 	int ret;
 
-	if (dev->vqs != NULL) {
+	nr_vq = request_vq_num + fixed_vq_num;
+	if (nr_vq == 0) {
+		/* perfectly fine to have a device with no virtqueues. */
 		return 0;
 	}
 
+	assert(dev->vqs == NULL);
 	dev->vqs = rte_zmalloc(NULL, sizeof(struct virtqueue *) * nr_vq, 0);
 	if (!dev->vqs) {
 		SPDK_ERRLOG("failed to allocate %"PRIu16" vqs\n", nr_vq);
@@ -285,6 +288,8 @@ virtio_alloc_queues(struct virtio_dev *dev)
 		}
 	}
 
+	dev->max_queues = nr_vq;
+	dev->fixed_queues_num = fixed_vq_num;
 	return 0;
 }
 
@@ -330,10 +335,8 @@ virtio_dev_construct(struct virtio_dev *vdev, const struct virtio_dev_ops *ops,
 }
 
 int
-virtio_dev_restart(struct virtio_dev *dev, uint64_t req_features)
+virtio_dev_reset(struct virtio_dev *dev, uint64_t req_features)
 {
-	int ret;
-
 	req_features |= (1ULL << VIRTIO_F_VERSION_1);
 
 	/* Reset the device although not necessary at startup */
@@ -344,23 +347,27 @@ virtio_dev_restart(struct virtio_dev *dev, uint64_t req_features)
 
 	/* Tell the host we've known how to drive the device. */
 	virtio_dev_set_status(dev, VIRTIO_CONFIG_S_DRIVER);
-	if (virtio_negotiate_features(dev, req_features) < 0) {
-		return -1;
-	}
 
-	ret = virtio_alloc_queues(dev);
+	return virtio_negotiate_features(dev, req_features);
+}
+
+int
+virtio_dev_start(struct virtio_dev *vdev, uint16_t max_queues, uint16_t fixed_queue_num)
+{
+	int ret;
+
+	ret = virtio_alloc_queues(vdev, max_queues, fixed_queue_num);
 	if (ret < 0) {
 		return ret;
 	}
 
-	virtio_dev_set_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+	virtio_dev_set_status(vdev, VIRTIO_CONFIG_S_DRIVER_OK);
 	return 0;
 }
 
 void
 virtio_dev_destruct(struct virtio_dev *dev)
 {
-	virtio_free_queues(dev);
 	virtio_dev_backend_ops(dev)->destruct_dev(dev);
 	pthread_mutex_destroy(&dev->mutex);
 }
@@ -687,6 +694,7 @@ virtio_dev_stop(struct virtio_dev *dev)
 	virtio_dev_backend_ops(dev)->set_status(dev, VIRTIO_CONFIG_S_RESET);
 	/* flush status write */
 	virtio_dev_backend_ops(dev)->get_status(dev);
+	virtio_free_queues(dev);
 }
 
 void

lib/virtio/virtio_pci.c

@@ -508,7 +508,6 @@ virtio_pci_dev_init(struct virtio_dev *vdev, const char *name,
 	vdev->name = name_dup;
 	vdev->is_hw = 1;
 	vdev->modern = 1;
-	vdev->max_queues = SPDK_VIRTIO_MAX_VIRTQUEUES;
 
 	return 0;
 }

lib/virtio/virtio_user.c

@@ -134,8 +134,23 @@ static int
 virtio_user_start_device(struct virtio_dev *vdev)
 {
 	struct virtio_user_dev *dev = vdev->ctx;
+	uint64_t host_max_queues;
 	int ret;
 
+	/* negotiate the number of I/O queues. */
+	ret = dev->ops->send_request(dev, VHOST_USER_GET_QUEUE_NUM, &host_max_queues);
+	if (ret < 0) {
+		return -1;
+	}
+
+	if (vdev->max_queues > host_max_queues + vdev->fixed_queues_num) {
+		SPDK_WARNLOG("%s: requested %"PRIu16" request queues"
+			     "but only %"PRIu64" available\n",
+			     vdev->name, vdev->max_queues - vdev->fixed_queues_num,
+			     host_max_queues);
+		vdev->max_queues = host_max_queues;
+	}
+
 	/* tell vhost to create queues */
 	if (virtio_user_queue_setup(vdev, virtio_user_create_queue) < 0) {
 		return -1;
@@ -387,19 +402,15 @@ static const struct virtio_dev_ops virtio_user_ops = {
 
 int
 virtio_user_dev_init(struct virtio_dev *vdev, const char *name, const char *path,
-		     uint16_t requested_queues, uint32_t queue_size, uint16_t fixed_queue_num)
+		     uint32_t queue_size)
 {
 	struct virtio_user_dev *dev;
-	uint64_t max_queues;
 	char err_str[64];
 	int rc;
 
 	if (name == NULL) {
 		SPDK_ERRLOG("No name gived for controller: %s\n", path);
 		return -1;
-	} else if (requested_queues == 0) {
-		SPDK_ERRLOG("Can't create controller with no queues: %s\n", path);
-		return -1;
 	}
 
 	dev = calloc(1, sizeof(*dev));
@@ -429,20 +440,6 @@ virtio_user_dev_init(struct virtio_dev *vdev, const char *name, const char *path
 		goto err;
 	}
 
-	if (dev->ops->send_request(dev, VHOST_USER_GET_QUEUE_NUM, &max_queues) < 0) {
-		spdk_strerror_r(errno, err_str, sizeof(err_str));
-		SPDK_ERRLOG("get_queue_num fails: %s\n", err_str);
-		goto err;
-	}
-
-	if (requested_queues > max_queues) {
-		SPDK_ERRLOG("requested %"PRIu16" request queues but only %"PRIu64" available\n",
-			    requested_queues, max_queues);
-		goto err;
-	}
-
-	vdev->max_queues = fixed_queue_num + requested_queues;
-
 	if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL) < 0) {
 		spdk_strerror_r(errno, err_str, sizeof(err_str));
 		SPDK_ERRLOG("set_owner fails: %s\n", err_str);