kvmtool: vhost MQ support

kvmtool: vhost MQ support

[BARBALACE Antonio]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

This patch enables vhost MQ to support in kvmtool without any Linux kernel modification.
The patch takes the same approach as QEMU -- for each queue pair a new /dev/vhost-net fd is created.
Fds are kept in ndev->vhost_fds, with ndev->vhost_fd == ndev->vhost_fds[0] (to avoid further modification to the existent source code).
Thanks, Antonio Barbalace
The University of Edinburgh is a charitable body, registered in Scotland, with registration number SC005336.

[-- Attachment #2: kvmtool-vhost_mq.patch --]
[-- Type: application/octet-stream, Size: 6423 bytes --]

diff --git a/virtio/net.c b/virtio/net.c
index 1ee3c19..bae3019 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -58,6 +58,7 @@ struct net_dev {
 	u32				features, queue_pairs;
 
 	int				vhost_fd;
+	int				vhost_fds[VIRTIO_NET_NUM_QUEUES];
 	int				tap_fd;
 	char				tap_name[IFNAMSIZ];
 	bool				tap_ufo;
@@ -512,6 +513,7 @@ static int virtio_net__vhost_set_features(struct net_dev *ndev)
 {
 	u64 features = 1UL << VIRTIO_RING_F_EVENT_IDX;
 	u64 vhost_features;
+	int i, r = 0;
 
 	if (ioctl(ndev->vhost_fd, VHOST_GET_FEATURES, &vhost_features) != 0)
 		die_perror("VHOST_GET_FEATURES failed");
@@ -521,7 +523,9 @@ static int virtio_net__vhost_set_features(struct net_dev *ndev)
 			has_virtio_feature(ndev, VIRTIO_NET_F_MRG_RXBUF))
 		features |= 1UL << VIRTIO_NET_F_MRG_RXBUF;
 
-	return ioctl(ndev->vhost_fd, VHOST_SET_FEATURES, &features);
+	for (i=0; ((u32)i < ndev->queue_pairs) && (r >= 0); i++)
+		r = ioctl(ndev->vhost_fds[i], VHOST_SET_FEATURES, &features); 
+	return r;
 }
 
 static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
@@ -578,7 +582,7 @@ static bool is_ctrl_vq(struct net_dev *ndev, u32 vq)
 static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
 		   u32 pfn)
 {
-	struct vhost_vring_state state = { .index = vq };
+	struct vhost_vring_state state = { .index = (vq %2) };
 	struct net_dev_queue *net_queue;
 	struct vhost_vring_addr addr;
 	struct net_dev *ndev = dev;
@@ -619,23 +623,24 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
 	if (queue->endian != VIRTIO_ENDIAN_HOST)
 		die_perror("VHOST requires the same endianness in guest and host");
 
-	state.num = queue->vring.num;
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_NUM, &state);
+	state.num = queue->vring.num; //number of decriptors
+        r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_NUM, &state);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_NUM failed");
-	state.num = 0;
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_BASE, &state);
+
+	state.num = 0; //descriptors base
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_BASE, &state);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_BASE failed");
 
 	addr = (struct vhost_vring_addr) {
-		.index = vq,
+		.index = (vq %2),
 		.desc_user_addr = (u64)(unsigned long)queue->vring.desc,
 		.avail_user_addr = (u64)(unsigned long)queue->vring.avail,
 		.used_user_addr = (u64)(unsigned long)queue->vring.used,
 	};
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_ADDR, &addr);
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_ADDR, &addr);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_ADDR failed");
 
@@ -659,7 +664,7 @@ static void exit_vq(struct kvm *kvm, void *dev, u32 vq)
 	 */
 	if (ndev->vhost_fd && !is_ctrl_vq(ndev, vq)) {
 		pr_warning("Cannot reset VHOST queue");
-		ioctl(ndev->vhost_fd, VHOST_RESET_OWNER);
+		ioctl(ndev->vhost_fds[(vq /2)], VHOST_RESET_OWNER);
 		return;
 	}
 
@@ -682,7 +687,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
 		return;
 
 	file = (struct vhost_vring_file) {
-		.index	= vq,
+		.index	= (vq % 2),
 		.fd	= eventfd(0, 0),
 	};
 
@@ -693,31 +698,32 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
 	queue->irqfd = file.fd;
 	queue->gsi = gsi;
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_CALL, &file);
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_CALL, &file);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_CALL failed");
+
 	file.fd = ndev->tap_fd;
