Re: [PATCH v2] hw/nvme: be more careful when deasserting IRQs

[Klaus Jensen <its@irrelevant.dk>]

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

On Jun 15 09:42, Jakub Jermář wrote:
>On 6/14/21 8:19 PM, Klaus Jensen wrote:
>>On Jun 14 15:54, Jakub Jermář wrote:
>>>An IRQ vector used by a completion queue cannot be deasserted without
>>>first checking if the same vector does not need to stay asserted for
>>>some other completion queue. To this end the controller structure is
>>>extended by a counter of asserted completion queues.
>>>
>>>To prevent incrementing the counter for completion queues that are
>>>asserted repeatedly, each completion queue is extended by a flag which
>>>tells whether the queue is currently asserted.
>>>
>>
>>I wasn't clear on this on my last review, but the misunderstanding 
>>here is that the completion queue vector somehow matters for 
>>pin-based interrupts - it does not. There is only *one* interrupt 
>>vector and if the controller is not using MSI-X, then the Interrupt 
>>Vector (IV) field of the Create I/O Completion Queue command *must* 
>>be zero.
>>
>>In other words, all that matters is if there are one or more CQEs 
>>posted (in any queue).
>
>Ah, my bad. I got confused by the assert(vector < 32).
>
>>Would this (untested) patch do the trick?
>
>It works for my testcase, but I am not sure this will work when 
>deassert is called when the IRQ is not necessarily asserted, such as 
>in nvme_del_cq.
>

You are right, I think it needs a check on cq->tail != cq->head before 
decrementing cq_pending.

>Jakub
>
>>diff --git i/hw/nvme/nvme.h w/hw/nvme/nvme.h
>>index 93a7e0e5380e..60250b579464 100644
>>--- i/hw/nvme/nvme.h
>>+++ w/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 i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
>>index 7dea64b72e6a..4de4b3177463 100644
>>--- i/hw/nvme/ctrl.c
>>+++ w/hw/nvme/ctrl.c
>>@@ -473,6 +473,10 @@ static void nvme_irq_deassert(NvmeCtrl *n, 
>>NvmeCQueue *cq)
>>              return;
>>          } else {
>>              assert(cq->vector < 32);
>>+            if (--(n->cq_pending)) {
>>+                return;
>>+            }
>>+
>>              n->irq_status &= ~(1 << cq->vector);
>>              nvme_irq_check(n);
>>          }
>>@@ -1258,6 +1262,7 @@ static void nvme_post_cqes(void *opaque)
>>      NvmeCQueue *cq = opaque;
>>      NvmeCtrl *n = cq->ctrl;
>>      NvmeRequest *req, *next;
>>+    bool empty = cq->head == cq->tail;
>>      int ret;
>>
>>      QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
>>@@ -1287,6 +1292,10 @@ static void nvme_post_cqes(void *opaque)
>>          QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
>>      }
>>      if (cq->tail != cq->head) {
>>+        if (empty) {
>>+            n->cq_pending++;
>>+        }
>>+
>>          nvme_irq_assert(n, cq);
>>      }
>>  }
>>
>>
>>>Signed-off-by: Jakub Jermar <jakub.jermar@kernkonzept.com>
>>>---
>>>hw/nvme/ctrl.c | 22 ++++++++++++++++------
>>>hw/nvme/nvme.h |  2 ++
>>>2 files changed, 18 insertions(+), 6 deletions(-)
>>>
>>>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>>>index 0bcaf7192f..97a5d768ee 100644
>>>--- a/hw/nvme/ctrl.c
>>>+++ b/hw/nvme/ctrl.c
>>>@@ -451,9 +451,13 @@ static void nvme_irq_assert(NvmeCtrl *n, 
>>>NvmeCQueue *cq)
>>>            msix_notify(&(n->parent_obj), cq->vector);
>>>        } else {
>>>            trace_pci_nvme_irq_pin();
>>>-            assert(cq->vector < 32);
>>>-            n->irq_status |= 1 << cq->vector;
>>>-            nvme_irq_check(n);
>>>+            if (!cq->irq_asserted) {
>>>+                cq->irq_asserted = true;
>>>+                assert(cq->vector < 32);
>>>+                n->irq_asserted_cnt[cq->vector]++;
>>>+                n->irq_status |= 1 << cq->vector;
>>>+                nvme_irq_check(n);
>>>+            }
>>>        }
>>>    } else {
>>>        trace_pci_nvme_irq_masked();
>>>@@ -466,9 +470,15 @@ static void nvme_irq_deassert(NvmeCtrl *n, 
>>>NvmeCQueue *cq)
>>>        if (msix_enabled(&(n->parent_obj))) {
>>>            return;
>>>        } else {
>>>-            assert(cq->vector < 32);
>>>-            n->irq_status &= ~(1 << cq->vector);
>>>-            nvme_irq_check(n);
>>>+            if (cq->irq_asserted) {
>>>+                cq->irq_asserted = false;
>>>+                assert(cq->vector < 32);
>>>+                assert(n->irq_asserted_cnt[cq->vector]);
>>>+                if (n->irq_asserted_cnt[cq->vector]-- == 1) {
>>>+                    n->irq_status &= ~(1 << cq->vector);
>>>+                }
>>>+                nvme_irq_check(n);
>>>+            }
>>>        }
>>>    }
>>>}
>>>diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>>>index 81a35cda14..753bf7a923 100644
>>>--- a/hw/nvme/nvme.h
>>>+++ b/hw/nvme/nvme.h
>>>@@ -352,6 +352,7 @@ typedef struct NvmeCQueue {
>>>    uint32_t    head;
>>>    uint32_t    tail;
>>>    uint32_t    vector;
>>>+    bool        irq_asserted;
>>>    uint32_t    size;
>>>    uint64_t    dma_addr;
>>>    QEMUTimer   *timer;
>>>@@ -404,6 +405,7 @@ typedef struct NvmeCtrl {
>>>    uint32_t    max_q_ents;
>>>    uint8_t     outstanding_aers;
>>>    uint32_t    irq_status;
>>>+    uint16_t    irq_asserted_cnt[32];
>>>    uint64_t    host_timestamp;                 /* Timestamp sent 
>>>by the host */
>>>    uint64_t    timestamp_set_qemu_clock_ms;    /* QEMU clock time */
>>>    uint64_t    starttime_ms;
>>>-- 
>>>2.31.1
>>>
>>>
>>
>
>

-- 
One of us - No more doubt, silence or taboo about mental illness.

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