[PATCH V2 RFC 0/9] virtio: fix hang(loop) after hot-unplug vlan

[PATCH V2 RFC 0/9] virtio: fix hang(loop) after hot-unplug vlan

[Heinz Graalfs]

Hi,

this patch-set solves a hang situation when a vlan network device is
hot-unplugged from a KVM guest.

On System z there exists no handshake mechanism between host and guest
when a device is hot-unplugged. The device is removed and no further I/O
is possible.

The guest is notified about the hard removal with a CRW machine check.
As per architecture, the host must repond to any I/O operation for the
removed device with an error condition as if the device had never been
there.

During machine check handling in the guest, virtio exit functions try to
perform cleanup operations by triggering final I/O, including appropriate
host kicks. These operations fail, or do not complete, and lead to several
kinds of hang situations. In particular, virtio-ccw guest->host notification
on an unplugged device will receive an error; this is, however, not reflected
back to the affected virtqueues.

Here are the details of the error.

A hang (loop) occurs when a machine check is handled on System z due to a
vlan device removal. A loop spinning for a response for final IO in
virtnet_send_command() will never complete successfully because of a previous
unsuccessfull host kick operation (virtqueue_kick()).


Patch [1] changes the guest->host notification API. A potential error returned
by the host during notify() should not be ignored, but used in order to reflect
the error back to the affected virtqueue.

Patch [2] changes virtqueue_kick() and virtqueue_notify() to return a bool
depending on the result of the host notification operation. If the host
kick failed the current virtqueue is now flagged as 'broken'.

Patches [3,4] add code to verify host kicks by testing the return value of
virtqueue_kick() in order to avoid potential loops.

Patch [5] adds a new function virtqueue_is_broken(). This function should be
used to verify the state of a virtqueue when a previous virtqueue_get_buf()
returned a NULL pointer.

Patch [6,7,8,9] add virtqueue_is_broken() calls to handle potential errors
when a virtqueue_bet_buf() doesn't deliver any more buffers.


Heinz Graalfs (9):
  virtio_ring: change host notification API
  virtio_ring: let virtqueue_{kick()/notify()} return a bool
  virtio_net: verify if virtqueue_kick() succeeded
  virtio_test: verify if virtqueue_kick() succeeded
  virtio_ring: add new function virtqueue_is_broken()
  virtio_blk: verify if queue is broken after virtqueue_get_buf()
  virtio_console: verify if queue is broken after virtqueue_get_buf()
  virtio_net: verify if queue is broken after virtqueue_get_buf()
  virtio_scsi: verify if queue is broken after virtqueue_get_buf()

 drivers/block/virtio_blk.c             |  2 ++
 drivers/char/virtio_console.c          |  6 ++++--
 drivers/lguest/lguest_device.c         |  3 ++-
 drivers/net/virtio_net.c               | 12 +++++++-----
 drivers/remoteproc/remoteproc_virtio.c |  3 ++-
 drivers/s390/kvm/kvm_virtio.c          |  8 ++++++--
 drivers/s390/kvm/virtio_ccw.c          |  5 ++++-
 drivers/scsi/virtio_scsi.c             |  3 ++-
 drivers/virtio/virtio_mmio.c           |  3 ++-
 drivers/virtio/virtio_pci.c            |  3 ++-
 drivers/virtio/virtio_ring.c           | 32 ++++++++++++++++++++++++++------
 include/linux/virtio.h                 |  6 ++++--
 include/linux/virtio_ring.h            |  2 +-
 tools/virtio/virtio_test.c             |  6 ++++--
 tools/virtio/vringh_test.c             | 13 +++++++++----
 15 files changed, 77 insertions(+), 30 deletions(-)

-- 
1.8.3.1

[PATCH V2 RFC 1/9] virtio_ring: change host notification API

[Heinz Graalfs]

Currently a host kick error is silently ignored and not reflected in
the virtqueue of a particular virtio device.

Changing the notify API for guest->host notification seems to be one
prerequisite in order to be able to handle such errors in the context
where the kick is triggered.

