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

[Steven Sistare]

On 5/19/2021 6:38 PM, Alex Williamson wrote:
> 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?

Perhaps you mis-read the code?  The function called in the first branch is 
different than
the function called in the second branch.  And event_notifier_init_fd is void 
and never fails.

Eschew obfuscation.

Gesundheit.

>> @@ -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.

Good catch, thank you.  This is a rebase error.  The unconditional call to 
vfio_vector_init
should not be there.  With that fix, we have:

    if (!vector->use) {
        vfio_vector_init(vdev, nr, -1);
    }

and there is no functional change; the actions performed in vfio_vector_init 
are identical to 
those deleted here.

>>      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...

This patch just adds the eventfd arg.  The next few patches pass valid 
eventfd's from the
cpr code paths.

- Steve

>>      /* 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,
>