-	r = ioctl(ndev->vhost_fd, VHOST_NET_SET_BACKEND, &file);
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_NET_SET_BACKEND, &file);
 	if (r != 0)
 		die("VHOST_NET_SET_BACKEND failed %d", errno);
-
 }
 
 static void notify_vq_eventfd(struct kvm *kvm, void *dev, u32 vq, u32 efd)
 {
 	struct net_dev *ndev = dev;
 	struct vhost_vring_file file = {
-		.index	= vq,
+		.index	= (vq % 2),
 		.fd	= efd,
 	};
 	int r;
 
-	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq))
+	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq)) {
 		return;
+	}
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_KICK, &file);
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_KICK, &file);
 	if (r < 0)
-		die_perror("VHOST_SET_VRING_KICK failed");
+		die_perror("VHOST_SET_VRING_KICK failed test");
 }
 
 static int notify_vq(struct kvm *kvm, void *dev, u32 vq)
@@ -777,10 +783,6 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
 	struct vhost_memory *mem;
 	int r, i;
 
-	ndev->vhost_fd = open("/dev/vhost-net", O_RDWR);
-	if (ndev->vhost_fd < 0)
-		die_perror("Failed openning vhost-net device");
-
 	mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
 	if (mem == NULL)
 		die("Failed allocating memory for vhost memory map");
@@ -796,13 +798,22 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
 	}
 	mem->nregions = i;
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_OWNER);
-	if (r != 0)
-		die_perror("VHOST_SET_OWNER failed");
+	for (i=0; ((u32)i < ndev->queue_pairs) && 
+			(i < VIRTIO_NET_NUM_QUEUES); i++) {
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_MEM_TABLE, mem);
-	if (r != 0)
-		die_perror("VHOST_SET_MEM_TABLE failed");
+		ndev->vhost_fds[i] = open("/dev/vhost-net", O_RDWR);
+	        if (ndev->vhost_fds[i] < 0)
+			die_perror("Failed openning vhost-net device");
+
+		r = ioctl(ndev->vhost_fds[i], VHOST_SET_OWNER);
+		if (r != 0)
+			die_perror("VHOST_SET_OWNER failed");
+	
+		r = ioctl(ndev->vhost_fds[i], VHOST_SET_MEM_TABLE, mem);
+		if (r != 0)
+			die_perror("VHOST_SET_MEM_TABLE failed");
+	}
+	ndev->vhost_fd = ndev->vhost_fds[0];
 
 	ndev->vdev.use_vhost = true;
 
@@ -966,7 +977,6 @@ static int virtio_net__init_one(struct virtio_net_params *params)
 				   "falling back to %s.", params->trans,
 				   virtio_trans_name(trans));
 	}