This patch changes the notify API. The notify function must return a
negative int return value in case the host notification failed.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/lguest/lguest_device.c         |  3 ++-
 drivers/remoteproc/remoteproc_virtio.c |  3 ++-
 drivers/s390/kvm/kvm_virtio.c          |  8 ++++++--
 drivers/s390/kvm/virtio_ccw.c          |  5 ++++-
 drivers/virtio/virtio_mmio.c           |  3 ++-
 drivers/virtio/virtio_pci.c            |  3 ++-
 drivers/virtio/virtio_ring.c           |  4 ++--
 include/linux/virtio_ring.h            |  2 +-
 tools/virtio/virtio_test.c             |  3 ++-
 tools/virtio/vringh_test.c             | 13 +++++++++----
 10 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index b3256ff..1275f18 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -229,7 +229,7 @@ struct lguest_vq_info {
  * make a hypercall.  We hand the physical address of the virtqueue so the Host
  * knows which virtqueue we're talking about.
  */
-static void lg_notify(struct virtqueue *vq)
+static int lg_notify(struct virtqueue *vq)
 {
 	/*
 	 * We store our virtqueue information in the "priv" pointer of the
@@ -238,6 +238,7 @@ static void lg_notify(struct virtqueue *vq)
 	struct lguest_vq_info *lvq = vq->priv;
 
 	hcall(LHCALL_NOTIFY, lvq->config.pfn << PAGE_SHIFT, 0, 0, 0);
+	return 0;
 }
 
 /* An extern declaration inside a C file is bad form.  Don't do it. */
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index b09c75c..39723ee 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -30,7 +30,7 @@
 #include "remoteproc_internal.h"
 
 /* kick the remote processor, and let it know which virtqueue to poke at */
-static void rproc_virtio_notify(struct virtqueue *vq)
+static int rproc_virtio_notify(struct virtqueue *vq)
 {
 	struct rproc_vring *rvring = vq->priv;
 	struct rproc *rproc = rvring->rvdev->rproc;
@@ -39,6 +39,7 @@ static void rproc_virtio_notify(struct virtqueue *vq)
 	dev_dbg(&rproc->dev, "kicking vq index: %d\n", notifyid);
 
 	rproc->ops->kick(rproc, notifyid);
+	return 0;
 }
 
 /**
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 2ea6165..4097a46 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -166,11 +166,15 @@ static void kvm_reset(struct virtio_device *vdev)
  * make a hypercall.  We hand the address  of the virtqueue so the Host
  * knows which virtqueue we're talking about.
  */
-static void kvm_notify(struct virtqueue *vq)
+static int kvm_notify(struct virtqueue *vq)
 {
+	long rc;
 	struct kvm_vqconfig *config = vq->priv;
 
-	kvm_hypercall1(KVM_S390_VIRTIO_NOTIFY, config->address);
+	rc = kvm_hypercall1(KVM_S390_VIRTIO_NOTIFY, config->address);
+	if (rc < 0)
+		return rc;
+	return 0;
 }
 
 /*
diff --git a/drivers/s390/kvm/virtio_ccw.c b/drivers/s390/kvm/virtio_ccw.c
index 6a410d4..c640ecd 100644
--- a/drivers/s390/kvm/virtio_ccw.c
+++ b/drivers/s390/kvm/virtio_ccw.c
@@ -375,7 +375,7 @@ static inline long do_kvm_notify(struct subchannel_id schid,
 	return __rc;
 }
 
-static void virtio_ccw_kvm_notify(struct virtqueue *vq)
+static int virtio_ccw_kvm_notify(struct virtqueue *vq)
 {
 	struct virtio_ccw_vq_info *info = vq->priv;
 	struct virtio_ccw_device *vcdev;
@@ -384,6 +384,9 @@ static void virtio_ccw_kvm_notify(struct virtqueue *vq)
 	vcdev = to_vc_device(info->vq->vdev);
 	ccw_device_get_schid(vcdev->cdev, &schid);
 	info->cookie = do_kvm_notify(schid, vq->index, info->cookie);
+	if (info->cookie < 0)
+		return info->cookie;
+	return 0;
 }
 
 static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev,
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 1ba0d68..e213991 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -219,13 +219,14 @@ static void vm_reset(struct virtio_device *vdev)
 /* Transport interface */
 
 /* the notify function used when creating a virt queue */
-static void vm_notify(struct virtqueue *vq)
+static int vm_notify(struct virtqueue *vq)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
 
 	/* We write the queue's selector into the notification register to
 	 * signal the other end */
 	writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+	return 0;
 }
 
 /* Notify all virtqueues on an interrupt. */
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index a7ce730..7988137 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -197,13 +197,14 @@ static void vp_reset(struct virtio_device *vdev)
 }
 
 /* the notify function used when creating a virt queue */
