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

Itai Handler <[email protected]>
Newsgroups gmane.comp.emulators.qemu.block,gmane.comp.emulators.qemu,gmane.comp.emulators.qemu.stable
Message-ID <[email protected]>
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: [email protected]
Fixes: 83d7ed5c570d ("hw/nvme: fix pin-based interrupt behavior (again)")
Signed-off-by: Itai Handler <[email protected]>
---
 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--;
             }
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.