-
 	r = virtio_init(params->kvm, ndev, &ndev->vdev, ops, trans,
 			PCI_DEVICE_ID_VIRTIO_NET, VIRTIO_ID_NET, PCI_CLASS_NET);
 	if (r < 0) {
diff --git a/virtio/pci.c b/virtio/pci.c
index c652949..7a1532b 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -44,7 +44,8 @@ static int virtio_pci__init_ioeventfd(struct kvm *kvm, struct virtio_device *vde
 	 * Vhost will poll the eventfd in host kernel side, otherwise we
 	 * need to poll in userspace.
 	 */
-	if (!vdev->use_vhost)
+	if ( (!vdev->use_vhost) ||
+		((u32)vdev->ops->get_vq_count(kvm, vpci->dev)==(vq+1)) )
 		flags |= IOEVENTFD_FLAG_USER_POLL;
 
 	/* ioport */

Re: kvmtool: vhost MQ support

[Vitaly Kuznetsov]

BARBALACE Antonio <antonio.barbalace@ed.ac.uk> writes:

> This patch enables vhost MQ to support in kvmtool without any Linux kernel modification.
> The patch takes the same approach as QEMU -- for each queue pair a new /dev/vhost-net fd is created.
> Fds are kept in ndev->vhost_fds, with ndev->vhost_fd == ndev->vhost_fds[0] (to avoid further modification to the existent source code).
> Thanks, Antonio Barbalace
> The University of Edinburgh is a charitable body, registered in Scotland, with registration number SC005336.
> diff --git a/virtio/net.c b/virtio/net.c
> index 1ee3c19..bae3019 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -58,6 +58,7 @@ struct net_dev {
>  	u32				features, queue_pairs;
>  
>  	int				vhost_fd;
> +	int				vhost_fds[VIRTIO_NET_NUM_QUEUES];
>  	int				tap_fd;
>  	char				tap_name[IFNAMSIZ];
>  	bool				tap_ufo;
> @@ -512,6 +513,7 @@ static int virtio_net__vhost_set_features(struct net_dev *ndev)
>  {
>  	u64 features = 1UL << VIRTIO_RING_F_EVENT_IDX;
>  	u64 vhost_features;
> +	int i, r = 0;
>  
>  	if (ioctl(ndev->vhost_fd, VHOST_GET_FEATURES, &vhost_features) != 0)
>  		die_perror("VHOST_GET_FEATURES failed");
> @@ -521,7 +523,9 @@ static int virtio_net__vhost_set_features(struct net_dev *ndev)
>  			has_virtio_feature(ndev, VIRTIO_NET_F_MRG_RXBUF))
>  		features |= 1UL << VIRTIO_NET_F_MRG_RXBUF;
>  
> -	return ioctl(ndev->vhost_fd, VHOST_SET_FEATURES, &features);
> +	for (i=0; ((u32)i < ndev->queue_pairs) && (r >= 0); i++)

Neither a virtio nor a kvmtool person here, just some comments about the
style below.

Nit: more spaces needed:
	for (i = 0; 

(u32) cast is not really needed because ndev->queue_pairs is caped with
VIRTIO_NET_NUM_QUEUES and 

ndev->queue_pairs = max(1, min(VIRTIO_NET_NUM_QUEUES, params->mq));

alternatively, you can just declare i as 'u32'.

> +		r = ioctl(ndev->vhost_fds[i], VHOST_SET_FEATURES, &features); 

To improve the readability I'd suggest to write this as

	for (i=0; i < ndev->queue_pairs; i++) {
		r = ioctl(ndev->vhost_fds[i], VHOST_SET_FEATURES, &features);
		if (r)
			break;
	}


> +	return r;
>  }
>  
>  static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
> @@ -578,7 +582,7 @@ static bool is_ctrl_vq(struct net_dev *ndev, u32 vq)
>  static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
>  		   u32 pfn)
>  {
> -	struct vhost_vring_state state = { .index = vq };
> +	struct vhost_vring_state state = { .index = (vq %2) };

Nit: superfluous parentheses

>  	struct net_dev_queue *net_queue;
>  	struct vhost_vring_addr addr;
>  	struct net_dev *ndev = dev;
> @@ -619,23 +623,24 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
>  	if (queue->endian != VIRTIO_ENDIAN_HOST)
>  		die_perror("VHOST requires the same endianness in guest and host");
>  
> -	state.num = queue->vring.num;
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_NUM, &state);
> +	state.num = queue->vring.num; //number of decriptors

I don't see C++ style comments anywhere in this file, use /* */ instead.

> +        r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_NUM, &state);

Indentation. Also, you seem to be using 'vq / 2' a lot, maybe assign
this to a local variable.

>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_NUM failed");
> -	state.num = 0;
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_BASE, &state);
> +
> +	state.num = 0; //descriptors base

Comment style.

> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_BASE, &state);
>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_BASE failed");
>  
>  	addr = (struct vhost_vring_addr) {
> -		.index = vq,
> +		.index = (vq %2),
>  		.desc_user_addr = (u64)(unsigned long)queue->vring.desc,
>  		.avail_user_addr = (u64)(unsigned long)queue->vring.avail,
>  		.used_user_addr = (u64)(unsigned long)queue->vring.used,
>  	};
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_ADDR, &addr);
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_ADDR, &addr);
>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_ADDR failed");
>  
> @@ -659,7 +664,7 @@ static void exit_vq(struct kvm *kvm, void *dev, u32 vq)
>  	 */
>  	if (ndev->vhost_fd && !is_ctrl_vq(ndev, vq)) {
>  		pr_warning("Cannot reset VHOST queue");
> -		ioctl(ndev->vhost_fd, VHOST_RESET_OWNER);
> +		ioctl(ndev->vhost_fds[(vq /2)], VHOST_RESET_OWNER);
>  		return;
>  	}
>  
> @@ -682,7 +687,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
>  		return;
>  
>  	file = (struct vhost_vring_file) {
> -		.index	= vq,
> +		.index	= (vq % 2),
>  		.fd	= eventfd(0, 0),
>  	};
>  
> @@ -693,31 +698,32 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
>  	queue->irqfd = file.fd;
>  	queue->gsi = gsi;
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_CALL, &file);
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_CALL, &file);
>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_CALL failed");
> +
>  	file.fd = ndev->tap_fd;
> -	r = ioctl(ndev->vhost_fd, VHOST_NET_SET_BACKEND, &file);
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_NET_SET_BACKEND, &file);
>  	if (r != 0)
>  		die("VHOST_NET_SET_BACKEND failed %d", errno);
> -
>  }
>  
>  static void notify_vq_eventfd(struct kvm *kvm, void *dev, u32 vq, u32 efd)
>  {
>  	struct net_dev *ndev = dev;
>  	struct vhost_vring_file file = {
> -		.index	= vq,
> +		.index	= (vq % 2),
>  		.fd	= efd,
>  	};
>  	int r;
>  
> -	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq))
> +	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq)) {
>  		return;
> +	}
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_KICK, &file);
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_KICK, &file);
>  	if (r < 0)
> -		die_perror("VHOST_SET_VRING_KICK failed");
> +		die_perror("VHOST_SET_VRING_KICK failed test");

