[PATCH] hw/nvme: fix cq_pending underflow that wedges pin-based interrupts

[PATCH] hw/nvme: fix cq_pending underflow that wedges pin-based interrupts

[Itai Handler]

n->cq_pending counts the completion queues that hold CQEs the host has
not acknowledged yet. With pin-based interrupts it is what keeps the
line asserted while any queue still has unacknowledged CQEs:
nvme_irq_deassert() clears the vector's bit in irq_status only when the
count drops to zero.

The accounting is not symmetric. nvme_post_cqes() increments the
counter once per empty -> non-empty transition of a CQ, but the two CQ
head doorbell paths, nvme_process_db() and nvme_cq_notifier(),
decrement it on every update that leaves the queue empty, including an
update to a queue that was already empty. nvme_del_cq() gets this right
and checks cq->tail != cq->head before decrementing; the doorbell paths
do not. A host that writes the CQ head doorbell without having consumed
a CQE - a redundant write, e.g. from a completion path racing with
another one that already drained the queue - thus decrements more often
than the device incremented, and cq_pending goes negative.

Once it is negative it never gets back to zero: nvme_irq_deassert()
stops clearing irq_status, nvme_irq_check() keeps re-asserting the pin,
and the guest takes an endless interrupt for an empty completion queue.
Linux eventually gives up on the line

  irq 26: nobody cared (try booting with the "irqpoll" option)
  handlers: [<...>] nvme_irq
  Disabling IRQ #26

and all I/O to the device hangs. MSI-X guests are not affected, they
never look at cq_pending.

Only decrement when the queue actually had unacknowledged CQEs. That
restores the pairing with the increment in nvme_post_cqes() and makes
the counter unable to underflow.

Cc: qemu-stable@nongnu.org
Fixes: 83d7ed5c570d ("hw/nvme: fix pin-based interrupt behavior (again)")
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
 hw/nvme/ctrl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index bd6ad64..655af08 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4761,15 +4761,18 @@ static void nvme_cq_notifier(EventNotifier *e)
 {
     NvmeCQueue *cq = container_of(e, NvmeCQueue, notifier);
     NvmeCtrl *n = cq->ctrl;
+    bool pending;
 
     if (!event_notifier_test_and_clear(e)) {
         return;
     }
 
+    pending = cq->tail != cq->head;
+
     nvme_update_cq_head(cq);
 
     if (cq->tail == cq->head) {
-        if (cq->irq_enabled) {
+        if (cq->irq_enabled && pending) {
             n->cq_pending--;
         }
 
@@ -8509,6 +8512,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
 
         uint16_t new_head = val & 0xffff;
         NvmeCQueue *cq;
+        bool pending;
 
         qid = (addr - (0x1000 + (1 << 2))) >> 3;
         if (unlikely(nvme_check_cqid(n, qid))) {
@@ -8563,13 +8567,14 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
             qemu_bh_schedule(cq->bh);
         }
 
+        pending = cq->tail != cq->head;
         cq->head = new_head;
         if (!qid && n->dbbuf_enabled) {
             stl_le_pci_dma(pci, cq->db_addr, cq->head, MEMTXATTRS_UNSPECIFIED);
         }
 
         if (cq->tail == cq->head) {
-            if (cq->irq_enabled) {
+            if (cq->irq_enabled && pending) {
                 n->cq_pending--;
             }