Hi Keith,
On Mon, Nov 26, 2018 at 09:17:17AM -0700, Keith Busch wrote:
> On Sun, Nov 25, 2018 at 03:03:55PM -0800, Guenter Roeck wrote:
> > So far the code generates interrupts even if it doesn't pass any new
> > information to the user. This can result in spurious interrupts as
> > sometimes observed with Linux.
> >
> > Only generate interrupts if warranted, ie if anything new is passed
> > to the emulated code.
> >
> > Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> > ---
> > hw/block/nvme.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 9fbe567..c543c01 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -252,6 +252,7 @@ static void nvme_post_cqes(void *opaque)
> > NvmeCQueue *cq = opaque;
> > NvmeCtrl *n = cq->ctrl;
> > NvmeRequest *req, *next;
> > + bool assert_irq = false;
> >
> > QTAILQ_FOREACH_SAFE(req, &cq->req_list, entry, next) {
> > NvmeSQueue *sq;
> > @@ -271,8 +272,11 @@ static void nvme_post_cqes(void *opaque)
> > pci_dma_write(&n->parent_obj, addr, (void *)&req->cqe,
> > sizeof(req->cqe));
> > QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
> > + assert_irq = true;
> > + }
> > + if (assert_irq) {
> > + nvme_irq_assert(n, cq);
> > }
> > - nvme_irq_assert(n, cq);
> > }
> >
> > static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
> > --
>
> There is a corner case this may break. For example, a controller posts
> 2 completions. The host happens to only see one of those completions due
> to the inherent race when reading the phase bit. After the host updates
> the CQ DB, the controller ought to send another interrupt even if it
> didn't post any new CQEs to prevent command completion timeouts. This
> might get the right behavior:
>
> ---
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..486db6e561 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
> sizeof(req->cqe));
> QTAILQ_INSERT_TAIL(&sq->req_list, req, entry);
> }
> - nvme_irq_assert(n, cq);
> + if (cq->tail != cq->head) {
> + nvme_irq_assert(n, cq);
> + }
> }
>
That works for me as well, and looks much cleaner. Should I resubmit,
or do you want to take it from here ?
Thanks,
Guenter
