On Fri, 21 May 2021 09:33:13 -0400
Steven Sistare <steven.sist...@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.sist...@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.sist...@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
