Re: [PATCH v3 7/9] vfio: add infrastructure to commit the deferred kvm routing

[Alex Williamson]

On Tue, 21 Sep 2021 07:02:00 +0800
"Longpeng(Mike)" <longpeng2@huawei.com> wrote:

> 'defer_kvm_irq_routing' indicates whether we should defer to commit
> the kvm routing.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  hw/vfio/pci.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/vfio/pci.h |  1 +
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8e97ca93cf..8fe238b11d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -423,12 +423,24 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, 
> VFIOMSIVector *vector,
>          return;
>      }
>  
> -    virq = kvm_irqchip_add_msi_route(kvm_state, vector_n, &vdev->pdev);
> +    virq = kvm_irqchip_add_deferred_msi_route(kvm_state, vector_n, 
> &vdev->pdev);
>      if (virq < 0) {
>          event_notifier_cleanup(&vector->kvm_interrupt);
>          return;
>      }
>  
> +    if (vdev->defer_kvm_irq_routing) {
> +        /*
> +         * Hold the allocated virq in vector->virq temporarily, will
> +         * reset it to -1 when we fail to add the corresponding irqfd
> +         * in vfio_commit_kvm_msi_virq().

s/when/if/

> +         */
> +        vector->virq = virq;

Do we need to make this unique to the deferred case or could we use
vector->virq directly and fill it with -1 on all error paths like we do
on a failure in vfio_commit_kvm_msi_virq()?


> +        return;
> +    }
> +
> +    kvm_irqchip_commit_routes(kvm_state);
> +
>      if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, &vector->kvm_interrupt,
>                                         NULL, virq) < 0) {
>          kvm_irqchip_release_virq(kvm_state, virq);
> @@ -567,6 +579,35 @@ static void vfio_msix_vector_release(PCIDevice *pdev, 
> unsigned int nr)
>      }
>  }
>  
> +/* TODO: invoked when enclabe msi/msix vectors */

"enclabe"?  Is this meant to be "enable"?

> +static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice 
> *vdev)

I'd move this function, if not this entire change, to patch 9 rather
than adding these attributes for an unused function.  Thanks,

Alex

> +{
> +    int i;
> +    VFIOMSIVector *vector;
> +
> +    if (!vdev->defer_kvm_irq_routing || !vdev->nr_vectors) {
> +        return;
> +    }
> +
> +    kvm_irqchip_commit_routes(kvm_state);
> +
> +    for (i = 0; i < vdev->nr_vectors; i++) {
> +        vector = &vdev->msi_vectors[i];
> +
> +        if (!vector->use || vector->virq < 0) {
> +            continue;
> +        }
> +
> +        if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> +                                               &vector->kvm_interrupt,
> +                                               NULL, vector->virq) < 0) {
> +            kvm_irqchip_release_virq(kvm_state, vector->virq);
> +            event_notifier_cleanup(&vector->kvm_interrupt);
> +            vector->virq = -1;
> +        }
> +    }
> +}
> +
>  static void vfio_msix_enable(VFIOPCIDevice *vdev)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 64777516d1..d3c5177d37 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -171,6 +171,7 @@ struct VFIOPCIDevice {
>      bool no_kvm_ioeventfd;
>      bool no_vfio_ioeventfd;
>      bool enable_ramfb;
> +    bool defer_kvm_irq_routing;
>      VFIODisplay *dpy;
>      Notifier irqchip_change_notifier;
>  };