-static void vp_notify(struct virtqueue *vq)
+static int vp_notify(struct virtqueue *vq)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
 
 	/* we write the queue's selector into the notification register to
 	 * signal the other end */
 	iowrite16(vq->index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY);
+	return 0;
 }
 
 /* Handle a configuration change: Tell driver if it wants to know. */
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5217baf..aa40f6c 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -81,7 +81,7 @@ struct vring_virtqueue
 	u16 last_used_idx;
 
 	/* How to notify other side. FIXME: commonalize hcalls! */
-	void (*notify)(struct virtqueue *vq);
+	int (*notify)(struct virtqueue *vq);
 
 #ifdef DEBUG
 	/* They're supposed to lock for us. */
@@ -741,7 +741,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
 				      void *pages,
-				      void (*notify)(struct virtqueue *),
+				      int (*notify)(struct virtqueue *),
 				      void (*callback)(struct virtqueue *),
 				      const char *name)
 {
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index ca3ad41..74fa580 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -70,7 +70,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 				      struct virtio_device *vdev,
 				      bool weak_barriers,
 				      void *pages,
-				      void (*notify)(struct virtqueue *vq),
+				      int (*notify)(struct virtqueue *vq),
 				      void (*callback)(struct virtqueue *vq),
 				      const char *name);
 void vring_del_virtqueue(struct virtqueue *vq);
diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
index da7a195..8e3e432 100644
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -41,13 +41,14 @@ struct vdev_info {
 	struct vhost_memory *mem;
 };
 
-void vq_notify(struct virtqueue *vq)
+int vq_notify(struct virtqueue *vq)
 {
 	struct vq_info *info = vq->priv;
 	unsigned long long v = 1;
 	int r;
 	r = write(info->kick, &v, sizeof v);
 	assert(r == sizeof v);
+	return 0;
 }
 
 void vq_callback(struct virtqueue *vq)
diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c
index d053ea4..965ccfe 100644
--- a/tools/virtio/vringh_test.c
+++ b/tools/virtio/vringh_test.c
@@ -22,7 +22,7 @@ static u64 user_addr_offset;
 #define RINGSIZE 256
 #define ALIGN 4096
 
-static void never_notify_host(struct virtqueue *vq)
+static int never_notify_host(struct virtqueue *vq)
 {
 	abort();
 }
@@ -65,17 +65,22 @@ struct guest_virtio_device {
 	unsigned long notifies;
 };
 
-static void parallel_notify_host(struct virtqueue *vq)
+static int parallel_notify_host(struct virtqueue *vq)
 {
+	int rc;
 	struct guest_virtio_device *gvdev;
 
 	gvdev = container_of(vq->vdev, struct guest_virtio_device, vdev);
-	write(gvdev->to_host_fd, "", 1);
+	rc = write(gvdev->to_host_fd, "", 1);
+	if (rc < 0)
+		return rc;
 	gvdev->notifies++;
+	return 0;
 }
 
-static void no_notify_host(struct virtqueue *vq)
+static int no_notify_host(struct virtqueue *vq)
 {
+	return 0;
 }
 
 #define NUM_XFERS (10000000)
-- 
1.8.3.1

[PATCH V2 RFC 2/9] virtio_ring: let virtqueue_{kick()/notify()} return a bool

[Heinz Graalfs]

virtqueue_{kick()/notify()} should exploit the new host notification API.
If the notify call returned with a negative value the host kick failed
(e.g. a kick triggered after a device was hot-unplugged). In this case
the virtqueue is set to 'broken' and false is returned, otherwise true.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/virtio/virtio_ring.c | 20 ++++++++++++++++----
 include/linux/virtio.h       |  4 ++--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index aa40f6c..a7e0eec 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -459,13 +459,22 @@ EXPORT_SYMBOL_GPL(virtqueue_kick_prepare);
  * @vq: the struct virtqueue
  *
  * This does not need to be serialized.
