Re: [PATCH v2] hw/nvme: fix pin-based interrupt behavior (again)

[Klaus Jensen <its@irrelevant.dk>]

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

On Jun 17 20:55, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>Jakub noticed[1] that, when using pin-based interrupts, the device will
>unconditionally deasssert when any CQEs are acknowledged. However, the
>pin should not be deasserted if other completion queues still holds
>unacknowledged CQEs.
>
>The bug is an artifact of commit ca247d35098d ("hw/block/nvme: fix
>pin-based interrupt behavior") which fixed one bug but introduced
>another. This is the third time someone tries to fix pin-based
>interrupts (see commit 5e9aa92eb1a5 ("hw/block: Fix pin-based interrupt
>behaviour of NVMe"))...
>
>Third time's the charm, so fix it, again, by keeping track of how many
>CQs have unacknowledged CQEs and only deassert when all are cleared.
>
>  [1]: <20210610114624.304681-1-jakub.jermar@kernkonzept.com>
>
>Fixes: ca247d35098d ("hw/block/nvme: fix pin-based interrupt behavior")
>Reported-by: Jakub Jermář <jakub.jermar@kernkonzept.com>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
>
>v2:
>  - only refcount for CQs with interrupts enabled (Keith)
>
> hw/nvme/nvme.h |  1 +
> hw/nvme/ctrl.c | 18 +++++++++++++++++-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
>diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>index 93a7e0e5380e..60250b579464 100644
>--- a/hw/nvme/nvme.h
>+++ b/hw/nvme/nvme.h
>@@ -405,6 +405,7 @@ typedef struct NvmeCtrl {
>     uint32_t    max_q_ents;
>     uint8_t     outstanding_aers;
>     uint32_t    irq_status;
>+    int         cq_pending;
>     uint64_t    host_timestamp;                 /* Timestamp sent by the host */
>     uint64_t    timestamp_set_qemu_clock_ms;    /* QEMU clock time */
>     uint64_t    starttime_ms;
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 7dea64b72e6a..b8d4f9ce8532 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -473,7 +473,9 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue *cq)
>             return;
>         } else {
>             assert(cq->vector < 32);
>-            n->irq_status &= ~(1 << cq->vector);
>+            if (!n->cq_pending) {
>+                n->irq_status &= ~(1 << cq->vector);
>+            }
>             nvme_irq_check(n);
>         }
>     }
>@@ -1258,6 +1260,7 @@ static void nvme_post_cqes(void *opaque)
>     NvmeCQueue *cq = opaque;
>     NvmeCtrl *n = cq->ctrl;
>     NvmeRequest *req, *next;
>+    bool pending = cq->head != cq->tail;
>     int ret;
>
>     QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
>@@ -1287,6 +1290,10 @@ static void nvme_post_cqes(void *opaque)
>         QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
>     }
>     if (cq->tail != cq->head) {
>+        if (cq->irq_enabled && !pending) {
>+            n->cq_pending++;
>+        }
>+
>         nvme_irq_assert(n, cq);
>     }
> }
>@@ -4099,6 +4106,11 @@ static uint16_t nvme_del_cq(NvmeCtrl *n, NvmeRequest *req)
>         trace_pci_nvme_err_invalid_del_cq_notempty(qid);
>         return NVME_INVALID_QUEUE_DEL;
>     }
>+
>+    if (cq->irq_enabled && cq->tail != cq->head) {
>+        n->cq_pending--;
>+    }
>+
>     nvme_irq_deassert(n, cq);
>     trace_pci_nvme_del_cq(qid);
>     nvme_free_cq(cq, n);
>@@ -5758,6 +5770,10 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
>         }
>
>         if (cq->tail == cq->head) {
>+            if (cq->irq_enabled) {
>+                n->cq_pending--;
>+            }
>+
>             nvme_irq_deassert(n, cq);
>         }
>     } else {
>-- 
>2.32.0
>

Bump :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]