Re: [PATCH 1/5] vfio: use helper to simplfy the failure path in vfio_msi_enable

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 25 Aug 2021 15:56:16 +0800
"Longpeng(Mike)" <longpeng2@huawei.com> wrote:

> The main difference of the failure path in vfio_msi_enable and
> vfio_msi_disable_common is enable INTX or not.
> 
> Extend the vfio_msi_disable_common to provide a arg to decide

"an arg"

> whether need to fallback, and then we can use this helper to
> instead the redundant code in vfio_msi_enable.

Do you mean s/instead/replace/?

> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  hw/vfio/pci.c | 34 ++++++++++++----------------------
>  1 file changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e1ea1d8..7cc43fe 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -47,6 +47,7 @@
>  
>  static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
>  static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
> +static void vfio_msi_disable_common(VFIOPCIDevice *vdev, bool enable_intx);
>  
>  /*
>   * Disabling BAR mmaping can be slow, but toggling it around INTx can
> @@ -650,29 +651,17 @@ retry:
>      if (ret) {
>          if (ret < 0) {
>              error_report("vfio: Error: Failed to setup MSI fds: %m");
> -        } else if (ret != vdev->nr_vectors) {

This part of the change is subtle and not mentioned in the commit log.
It does seem unnecessary to test against this specific return value
since any positive return is an error indicating the number of vectors
we should retry with, but this change should be described in a separate
patch.

> +        } else {
>              error_report("vfio: Error: Failed to enable %d "
>                           "MSI vectors, retry with %d", vdev->nr_vectors, ret);
>          }
>  
> -        for (i = 0; i < vdev->nr_vectors; i++) {
> -            VFIOMSIVector *vector = &vdev->msi_vectors[i];
> -            if (vector->virq >= 0) {
> -                vfio_remove_kvm_msi_virq(vector);
> -            }
> -            qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
> -                                NULL, NULL, NULL);
> -            event_notifier_cleanup(&vector->interrupt);
> -        }
> -
> -        g_free(vdev->msi_vectors);
> -        vdev->msi_vectors = NULL;
> +        vfio_msi_disable_common(vdev, false);
>  
> -        if (ret > 0 && ret != vdev->nr_vectors) {
> +        if (ret > 0) {
>              vdev->nr_vectors = ret;
>              goto retry;
>          }
> -        vdev->nr_vectors = 0;
>  
>          /*
>           * Failing to setup MSI doesn't really fall within any specification.
> @@ -680,7 +669,6 @@ retry:
>           * out to fall back to INTx for this device.
>           */
>          error_report("vfio: Error: Failed to enable MSI");
> -        vdev->interrupt = VFIO_INT_NONE;
>  
>          return;
>      }
> @@ -688,7 +676,7 @@ retry:
>      trace_vfio_msi_enable(vdev->vbasedev.name, vdev->nr_vectors);
>  }
>  
> -static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
> +static void vfio_msi_disable_common(VFIOPCIDevice *vdev, bool enable_intx)

I'd rather avoid these sort of modal options to functions where we can.
Maybe this suggests instead that re-enabling INTx should be removed
from the common helper and callers needing to do so should do it
outside of the common helper.  Thanks,

Alex


>  {
>      Error *err = NULL;
>      int i;
> @@ -710,9 +698,11 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
>      vdev->nr_vectors = 0;
>      vdev->interrupt = VFIO_INT_NONE;
>  
> -    vfio_intx_enable(vdev, &err);
> -    if (err) {
> -        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +    if (enable_intx) {
> +        vfio_intx_enable(vdev, &err);
> +        if (err) {
> +            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
>      }
>  }
>  
> @@ -737,7 +727,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
>          vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>      }
>  
> -    vfio_msi_disable_common(vdev);
> +    vfio_msi_disable_common(vdev, true);
>  
>      memset(vdev->msix->pending, 0,
>             BITS_TO_LONGS(vdev->msix->entries) * sizeof(unsigned long));
> @@ -748,7 +738,7 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
>  static void vfio_msi_disable(VFIOPCIDevice *vdev)
>  {
>      vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSI_IRQ_INDEX);
> -    vfio_msi_disable_common(vdev);
> +    vfio_msi_disable_common(vdev, true);
>  
>      trace_vfio_msi_disable(vdev->vbasedev.name);
>  }