[PATCH v3 0/9] optimize the downtime for vfio migration

[PATCH v3 0/9] optimize the downtime for vfio migration

[Longpeng(Mike)]

Hi guys,

In vfio migration resume phase, the cost would increase if the
vfio device has more unmasked vectors. We try to optimize it in
this series.

You can see the commit message in PATCH 9 for details.

Patch 1-5 are simple cleanups and fixup.
Patch 6-8 are the preparations for the optimization.
Patch 9 optimizes the vfio msix setup path.

Changes v2->v3:
 - fix two errors [Longpeng]

Changes v1->v2:
 - fix several typos and grammatical errors [Alex, Philippe]
 - split fixups and cleanups into separate patches  [Alex, Philippe]
 - introduce kvm_irqchip_add_deferred_msi_route to
   minimize code changes    [Alex]
 - enable the optimization in msi setup path    [Alex]

Longpeng (Mike) (9):
  vfio: simplify the conditional statements in vfio_msi_enable
  vfio: move re-enabling INTX out of the common helper
  vfio: simplify the failure path in vfio_msi_enable
  msix: simplify the conditional in msix_set/unset_vector_notifiers
  msix: reset poll_notifier to NULL if fail to set notifiers
  kvm: irqchip: extract kvm_irqchip_add_deferred_msi_route
  vfio: add infrastructure to commit the deferred kvm routing
  Revert "vfio: Avoid disabling and enabling vectors repeatedly in VFIO
    migration"
  vfio: defer to commit kvm irq routing when enable msi/msix

 accel/kvm/kvm-all.c  |  15 +++++-
 hw/pci/msix.c        |   7 ++-
 hw/vfio/pci.c        | 126 ++++++++++++++++++++++++++++---------------
 hw/vfio/pci.h        |   1 +
 include/sysemu/kvm.h |   6 +++
 5 files changed, 106 insertions(+), 49 deletions(-)

-- 
2.23.0

[PATCH v3 1/9] vfio: simplify the conditional statements in vfio_msi_enable

[Longpeng(Mike)]

It's unnecessary to test against the specific return value of
VFIO_DEVICE_SET_IRQS, since any positive return is an error
indicating the number of vectors we should retry with.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/vfio/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e1ea1d8a23..f7a3a13fb0 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -650,7 +650,7 @@ retry:
     if (ret) {
         if (ret < 0) {
             error_report("vfio: Error: Failed to setup MSI fds: %m");
-        } else if (ret != vdev->nr_vectors) {
+        } else {
             error_report("vfio: Error: Failed to enable %d "
                          "MSI vectors, retry with %d", vdev->nr_vectors, ret);
         }
@@ -668,7 +668,7 @@ retry:
         g_free(vdev->msi_vectors);
         vdev->msi_vectors = NULL;
 
-        if (ret > 0 && ret != vdev->nr_vectors) {
+        if (ret > 0) {
             vdev->nr_vectors = ret;
             goto retry;
         }
-- 
2.23.0

[PATCH v3 2/9] vfio: move re-enabling INTX out of the common helper

[Longpeng(Mike)]

Move re-enabling INTX out, and the callers should decide to
re-enable it or not.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/vfio/pci.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index f7a3a13fb0..1e6797fd4b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -690,7 +690,6 @@ retry:
 
 static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
 {
-    Error *err = NULL;
     int i;
 
     for (i = 0; i < vdev->nr_vectors; i++) {
@@ -709,15 +708,11 @@ static void vfio_msi_disable_common(VFIOPCIDevice *vdev)
     vdev->msi_vectors = NULL;
     vdev->nr_vectors = 0;
     vdev->interrupt = VFIO_INT_NONE;
-
-    vfio_intx_enable(vdev, &err);
-    if (err) {
-        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
-    }
 }
 
 static void vfio_msix_disable(VFIOPCIDevice *vdev)
 {
+    Error *err = NULL;
     int i;
 
     msix_unset_vector_notifiers(&vdev->pdev);
@@ -738,6 +733,10 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
     }
 
     vfio_msi_disable_common(vdev);
+    vfio_intx_enable(vdev, &err);
+    if (err) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+    }
 
     memset(vdev->msix->pending, 0,
            BITS_TO_LONGS(vdev->msix->entries) * sizeof(unsigned long));
@@ -747,8 +746,14 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
 
 static void vfio_msi_disable(VFIOPCIDevice *vdev)
 {
+    Error *err = NULL;
+
     vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSI_IRQ_INDEX);
     vfio_msi_disable_common(vdev);
+    vfio_intx_enable(vdev, &err);
+    if (err) {
+        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+    }
 
     trace_vfio_msi_disable(vdev->vbasedev.name);
 }
-- 
2.23.0

[PATCH v3 3/9] vfio: simplify the failure path in vfio_msi_enable

[Longpeng(Mike)]

Use vfio_msi_disable_common to simplify the error handling
in vfio_msi_enable.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/vfio/pci.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 1e6797fd4b..8e97ca93cf 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -47,6 +47,7 @@
 
 static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
 static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
+static void vfio_msi_disable_common(VFIOPCIDevice *vdev);
 
 /*
  * Disabling BAR mmaping can be slow, but toggling it around INTx can
@@ -655,24 +656,12 @@ retry:
                          "MSI vectors, retry with %d", vdev->nr_vectors, ret);
         }
 
-        for (i = 0; i < vdev->nr_vectors; i++) {
-            VFIOMSIVector *vector = &vdev->msi_vectors[i];
-            if (vector->virq >= 0) {
-                vfio_remove_kvm_msi_virq(vector);
-            }
-            qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
-                                NULL, NULL, NULL);
-            event_notifier_cleanup(&vector->interrupt);
-        }
-
-        g_free(vdev->msi_vectors);
-        vdev->msi_vectors = NULL;
+        vfio_msi_disable_common(vdev);
 
         if (ret > 0) {
             vdev->nr_vectors = ret;
             goto retry;
         }
-        vdev->nr_vectors = 0;
 
         /*
          * Failing to setup MSI doesn't really fall within any specification.
@@ -680,7 +669,6 @@ retry:
          * out to fall back to INTx for this device.
          */
         error_report("vfio: Error: Failed to enable MSI");
-        vdev->interrupt = VFIO_INT_NONE;
 
         return;
     }
