Re: [PATCH v2] hw/nvme: fix pin-based interrupt behavior (again)
On Jun 17 20:55, 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 --- 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_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..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 Applied to nvme-next! signature.asc Description: PGP signature
Re: [PATCH v2] hw/nvme: fix pin-based interrupt behavior (again)
On Thu, Jun 17, 2021 at 08:55:42PM +0200, 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 Looks good. Reviewed-by: Keith Busch
Re: [PATCH v2] hw/nvme: fix pin-based interrupt behavior (again)
On Jun 17 20:55, 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 --- 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_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..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 :) signature.asc Description: PGP signature
[PATCH v2] 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 --- 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_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..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