+ *
+ * Returns false if host notify failed or queue is broken, otherwise true.
  */
-void virtqueue_notify(struct virtqueue *_vq)
+bool virtqueue_notify(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
+	if (unlikely(vq->broken))
+		return false;
+
 	/* Prod other side to tell it about changes. */
-	vq->notify(_vq);
+	if (vq->notify(_vq) < 0) {
+		vq->broken = true;
+		return false;
+	}
+	return true;
 }
 EXPORT_SYMBOL_GPL(virtqueue_notify);
 
@@ -478,11 +487,14 @@ EXPORT_SYMBOL_GPL(virtqueue_notify);
  *
  * Caller must ensure we don't call this with other virtqueue
  * operations at the same time (except where noted).
+ *
+ * Returns false if kick failed, otherwise true.
  */
-void virtqueue_kick(struct virtqueue *vq)
+bool virtqueue_kick(struct virtqueue *vq)
 {
 	if (virtqueue_kick_prepare(vq))
-		virtqueue_notify(vq);
+		return virtqueue_notify(vq);
+	return true;
 }
 EXPORT_SYMBOL_GPL(virtqueue_kick);
 
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 9ff8645..4557e8a 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -58,11 +58,11 @@ int virtqueue_add_sgs(struct virtqueue *vq,
 		      void *data,
 		      gfp_t gfp);
 
-void virtqueue_kick(struct virtqueue *vq);
+bool virtqueue_kick(struct virtqueue *vq);
 
 bool virtqueue_kick_prepare(struct virtqueue *vq);
 
-void virtqueue_notify(struct virtqueue *vq);
+bool virtqueue_notify(struct virtqueue *vq);
 
 void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len);
 
-- 
1.8.3.1

[PATCH V2 RFC 3/9] virtio_net: verify if virtqueue_kick() succeeded

[Heinz Graalfs]

Verify if a host kick succeeded by checking return value of virtqueue_kick().

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/net/virtio_net.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ab2e5d0..6584e3a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -542,7 +542,8 @@ static bool try_fill_recv(struct receive_queue *rq, gfp_t gfp)
 	} while (rq->vq->num_free);
 	if (unlikely(rq->num > rq->max))
 		rq->max = rq->num;
-	virtqueue_kick(rq->vq);
+	if (unlikely(!virtqueue_kick(rq->vq)))
+		return false;
 	return !oom;
 }
 
@@ -728,7 +729,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	err = xmit_skb(sq, skb);
 
 	/* This should not happen! */