-- 
2.23.0

[PATCH v3 4/9] msix: simplify the conditional in msix_set/unset_vector_notifiers

[Longpeng(Mike)]

'msix_function_masked' is synchronized with the device's config,
we can use it to replace the complex conditional statementis in
msix_set/unset_vector_notifiers.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/pci/msix.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index ae9331cd0b..67682289af 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -592,8 +592,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
     dev->msix_vector_release_notifier = release_notifier;
     dev->msix_vector_poll_notifier = poll_notifier;
 
-    if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
-        (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
+    if (!dev->msix_function_masked) {
         for (vector = 0; vector < dev->msix_entries_nr; vector++) {
             ret = msix_set_notifier_for_vector(dev, vector);
             if (ret < 0) {
@@ -622,8 +621,7 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
     assert(dev->msix_vector_use_notifier &&
            dev->msix_vector_release_notifier);
 
-    if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
-        (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
+    if (!dev->msix_function_masked) {
         for (vector = 0; vector < dev->msix_entries_nr; vector++) {
             msix_unset_notifier_for_vector(dev, vector);
         }
-- 
2.23.0

[PATCH v3 5/9] msix: reset poll_notifier to NULL if fail to set notifiers

[Longpeng(Mike)]

'msix_vector_poll_notifier' should be reset to NULL in the error
path in msix_set_vector_notifiers().

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/pci/msix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 67682289af..805770942b 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -611,6 +611,7 @@ undo:
     }
     dev->msix_vector_use_notifier = NULL;
     dev->msix_vector_release_notifier = NULL;
+    dev->msix_vector_poll_notifier = NULL;
     return ret;
 }
 
-- 
2.23.0

[PATCH v3 6/9] kvm: irqchip: extract kvm_irqchip_add_deferred_msi_route

[Longpeng(Mike)]

Extract a common helper that add MSI route for specific vector
but does not commit immediately.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 accel/kvm/kvm-all.c  | 15 +++++++++++++--
 include/sysemu/kvm.h |  6 ++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e5b10dd129..d603afc44c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1950,7 +1950,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
     return kvm_set_irq(s, route->kroute.gsi, 1);
 }
 
-int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
+int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, PCIDevice *dev)
 {
     struct kvm_irq_routing_entry kroute = {};
     int virq;
@@ -1993,7 +1993,18 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
 
     kvm_add_routing_entry(s, &kroute);
     kvm_arch_add_msi_route_post(&kroute, vector, dev);
-    kvm_irqchip_commit_routes(s);
+
+    return virq;
+}
+
+int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
+{
+    int virq;
+
+    virq = kvm_irqchip_add_deferred_msi_route(s, vector, dev);
+    if (virq >= 0) {
+        kvm_irqchip_commit_routes(s);
+    }
 
     return virq;
 }
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a1ab1ee12d..8de0d9a715 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -476,6 +476,12 @@ void kvm_init_cpu_signals(CPUState *cpu);
  * @return: virq (>=0) when success, errno (<0) when failed.
  */
 int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev);
+/**
+ * Add MSI route for specific vector but does not commit to KVM
+ * immediately
+ */
+int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector,
+                                       PCIDevice *dev);
 int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
                                  PCIDevice *dev);
 void kvm_irqchip_commit_routes(KVMState *s);
-- 
2.23.0

[PATCH v3 7/9] vfio: add infrastructure to commit the deferred kvm routing

[Longpeng(Mike)]

'defer_kvm_irq_routing' indicates whether we should defer to commit
the kvm routing.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/vfio/pci.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 hw/vfio/pci.h |  1 +
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8e97ca93cf..8fe238b11d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -423,12 +423,24 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
         return;
     }
 
-    virq = kvm_irqchip_add_msi_route(kvm_state, vector_n, &vdev->pdev);
+    virq = kvm_irqchip_add_deferred_msi_route(kvm_state, vector_n, &vdev->pdev);
     if (virq < 0) {
         event_notifier_cleanup(&vector->kvm_interrupt);
         return;
     }
 
+    if (vdev->defer_kvm_irq_routing) {
+        /*
+         * Hold the allocated virq in vector->virq temporarily, will
+         * reset it to -1 when we fail to add the corresponding irqfd
+         * in vfio_commit_kvm_msi_virq().
+         */
+        vector->virq = virq;
+        return;
+    }
+
+    kvm_irqchip_commit_routes(kvm_state);
+
     if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, &vector->kvm_interrupt,
                                        NULL, virq) < 0) {
         kvm_irqchip_release_virq(kvm_state, virq);
@@ -567,6 +579,35 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
     }
 }
 