What is this 'failed test' about? Stray change?

>  }
>  
>  static int notify_vq(struct kvm *kvm, void *dev, u32 vq)
> @@ -777,10 +783,6 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
>  	struct vhost_memory *mem;
>  	int r, i;
>  
> -	ndev->vhost_fd = open("/dev/vhost-net", O_RDWR);
> -	if (ndev->vhost_fd < 0)
> -		die_perror("Failed openning vhost-net device");
> -
>  	mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
>  	if (mem == NULL)
>  		die("Failed allocating memory for vhost memory map");
> @@ -796,13 +798,22 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
>  	}
>  	mem->nregions = i;
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_OWNER);
> -	if (r != 0)
> -		die_perror("VHOST_SET_OWNER failed");
> +	for (i=0; ((u32)i < ndev->queue_pairs) && 
> +			(i < VIRTIO_NET_NUM_QUEUES); i++) {
>  

Same as above.

> -	r = ioctl(ndev->vhost_fd, VHOST_SET_MEM_TABLE, mem);
> -	if (r != 0)
> -		die_perror("VHOST_SET_MEM_TABLE failed");
> +		ndev->vhost_fds[i] = open("/dev/vhost-net", O_RDWR);
> +	        if (ndev->vhost_fds[i] < 0)
> +			die_perror("Failed openning vhost-net device");
> +
> +		r = ioctl(ndev->vhost_fds[i], VHOST_SET_OWNER);
> +		if (r != 0)
> +			die_perror("VHOST_SET_OWNER failed");
> +	
> +		r = ioctl(ndev->vhost_fds[i], VHOST_SET_MEM_TABLE, mem);
> +		if (r != 0)
> +			die_perror("VHOST_SET_MEM_TABLE failed");
> +	}
> +	ndev->vhost_fd = ndev->vhost_fds[0];

Do we actually need 'vhost_fd' at all, can we just use vhost_fds[0] in a
few places where it is still present?

>  
>  	ndev->vdev.use_vhost = true;
>  
> @@ -966,7 +977,6 @@ static int virtio_net__init_one(struct virtio_net_params *params)
>  				   "falling back to %s.", params->trans,
>  				   virtio_trans_name(trans));
>  	}
> -

Stray change?

