diff mbox series

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

Message ID 1620390320-301716-12-git-send-email-steven.sistare@oracle.com
State New
Headers show
Series Live Update | expand

Commit Message

Steven Sistare May 7, 2021, 12:25 p.m. UTC 
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(-)

Comments

Alex Williamson May 19, 2021, 10:38 p.m. UTC | #1 
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,
Steven Sistare May 21, 2021, 1:33 p.m. UTC | #2 
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,
>
Alex Williamson May 21, 2021, 9:07 p.m. UTC | #3 
On Fri, 21 May 2021 09:33:13 -0400
Steven Sistare <steven.sistare@oracle.com> wrote:

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

D'oh!  I looked at that so many times trying to figure out what I was
missing and still didn't spot the "_fd" on the first function.  The
fact that @active is an int used as a bool in the non-fd version didn't
help.  Maybe we need our own wrapper just to spread the code out a
bit...

/* Create new or reuse existing eventfd */
static int vfio_event_notifier_init(EventNotifier *e, int fd)
{
    if (fd < 0) {
        return event_notifier_init(e, 0);
    }

    event_notifier_init_fd(e, fd);
    return 0;
}

Or I should just user bigger fonts, but that's somehow more apparent to
me and can be reused below.

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

Yup.

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

Yeah, I couldn't put the pieces together though after repeatedly
misreading eventfd being used as a bool in event_notifier_init(), even
though -1 here should have clued me in too.  Thanks,

Alex
Steven Sistare May 21, 2021, 9:18 p.m. UTC | #4 
On 5/21/2021 5:07 PM, Alex Williamson wrote:
> On Fri, 21 May 2021 09:33:13 -0400
> Steven Sistare <steven.sistare@oracle.com> wrote:
> 
>> 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.
> 
> D'oh!  I looked at that so many times trying to figure out what I was
> missing and still didn't spot the "_fd" on the first function.  The
> fact that @active is an int used as a bool in the non-fd version didn't
> help.  Maybe we need our own wrapper just to spread the code out a
> bit...
> 
> /* Create new or reuse existing eventfd */
> static int vfio_event_notifier_init(EventNotifier *e, int fd)
> {
>     if (fd < 0) {
>         return event_notifier_init(e, 0);
>     }
> 
>     event_notifier_init_fd(e, fd);
>     return 0;
> }

Will do, for both here and below - Steve

> Or I should just user bigger fonts, but that's somehow more apparent to
> me and can be reused below.
> 
>>>> @@ -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.
> 
> Yup.
> 
>>>>      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.
> 
> Yeah, I couldn't put the pieces together though after repeatedly
> misreading eventfd being used as a bool in event_notifier_init(), even
> though -1 here should have clued me in too.  Thanks,
> 
> Alex
>
diff mbox series

Patch

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;
     }
 
@@ -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");
+    }
+    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);
     }
 
     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);
     }
 
     /* 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,