+/* TODO: invoked when enclabe msi/msix vectors */
+static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev)
+{
+    int i;
+    VFIOMSIVector *vector;
+
+    if (!vdev->defer_kvm_irq_routing || !vdev->nr_vectors) {
+        return;
+    }
+
+    kvm_irqchip_commit_routes(kvm_state);
+
+    for (i = 0; i < vdev->nr_vectors; i++) {
+        vector = &vdev->msi_vectors[i];
+
+        if (!vector->use || vector->virq < 0) {
+            continue;
+        }
+
+        if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
+                                               &vector->kvm_interrupt,
+                                               NULL, vector->virq) < 0) {
+            kvm_irqchip_release_virq(kvm_state, vector->virq);
+            event_notifier_cleanup(&vector->kvm_interrupt);
+            vector->virq = -1;
+        }
+    }
+}
+
 static void vfio_msix_enable(VFIOPCIDevice *vdev)
 {
     PCIDevice *pdev = &vdev->pdev;
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 64777516d1..d3c5177d37 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -171,6 +171,7 @@ struct VFIOPCIDevice {
     bool no_kvm_ioeventfd;
     bool no_vfio_ioeventfd;
     bool enable_ramfb;
+    bool defer_kvm_irq_routing;
     VFIODisplay *dpy;
     Notifier irqchip_change_notifier;
 };
-- 
2.23.0

[PATCH v3 8/9] Revert "vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration"

[Longpeng(Mike)]

Commit ecebe53fe993 ("vfio: Avoid disabling and enabling vectors
repeatedly in VFIO migration") avoid inefficiently disabling and
enabling vectors repeatedly and let the unmasked vectors to be
enabled one by one.

But we want to batch multiple routes and defer the commit, and only
commit once out side the loop of setting vector notifiers, so we
cannot to enable the vectors one by one in the loop now.

Revert that commit and we will take another way in the next patch,
it can not only avoid disabling/enabling vectors repeatedly, but
also satisfy our requirement of defer to commit.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/vfio/pci.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 8fe238b11d..2de1cc5425 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -610,9 +610,6 @@ static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev
 
 static void vfio_msix_enable(VFIOPCIDevice *vdev)
 {
-    PCIDevice *pdev = &vdev->pdev;
-    unsigned int nr, max_vec = 0;
-
     vfio_disable_interrupts(vdev);
 
     vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
@@ -631,22 +628,11 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
      * triggering to userspace, then immediately release the vector, leaving
      * the physical device with no vectors enabled, but MSI-X enabled, just
      * like the guest view.
-     * If there are already unmasked vectors (in migration resume phase and
-     * some guest startups) which will be enabled soon, we can allocate all
-     * of them here to avoid inefficiently disabling and enabling vectors
-     * repeatedly later.
      */
-    if (!pdev->msix_function_masked) {
-        for (nr = 0; nr < msix_nr_vectors_allocated(pdev); nr++) {
-            if (!msix_is_masked(pdev, nr)) {
-                max_vec = nr;
-            }
-        }
-    }
-    vfio_msix_vector_do_use(pdev, max_vec, NULL, NULL);
-    vfio_msix_vector_release(pdev, max_vec);
+    vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
+    vfio_msix_vector_release(&vdev->pdev, 0);
 
-    if (msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
+    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
                                   vfio_msix_vector_release, NULL)) {
         error_report("vfio: msix_set_vector_notifiers failed");
     }
-- 
2.23.0

[PATCH v3 9/9] vfio: defer to commit kvm irq routing when enable msi/msix

[Longpeng(Mike)]

In migration resume phase, all unmasked msix vectors need to be
setup when load the VF state. However, the setup operation would
take longer if the VM has more VFs and each VF has more unmasked
vectors.

The hot spot is kvm_irqchip_commit_routes, it'll scan and update
all irqfds that already assigned each invocation, so more vectors
means need more time to process them.

vfio_pci_load_config
  vfio_msix_enable
    msix_set_vector_notifiers
      for (vector = 0; vector < dev->msix_entries_nr; vector++) {
        vfio_msix_vector_do_use
          vfio_add_kvm_msi_virq
            kvm_irqchip_commit_routes <-- expensive
      }

We can reduce the cost by only commit once outside the loop. The
routes is cached in kvm_state, we commit them first and then bind
irqfd for each vector.

The test VM has 128 vcpus and 8 VF (each one has 65 vectors),
we measure the cost of the vfio_msix_enable for each VF, and
we can see 90+% costs can be reduce.

VF      Count of irqfds[*]  Original        With this patch

1st           65            8               2
2nd           130           15              2
3rd           195           22              2
4th           260           24              3
5th           325           36              2
6th           390           44              3
7th           455           51              3
8th           520           58              4
Total                       258ms           21ms

[*] Count of irqfds
How many irqfds that already assigned and need to process in this
round.

The optimition can be applied to msi type too.

Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
---
 hw/vfio/pci.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2de1cc5425..b26129bddf 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -513,11 +513,13 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
      * increase them as needed.
      */
     if (vdev->nr_vectors < nr + 1) {
-        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
         vdev->nr_vectors = nr + 1;
-        ret = vfio_enable_vectors(vdev, true);
-        if (ret) {
-            error_report("vfio: failed to enable vectors, %d", ret);
+        if (!vdev->defer_kvm_irq_routing) {
+            vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
+            ret = vfio_enable_vectors(vdev, true);
+            if (ret) {
+                error_report("vfio: failed to enable vectors, %d", ret);
+            }
         }
     } else {
         Error *err = NULL;
@@ -579,8 +581,7 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
     }
 }
 
-/* TODO: invoked when enclabe msi/msix vectors */
-static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev)
+static void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev)
 {
     int i;
     VFIOMSIVector *vector;
@@ -610,6 +611,9 @@ static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev
 
 static void vfio_msix_enable(VFIOPCIDevice *vdev)
 {
+    PCIDevice *pdev = &vdev->pdev;
+    int ret;
+
     vfio_disable_interrupts(vdev);
 
     vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
@@ -632,11 +636,22 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
     vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
     vfio_msix_vector_release(&vdev->pdev, 0);
 
-    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
-                                  vfio_msix_vector_release, NULL)) {
+    vdev->defer_kvm_irq_routing = true;
+
+    ret = msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
+                                    vfio_msix_vector_release, NULL);
+    if (ret < 0) {
         error_report("vfio: msix_set_vector_notifiers failed");
+    } else if (!pdev->msix_function_masked) {
+        vfio_commit_kvm_msi_virq(vdev);
+        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
+        ret = vfio_enable_vectors(vdev, true);
+        if (ret) {
+            error_report("vfio: failed to enable vectors, %d", ret);
+        }
     }
 
+    vdev->defer_kvm_irq_routing = false;
     trace_vfio_msix_enable(vdev->vbasedev.name);
 }
 
@@ -645,6 +660,7 @@ static void vfio_msi_enable(VFIOPCIDevice *vdev)
     int ret, i;
 
     vfio_disable_interrupts(vdev);
+    vdev->defer_kvm_irq_routing = true;
 
     vdev->nr_vectors = msi_nr_vectors_allocated(&vdev->pdev);
 retry:
@@ -671,6 +687,8 @@ retry:
         vfio_add_kvm_msi_virq(vdev, vector, i, false);
     }
 
+    vfio_commit_kvm_msi_virq(vdev);
+
     /* Set interrupt type prior to possible interrupts */
     vdev->interrupt = VFIO_INT_MSI;
 
