Re: [Qemu-devel] [PATCH] nvme: Only generate interupt if warranted

[Guenter Roeck]

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 <address@hidden>
> > ---
> >  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