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

[Klaus Jensen]

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;
> +    }
>  
>      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)) {

Jinhao,

Do you have any comments on the above patch - does it make sense to you,
considering the effort you've done into researching how virtio does
this?

signature.asc
Description: PGP signature