Re: [PATCH 4/4] hw/nvme: add MSI-x mask handlers for irqfd

From: Klaus Jensen <its@irrelevant.dk>
To: Jinhao Fan <fanjinhao21s@ict.ac.cn>
Cc: qemu-devel@nongnu.org, kbusch@kernel.org, stefanha@gmail.com,
	"open list:nvme" <qemu-block@nongnu.org>
Subject: Re: [PATCH 4/4] hw/nvme: add MSI-x mask handlers for irqfd
Date: Tue, 23 Aug 2022 13:04:33 +0200	[thread overview]
Message-ID: <YwS0QaE7N3vzSex8@apples> (raw)
In-Reply-To: <20220811153739.3079672-5-fanjinhao21s@ict.ac.cn>

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

On Aug 11 23:37, Jinhao Fan wrote:
> When irqfd is enabled, we bypass QEMU's irq emulation and let KVM to
> directly assert the irq. However, KVM is not aware of the device's MSI-x
> masking status. Add MSI-x mask bookkeeping in NVMe emulation and
> detach the corresponding irqfd when the certain vector is masked.
> 
> Signed-off-by: Jinhao Fan <fanjinhao21s@ict.ac.cn>
> ---
>  hw/nvme/ctrl.c       | 82 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/nvme/nvme.h       |  2 ++
>  hw/nvme/trace-events |  3 ++
>  3 files changed, 87 insertions(+)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 63f988f2f9..ac5460c7c8 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7478,10 +7478,84 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev, uint8_t offset)
>  
>      return 0;
>  }
> +static int nvme_vector_unmask(PCIDevice *pci_dev, unsigned vector,
> +                               MSIMessage msg)
> +{
> +    NvmeCtrl *n = NVME(pci_dev);
> +    int ret;
> +
> +    trace_pci_nvme_irq_unmask(vector, msg.address, msg.data);
> +    
> +    for (uint32_t i = 0; i < n->params.max_ioqpairs + 1; i++) {

This loop is OK for now, but could be done better with a reverse
mapping.

This is just an observation. Don't spend time on doing it since we are
not aware of any host actually doing masking/unmasking.

> +        NvmeCQueue *cq = n->cq[i];
> +        /* 
> +         * If this function is called, then irqfd must be available. Therefore,
> +         * irqfd must be in use if cq->assert_notifier.initialized is true.
> +         */

The wording indicates that you want to assert() that assumption instead.

> +        if (cq && cq->vector == vector && cq->assert_notifier.initialized) {
> +            if (cq->msg.data != msg.data || cq->msg.address != msg.address) {
> +                ret = kvm_irqchip_update_msi_route(kvm_state, cq->virq, msg,
> +                                                   pci_dev);
> +                if (ret < 0) {
> +                    return ret;
> +                }
> +                kvm_irqchip_commit_routes(kvm_state);
> +                cq->msg = msg;
> +            }
> +
> +            ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> +                                                     &cq->assert_notifier,
> +                                                     NULL, cq->virq);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void nvme_vector_mask(PCIDevice *pci_dev, unsigned vector)
> +{
> +    NvmeCtrl *n = NVME(pci_dev);
> +
> +    trace_pci_nvme_irq_mask(vector);
> +    
> +    for (uint32_t i = 0; i < n->params.max_ioqpairs + 1; i++) {
> +        NvmeCQueue *cq = n->cq[i];
> +        if (cq && cq->vector == vector && cq->assert_notifier.initialized) {
> +            kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state,
> +                                                  &cq->assert_notifier,
> +                                                  cq->virq);
> +        }
> +    }
> +}
> +
> +static void nvme_vector_poll(PCIDevice *pci_dev,
> +                             unsigned int vector_start,
> +                             unsigned int vector_end)
> +{
> +    NvmeCtrl *n = NVME(pci_dev);
> +
> +    trace_pci_nvme_irq_poll(vector_start, vector_end);
> +
> +    for (uint32_t i = 0; i < n->params.max_ioqpairs + 1; i++) {
> +        NvmeCQueue *cq = n->cq[i];
> +        if (cq && cq->vector >= vector_start && cq->vector <= vector_end 
> +            && msix_is_masked(pci_dev, cq->vector) 
> +            && cq->assert_notifier.initialized) {
> +            if (event_notifier_test_and_clear(&cq->assert_notifier)) {
> +                msix_set_pending(pci_dev, i);
> +            }
> +        }
> +    }
> +}

I'm a little fuzzy on this - what causes this function to be invoked?

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