>  	r = virtio_init(params->kvm, ndev, &ndev->vdev, ops, trans,
>  			PCI_DEVICE_ID_VIRTIO_NET, VIRTIO_ID_NET, PCI_CLASS_NET);
>  	if (r < 0) {
> diff --git a/virtio/pci.c b/virtio/pci.c
> index c652949..7a1532b 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -44,7 +44,8 @@ static int virtio_pci__init_ioeventfd(struct kvm *kvm, struct virtio_device *vde
>  	 * Vhost will poll the eventfd in host kernel side, otherwise we
>  	 * need to poll in userspace.
>  	 */
> -	if (!vdev->use_vhost)
> +	if ( (!vdev->use_vhost) ||

Xen coding style detected :-) Just

	if (!vdev->use_vhost || (vdev->ops->get_vq_count(kvm, vpci->dev) = vq + 1)

would do fine.

> +		((u32)vdev->ops->get_vq_count(kvm, vpci->dev)==(vq+1)) )
>  		flags |= IOEVENTFD_FLAG_USER_POLL;
>  
>  	/* ioport */

-- 
Vitaly

RE: kvmtool: vhost MQ support

[BARBALACE Antonio]

[-- Attachment #1: Type: text/plain, Size: 9252 bytes --]

Hi Vitaly,
 Thanks for your quick feedback, sorry for the several formatting issues! I applied all your comments to the attached patch.
 For the moment, I would prefer to keep 'vhost_fd', but happy to remove it if you would like me to do so -- just let me know.
Cheers,
Antonio

-----Original Message-----
From: Vitaly Kuznetsov <vkuznets@redhat.com> 
Sent: Tuesday, October 6, 2020 12:58 PM
To: BARBALACE Antonio <antonio.barbalace@ed.ac.uk>
Cc: will@kernel.org; julien.thierry.kdev@gmail.com; kvm@vger.kernel.org
Subject: Re: kvmtool: vhost MQ support

BARBALACE Antonio <antonio.barbalace@ed.ac.uk> writes:

> This patch enables vhost MQ to support in kvmtool without any Linux kernel modification.
> The patch takes the same approach as QEMU -- for each queue pair a new /dev/vhost-net fd is created.
> Fds are kept in ndev->vhost_fds, with ndev->vhost_fd == ndev->vhost_fds[0] (to avoid further modification to the existent source code).
> Thanks, Antonio Barbalace
> The University of Edinburgh is a charitable body, registered in Scotland, with registration number SC005336.
> diff --git a/virtio/net.c b/virtio/net.c index 1ee3c19..bae3019 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -58,6 +58,7 @@ struct net_dev {
>  	u32				features, queue_pairs;
>  
>  	int				vhost_fd;
> +	int				vhost_fds[VIRTIO_NET_NUM_QUEUES];
>  	int				tap_fd;
>  	char				tap_name[IFNAMSIZ];
>  	bool				tap_ufo;
> @@ -512,6 +513,7 @@ static int virtio_net__vhost_set_features(struct 
> net_dev *ndev)  {
>  	u64 features = 1UL << VIRTIO_RING_F_EVENT_IDX;
>  	u64 vhost_features;
> +	int i, r = 0;
>  
>  	if (ioctl(ndev->vhost_fd, VHOST_GET_FEATURES, &vhost_features) != 0)
>  		die_perror("VHOST_GET_FEATURES failed"); @@ -521,7 +523,9 @@ static 
> int virtio_net__vhost_set_features(struct net_dev *ndev)
>  			has_virtio_feature(ndev, VIRTIO_NET_F_MRG_RXBUF))
>  		features |= 1UL << VIRTIO_NET_F_MRG_RXBUF;
>  
> -	return ioctl(ndev->vhost_fd, VHOST_SET_FEATURES, &features);
> +	for (i=0; ((u32)i < ndev->queue_pairs) && (r >= 0); i++)

Neither a virtio nor a kvmtool person here, just some comments about the style below.

Nit: more spaces needed:
	for (i = 0; 

(u32) cast is not really needed because ndev->queue_pairs is caped with VIRTIO_NET_NUM_QUEUES and 

ndev->queue_pairs = max(1, min(VIRTIO_NET_NUM_QUEUES, params->mq));

alternatively, you can just declare i as 'u32'.

> +		r = ioctl(ndev->vhost_fds[i], VHOST_SET_FEATURES, &features);

To improve the readability I'd suggest to write this as

	for (i=0; i < ndev->queue_pairs; i++) {
		r = ioctl(ndev->vhost_fds[i], VHOST_SET_FEATURES, &features);
		if (r)
			break;
	}


> +	return r;
>  }
>  
>  static void set_guest_features(struct kvm *kvm, void *dev, u32 
> features) @@ -578,7 +582,7 @@ static bool is_ctrl_vq(struct net_dev 
> *ndev, u32 vq)  static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
>  		   u32 pfn)
>  {
> -	struct vhost_vring_state state = { .index = vq };
> +	struct vhost_vring_state state = { .index = (vq %2) };

Nit: superfluous parentheses

>  	struct net_dev_queue *net_queue;
>  	struct vhost_vring_addr addr;
>  	struct net_dev *ndev = dev;
> @@ -619,23 +623,24 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
>  	if (queue->endian != VIRTIO_ENDIAN_HOST)
>  		die_perror("VHOST requires the same endianness in guest and host");
>  
> -	state.num = queue->vring.num;
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_NUM, &state);
> +	state.num = queue->vring.num; //number of decriptors

I don't see C++ style comments anywhere in this file, use /* */ instead.

> +        r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_NUM, 
> + &state);

Indentation. Also, you seem to be using 'vq / 2' a lot, maybe assign this to a local variable.

>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_NUM failed");
> -	state.num = 0;
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_BASE, &state);
> +
> +	state.num = 0; //descriptors base

Comment style.

> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_BASE, &state);
>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_BASE failed");
>  
>  	addr = (struct vhost_vring_addr) {
> -		.index = vq,
> +		.index = (vq %2),
>  		.desc_user_addr = (u64)(unsigned long)queue->vring.desc,
>  		.avail_user_addr = (u64)(unsigned long)queue->vring.avail,
>  		.used_user_addr = (u64)(unsigned long)queue->vring.used,
>  	};
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_ADDR, &addr);
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_ADDR, &addr);
>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_ADDR failed");
>  
> @@ -659,7 +664,7 @@ static void exit_vq(struct kvm *kvm, void *dev, u32 vq)
>  	 */
>  	if (ndev->vhost_fd && !is_ctrl_vq(ndev, vq)) {
>  		pr_warning("Cannot reset VHOST queue");
> -		ioctl(ndev->vhost_fd, VHOST_RESET_OWNER);
> +		ioctl(ndev->vhost_fds[(vq /2)], VHOST_RESET_OWNER);
>  		return;
>  	}
>  
> @@ -682,7 +687,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
>  		return;
>  
>  	file = (struct vhost_vring_file) {
> -		.index	= vq,
> +		.index	= (vq % 2),
>  		.fd	= eventfd(0, 0),
>  	};
>  
> @@ -693,31 +698,32 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
>  	queue->irqfd = file.fd;
>  	queue->gsi = gsi;
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_CALL, &file);
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_CALL, &file);
>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_CALL failed");
> +
>  	file.fd = ndev->tap_fd;
> -	r = ioctl(ndev->vhost_fd, VHOST_NET_SET_BACKEND, &file);
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_NET_SET_BACKEND, &file);
>  	if (r != 0)
>  		die("VHOST_NET_SET_BACKEND failed %d", errno);
> -
>  }
>  
>  static void notify_vq_eventfd(struct kvm *kvm, void *dev, u32 vq, u32 
> efd)  {
>  	struct net_dev *ndev = dev;
>  	struct vhost_vring_file file = {
> -		.index	= vq,
> +		.index	= (vq % 2),
>  		.fd	= efd,
>  	};
>  	int r;
>  
> -	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq))
> +	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq)) {
>  		return;
> +	}
> +	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_KICK, &file);
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_KICK, &file);
>  	if (r < 0)
> -		die_perror("VHOST_SET_VRING_KICK failed");
> +		die_perror("VHOST_SET_VRING_KICK failed test");

