Re: [PATCH v4] hw/nvme: Use ioeventfd to handle doorbell updates

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

On Jul 27 16:16, Jinhao Fan wrote:
> at 3:06 PM, Klaus Jensen <its@irrelevant.dk> wrote:
> 
> > On Jul 26 14:08, Klaus Jensen wrote:
> >> Alright. Forget about the iommu, that was just a coincidence.
> >> 
> >> This patch seems to fix it. I guess it is the
> >> event_notifier_set_handler(..., NULL) that does the trick, but I'd like
> >> to understand why ;)
> >> 
> >> 
> >> diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
> >> index 533ad14e7a61..3bc3c6bfbe78 100644
> >> --- i/hw/nvme/ctrl.c
> >> +++ w/hw/nvme/ctrl.c
> >> @@ -4238,7 +4238,9 @@ static void nvme_cq_notifier(EventNotifier *e)
> >>     NvmeCQueue *cq = container_of(e, NvmeCQueue, notifier);
> >>     NvmeCtrl *n = cq->ctrl;
> >> 
> >> -    event_notifier_test_and_clear(&cq->notifier);
> >> +    if (!event_notifier_test_and_clear(e)) {
> >> +        return;
> >> +    }
> >> 
> >>     nvme_update_cq_head(cq);
> >> 
> >> @@ -4275,7 +4277,9 @@ static void nvme_sq_notifier(EventNotifier *e)
> >> {
> >>     NvmeSQueue *sq = container_of(e, NvmeSQueue, notifier);
> >> 
> >> -    event_notifier_test_and_clear(&sq->notifier);
> >> +    if (!event_notifier_test_and_clear(e)) {
> >> +        return;
> >> +    }
> >> 
> 
> Yes, virtio also checks the return value of event_notifier_test_and_clear().
> 
> >>     nvme_process_sq(sq);
> >> }
> >> @@ -4307,6 +4311,8 @@ static void nvme_free_sq(NvmeSQueue *sq, NvmeCtrl *n)
> >>     if (sq->ioeventfd_enabled) {
> >>         memory_region_del_eventfd(&n->iomem,
> >>                                   0x1000 + offset, 4, false, 0, &sq->notifier);
> >> +        event_notifier_set_handler(&sq->notifier, NULL);
> >> +        nvme_sq_notifier(&sq->notifier);
> >>         event_notifier_cleanup(&sq->notifier);
> >>     }
> >>     g_free(sq->io_req);
> >> @@ -4697,6 +4703,8 @@ static void nvme_free_cq(NvmeCQueue *cq, NvmeCtrl *n)
> >>     if (cq->ioeventfd_enabled) {
> >>         memory_region_del_eventfd(&n->iomem,
> >>                                   0x1000 + offset, 4, false, 0, &cq->notifier);
> >> +        event_notifier_set_handler(&cq->notifier, NULL);
> >> +        nvme_cq_notifier(&cq->notifier);
> >>         event_notifier_cleanup(&cq->notifier);
> >>     }
> >>     if (msix_enabled(&n->parent_obj)) {
> > 
> 
> I’m a bit messed up here. I will further check how virtio handles queue deletion today.

Yeah, the API documentation is not exactly exhaustive on the specifics
here, so I kinda inferred this from the virtio code.

If this is the way to do it, then I think I will follow up with a patch
for the event notifier api with a not on how teardown should be done.

Paolo - get_maintainer.pl spits you out for main-loop.h. Any chance you
could shed some light on this?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]