diff mbox series

[V6,18/27] vfio-pci: refactor for cpr

Message ID 1628286241-217457-19-git-send-email-steven.sistare@oracle.com (mailing list archive)
State New, archived
Headers show
Series Live Update | expand

Commit Message

Steven Sistare Aug. 6, 2021, 9:43 p.m. UTC 
Export vfio_address_spaces and vfio_listener_skipped_section.
Add optional name 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                 | 50 +++++++++++++++++++++++++++++++------------
 include/hw/vfio/vfio-common.h |  3 +++
 3 files changed, 41 insertions(+), 16 deletions(-)

Comments

Alex Williamson Aug. 10, 2021, 4:53 p.m. UTC | #1 
On Fri,  6 Aug 2021 14:43:52 -0700
Steve Sistare <steven.sistare@oracle.com> wrote:

> Export vfio_address_spaces and vfio_listener_skipped_section.
> Add optional name 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.

Why is the name arg optional?  It seems really inconsistent to me that
everything other than MSI/X uses this with a name, but MSI/X use NULL
and in an entirely separate pre-save step we then iterate through all
the {event,irq}fds to save them.  If we asked for a named notifier,
shouldn't we go ahead and save it under that name at that time?  ie.

static int vfio_named_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
                                    const char *name, int nr)
{
    int ret, fd = load_event_fd(vdev, name, nr);

    if (fd >= 0) {
        event_notifier_init_fd(e, fd);
    } else {
        ret = event_notifier_init(e, 0);
        if (ret) {
            return ret;
        }
        save_event_fd(vdev, name, nr, e);
    }
    return 0;
}

Are we not doing this to avoid runtime overhead?

In the process, maybe we can use more descriptive names than
"interrupt", ex. "msi" or "msix".

It also feels a bit forced to me that the entire fd saving uses {name,
id} but vfio is the only caller that makes use of a non-zero id.
Should we instead just wrap all the calls from vfio to append the id to
the name so the common code can just use strcmp()?  Thanks,

Alex
Steven Sistare Aug. 23, 2021, 4:52 p.m. UTC | #2 
Thanks for reviewing, and sorry for the delayed response, I just returned from vacation.

On 8/10/2021 12:53 PM, Alex Williamson wrote:
> On Fri,  6 Aug 2021 14:43:52 -0700
> Steve Sistare <steven.sistare@oracle.com> wrote:
> 
>> Export vfio_address_spaces and vfio_listener_skipped_section.
>> Add optional name 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.
> 
> Why is the name arg optional?  It seems really inconsistent to me that
> everything other than MSI/X uses this with a name, but MSI/X use NULL
> and in an entirely separate pre-save step we then iterate through all
> the {event,irq}fds to save them.  If we asked for a named notifier,
> shouldn't we go ahead and save it under that name at that time?  ie.
> 
> static int vfio_named_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
>                                     const char *name, int nr)
> {
>     int ret, fd = load_event_fd(vdev, name, nr);
> 
>     if (fd >= 0) {
>         event_notifier_init_fd(e, fd);
>     } else {
>         ret = event_notifier_init(e, 0);
>         if (ret) {
>             return ret;
>         }
>         save_event_fd(vdev, name, nr, e);
>     }
>     return 0;
> }
> 
> Are we not doing this to avoid runtime overhead?

OK, I will delete the pre-save function and align the life-cycle of the fd and the event
notifier. (Currently the vfio-pci code does not call cpr_delete_fd.)

> In the process, maybe we can use more descriptive names than
> "interrupt", ex. "msi" or "msix".

I chose "interrupt" and "kvm_interrupt" to match the names of the corresponding 
VFIOMSIVector fields.  Ditto for intx-interrupt, intx-unmask, err, and req, with
minor differences.

> It also feels a bit forced to me that the entire fd saving uses {name,
> id} but vfio is the only caller that makes use of a non-zero id.
> Should we instead just wrap all the calls from vfio to append the id to
> the name so the common code can just use strcmp()?  Thanks,

I liked the simplification in the vfio code, but I will remove the id if you prefer,
and add g_autoptr and g_strdup_printf to each call site.

- Steve
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8728d4d..7918c0d 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -43,7 +43,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
@@ -558,7 +558,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 e1ea1d8..e8e371e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -48,6 +48,20 @@ 
 static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
 static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
 
+/* Create new or reuse existing eventfd */
+static int vfio_named_notifier_init(VFIOPCIDevice *vdev, EventNotifier *e,
+                                    const char *name, int nr)
+{
+    int fd = -1;   /* placeholder until a subsequent patch */
+
+    if (fd >= 0) {
+        event_notifier_init_fd(e, fd);
+        return 0;
+    } else {
+        return event_notifier_init(e, 0);
+    }
+}
+
 /*
  * Disabling BAR mmaping can be slow, but toggling it around INTx can
  * also be a huge overhead.  We try to get the best of both worlds by
@@ -410,7 +424,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)
+                                  const char *name, int nr, bool msix)
 {
     int virq;
 
@@ -418,11 +432,11 @@  static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
         return;
     }
 
-    if (event_notifier_init(&vector->kvm_interrupt, 0)) {
+    if (vfio_named_notifier_init(vdev, &vector->kvm_interrupt, name, nr)) {
         return;
     }
 
-    virq = kvm_irqchip_add_msi_route(kvm_state, vector_n, &vdev->pdev);
+    virq = kvm_irqchip_add_msi_route(kvm_state, nr, &vdev->pdev);
     if (virq < 0) {
         event_notifier_cleanup(&vector->kvm_interrupt);
         return;
@@ -454,6 +468,20 @@  static void vfio_update_kvm_msi_virq(VFIOMSIVector *vector, MSIMessage msg,
     kvm_irqchip_commit_routes(kvm_state);
 }
 
+static void vfio_vector_init(VFIOPCIDevice *vdev, const char *name, int nr)
+{
+    VFIOMSIVector *vector = &vdev->msi_vectors[nr];
+    PCIDevice *pdev = &vdev->pdev;
+
+    vector->vdev = vdev;
+    vector->virq = -1;
+    if (vfio_named_notifier_init(vdev, &vector->interrupt, name, nr)) {
+        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,13 +494,7 @@  static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
     vector = &vdev->msi_vectors[nr];
 
     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, NULL, nr);
     }
 
     qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
@@ -490,7 +512,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, NULL, nr, true);
         }
     }
 
@@ -640,7 +662,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, NULL, i, false);
     }
 
     /* Set interrupt type prior to possible interrupts */
@@ -2677,7 +2699,7 @@  static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    if (event_notifier_init(&vdev->err_notifier, 0)) {
+    if (vfio_named_notifier_init(vdev, &vdev->err_notifier, "err", 0)) {
         error_report("vfio: Unable to init event notifier for error detection");
         vdev->pci_aer = false;
         return;
@@ -2743,7 +2765,7 @@  static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
         return;
     }
 
-    if (event_notifier_init(&vdev->req_notifier, 0)) {
+    if (vfio_named_notifier_init(vdev, &vdev->req_notifier, "req", 0)) {
         error_report("vfio: Unable to init event notifier for device request");
         return;
     }
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 8af11b0..cb04cc6 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -216,6 +216,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);
@@ -234,6 +236,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,