What is this 'failed test' about? Stray change?

>  }
>  
>  static int notify_vq(struct kvm *kvm, void *dev, u32 vq) @@ -777,10 
> +783,6 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
>  	struct vhost_memory *mem;
>  	int r, i;
>  
> -	ndev->vhost_fd = open("/dev/vhost-net", O_RDWR);
> -	if (ndev->vhost_fd < 0)
> -		die_perror("Failed openning vhost-net device");
> -
>  	mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
>  	if (mem == NULL)
>  		die("Failed allocating memory for vhost memory map"); @@ -796,13 
> +798,22 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
>  	}
>  	mem->nregions = i;
>  
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_OWNER);
> -	if (r != 0)
> -		die_perror("VHOST_SET_OWNER failed");
> +	for (i=0; ((u32)i < ndev->queue_pairs) && 
> +			(i < VIRTIO_NET_NUM_QUEUES); i++) {
>  

Same as above.

> -	r = ioctl(ndev->vhost_fd, VHOST_SET_MEM_TABLE, mem);
> -	if (r != 0)
> -		die_perror("VHOST_SET_MEM_TABLE failed");
> +		ndev->vhost_fds[i] = open("/dev/vhost-net", O_RDWR);
> +	        if (ndev->vhost_fds[i] < 0)
> +			die_perror("Failed openning vhost-net device");
> +
> +		r = ioctl(ndev->vhost_fds[i], VHOST_SET_OWNER);
> +		if (r != 0)
> +			die_perror("VHOST_SET_OWNER failed");
> +	
> +		r = ioctl(ndev->vhost_fds[i], VHOST_SET_MEM_TABLE, mem);
> +		if (r != 0)
> +			die_perror("VHOST_SET_MEM_TABLE failed");
> +	}
> +	ndev->vhost_fd = ndev->vhost_fds[0];

Do we actually need 'vhost_fd' at all, can we just use vhost_fds[0] in a few places where it is still present?

>  
>  	ndev->vdev.use_vhost = true;
>  
> @@ -966,7 +977,6 @@ static int virtio_net__init_one(struct virtio_net_params *params)
>  				   "falling back to %s.", params->trans,
>  				   virtio_trans_name(trans));
>  	}
> -

Stray change?

