[dpdk-dev] [RFC 0/2] vhost-user: add VHOST_USER_SET_QUEUE_NUM support

[dpdk-dev] [RFC 0/2] vhost-user: add VHOST_USER_SET_QUEUE_NUM support

[Maxime Coquelin]

Hi this two-patches series first ensure that no queues will be allocated
while the device is in a running state, and then implements support for
VHOST_USER_SET_QUEUE_NUM vhost-user protocol request.

The goal is to prevent the device to either corrupt some guest memory
when disabling guest notifications, or to be stuck waiting forever for
queues not setup by the guest driver to be initialized, depending on QEMU
version.

One way to reproduce these kind of issues is to start a Windows guest with
setting more queue pairs in QEMU cmdline than vcpus.

I tagged this series as RFC, because the QEMU part [0], which contains
the spec update isn't acked yet.

[0]: http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg02648.html

Maxime Coquelin (2):
  vhost-user: don't allocate new queue once device is running
  vhost-user: add support for VHOST_USER_SET_QUEUE_NUM

 lib/librte_vhost/vhost_user.c | 50 ++++++++++++++++++++++++++++++++++++++++---
 lib/librte_vhost/vhost_user.h |  5 ++++-
 2 files changed, 51 insertions(+), 4 deletions(-)

-- 
2.14.3

[dpdk-dev] [RFC 1/2] vhost-user: don't allocate new queue once device is running

[Maxime Coquelin]

Once the device is created, it is not possible to hot-add new
queues. If it happens, it could confuse the application, as
the new queue might not be initialized but nr_vring is
incremented.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index e54795a41..f94fd16cf 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1220,13 +1220,23 @@ vhost_user_check_and_alloc_queue_pair(struct virtio_net *dev, VhostUserMsg *msg)
 	if (vring_idx >= VHOST_MAX_VRING) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"invalid vring index: %u\n", vring_idx);
-		return -1;
+		return -EINVAL;
 	}
 
 	if (dev->virtqueue[vring_idx])
 		return 0;
 
-	return alloc_vring_queue(dev, vring_idx);
+	/* Queues cannot be added dynamically */
+	if (dev->flags & VIRTIO_DEV_RUNNING) {
+		RTE_LOG(DEBUG, VHOST_CONFIG,
+				"Cannot allocate new queue, device is running\n");
+		return -EPERM;
+	}
+
+	if (alloc_vring_queue(dev, vring_idx) < 0)
+		return -ENOMEM;
+
+	return 0;
 }
 
 int
@@ -1274,7 +1284,10 @@ vhost_user_msg_handler(int vid, int fd)
 			vhost_message_str[msg.request.master]);
 
 	ret = vhost_user_check_and_alloc_queue_pair(dev, &msg);
-	if (ret < 0) {
+	if (ret == -EPERM) {
+		RTE_LOG(DEBUG, VHOST_CONFIG, "Skipping message\n");
+			goto out;
+	} else if (ret < 0) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"failed to alloc queue\n");
 		return -1;
@@ -1383,6 +1396,7 @@ vhost_user_msg_handler(int vid, int fd)
 
 	}
 