-	if (unlikely(err)) {
+	if (unlikely(err) || unlikely(!virtqueue_kick(sq->vq))) {
 		dev->stats.tx_fifo_errors++;
 		if (net_ratelimit())
 			dev_warn(&dev->dev,
@@ -737,7 +738,6 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
-	virtqueue_kick(sq->vq);
 
 	/* Don't wait up for transmitted skbs to be freed. */
 	skb_orphan(skb);
@@ -796,7 +796,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	BUG_ON(virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC)
 	       < 0);
 
-	virtqueue_kick(vi->cvq);
+	if (unlikely(!virtqueue_kick(vi->cvq)))
+		return status == VIRTIO_NET_OK;
 
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.
-- 
1.8.3.1

[PATCH V2 RFC 4/9] virtio_test: verify if virtqueue_kick() succeeded

[Heinz Graalfs]

Verify if a host kick succeeded by checking return value of virtqueue_kick().

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 tools/virtio/virtio_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
index 8e3e432..8691317 100644
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -172,7 +172,8 @@ static void run_test(struct vdev_info *dev, struct vq_info *vq,
 							 GFP_ATOMIC);
 				if (likely(r == 0)) {
 					++started;
-					virtqueue_kick(vq->vq);
+					if (unlikely(!virtqueue_kick(vq->vq))
+						r = -1;
 				}
 			} else
 				r = -1;
-- 
1.8.3.1

[PATCH V2 RFC 5/9] virtio_ring: add new function virtqueue_is_broken()

[Heinz Graalfs]

Add new function virtqueue_is_broken(). Callers of virtqueue_get_buf()
should check for a broken queue.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/virtio/virtio_ring.c | 8 ++++++++
 include/linux/virtio.h       | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a7e0eec..7eb844b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -848,4 +848,12 @@ unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_vring_size);
 
+bool virtqueue_is_broken(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	return vq->broken;
+}
+EXPORT_SYMBOL_GPL(virtqueue_is_broken);
+
 MODULE_LICENSE("GPL");
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 4557e8a..f15f6e7 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -76,6 +76,8 @@ void *virtqueue_detach_unused_buf(struct virtqueue *vq);
 
 unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
 
+bool virtqueue_is_broken(struct virtqueue *vq);
+
 /**
  * virtio_device - representation of a device using virtio
  * @index: unique position on the virtio bus
-- 
1.8.3.1

[PATCH V2 RFC 6/9] virtio_blk: verify if queue is broken after virtqueue_get_buf()

[Heinz Graalfs]

In case virtqueue_get_buf() returned with a NULL pointer verify if the
virtqueue is broken in order to leave while loop.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/block/virtio_blk.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 6472395..2d43be4 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -292,6 +292,8 @@ static void virtblk_done(struct virtqueue *vq)
 				req_done = true;
 			}
 		}
+		if (unlikely(virtqueue_is_broken(vq)))
+			break;
 	} while (!virtqueue_enable_cb(vq));
 	/* In case queue is stopped waiting for more buffers. */
 	if (req_done)
-- 
1.8.3.1

[PATCH V2 RFC 7/9] virtio_console: verify if queue is broken after virtqueue_get_buf()

[Heinz Graalfs]

If virtqueue_get_buf() returns with a NULL pointer it should be verified
if the virtqueue is broken, in order to avoid loop calling cpu_relax().

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/char/virtio_console.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1b456fe..9dce2f0 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -574,7 +574,8 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
 	spin_lock(&portdev->c_ovq_lock);
 	if (virtqueue_add_outbuf(vq, sg, 1, &cpkt, GFP_ATOMIC) == 0) {
 		virtqueue_kick(vq);
-		while (!virtqueue_get_buf(vq, &len))
+		while (!virtqueue_get_buf(vq, &len)
+			&& !virtqueue_is_broken(vq))
 			cpu_relax();
 	}
 	spin_unlock(&portdev->c_ovq_lock);
@@ -647,7 +648,8 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 	 * we need to kmalloc a GFP_ATOMIC buffer each time the
 	 * console driver writes something out.
 	 */
-	while (!virtqueue_get_buf(out_vq, &len))
+	while (!virtqueue_get_buf(out_vq, &len)
+		&& !virtqueue_is_broken(out_vq))
 		cpu_relax();
 done:
 	spin_unlock_irqrestore(&port->outvq_lock, flags);
-- 
1.8.3.1

[PATCH V2 RFC 8/9] virtio_net: verify if queue is broken after virtqueue_get_buf()

[Heinz Graalfs]

If a virtqueue_get_buf() call returns a NULL pointer a possibly endless while
loop should be avoided by checking for a broken virtqueue.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/net/virtio_net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6584e3a..f092b9d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -802,7 +802,8 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	/* Spin for a response, the kick causes an ioport write, trapping
 	 * into the hypervisor, so the request should be handled immediately.
 	 */
-	while (!virtqueue_get_buf(vi->cvq, &tmp))
+	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
+	       !virtqueue_is_broken(vi->cvq))
 		cpu_relax();
 
 	return status == VIRTIO_NET_OK;
-- 
1.8.3.1

[PATCH V2 RFC 9/9] virtio_scsi: verify if queue is broken after virtqueue_get_buf()

[Heinz Graalfs]

If virtqueue_get_buf() returned with a NULL pointer avoid a possibly
endless loop by checking for a broken virtqueue.

Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
---
 drivers/scsi/virtio_scsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 74b88ef..45bcdb5 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -224,7 +224,8 @@ static void virtscsi_vq_done(struct virtio_scsi *vscsi,
 		virtqueue_disable_cb(vq);
 		while ((buf = virtqueue_get_buf(vq, &len)) != NULL)
 			fn(vscsi, buf);
-	} while (!virtqueue_enable_cb(vq));
+	} while (unlikely(!virtqueue_is_broken(vq)) &&
+		 !virtqueue_enable_cb(vq));
 	spin_unlock_irqrestore(&virtscsi_vq->vq_lock, flags);
 }
 