@@ -697,9 +715,11 @@ retry:
          */
         error_report("vfio: Error: Failed to enable MSI");
 
+        vdev->defer_kvm_irq_routing = false;
         return;
     }
 
+    vdev->defer_kvm_irq_routing = false;
     trace_vfio_msi_enable(vdev->vbasedev.name, vdev->nr_vectors);
 }
 
-- 
2.23.0

Re: [PATCH v3 4/9] msix: simplify the conditional in msix_set/unset_vector_notifiers

[Alex Williamson]

On Tue, 21 Sep 2021 07:01:57 +0800
"Longpeng(Mike)" <longpeng2@huawei.com> wrote:

> 'msix_function_masked' is synchronized with the device's config,
> we can use it to replace the complex conditional statementis in
> msix_set/unset_vector_notifiers.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  hw/pci/msix.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index ae9331cd0b..67682289af 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -592,8 +592,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
>      dev->msix_vector_release_notifier = release_notifier;
>      dev->msix_vector_poll_notifier = poll_notifier;
>  
> -    if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> -        (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> +    if (!dev->msix_function_masked) {
>          for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>              ret = msix_set_notifier_for_vector(dev, vector);
>              if (ret < 0) {
> @@ -622,8 +621,7 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
>      assert(dev->msix_vector_use_notifier &&
>             dev->msix_vector_release_notifier);
>  
> -    if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> -        (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> +    if (!dev->msix_function_masked) {
>          for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>              msix_unset_notifier_for_vector(dev, vector);
>          }

This appears to be a cleanup that's not required for the functionality
of this series.  I'd suggest proposing it separately.  Same for the
patch 5/9 in this series.  If it makes a functional difference it
should be described in the commit log.  Thanks,

Alex

Re: [PATCH v3 8/9] Revert "vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration"

[Alex Williamson]

On Tue, 21 Sep 2021 07:02:01 +0800
"Longpeng(Mike)" <longpeng2@huawei.com> wrote:

> Commit ecebe53fe993 ("vfio: Avoid disabling and enabling vectors
> repeatedly in VFIO migration") avoid inefficiently disabling and

s/avoid/avoids/

> enabling vectors repeatedly and let the unmasked vectors to be

s/let/lets/  s/to//

> enabled one by one.
> 
> But we want to batch multiple routes and defer the commit, and only
> commit once out side the loop of setting vector notifiers, so we

s/out side/outside/

> cannot to enable the vectors one by one in the loop now.

s/to//

Thanks,
Alex

> 
> Revert that commit and we will take another way in the next patch,
> it can not only avoid disabling/enabling vectors repeatedly, but
> also satisfy our requirement of defer to commit.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  hw/vfio/pci.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8fe238b11d..2de1cc5425 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -610,9 +610,6 @@ static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev
>  
>  static void vfio_msix_enable(VFIOPCIDevice *vdev)
>  {
> -    PCIDevice *pdev = &vdev->pdev;
> -    unsigned int nr, max_vec = 0;
> -
>      vfio_disable_interrupts(vdev);
>  
>      vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
> @@ -631,22 +628,11 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
>       * triggering to userspace, then immediately release the vector, leaving
>       * the physical device with no vectors enabled, but MSI-X enabled, just
>       * like the guest view.
> -     * If there are already unmasked vectors (in migration resume phase and
> -     * some guest startups) which will be enabled soon, we can allocate all
> -     * of them here to avoid inefficiently disabling and enabling vectors
> -     * repeatedly later.
>       */
> -    if (!pdev->msix_function_masked) {
> -        for (nr = 0; nr < msix_nr_vectors_allocated(pdev); nr++) {
> -            if (!msix_is_masked(pdev, nr)) {
> -                max_vec = nr;
> -            }
> -        }
> -    }
> -    vfio_msix_vector_do_use(pdev, max_vec, NULL, NULL);
> -    vfio_msix_vector_release(pdev, max_vec);
> +    vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
> +    vfio_msix_vector_release(&vdev->pdev, 0);
>  
> -    if (msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
> +    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
>                                    vfio_msix_vector_release, NULL)) {
>          error_report("vfio: msix_set_vector_notifiers failed");
>      }

Re: [PATCH v3 7/9] vfio: add infrastructure to commit the deferred kvm routing

[Alex Williamson]

On Tue, 21 Sep 2021 07:02:00 +0800
"Longpeng(Mike)" <longpeng2@huawei.com> wrote:

> 'defer_kvm_irq_routing' indicates whether we should defer to commit
> the kvm routing.
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  hw/vfio/pci.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  hw/vfio/pci.h |  1 +
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8e97ca93cf..8fe238b11d 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -423,12 +423,24 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
>          return;
>      }
>  
> -    virq = kvm_irqchip_add_msi_route(kvm_state, vector_n, &vdev->pdev);
> +    virq = kvm_irqchip_add_deferred_msi_route(kvm_state, vector_n, &vdev->pdev);
>      if (virq < 0) {
>          event_notifier_cleanup(&vector->kvm_interrupt);
>          return;
>      }
>  
> +    if (vdev->defer_kvm_irq_routing) {
> +        /*
> +         * Hold the allocated virq in vector->virq temporarily, will
> +         * reset it to -1 when we fail to add the corresponding irqfd
> +         * in vfio_commit_kvm_msi_virq().

s/when/if/

> +         */
> +        vector->virq = virq;

Do we need to make this unique to the deferred case or could we use
vector->virq directly and fill it with -1 on all error paths like we do
on a failure in vfio_commit_kvm_msi_virq()?


> +        return;
> +    }
> +
> +    kvm_irqchip_commit_routes(kvm_state);
> +
>      if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, &vector->kvm_interrupt,
>                                         NULL, virq) < 0) {
>          kvm_irqchip_release_virq(kvm_state, virq);
> @@ -567,6 +579,35 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>      }
>  }
>  
> +/* TODO: invoked when enclabe msi/msix vectors */

"enclabe"?  Is this meant to be "enable"?

> +static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev)

I'd move this function, if not this entire change, to patch 9 rather
than adding these attributes for an unused function.  Thanks,

Alex