+out:
 	if (msg.flags & VHOST_USER_NEED_REPLY) {
 		msg.payload.u64 = !!ret;
 		msg.size = sizeof(msg.payload.u64);
-- 
2.14.3

[dpdk-dev] [RFC 2/2] vhost-user: add support for VHOST_USER_SET_QUEUE_NUM

[Maxime Coquelin]

With some guest drivers, the number of queues setup by the
guest driver can be less than the number of queues declared in Qemu.

This is the case for example with Windows virtio-net driver, which
only setups as much queue pairs as the number of vCPUs.

When it happens, depending on QEMU version, the vhost-user port can:
- Either be started with some uninitialized queues, which ends up
corrupting GPA at 0 when guest notifications get disabled,
- Or be stuck forever, waiting for uninitialized rings to be ready.

This patch implements handling of VHOST_USER_SET_QUEUE_NUM new
protocol request, which is sent by QEMU to report the number
of queue pairs initialized by the guest driver.

When received, all unused queues get removed from the vhost-user
port, taking care that it is not already running.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 30 ++++++++++++++++++++++++++++++
 lib/librte_vhost/vhost_user.h |  5 ++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index f94fd16cf..73388027e 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -50,6 +50,7 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_NET_SET_MTU]  = "VHOST_USER_NET_SET_MTU",
 	[VHOST_USER_SET_SLAVE_REQ_FD]  = "VHOST_USER_SET_SLAVE_REQ_FD",
 	[VHOST_USER_IOTLB_MSG]  = "VHOST_USER_IOTLB_MSG",
+	[VHOST_USER_SET_QUEUE_NUM] = "VHOST_USER_SET_QUEUE_NUM",
 };
 
 static uint64_t
@@ -1137,6 +1138,32 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, struct VhostUserMsg *msg)
 	return 0;
 }
 
+static int
+vhost_user_set_queue_num(struct virtio_net *dev, struct VhostUserMsg *msg)
+{
+	uint64_t queue_num = msg->payload.u64 * 2; /* Payload is nr of qpairs */
+
+	if (dev->nr_vring > queue_num && dev->flags & VIRTIO_DEV_RUNNING) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+				"Cannot remove queues while device is running\n");
+		return -1;
+	}
+
+	while (dev->nr_vring > queue_num) {
+		struct vhost_virtqueue *vq;
+
+		vq = dev->virtqueue[--dev->nr_vring];
+		if (!vq)
+			continue;
+
+		dev->virtqueue[dev->nr_vring] = NULL;
+		cleanup_vq(vq, 1);
+		free_vq(vq);
+	}
+
+	return 0;
+}
+
 /* return bytes# of read on success or negative val on failure. */
 static int
 read_vhost_message(int sockfd, struct VhostUserMsg *msg)
@@ -1389,6 +1416,9 @@ vhost_user_msg_handler(int vid, int fd)
 	case VHOST_USER_IOTLB_MSG:
 		ret = vhost_user_iotlb_msg(&dev, &msg);
 		break;
+	case VHOST_USER_SET_QUEUE_NUM:
+		vhost_user_set_queue_num(dev, &msg);
+		break;
 
 	default:
 		ret = -1;
diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index d4bd604b9..ab6557d9e 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -20,13 +20,15 @@
 #define VHOST_USER_PROTOCOL_F_REPLY_ACK	3
 #define VHOST_USER_PROTOCOL_F_NET_MTU 4
 #define VHOST_USER_PROTOCOL_F_SLAVE_REQ 5
+#define VHOST_USER_PROTOCOL_F_SET_QUEUE_NUM 7
 
 #define VHOST_USER_PROTOCOL_FEATURES	((1ULL << VHOST_USER_PROTOCOL_F_MQ) | \
 					 (1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD) |\
 					 (1ULL << VHOST_USER_PROTOCOL_F_RARP) | \
 					 (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK) | \
 					 (1ULL << VHOST_USER_PROTOCOL_F_NET_MTU) | \
-					 (1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ))
+					 (1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ) | \
+					 (1ULL << VHOST_USER_PROTOCOL_F_SET_QUEUE_NUM))
 
 typedef enum VhostUserRequest {
 	VHOST_USER_NONE = 0,
@@ -52,6 +54,7 @@ typedef enum VhostUserRequest {
 	VHOST_USER_NET_SET_MTU = 20,
 	VHOST_USER_SET_SLAVE_REQ_FD = 21,
 	VHOST_USER_IOTLB_MSG = 22,
+	VHOST_USER_SET_QUEUE_NUM = 24,
 	VHOST_USER_MAX
 } VhostUserRequest;
 
-- 
2.14.3