diff mbox series

[kvmtool,04/16] virtio/vhost: Factor notify_vq_gsi()

Message ID 20230419132119.124457-5-jean-philippe@linaro.org (mailing list archive)
State New, archived
Headers show
Series Fix vhost-net, scsi and vsock | expand

Commit Message

Jean-Philippe Brucker April 19, 2023, 1:21 p.m. UTC 
All vhost devices should perform the same operations when initializing
the IRQFD. Move it to virtio/vhost.c

This fixes vsock, which didn't go through the irq__add_irqfd() helper
and couldn't be used on systems that require GSI translation (GICv2m).

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 include/kvm/virtio.h |  8 ++++++++
 virtio/net.c         | 30 ++++--------------------------
 virtio/scsi.c        | 15 ++-------------
 virtio/vhost.c       | 35 +++++++++++++++++++++++++++++++++++
 virtio/vsock.c       | 26 +++-----------------------
 5 files changed, 52 insertions(+), 62 deletions(-)

Comments

Andre Przywara April 20, 2023, 2:52 p.m. UTC | #1 
On Wed, 19 Apr 2023 14:21:08 +0100
Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

Hi,

> All vhost devices should perform the same operations when initializing
> the IRQFD. Move it to virtio/vhost.c
> 
> This fixes vsock, which didn't go through the irq__add_irqfd() helper
> and couldn't be used on systems that require GSI translation (GICv2m).
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  include/kvm/virtio.h |  8 ++++++++
>  virtio/net.c         | 30 ++++--------------------------
>  virtio/scsi.c        | 15 ++-------------
>  virtio/vhost.c       | 35 +++++++++++++++++++++++++++++++++++
>  virtio/vsock.c       | 26 +++-----------------------
>  5 files changed, 52 insertions(+), 62 deletions(-)
> 
> diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
> index 4a364a02..96c7b3ea 100644
> --- a/include/kvm/virtio.h
> +++ b/include/kvm/virtio.h
> @@ -77,6 +77,10 @@ struct virt_queue {
>  	u16		endian;
>  	bool		use_event_idx;
>  	bool		enabled;
> +
> +	/* vhost IRQ handling */
> +	int		gsi;
> +	int		irqfd;
>  };
>  
>  /*
> @@ -252,6 +256,10 @@ void virtio_vhost_set_vring(struct kvm *kvm, int vhost_fd, u32 index,
>  			    struct virt_queue *queue);
>  void virtio_vhost_set_vring_kick(struct kvm *kvm, int vhost_fd,
>  				 u32 index, int event_fd);
> +void virtio_vhost_set_vring_call(struct kvm *kvm, int vhost_fd, u32 index,
> +				 u32 gsi, struct virt_queue *queue);
> +void virtio_vhost_reset_vring(struct kvm *kvm, int vhost_fd, u32 index,
> +			      struct virt_queue *queue);
>  
>  int virtio_transport_parser(const struct option *opt, const char *arg, int unset);
>  
> diff --git a/virtio/net.c b/virtio/net.c
> index b935d24f..519dcbb7 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -4,12 +4,12 @@
>  #include "kvm/mutex.h"
>  #include "kvm/util.h"
>  #include "kvm/kvm.h"
> -#include "kvm/irq.h"
>  #include "kvm/uip.h"
>  #include "kvm/guest_compat.h"
>  #include "kvm/iovec.h"
>  #include "kvm/strbuf.h"
>  
> +#include <linux/list.h>
>  #include <linux/vhost.h>
>  #include <linux/virtio_net.h>
>  #include <linux/if_tun.h>
> @@ -25,7 +25,6 @@
>  #include <sys/ioctl.h>
>  #include <sys/types.h>
>  #include <sys/wait.h>
> -#include <sys/eventfd.h>
>  
>  #define VIRTIO_NET_QUEUE_SIZE		256
>  #define VIRTIO_NET_NUM_QUEUES		8
> @@ -44,8 +43,6 @@ struct net_dev_queue {
>  	pthread_t			thread;
>  	struct mutex			lock;
>  	pthread_cond_t			cond;
> -	int				gsi;
> -	int				irqfd;
>  };
>  
>  struct net_dev {
> @@ -647,11 +644,7 @@ static void exit_vq(struct kvm *kvm, void *dev, u32 vq)
>  	struct net_dev *ndev = dev;
>  	struct net_dev_queue *queue = &ndev->queues[vq];
>  
> -	if (!is_ctrl_vq(ndev, vq) && queue->gsi) {

So this first condition here seems to be lost in the transformation. Is
or was that never needed?

Apart from that the changes look fine.

Cheers,
Andre

> -		irq__del_irqfd(kvm, queue->gsi, queue->irqfd);
> -		close(queue->irqfd);
> -		queue->gsi = queue->irqfd = 0;
> -	}
> +	virtio_vhost_reset_vring(kvm, ndev->vhost_fd, vq, &queue->vq);
>  
>  	/*
>  	 * TODO: vhost reset owner. It's the only way to cleanly stop vhost, but
> @@ -675,27 +668,12 @@ static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
>  {
>  	struct net_dev *ndev = dev;
>  	struct net_dev_queue *queue = &ndev->queues[vq];
> -	struct vhost_vring_file file;
> -	int r;
>  
>  	if (ndev->vhost_fd == 0)
>  		return;
>  
> -	file = (struct vhost_vring_file) {
> -		.index	= vq,
> -		.fd	= eventfd(0, 0),
> -	};
> -
> -	r = irq__add_irqfd(kvm, gsi, file.fd, -1);
> -	if (r < 0)
> -		die_perror("KVM_IRQFD failed");
> -
> -	queue->irqfd = file.fd;
> -	queue->gsi = gsi;
> -
> -	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_CALL, &file);
> -	if (r < 0)
> -		die_perror("VHOST_SET_VRING_CALL failed");
> +	virtio_vhost_set_vring_call(kvm, ndev->vhost_fd, vq, gsi,
> +				    &queue->vq);
>  }
>  
>  static void notify_vq_eventfd(struct kvm *kvm, void *dev, u32 vq, u32 efd)
> diff --git a/virtio/scsi.c b/virtio/scsi.c
> index 1f757404..29acf57c 100644
> --- a/virtio/scsi.c
> +++ b/virtio/scsi.c
> @@ -92,25 +92,14 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq)
>  
>  static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
>  {
> -	struct vhost_vring_file file;
>  	struct scsi_dev *sdev = dev;
>  	int r;
>  
>  	if (sdev->vhost_fd == 0)
>  		return;
>  
> -	file = (struct vhost_vring_file) {
> -		.index	= vq,
> -		.fd	= eventfd(0, 0),
> -	};
> -
> -	r = irq__add_irqfd(kvm, gsi, file.fd, -1);
> -	if (r < 0)
> -		die_perror("KVM_IRQFD failed");
> -
> -	r = ioctl(sdev->vhost_fd, VHOST_SET_VRING_CALL, &file);
> -	if (r < 0)
> -		die_perror("VHOST_SET_VRING_CALL failed");
> +	virtio_vhost_set_vring_call(kvm, sdev->vhost_fd, vq, gsi,
> +				    &sdev->vqs[vq]);
>  
>  	if (vq > 0)
>  		return;
> diff --git a/virtio/vhost.c b/virtio/vhost.c
> index 3acfd30a..cd83645c 100644
> --- a/virtio/vhost.c
> +++ b/virtio/vhost.c
> @@ -1,9 +1,12 @@
> +#include "kvm/irq.h"
>  #include "kvm/virtio.h"
>  
>  #include <linux/kvm.h>
>  #include <linux/vhost.h>
>  #include <linux/list.h>
>  
> +#include <sys/eventfd.h>
> +
>  void virtio_vhost_init(struct kvm *kvm, int vhost_fd)
>  {
>  	struct kvm_mem_bank *bank;
> @@ -79,3 +82,35 @@ void virtio_vhost_set_vring_kick(struct kvm *kvm, int vhost_fd,
>  	if (r < 0)
>  		die_perror("VHOST_SET_VRING_KICK failed");
>  }
> +
> +void virtio_vhost_set_vring_call(struct kvm *kvm, int vhost_fd, u32 index,
> +				 u32 gsi, struct virt_queue *queue)
> +{
> +	int r;
> +	struct vhost_vring_file file = {
> +		.index	= index,
> +		.fd	= eventfd(0, 0),
> +	};
> +
> +	r = irq__add_irqfd(kvm, gsi, file.fd, -1);
> +	if (r < 0)
> +		die_perror("KVM_IRQFD failed");
> +
> +	r = ioctl(vhost_fd, VHOST_SET_VRING_CALL, &file);
> +	if (r < 0)
> +		die_perror("VHOST_SET_VRING_CALL failed");
> +
> +	queue->irqfd = file.fd;
> +	queue->gsi = gsi;
> +}
> +
> +void virtio_vhost_reset_vring(struct kvm *kvm, int vhost_fd, u32 index,
> +			      struct virt_queue *queue)
> +
> +{
> +	if (queue->gsi) {
> +		irq__del_irqfd(kvm, queue->gsi, queue->irqfd);
> +		close(queue->irqfd);
> +		queue->gsi = queue->irqfd = 0;
> +	}
> +}
> diff --git a/virtio/vsock.c b/virtio/vsock.c
> index 0ada9e09..559fbaba 100644
> --- a/virtio/vsock.c
> +++ b/virtio/vsock.c
> @@ -131,33 +131,13 @@ static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
>  
>  static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
>  {
> -	struct vhost_vring_file file;
>  	struct vsock_dev *vdev = dev;
> -	struct kvm_irqfd irq;
> -	int r;
> -
> -	if (vdev->vhost_fd == -1)
> -		return;
>  
> -	if (is_event_vq(vq))
> +	if (vdev->vhost_fd == -1 || is_event_vq(vq))
>  		return;
>  
> -	irq = (struct kvm_irqfd) {
> -		.gsi	= gsi,
> -		.fd	= eventfd(0, 0),
> -	};
> -	file = (struct vhost_vring_file) {
> -		.index	= vq,
> -		.fd	= irq.fd,
> -	};
> -
> -	r = ioctl(kvm->vm_fd, KVM_IRQFD, &irq);
> -	if (r < 0)
> -		die_perror("KVM_IRQFD failed");
> -
> -	r = ioctl(vdev->vhost_fd, VHOST_SET_VRING_CALL, &file);
> -	if (r < 0)
> -		die_perror("VHOST_SET_VRING_CALL failed");
> +	virtio_vhost_set_vring_call(kvm, vdev->vhost_fd, vq, gsi,
> +				    &vdev->vqs[vq]);
>  }
>  
>  static unsigned int get_vq_count(struct kvm *kvm, void *dev)
Jean-Philippe Brucker April 25, 2023, 11:53 a.m. UTC | #2 
On Thu, Apr 20, 2023 at 03:52:49PM +0100, Andre Przywara wrote:
> >  struct net_dev {
> > @@ -647,11 +644,7 @@ static void exit_vq(struct kvm *kvm, void *dev, u32 vq)
> >  	struct net_dev *ndev = dev;
> >  	struct net_dev_queue *queue = &ndev->queues[vq];
> >  
> > -	if (!is_ctrl_vq(ndev, vq) && queue->gsi) {
> 
> So this first condition here seems to be lost in the transformation. Is
> or was that never needed?

Hm, good question. The theory is that it was never needed, because the
control virtqueue is owned by kvmtool and not vhost, so we never set
queue->gsi for the control queue, in which case virtio_vhost_reset_vring()
is a nop. However in net, notify_vq_gsi() doesn't actually check which vq
this is, and does set queue->gsi for the control queue. It's a bug but
hasn't caused trouble so far because the Linux guest doesn't setup MSIs
for the control queue (it polls the queue until the command completes).
I'll add a check to notify_vq_gsi().

> Apart from that the changes look fine.
> 

Thanks for reviewing this!

Jean
diff mbox series

Patch

diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
index 4a364a02..96c7b3ea 100644
--- a/include/kvm/virtio.h
+++ b/include/kvm/virtio.h
@@ -77,6 +77,10 @@  struct virt_queue {
 	u16		endian;
 	bool		use_event_idx;
 	bool		enabled;
+
+	/* vhost IRQ handling */
+	int		gsi;
+	int		irqfd;
 };
 
 /*
@@ -252,6 +256,10 @@  void virtio_vhost_set_vring(struct kvm *kvm, int vhost_fd, u32 index,
 			    struct virt_queue *queue);
 void virtio_vhost_set_vring_kick(struct kvm *kvm, int vhost_fd,
 				 u32 index, int event_fd);
+void virtio_vhost_set_vring_call(struct kvm *kvm, int vhost_fd, u32 index,
+				 u32 gsi, struct virt_queue *queue);
+void virtio_vhost_reset_vring(struct kvm *kvm, int vhost_fd, u32 index,
+			      struct virt_queue *queue);
 
 int virtio_transport_parser(const struct option *opt, const char *arg, int unset);
 
diff --git a/virtio/net.c b/virtio/net.c
index b935d24f..519dcbb7 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -4,12 +4,12 @@ 
 #include "kvm/mutex.h"
 #include "kvm/util.h"
 #include "kvm/kvm.h"
-#include "kvm/irq.h"
 #include "kvm/uip.h"
 #include "kvm/guest_compat.h"
 #include "kvm/iovec.h"
 #include "kvm/strbuf.h"
 
+#include <linux/list.h>
 #include <linux/vhost.h>
 #include <linux/virtio_net.h>
 #include <linux/if_tun.h>
@@ -25,7 +25,6 @@ 
 #include <sys/ioctl.h>
 #include <sys/types.h>
 #include <sys/wait.h>
-#include <sys/eventfd.h>
 
 #define VIRTIO_NET_QUEUE_SIZE		256
 #define VIRTIO_NET_NUM_QUEUES		8
@@ -44,8 +43,6 @@  struct net_dev_queue {
 	pthread_t			thread;
 	struct mutex			lock;
 	pthread_cond_t			cond;
-	int				gsi;
-	int				irqfd;
 };
 
 struct net_dev {
@@ -647,11 +644,7 @@  static void exit_vq(struct kvm *kvm, void *dev, u32 vq)
 	struct net_dev *ndev = dev;
 	struct net_dev_queue *queue = &ndev->queues[vq];
 
-	if (!is_ctrl_vq(ndev, vq) && queue->gsi) {
-		irq__del_irqfd(kvm, queue->gsi, queue->irqfd);
-		close(queue->irqfd);
-		queue->gsi = queue->irqfd = 0;
-	}
+	virtio_vhost_reset_vring(kvm, ndev->vhost_fd, vq, &queue->vq);
 
 	/*
 	 * TODO: vhost reset owner. It's the only way to cleanly stop vhost, but
@@ -675,27 +668,12 @@  static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
 {
 	struct net_dev *ndev = dev;
 	struct net_dev_queue *queue = &ndev->queues[vq];
-	struct vhost_vring_file file;
-	int r;
 
 	if (ndev->vhost_fd == 0)
 		return;
 
-	file = (struct vhost_vring_file) {
-		.index	= vq,
-		.fd	= eventfd(0, 0),
-	};
-
-	r = irq__add_irqfd(kvm, gsi, file.fd, -1);
-	if (r < 0)
-		die_perror("KVM_IRQFD failed");
-
-	queue->irqfd = file.fd;
-	queue->gsi = gsi;
-
-	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_CALL, &file);
-	if (r < 0)
-		die_perror("VHOST_SET_VRING_CALL failed");
+	virtio_vhost_set_vring_call(kvm, ndev->vhost_fd, vq, gsi,
+				    &queue->vq);
 }
 
 static void notify_vq_eventfd(struct kvm *kvm, void *dev, u32 vq, u32 efd)
diff --git a/virtio/scsi.c b/virtio/scsi.c
index 1f757404..29acf57c 100644
--- a/virtio/scsi.c
+++ b/virtio/scsi.c
@@ -92,25 +92,14 @@  static int init_vq(struct kvm *kvm, void *dev, u32 vq)
 
 static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
 {
-	struct vhost_vring_file file;
 	struct scsi_dev *sdev = dev;
 	int r;
 
 	if (sdev->vhost_fd == 0)
 		return;
 
-	file = (struct vhost_vring_file) {
-		.index	= vq,
-		.fd	= eventfd(0, 0),
-	};
-
-	r = irq__add_irqfd(kvm, gsi, file.fd, -1);
-	if (r < 0)
-		die_perror("KVM_IRQFD failed");
-
-	r = ioctl(sdev->vhost_fd, VHOST_SET_VRING_CALL, &file);
-	if (r < 0)
-		die_perror("VHOST_SET_VRING_CALL failed");
+	virtio_vhost_set_vring_call(kvm, sdev->vhost_fd, vq, gsi,
+				    &sdev->vqs[vq]);
 
 	if (vq > 0)
 		return;
diff --git a/virtio/vhost.c b/virtio/vhost.c
index 3acfd30a..cd83645c 100644
--- a/virtio/vhost.c
+++ b/virtio/vhost.c
@@ -1,9 +1,12 @@ 
+#include "kvm/irq.h"
 #include "kvm/virtio.h"
 
 #include <linux/kvm.h>
 #include <linux/vhost.h>
 #include <linux/list.h>
 
+#include <sys/eventfd.h>
+
 void virtio_vhost_init(struct kvm *kvm, int vhost_fd)
 {
 	struct kvm_mem_bank *bank;
@@ -79,3 +82,35 @@  void virtio_vhost_set_vring_kick(struct kvm *kvm, int vhost_fd,
 	if (r < 0)
 		die_perror("VHOST_SET_VRING_KICK failed");
 }
+
+void virtio_vhost_set_vring_call(struct kvm *kvm, int vhost_fd, u32 index,
+				 u32 gsi, struct virt_queue *queue)
+{
+	int r;
+	struct vhost_vring_file file = {
+		.index	= index,
+		.fd	= eventfd(0, 0),
+	};
+
+	r = irq__add_irqfd(kvm, gsi, file.fd, -1);
+	if (r < 0)
+		die_perror("KVM_IRQFD failed");
+
+	r = ioctl(vhost_fd, VHOST_SET_VRING_CALL, &file);
+	if (r < 0)
+		die_perror("VHOST_SET_VRING_CALL failed");
+
+	queue->irqfd = file.fd;
+	queue->gsi = gsi;
+}
+
+void virtio_vhost_reset_vring(struct kvm *kvm, int vhost_fd, u32 index,
+			      struct virt_queue *queue)
+
+{
+	if (queue->gsi) {
+		irq__del_irqfd(kvm, queue->gsi, queue->irqfd);
+		close(queue->irqfd);
+		queue->gsi = queue->irqfd = 0;
+	}
+}
diff --git a/virtio/vsock.c b/virtio/vsock.c
index 0ada9e09..559fbaba 100644
--- a/virtio/vsock.c
+++ b/virtio/vsock.c
@@ -131,33 +131,13 @@  static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
 
 static void notify_vq_gsi(struct kvm *kvm, void *dev, u32 vq, u32 gsi)
 {
-	struct vhost_vring_file file;
 	struct vsock_dev *vdev = dev;
-	struct kvm_irqfd irq;
-	int r;
-
-	if (vdev->vhost_fd == -1)
-		return;
 
-	if (is_event_vq(vq))
+	if (vdev->vhost_fd == -1 || is_event_vq(vq))
 		return;
 
-	irq = (struct kvm_irqfd) {
-		.gsi	= gsi,
-		.fd	= eventfd(0, 0),
-	};
-	file = (struct vhost_vring_file) {
-		.index	= vq,
-		.fd	= irq.fd,
-	};
-
-	r = ioctl(kvm->vm_fd, KVM_IRQFD, &irq);
-	if (r < 0)
-		die_perror("KVM_IRQFD failed");
-
-	r = ioctl(vdev->vhost_fd, VHOST_SET_VRING_CALL, &file);
-	if (r < 0)
-		die_perror("VHOST_SET_VRING_CALL failed");
+	virtio_vhost_set_vring_call(kvm, vdev->vhost_fd, vq, gsi,
+				    &vdev->vqs[vq]);
 }
 
 static unsigned int get_vq_count(struct kvm *kvm, void *dev)