> +{
> +    int i;
> +    VFIOMSIVector *vector;
> +
> +    if (!vdev->defer_kvm_irq_routing || !vdev->nr_vectors) {
> +        return;
> +    }
> +
> +    kvm_irqchip_commit_routes(kvm_state);
> +
> +    for (i = 0; i < vdev->nr_vectors; i++) {
> +        vector = &vdev->msi_vectors[i];
> +
> +        if (!vector->use || vector->virq < 0) {
> +            continue;
> +        }
> +
> +        if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> +                                               &vector->kvm_interrupt,
> +                                               NULL, vector->virq) < 0) {
> +            kvm_irqchip_release_virq(kvm_state, vector->virq);
> +            event_notifier_cleanup(&vector->kvm_interrupt);
> +            vector->virq = -1;
> +        }
> +    }
> +}
> +
>  static void vfio_msix_enable(VFIOPCIDevice *vdev)
>  {
>      PCIDevice *pdev = &vdev->pdev;
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 64777516d1..d3c5177d37 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -171,6 +171,7 @@ struct VFIOPCIDevice {
>      bool no_kvm_ioeventfd;
>      bool no_vfio_ioeventfd;
>      bool enable_ramfb;
> +    bool defer_kvm_irq_routing;
>      VFIODisplay *dpy;
>      Notifier irqchip_change_notifier;
>  };

Re: [PATCH v3 9/9] vfio: defer to commit kvm irq routing when enable msi/msix

[Alex Williamson]

On Tue, 21 Sep 2021 07:02:02 +0800
"Longpeng(Mike)" <longpeng2@huawei.com> wrote:

> In migration resume phase, all unmasked msix vectors need to be
> setup when load the VF state. However, the setup operation would

s/load/loading/

> take longer if the VM has more VFs and each VF has more unmasked
> vectors.
> 
> The hot spot is kvm_irqchip_commit_routes, it'll scan and update
> all irqfds that already assigned each invocation, so more vectors

s/that/that are/

> means need more time to process them.
> 
> vfio_pci_load_config
>   vfio_msix_enable
>     msix_set_vector_notifiers
>       for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>         vfio_msix_vector_do_use
>           vfio_add_kvm_msi_virq
>             kvm_irqchip_commit_routes <-- expensive
>       }
> 
> We can reduce the cost by only commit once outside the loop. The

s/commit/committing/

> routes is cached in kvm_state, we commit them first and then bind

s/is/are/

> irqfd for each vector.
> 
> The test VM has 128 vcpus and 8 VF (each one has 65 vectors),
> we measure the cost of the vfio_msix_enable for each VF, and
> we can see 90+% costs can be reduce.
> 
> VF      Count of irqfds[*]  Original        With this patch
> 
> 1st           65            8               2
> 2nd           130           15              2
> 3rd           195           22              2
> 4th           260           24              3
> 5th           325           36              2
> 6th           390           44              3
> 7th           455           51              3
> 8th           520           58              4
> Total                       258ms           21ms
> 
> [*] Count of irqfds
> How many irqfds that already assigned and need to process in this
> round.
> 
> The optimition can be applied to msi type too.

s/optimition/optimization/

> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  hw/vfio/pci.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2de1cc5425..b26129bddf 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -513,11 +513,13 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>       * increase them as needed.
>       */
>      if (vdev->nr_vectors < nr + 1) {
> -        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>          vdev->nr_vectors = nr + 1;
> -        ret = vfio_enable_vectors(vdev, true);
> -        if (ret) {
> -            error_report("vfio: failed to enable vectors, %d", ret);
> +        if (!vdev->defer_kvm_irq_routing) {
> +            vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
> +            ret = vfio_enable_vectors(vdev, true);
> +            if (ret) {
> +                error_report("vfio: failed to enable vectors, %d", ret);
> +            }
>          }
>      } else {
>          Error *err = NULL;
> @@ -579,8 +581,7 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>      }
>  }
>  
> -/* TODO: invoked when enclabe msi/msix vectors */
> -static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev)
> +static void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev)
>  {
>      int i;
>      VFIOMSIVector *vector;
> @@ -610,6 +611,9 @@ static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev
>  
>  static void vfio_msix_enable(VFIOPCIDevice *vdev)
>  {
> +    PCIDevice *pdev = &vdev->pdev;
> +    int ret;
> +
>      vfio_disable_interrupts(vdev);
>  
>      vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
> @@ -632,11 +636,22 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
>      vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
>      vfio_msix_vector_release(&vdev->pdev, 0);
>  
> -    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> -                                  vfio_msix_vector_release, NULL)) {

A comment would be useful here, maybe something like:

    /*
     * Setting vector notifiers triggers synchronous vector-use
     * callbacks for each active vector.  Deferring to commit the KVM
     * routes once rather than per vector provides a substantial
     * performance improvement.
     */

> +    vdev->defer_kvm_irq_routing = true;
> +
> +    ret = msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> +                                    vfio_msix_vector_release, NULL);
> +    if (ret < 0) {
>          error_report("vfio: msix_set_vector_notifiers failed");
> +    } else if (!pdev->msix_function_masked) {
> +        vfio_commit_kvm_msi_virq(vdev);
> +        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);

Couldn't we also optimize the do_use/release on vector 0 above to avoid
this gratuitous disable here?  We only want to make sure MSIX is always
enabled on the device when we exit this function, so maybe that code
becomes an "else" branch below?

> +        ret = vfio_enable_vectors(vdev, true);
> +        if (ret) {
> +            error_report("vfio: failed to enable vectors, %d", ret);
> +        }
>      }
>  
> +    vdev->defer_kvm_irq_routing = false;
>      trace_vfio_msix_enable(vdev->vbasedev.name);
>  }
>  
> @@ -645,6 +660,7 @@ static void vfio_msi_enable(VFIOPCIDevice *vdev)
>      int ret, i;
>  
>      vfio_disable_interrupts(vdev);
> +    vdev->defer_kvm_irq_routing = true;
>  
>      vdev->nr_vectors = msi_nr_vectors_allocated(&vdev->pdev);
>  retry:
> @@ -671,6 +687,8 @@ retry:
>          vfio_add_kvm_msi_virq(vdev, vector, i, false);
>      }
>  
> +    vfio_commit_kvm_msi_virq(vdev);
> +
>      /* Set interrupt type prior to possible interrupts */
>      vdev->interrupt = VFIO_INT_MSI;
>  
> @@ -697,9 +715,11 @@ retry:
>           */
>          error_report("vfio: Error: Failed to enable MSI");
>  
> +        vdev->defer_kvm_irq_routing = false;
>          return;
>      }
>  
> +    vdev->defer_kvm_irq_routing = false;