>  	r = virtio_init(params->kvm, ndev, &ndev->vdev, ops, trans,
>  			PCI_DEVICE_ID_VIRTIO_NET, VIRTIO_ID_NET, PCI_CLASS_NET);
>  	if (r < 0) {
> diff --git a/virtio/pci.c b/virtio/pci.c index c652949..7a1532b 100644
> --- a/virtio/pci.c
> +++ b/virtio/pci.c
> @@ -44,7 +44,8 @@ static int virtio_pci__init_ioeventfd(struct kvm *kvm, struct virtio_device *vde
>  	 * Vhost will poll the eventfd in host kernel side, otherwise we
>  	 * need to poll in userspace.
>  	 */
> -	if (!vdev->use_vhost)
> +	if ( (!vdev->use_vhost) ||

Xen coding style detected :-) Just

	if (!vdev->use_vhost || (vdev->ops->get_vq_count(kvm, vpci->dev) = vq + 1)

would do fine.

> +		((u32)vdev->ops->get_vq_count(kvm, vpci->dev)==(vq+1)) )
>  		flags |= IOEVENTFD_FLAG_USER_POLL;
>  
>  	/* ioport */

--
Vitaly


[-- Attachment #2: kvmtool-vhost_mq-20201007.patch --]
[-- Type: application/octet-stream, Size: 6057 bytes --]

diff --git a/virtio/net.c b/virtio/net.c
index 1ee3c19..acdd741 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -58,6 +58,7 @@ struct net_dev {
 	u32				features, queue_pairs;
 
 	int				vhost_fd;
+	int				vhost_fds[VIRTIO_NET_NUM_QUEUES];
 	int				tap_fd;
 	char				tap_name[IFNAMSIZ];
 	bool				tap_ufo;
@@ -512,6 +513,8 @@ static int virtio_net__vhost_set_features(struct net_dev *ndev)
 {
 	u64 features = 1UL << VIRTIO_RING_F_EVENT_IDX;
 	u64 vhost_features;
+	u32 i;
+	int r = 0;
 
 	if (ioctl(ndev->vhost_fd, VHOST_GET_FEATURES, &vhost_features) != 0)
 		die_perror("VHOST_GET_FEATURES failed");
@@ -521,7 +524,13 @@ static int virtio_net__vhost_set_features(struct net_dev *ndev)
 			has_virtio_feature(ndev, VIRTIO_NET_F_MRG_RXBUF))
 		features |= 1UL << VIRTIO_NET_F_MRG_RXBUF;
 
-	return ioctl(ndev->vhost_fd, VHOST_SET_FEATURES, &features);
+	for (i = 0 ; i < ndev->queue_pairs ; i++) {
+		r = ioctl(ndev->vhost_fds[i], VHOST_SET_FEATURES, &features); 
+		if (r < 0)
+			break;
+	}
+
+	return r;
 }
 
 static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
@@ -578,13 +587,13 @@ static bool is_ctrl_vq(struct net_dev *ndev, u32 vq)
 static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
 		   u32 pfn)
 {
-	struct vhost_vring_state state = { .index = vq };
+	struct vhost_vring_state state = { .index = vq % 2 };
 	struct net_dev_queue *net_queue;
 	struct vhost_vring_addr addr;
 	struct net_dev *ndev = dev;
 	struct virt_queue *queue;
 	void *p;
-	int r;
+	int r, vq_fd = vq / 2;
 
 	compat__remove_message(compat_id);
 
@@ -619,23 +628,24 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
 	if (queue->endian != VIRTIO_ENDIAN_HOST)
 		die_perror("VHOST requires the same endianness in guest and host");
 
-	state.num = queue->vring.num;
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_NUM, &state);
+	state.num = queue->vring.num; 
+	r = ioctl(ndev->vhost_fds[vq_fd], VHOST_SET_VRING_NUM, &state);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_NUM failed");
+
 	state.num = 0;
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_BASE, &state);
+	r = ioctl(ndev->vhost_fds[vq_fd], VHOST_SET_VRING_BASE, &state);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_BASE failed");
 
 	addr = (struct vhost_vring_addr) {
-		.index = vq,
+		.index = vq % 2,
 		.desc_user_addr = (u64)(unsigned long)queue->vring.desc,
 		.avail_user_addr = (u64)(unsigned long)queue->vring.avail,
 		.used_user_addr = (u64)(unsigned long)queue->vring.used,
 	};
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_ADDR, &addr);
+	r = ioctl(ndev->vhost_fds[vq_fd], VHOST_SET_VRING_ADDR, &addr);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_ADDR failed");
 
@@ -659,7 +669,7 @@ static void exit_vq(struct kvm *kvm, void *dev, u32 vq)
 	 */
 	if (ndev->vhost_fd && !is_ctrl_vq(ndev, vq)) {
 		pr_warning("Cannot reset VHOST queue");
-		ioctl(ndev->vhost_fd, VHOST_RESET_OWNER);
+		ioctl(ndev->vhost_fds[(vq /2)], VHOST_RESET_OWNER);
 		return;
 	}
 
