Re: [PATCH] hw/nvme: fix pin-based interrupt behavior (again)
On Jun 17 07:50, Keith Busch wrote:
On Thu, Jun 17, 2021 at 12:08:20PM +0200, Klaus Jensen wrote:
if (cq->tail != cq->head) {
+if (!pending) {
+n->cq_pending++;
+}
You should check cq->irq_enabled before incrementing cq_pending. You
don't want to leave the irq asserted when polled queues have outsanding
CQEs.
Good catch. Thanks!
Re: [PATCH] hw/nvme: fix pin-based interrupt behavior (again)
On Thu, Jun 17, 2021 at 12:08:20PM +0200, Klaus Jensen wrote:
> if (cq->tail != cq->head) {
> +if (!pending) {
> +n->cq_pending++;
> +}
You should check cq->irq_enabled before incrementing cq_pending. You
don't want to leave the irq asserted when polled queues have outsanding
CQEs.
Re: [PATCH] hw/nvme: fix pin-based interrupt behavior (again)
On 6/17/21 12:09 PM, Klaus Jensen wrote:
On Jun 17 12:08, Klaus Jensen wrote:
From: Klaus Jensen
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.jer...@kernkonzept.com>
Fixes: ca247d35098d ("hw/block/nvme: fix pin-based interrupt behavior")
Reported-by: Jakub Jermář
Signed-off-by: Klaus Jensen
---
hw/nvme/nvme.h | 1 +
hw/nvme/ctrl.c | 15 ++-
2 files changed, 15 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..9419f67c4e88 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 (!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->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,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr
addr, int val)
}
if (cq->tail == cq->head) {
+ n->cq_pending--;
nvme_irq_deassert(n, cq);
}
} else {
--
2.32.0
Jakub, can you test this in your environment?
Yep, still tests fine.
Jakub
Re: [PATCH] hw/nvme: fix pin-based interrupt behavior (again)
On Jun 17 12:08, Klaus Jensen wrote:
From: Klaus Jensen
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.jer...@kernkonzept.com>
Fixes: ca247d35098d ("hw/block/nvme: fix pin-based interrupt behavior")
Reported-by: Jakub Jermář
Signed-off-by: Klaus Jensen
---
hw/nvme/nvme.h | 1 +
hw/nvme/ctrl.c | 15 ++-
2 files changed, 15 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_tmax_q_ents;
uint8_t outstanding_aers;
uint32_tirq_status;
+int cq_pending;
uint64_thost_timestamp; /* Timestamp sent by the host */
uint64_ttimestamp_set_qemu_clock_ms;/* QEMU clock time */
uint64_tstarttime_ms;
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 7dea64b72e6a..9419f67c4e88 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 (!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->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,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
}
if (cq->tail == cq->head) {
+n->cq_pending--;
nvme_irq_deassert(n, cq);
}
} else {
--
2.32.0
Jakub, can you test this in your environment?
signature.asc
Description: PGP signature
[PATCH] hw/nvme: fix pin-based interrupt behavior (again)
From: Klaus Jensen
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.jer...@kernkonzept.com>
Fixes: ca247d35098d ("hw/block/nvme: fix pin-based interrupt behavior")
Reported-by: Jakub Jermář
Signed-off-by: Klaus Jensen
---
hw/nvme/nvme.h | 1 +
hw/nvme/ctrl.c | 15 ++-
2 files changed, 15 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_tmax_q_ents;
uint8_t outstanding_aers;
uint32_tirq_status;
+int cq_pending;
uint64_thost_timestamp; /* Timestamp sent by the host
*/
uint64_ttimestamp_set_qemu_clock_ms;/* QEMU clock time */
uint64_tstarttime_ms;
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 7dea64b72e6a..9419f67c4e88 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 (!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->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,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int
val)
}
if (cq->tail == cq->head) {
+n->cq_pending--;
nvme_irq_deassert(n, cq);
}
} else {
--
2.32.0
