From: Klaus Jensen <its@irrelevant.dk>
To: "Jakub Jermář" <jakub.jermar@kernkonzept.com>
Cc: kbusch@kernel.org, qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [PATCH v2] hw/nvme: be more careful when deasserting IRQs
Date: Mon, 14 Jun 2021 20:19:39 +0200 [thread overview]
Message-ID: <YMedu0Ry1YKJ/2SV@apples.localdomain> (raw)
In-Reply-To: <20210614135429.56475-1-jakub.jermar@kernkonzept.com>
[-- Attachment #1: Type: text/plain, Size: 5111 bytes --]
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).
Would this (untested) patch do the trick?
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 --]
next prev parent reply other threads:[~2021-06-14 18:23 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-14 13:54 [PATCH v2] hw/nvme: be more careful when deasserting IRQs Jakub Jermář
2021-06-14 18:19 ` Klaus Jensen [this message]
2021-06-15 7:42 ` Jakub Jermář
2021-06-15 8:12 ` Klaus Jensen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YMedu0Ry1YKJ/2SV@apples.localdomain \
--to=its@irrelevant.dk \
--cc=jakub.jermar@kernkonzept.com \
--cc=kbusch@kernel.org \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.