All of lore.kernel.org
 help / color / mirror / Atom feed
From: Klaus Jensen <its@irrelevant.dk>
To: qemu-devel@nongnu.org
Cc: "Keith Busch" <kbusch@kernel.org>,
	"Klaus Jensen" <k.jensen@samsung.com>,
	"Jakub Jermář" <jakub.jermar@kernkonzept.com>,
	qemu-block@nongnu.org
Subject: Re: [PATCH v2] hw/nvme: fix pin-based interrupt behavior (again)
Date: Mon, 28 Jun 2021 19:02:09 +0200	[thread overview]
Message-ID: <YNoAkfcgpu+iU/VZ@apples.localdomain> (raw)
In-Reply-To: <20210617185542.106252-1-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 --]

  reply	other threads:[~2021-06-28 17:06 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-17 18:55 Klaus Jensen
2021-06-28 17:02 ` Klaus Jensen [this message]
2021-06-28 20:35 ` Keith Busch
2021-06-29  5:19 ` 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=YNoAkfcgpu+iU/VZ@apples.localdomain \
    --to=its@irrelevant.dk \
    --cc=jakub.jermar@kernkonzept.com \
    --cc=k.jensen@samsung.com \
    --cc=kbusch@kernel.org \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --subject='Re: [PATCH v2] hw/nvme: fix pin-based interrupt behavior (again)' \
    /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

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.