| Message ID | 1620390320-301716-12-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Live Update | expand |
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,
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, >
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
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 --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,
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(-)