Why wouldn't we clear the flag in vfio_commit_kvm_msi_virq()?  It
almost feels like there should be a vfio_prepare_kvm_msi_virq_batch()
that enables the flag and an unconditional
vfio_commit_kvm_msi_virq_batch() that clears the flag and decides if
further work is necessary.  Thanks,

Alex

>      trace_vfio_msi_enable(vdev->vbasedev.name, vdev->nr_vectors);
>  }
>  

RE: [PATCH v3 9/9] vfio: defer to commit kvm irq routing when enable msi/msix

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Saturday, October 2, 2021 7:05 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: philmd@redhat.com; pbonzini@redhat.com; marcel.apfelbaum@gmail.com;
> mst@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> <arei.gonglei@huawei.com>; chenjiashang <chenjiashang@huawei.com>
> Subject: Re: [PATCH v3 9/9] vfio: defer to commit kvm irq routing when enable
> msi/msix
> 
> On Tue, 21 Sep 2021 07:02:02 +0800
> "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
> 
> > In migration resume phase, all unmasked msix vectors need to be
> > setup when load the VF state. However, the setup operation would
> 
> s/load/loading/
> 
> > take longer if the VM has more VFs and each VF has more unmasked
> > vectors.
> >
> > The hot spot is kvm_irqchip_commit_routes, it'll scan and update
> > all irqfds that already assigned each invocation, so more vectors
> 
> s/that/that are/
> 
> > means need more time to process them.
> >
> > vfio_pci_load_config
> >   vfio_msix_enable
> >     msix_set_vector_notifiers
> >       for (vector = 0; vector < dev->msix_entries_nr; vector++) {
> >         vfio_msix_vector_do_use
> >           vfio_add_kvm_msi_virq
> >             kvm_irqchip_commit_routes <-- expensive
> >       }
> >
> > We can reduce the cost by only commit once outside the loop. The
> 
> s/commit/committing/
> 

OK, will fix in the next version, thanks.

> > routes is cached in kvm_state, we commit them first and then bind
> 
> s/is/are/
> 

OK.

> > irqfd for each vector.
> >
> > The test VM has 128 vcpus and 8 VF (each one has 65 vectors),
> > we measure the cost of the vfio_msix_enable for each VF, and
> > we can see 90+% costs can be reduce.
> >
> > VF      Count of irqfds[*]  Original        With this patch
> >
> > 1st           65            8               2
> > 2nd           130           15              2
> > 3rd           195           22              2
> > 4th           260           24              3
> > 5th           325           36              2
> > 6th           390           44              3
> > 7th           455           51              3
> > 8th           520           58              4
> > Total                       258ms           21ms
> >
> > [*] Count of irqfds
> > How many irqfds that already assigned and need to process in this
> > round.
> >
> > The optimition can be applied to msi type too.
> 
> s/optimition/optimization/
> 

OK, thanks.

> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> >  hw/vfio/pci.c | 36 ++++++++++++++++++++++++++++--------
> >  1 file changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 2de1cc5425..b26129bddf 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -513,11 +513,13 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
> unsigned int nr,
> >       * increase them as needed.
> >       */
> >      if (vdev->nr_vectors < nr + 1) {
> > -        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
> >          vdev->nr_vectors = nr + 1;
> > -        ret = vfio_enable_vectors(vdev, true);
> > -        if (ret) {
> > -            error_report("vfio: failed to enable vectors, %d", ret);
> > +        if (!vdev->defer_kvm_irq_routing) {
> > +            vfio_disable_irqindex(&vdev->vbasedev,
> VFIO_PCI_MSIX_IRQ_INDEX);
> > +            ret = vfio_enable_vectors(vdev, true);
> > +            if (ret) {
> > +                error_report("vfio: failed to enable vectors, %d", ret);
> > +            }
> >          }
> >      } else {
> >          Error *err = NULL;
> > @@ -579,8 +581,7 @@ static void vfio_msix_vector_release(PCIDevice *pdev,
> unsigned int nr)
> >      }
> >  }
> >
> > -/* TODO: invoked when enclabe msi/msix vectors */
> > -static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice
> *vdev)
> > +static void vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev)
> >  {
> >      int i;
> >      VFIOMSIVector *vector;
> > @@ -610,6 +611,9 @@ static __attribute__((unused)) void
> vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev
> >
> >  static void vfio_msix_enable(VFIOPCIDevice *vdev)
> >  {
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    int ret;
> > +
> >      vfio_disable_interrupts(vdev);
> >
> >      vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
> > @@ -632,11 +636,22 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
> >      vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
> >      vfio_msix_vector_release(&vdev->pdev, 0);
> >
> > -    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> > -                                  vfio_msix_vector_release, NULL)) {
> 
> A comment would be useful here, maybe something like:
> 
>     /*
>      * Setting vector notifiers triggers synchronous vector-use
>      * callbacks for each active vector.  Deferring to commit the KVM
>      * routes once rather than per vector provides a substantial
>      * performance improvement.
>      */
> 

Will add in the next version.

> > +    vdev->defer_kvm_irq_routing = true;
> > +
> > +    ret = msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> > +                                    vfio_msix_vector_release, NULL);
> > +    if (ret < 0) {
> >          error_report("vfio: msix_set_vector_notifiers failed");
> > +    } else if (!pdev->msix_function_masked) {
> > +        vfio_commit_kvm_msi_virq(vdev);
> > +        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
> 
> Couldn't we also optimize the do_use/release on vector 0 above to avoid
> this gratuitous disable here? We only want to make sure MSIX is always

The disable here seems can be removed directly, because we already disable
interrupts at the beginning of the vfio_msix_enable() ?

> enabled on the device when we exit this function, so maybe that code
> becomes an "else" branch below?
> 

Do you mean something like:

if (ret < 0) {
    ....
} else if (!pdev->msix_function_masked) {
    ....
    ret = vfio_enable_vectors(vdev, true);
    ....
} else {
    /* do_use/release on vector 0 */
}

We'll get '-EINVAL' if invoke vfio_enable_vectors with vdev->nr_vectors=0,
this cannot happen before but it can now in this way. So maybe the "else if"
conditional expression should be convert to
"!pdev->msix_function_masked && vdev->nr_vectors" ?


> > +        ret = vfio_enable_vectors(vdev, true);
> > +        if (ret) {
> > +            error_report("vfio: failed to enable vectors, %d", ret);
> > +        }
> >      }
> >
> > +    vdev->defer_kvm_irq_routing = false;
> >      trace_vfio_msix_enable(vdev->vbasedev.name);
> >  }
> >
> > @@ -645,6 +660,7 @@ static void vfio_msi_enable(VFIOPCIDevice *vdev)
> >      int ret, i;
> >
> >      vfio_disable_interrupts(vdev);
> > +    vdev->defer_kvm_irq_routing = true;
> >
> >      vdev->nr_vectors = msi_nr_vectors_allocated(&vdev->pdev);
> >  retry:
> > @@ -671,6 +687,8 @@ retry:
> >          vfio_add_kvm_msi_virq(vdev, vector, i, false);
> >      }
> >
> > +    vfio_commit_kvm_msi_virq(vdev);
> > +
> >      /* Set interrupt type prior to possible interrupts */
> >      vdev->interrupt = VFIO_INT_MSI;
> >
> > @@ -697,9 +715,11 @@ retry:
> >           */
> >          error_report("vfio: Error: Failed to enable MSI");
> >
> > +        vdev->defer_kvm_irq_routing = false;
> >          return;
> >      }
> >
> > +    vdev->defer_kvm_irq_routing = false;
> 
> Why wouldn't we clear the flag in vfio_commit_kvm_msi_virq()?  It
> almost feels like there should be a vfio_prepare_kvm_msi_virq_batch()
> that enables the flag and an unconditional
> vfio_commit_kvm_msi_virq_batch() that clears the flag and decides if
> further work is necessary.  Thanks,
> 
> Alex
> 
> >      trace_vfio_msi_enable(vdev->vbasedev.name, vdev->nr_vectors);
> >  }
> >