-- 
1.8.3.1

Re: [PATCH V2 RFC 1/9] virtio_ring: change host notification API

[Rusty Russell]

Heinz Graalfs <graalfs@linux.vnet.ibm.com> writes:
> Currently a host kick error is silently ignored and not reflected in
> the virtqueue of a particular virtio device.
>
> Changing the notify API for guest->host notification seems to be one
> prerequisite in order to be able to handle such errors in the context
> where the kick is triggered.
>
> This patch changes the notify API. The notify function must return a
> negative int return value in case the host notification failed.

I think we need a bool here:

> -	kvm_hypercall1(KVM_S390_VIRTIO_NOTIFY, config->address);
> +	rc = kvm_hypercall1(KVM_S390_VIRTIO_NOTIFY, config->address);
> +	if (rc < 0)
> +		return rc;
> +	return 0;
>  }

I have no idea what this hypercall returns on failure...

> -static void virtio_ccw_kvm_notify(struct virtqueue *vq)
> +static int virtio_ccw_kvm_notify(struct virtqueue *vq)
>  {
>  	struct virtio_ccw_vq_info *info = vq->priv;
>  	struct virtio_ccw_device *vcdev;
> @@ -384,6 +384,9 @@ static void virtio_ccw_kvm_notify(struct virtqueue *vq)
>  	vcdev = to_vc_device(info->vq->vdev);
>  	ccw_device_get_schid(vcdev->cdev, &schid);
>  	info->cookie = do_kvm_notify(schid, vq->index, info->cookie);
> +	if (info->cookie < 0)
> +		return info->cookie;
> +	return 0;

Nor this one.

Since the caller can't really use the return value, I think a bool is
correct.

Cheers,
Rusty.

Re: [PATCH V2 RFC 9/9] virtio_scsi: verify if queue is broken after virtqueue_get_buf()

[Rusty Russell]

Heinz Graalfs <graalfs@linux.vnet.ibm.com> writes:
> If virtqueue_get_buf() returned with a NULL pointer avoid a possibly
> endless loop by checking for a broken virtqueue.
>
> Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>

Yes, these look good.

Please just repost the first one, and I'll apply.

Thanks,
Rusty.

Re: [PATCH V2 RFC 9/9] virtio_scsi: verify if queue is broken after virtqueue_get_buf()

[Rusty Russell]

Heinz Graalfs <graalfs@linux.vnet.ibm.com> writes:
> If virtqueue_get_buf() returned with a NULL pointer avoid a possibly
> endless loop by checking for a broken virtqueue.
>
> Signed-off-by: Heinz Graalfs <graalfs@linux.vnet.ibm.com>
> ---
>  drivers/scsi/virtio_scsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 74b88ef..45bcdb5 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -224,7 +224,8 @@ static void virtscsi_vq_done(struct virtio_scsi *vscsi,
>  		virtqueue_disable_cb(vq);
>  		while ((buf = virtqueue_get_buf(vq, &len)) != NULL)
>  			fn(vscsi, buf);
> -	} while (!virtqueue_enable_cb(vq));
> +	} while (unlikely(!virtqueue_is_broken(vq)) &&
> +		 !virtqueue_enable_cb(vq));
>  	spin_unlock_irqrestore(&virtscsi_vq->vq_lock, flags);

unlikely(!virtqueue_is_broken(vq))?

I didn't apply this one.

I'd prefer:

	spin_lock_irqsave(&virtscsi_vq->vq_lock, flags);
	do {
		virtqueue_disable_cb(vq);
		while ((buf = virtqueue_get_buf(vq, &len)) != NULL)
			fn(vscsi, buf);

                if (unlikely(virtqueue_is_broken(vq)))
                        break;
	} while (!virtqueue_enable_cb(vq));
	spin_unlock_irqrestore(&virtscsi_vq->vq_lock, flags);

Thanks,
Rusty.