@@ -682,7 +692,7 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
 		return;
 
 	file = (struct vhost_vring_file) {
-		.index	= vq,
+		.index	= vq % 2,
 		.fd	= eventfd(0, 0),
 	};
 
@@ -693,29 +703,30 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
 	queue->irqfd = file.fd;
 	queue->gsi = gsi;
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_CALL, &file);
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_CALL, &file);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_CALL failed");
+
 	file.fd = ndev->tap_fd;
-	r = ioctl(ndev->vhost_fd, VHOST_NET_SET_BACKEND, &file);
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_NET_SET_BACKEND, &file);
 	if (r != 0)
 		die("VHOST_NET_SET_BACKEND failed %d", errno);
-
 }
 
 static void notify_vq_eventfd(struct kvm *kvm, void *dev, u32 vq, u32 efd)
 {
 	struct net_dev *ndev = dev;
 	struct vhost_vring_file file = {
-		.index	= vq,
+		.index	= vq % 2,
 		.fd	= efd,
 	};
 	int r;
 
-	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq))
+	if (ndev->vhost_fd == 0 || is_ctrl_vq(ndev, vq)) {
 		return;
+	}
+	r = ioctl(ndev->vhost_fds[(vq /2)], VHOST_SET_VRING_KICK, &file);
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_KICK, &file);
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_KICK failed");
 }
@@ -777,10 +788,6 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
 	struct vhost_memory *mem;
 	int r, i;
 
-	ndev->vhost_fd = open("/dev/vhost-net", O_RDWR);
-	if (ndev->vhost_fd < 0)
-		die_perror("Failed openning vhost-net device");
-
 	mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
 	if (mem == NULL)
 		die("Failed allocating memory for vhost memory map");
@@ -796,13 +803,22 @@ static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
 	}
 	mem->nregions = i;
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_OWNER);
-	if (r != 0)
-		die_perror("VHOST_SET_OWNER failed");
+	for (i = 0 ; (i < (int)ndev->queue_pairs) && 
+			(i < VIRTIO_NET_NUM_QUEUES) ; i++) {
 
-	r = ioctl(ndev->vhost_fd, VHOST_SET_MEM_TABLE, mem);
-	if (r != 0)
-		die_perror("VHOST_SET_MEM_TABLE failed");
+		ndev->vhost_fds[i] = open("/dev/vhost-net", O_RDWR);
+		if (ndev->vhost_fds[i] < 0)
+			die_perror("Failed openning vhost-net device");
+
+		r = ioctl(ndev->vhost_fds[i], VHOST_SET_OWNER);
+		if (r != 0)
+			die_perror("VHOST_SET_OWNER failed");
+	
+		r = ioctl(ndev->vhost_fds[i], VHOST_SET_MEM_TABLE, mem);
+		if (r != 0)
+			die_perror("VHOST_SET_MEM_TABLE failed");
+	}
+	ndev->vhost_fd = ndev->vhost_fds[0];
 
 	ndev->vdev.use_vhost = true;
 
diff --git a/virtio/pci.c b/virtio/pci.c
index c652949..2c456df 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -44,7 +44,7 @@ static int virtio_pci__init_ioeventfd(struct kvm *kvm, struct virtio_device *vde
 	 * Vhost will poll the eventfd in host kernel side, otherwise we
 	 * need to poll in userspace.
 	 */
-	if (!vdev->use_vhost)
+	if (!vdev->use_vhost || (vdev->ops->get_vq_count(kvm, vpci->dev) == (int)vq + 1))
 		flags |= IOEVENTFD_FLAG_USER_POLL;
 
 	/* ioport */

RE: kvmtool: vhost MQ support

[Vitaly Kuznetsov]

BARBALACE Antonio <antonio.barbalace@ed.ac.uk> writes:

> Hi Vitaly,
>  Thanks for your quick feedback, sorry for the several formatting issues! I applied all your comments to the attached patch.
>  For the moment, I would prefer to keep 'vhost_fd', but happy to
>  remove it if you would like me to do so -- just let me know.

Hi Antonio,

in case you're doing some changes to the patch you're supposed to send
'PATCH v2' to the list. Also, it would be better if you mail it with
e.g. 'git send-email' and not as an attachment so it's easier for people
to review it.

(I'm not a kvmtool maintainer, these are just generic rules for kernel
patch processing)

-- 
Vitaly