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

[Steven Sistare]

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
>