RE: [PATCH v3 4/9] msix: simplify the conditional in msix_set/unset_vector_notifiers

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Saturday, October 2, 2021 7:04 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: philmd@redhat.com; pbonzini@redhat.com; marcel.apfelbaum@gmail.com;
> mst@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> <arei.gonglei@huawei.com>; chenjiashang <chenjiashang@huawei.com>
> Subject: Re: [PATCH v3 4/9] msix: simplify the conditional in
> msix_set/unset_vector_notifiers
> 
> On Tue, 21 Sep 2021 07:01:57 +0800
> "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
> 
> > 'msix_function_masked' is synchronized with the device's config,
> > we can use it to replace the complex conditional statementis in
> > msix_set/unset_vector_notifiers.
> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> >  hw/pci/msix.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > index ae9331cd0b..67682289af 100644
> > --- a/hw/pci/msix.c
> > +++ b/hw/pci/msix.c
> > @@ -592,8 +592,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
> >      dev->msix_vector_release_notifier = release_notifier;
> >      dev->msix_vector_poll_notifier = poll_notifier;
> >
> > -    if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> > -        (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> > +    if (!dev->msix_function_masked) {
> >          for (vector = 0; vector < dev->msix_entries_nr; vector++) {
> >              ret = msix_set_notifier_for_vector(dev, vector);
> >              if (ret < 0) {
> > @@ -622,8 +621,7 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
> >      assert(dev->msix_vector_use_notifier &&
> >             dev->msix_vector_release_notifier);
> >
> > -    if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> > -        (MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> > +    if (!dev->msix_function_masked) {
> >          for (vector = 0; vector < dev->msix_entries_nr; vector++) {
> >              msix_unset_notifier_for_vector(dev, vector);
> >          }
> 
> This appears to be a cleanup that's not required for the functionality
> of this series.  I'd suggest proposing it separately.  Same for the
> patch 5/9 in this series.  If it makes a functional difference it
> should be described in the commit log.  Thanks,
> 

OK, will remove these two patches in the v4, thanks.

> Alex

RE: [PATCH v3 7/9] vfio: add infrastructure to commit the deferred kvm routing

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Saturday, October 2, 2021 7:05 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: philmd@redhat.com; pbonzini@redhat.com; marcel.apfelbaum@gmail.com;
> mst@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> <arei.gonglei@huawei.com>; chenjiashang <chenjiashang@huawei.com>
> Subject: Re: [PATCH v3 7/9] vfio: add infrastructure to commit the deferred kvm
> routing
> 
> On Tue, 21 Sep 2021 07:02:00 +0800
> "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
> 
> > 'defer_kvm_irq_routing' indicates whether we should defer to commit
> > the kvm routing.
> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> >  hw/vfio/pci.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> >  hw/vfio/pci.h |  1 +
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 8e97ca93cf..8fe238b11d 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -423,12 +423,24 @@ static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev,
> VFIOMSIVector *vector,
> >          return;
> >      }
> >
> > -    virq = kvm_irqchip_add_msi_route(kvm_state, vector_n, &vdev->pdev);
> > +    virq = kvm_irqchip_add_deferred_msi_route(kvm_state, vector_n,
> &vdev->pdev);
> >      if (virq < 0) {
> >          event_notifier_cleanup(&vector->kvm_interrupt);
> >          return;
> >      }
> >
> > +    if (vdev->defer_kvm_irq_routing) {
> > +        /*
> > +         * Hold the allocated virq in vector->virq temporarily, will
> > +         * reset it to -1 when we fail to add the corresponding irqfd
> > +         * in vfio_commit_kvm_msi_virq().
> 
> s/when/if/
> 

OK, thanks.

> > +         */
> > +        vector->virq = virq;
> 
> Do we need to make this unique to the deferred case or could we use
> vector->virq directly and fill it with -1 on all error paths like we do
> on a failure in vfio_commit_kvm_msi_virq()?
> 

OK, I will use vector->irq directly, it looks neater.

> 
> > +        return;
> > +    }
> > +
> > +    kvm_irqchip_commit_routes(kvm_state);
> > +
> >      if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> &vector->kvm_interrupt,
> >                                         NULL, virq) < 0) {
> >          kvm_irqchip_release_virq(kvm_state, virq);
> > @@ -567,6 +579,35 @@ static void vfio_msix_vector_release(PCIDevice *pdev,
> unsigned int nr)
> >      }
> >  }
> >
> > +/* TODO: invoked when enclabe msi/msix vectors */
> 
> "enclabe"?  Is this meant to be "enable"?
> 

Yes, it's a typo.

> > +static __attribute__((unused)) void vfio_commit_kvm_msi_virq(VFIOPCIDevice
> *vdev)
> 
> I'd move this function, if not this entire change, to patch 9 rather
> than adding these attributes for an unused function.  Thanks,
> 

OK. I think I should merge this patch into patch 9 entirely if we decide to
move this function.

> Alex
> 
> > +{
> > +    int i;
> > +    VFIOMSIVector *vector;
> > +
> > +    if (!vdev->defer_kvm_irq_routing || !vdev->nr_vectors) {
> > +        return;
> > +    }
> > +
> > +    kvm_irqchip_commit_routes(kvm_state);
> > +
> > +    for (i = 0; i < vdev->nr_vectors; i++) {
> > +        vector = &vdev->msi_vectors[i];
> > +
> > +        if (!vector->use || vector->virq < 0) {
> > +            continue;
> > +        }
> > +
> > +        if (kvm_irqchip_add_irqfd_notifier_gsi(kvm_state,
> > +                                               &vector->kvm_interrupt,
> > +                                               NULL, vector->virq) < 0) {
> > +            kvm_irqchip_release_virq(kvm_state, vector->virq);
> > +            event_notifier_cleanup(&vector->kvm_interrupt);
> > +            vector->virq = -1;
> > +        }
> > +    }
> > +}
> > +
> >  static void vfio_msix_enable(VFIOPCIDevice *vdev)
> >  {
> >      PCIDevice *pdev = &vdev->pdev;
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 64777516d1..d3c5177d37 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -171,6 +171,7 @@ struct VFIOPCIDevice {
> >      bool no_kvm_ioeventfd;
> >      bool no_vfio_ioeventfd;
> >      bool enable_ramfb;
> > +    bool defer_kvm_irq_routing;
> >      VFIODisplay *dpy;
> >      Notifier irqchip_change_notifier;
> >  };

RE: [PATCH v3 8/9] Revert "vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration"

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Saturday, October 2, 2021 7:04 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: philmd@redhat.com; pbonzini@redhat.com; marcel.apfelbaum@gmail.com;
> mst@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> <arei.gonglei@huawei.com>; chenjiashang <chenjiashang@huawei.com>
> Subject: Re: [PATCH v3 8/9] Revert "vfio: Avoid disabling and enabling vectors
> repeatedly in VFIO migration"
> 
> On Tue, 21 Sep 2021 07:02:01 +0800
> "Longpeng(Mike)" <longpeng2@huawei.com> wrote:
> 
> > Commit ecebe53fe993 ("vfio: Avoid disabling and enabling vectors
> > repeatedly in VFIO migration") avoid inefficiently disabling and
> 
> s/avoid/avoids/
> 
> > enabling vectors repeatedly and let the unmasked vectors to be
> 
> s/let/lets/  s/to//
> 
> > enabled one by one.
> >
> > But we want to batch multiple routes and defer the commit, and only
> > commit once out side the loop of setting vector notifiers, so we
> 
> s/out side/outside/
> 
> > cannot to enable the vectors one by one in the loop now.
> 
> s/to//
> 

Thanks. All the typos and grammatical errors you pointed out will
be fixed in v4.

> Thanks,
> Alex
> 
> >
> > Revert that commit and we will take another way in the next patch,
> > it can not only avoid disabling/enabling vectors repeatedly, but
> > also satisfy our requirement of defer to commit.
> >
> > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> > ---
> >  hw/vfio/pci.c | 20 +++-----------------
> >  1 file changed, 3 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 8fe238b11d..2de1cc5425 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -610,9 +610,6 @@ static __attribute__((unused)) void
> vfio_commit_kvm_msi_virq(VFIOPCIDevice *vdev
> >
> >  static void vfio_msix_enable(VFIOPCIDevice *vdev)
> >  {
> > -    PCIDevice *pdev = &vdev->pdev;
> > -    unsigned int nr, max_vec = 0;
> > -
> >      vfio_disable_interrupts(vdev);
> >
> >      vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
> > @@ -631,22 +628,11 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
> >       * triggering to userspace, then immediately release the vector, leaving
> >       * the physical device with no vectors enabled, but MSI-X enabled, just
> >       * like the guest view.
> > -     * If there are already unmasked vectors (in migration resume phase and
> > -     * some guest startups) which will be enabled soon, we can allocate all
> > -     * of them here to avoid inefficiently disabling and enabling vectors
> > -     * repeatedly later.
> >       */
> > -    if (!pdev->msix_function_masked) {
> > -        for (nr = 0; nr < msix_nr_vectors_allocated(pdev); nr++) {
> > -            if (!msix_is_masked(pdev, nr)) {
> > -                max_vec = nr;
> > -            }
> > -        }
> > -    }
> > -    vfio_msix_vector_do_use(pdev, max_vec, NULL, NULL);
> > -    vfio_msix_vector_release(pdev, max_vec);
> > +    vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
> > +    vfio_msix_vector_release(&vdev->pdev, 0);
> >
> > -    if (msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
> > +    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> >                                    vfio_msix_vector_release, NULL)) {
> >          error_report("vfio: msix_set_vector_notifiers failed");
> >      }