Re: [PATCH V3 11/22] vfio-pci: refactor for cpr

[Alex Williamson]

On Fri,  7 May 2021 05:25:09 -0700
Steve Sistare <steven.sistare@oracle.com> wrote:

> Export vfio_address_spaces and vfio_listener_skipped_section.
> Add optional eventfd arg to vfio_add_kvm_msi_virq.
> Refactor vector use into a helper vfio_vector_init.
> All for use by cpr in a subsequent patch.  No functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  hw/vfio/common.c              |  4 ++--
>  hw/vfio/pci.c                 | 36 +++++++++++++++++++++++++-----------
>  include/hw/vfio/vfio-common.h |  3 +++
>  3 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ae5654f..9220e64 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -42,7 +42,7 @@
>  
>  VFIOGroupList vfio_group_list =
>      QLIST_HEAD_INITIALIZER(vfio_group_list);
> -static QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces =
> +VFIOAddressSpaceList vfio_address_spaces =
>      QLIST_HEAD_INITIALIZER(vfio_address_spaces);
>  
>  #ifdef CONFIG_KVM
> @@ -534,7 +534,7 @@ static int vfio_host_win_del(VFIOContainer *container, 
> hwaddr min_iova,
>      return -1;
>  }
>  
> -static bool vfio_listener_skipped_section(MemoryRegionSection *section)
> +bool vfio_listener_skipped_section(MemoryRegionSection *section)
>  {
>      return (!memory_region_is_ram(section->mr) &&
>              !memory_region_is_iommu(section->mr)) ||
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 5c65aa0..7a4fb6c 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -411,7 +411,7 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool 
> msix)
>  }
>  
>  static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
> -                                  int vector_n, bool msix)
> +                                  int vector_n, bool msix, int eventfd)
>  {
>      int virq;
>  
> @@ -419,7 +419,9 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, 
> VFIOMSIVector *vector,
>          return;
>      }
>  
> -    if (event_notifier_init(&vector->kvm_interrupt, 0)) {
> +    if (eventfd >= 0) {
> +        event_notifier_init_fd(&vector->kvm_interrupt, eventfd);
> +    } else if (event_notifier_init(&vector->kvm_interrupt, 0)) {
>          return;
>      }

This seems very obfuscated.  The "active" arg of event_notifier_init()
just seems to preload the eventfd with a signal.  What does that have
to do with an eventfd arg to this function?  What if the first branch
returns failure?

>  
> @@ -455,6 +457,22 @@ static void vfio_update_kvm_msi_virq(VFIOMSIVector 
> *vector, MSIMessage msg,
>      kvm_irqchip_commit_routes(kvm_state);
>  }
>  
> +static void vfio_vector_init(VFIOPCIDevice *vdev, int nr, int eventfd)
> +{
> +    VFIOMSIVector *vector = &vdev->msi_vectors[nr];
> +    PCIDevice *pdev = &vdev->pdev;
> +
> +    vector->vdev = vdev;
> +    vector->virq = -1;
> +    if (eventfd >= 0) {
> +        event_notifier_init_fd(&vector->interrupt, eventfd);
> +    } else if (event_notifier_init(&vector->interrupt, 0)) {
> +        error_report("vfio: Error: event_notifier_init failed");
> +    }

Gak, here's that same pattern.

> +    vector->use = true;
> +    msix_vector_use(pdev, nr);
> +}
> +
>  static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>                                     MSIMessage *msg, IOHandler *handler)
>  {
> @@ -466,14 +484,10 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, 
> unsigned int nr,
>  
>      vector = &vdev->msi_vectors[nr];
>  
> +    vfio_vector_init(vdev, nr, -1);
> +
>      if (!vector->use) {
> -        vector->vdev = vdev;
> -        vector->virq = -1;
> -        if (event_notifier_init(&vector->interrupt, 0)) {
> -            error_report("vfio: Error: event_notifier_init failed");
> -        }
> -        vector->use = true;
> -        msix_vector_use(pdev, nr);
> +        vfio_vector_init(vdev, nr, -1);
>      }

Huh?  That's not at all "no functional change".  Also the branch is
entirely dead code now.

>  
>      qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
> @@ -491,7 +505,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, 
> unsigned int nr,
>          }
>      } else {
>          if (msg) {
> -            vfio_add_kvm_msi_virq(vdev, vector, nr, true);
> +            vfio_add_kvm_msi_virq(vdev, vector, nr, true, -1);
>          }
>      }
>  
> @@ -641,7 +655,7 @@ retry:
>           * Attempt to enable route through KVM irqchip,
>           * default to userspace handling if unavailable.
>           */
> -        vfio_add_kvm_msi_virq(vdev, vector, i, false);
> +        vfio_add_kvm_msi_virq(vdev, vector, i, false, -1);
>      }

And then we're not really passing an eventfd anyway :-\  I'm so
confused...

Thanks,
Alex

>  
>      /* Set interrupt type prior to possible interrupts */
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 6141162..00acb85 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -204,6 +204,8 @@ int vfio_get_device(VFIOGroup *group, const char *name,
>  extern const MemoryRegionOps vfio_region_ops;
>  typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
>  extern VFIOGroupList vfio_group_list;
> +typedef QLIST_HEAD(, VFIOAddressSpace) VFIOAddressSpaceList;
> +extern VFIOAddressSpaceList vfio_address_spaces;
>  
>  bool vfio_mig_active(void);
>  int64_t vfio_mig_bytes_transferred(void);
> @@ -222,6 +224,7 @@ struct vfio_info_cap_header *
>  vfio_get_device_info_cap(struct vfio_device_info *info, uint16_t id);
>  #endif
>  extern const MemoryListener vfio_prereg_listener;
> +bool vfio_listener_skipped_section(MemoryRegionSection *section);
>  
>  int vfio_spapr_create_window(VFIOContainer *container,
>                               MemoryRegionSection *section,