All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] vfio: Some fixes and optimizations for VFIO migration
@ 2021-02-23  2:22 Shenming Lu
  2021-02-23  2:22 ` [PATCH v3 1/3] vfio: Move the saving of the config space to the right place in " Shenming Lu
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Shenming Lu @ 2021-02-23  2:22 UTC (permalink / raw)
  To: Alex Williamson, Kirti Wankhede, Cornelia Huck, qemu-devel, qemu-arm
  Cc: Lorenzo Pieralisi, Neo Jia, mst, Marc Zyngier,
	Dr . David Alan Gilbert, Eric Auger, yuzenghui, wanghaibin.wang,
	lushenming

This patch set includes two fixes and one optimization for VFIO migration
as blew:

Patch 1-2:
- Fix two ordering problems in migration.

Patch 3:
- Optimize the enabling process of the MSI-X vectors in migration.

History:

v2 -> v3:
- Nit fixes.
- Set error in migration stream for migration to fail in Patch 1.
- Tested Patch 3 with a Windows guest.

Thanks,
Shenming


Shenming Lu (3):
  vfio: Move the saving of the config space to the right place in VFIO
    migration
  vfio: Set the priority of the VFIO VM state change handler explicitly
  vfio: Avoid disabling and enabling vectors repeatedly in VFIO
    migration

 hw/pci/msix.c         |  2 +-
 hw/vfio/migration.c   | 28 +++++++++++++++++-----------
 hw/vfio/pci.c         | 20 +++++++++++++++++---
 include/hw/pci/msix.h |  1 +
 4 files changed, 36 insertions(+), 15 deletions(-)

-- 
2.19.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/3] vfio: Move the saving of the config space to the right place in VFIO migration
  2021-02-23  2:22 [PATCH v3 0/3] vfio: Some fixes and optimizations for VFIO migration Shenming Lu
@ 2021-02-23  2:22 ` Shenming Lu
  2021-03-01  5:57   ` Kirti Wankhede
  2021-02-23  2:22 ` [PATCH v3 2/3] vfio: Set the priority of the VFIO VM state change handler explicitly Shenming Lu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Shenming Lu @ 2021-02-23  2:22 UTC (permalink / raw)
  To: Alex Williamson, Kirti Wankhede, Cornelia Huck, qemu-devel, qemu-arm
  Cc: Lorenzo Pieralisi, Neo Jia, mst, Marc Zyngier,
	Dr . David Alan Gilbert, Eric Auger, yuzenghui, wanghaibin.wang,
	lushenming

On ARM64 the VFIO SET_IRQS ioctl is dependent on the VM interrupt
setup, if the restoring of the VFIO PCI device config space is
before the VGIC, an error might occur in the kernel.

So we move the saving of the config space to the non-iterable
process, thus it will be called after the VGIC according to
their priorities.

As for the possible dependence of the device specific migration
data on it's config space, we can let the vendor driver to
include any config info it needs in its own data stream.

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 hw/vfio/migration.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 00daa50ed8..f5bf67f642 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -575,11 +575,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
         return ret;
     }
 
-    ret = vfio_save_device_config_state(f, opaque);
-    if (ret) {
-        return ret;
-    }
-
     ret = vfio_update_pending(vbasedev);
     if (ret) {
         return ret;
@@ -620,6 +615,19 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
     return ret;
 }
 
+static void vfio_save_state(QEMUFile *f, void *opaque)
+{
+    VFIODevice *vbasedev = opaque;
+    int ret;
+
+    ret = vfio_save_device_config_state(f, opaque);
+    if (ret) {
+        error_report("%s: Failed to save device config space",
+                     vbasedev->name);
+        qemu_file_set_error(f, ret);
+    }
+}
+
 static int vfio_load_setup(QEMUFile *f, void *opaque)
 {
     VFIODevice *vbasedev = opaque;
@@ -670,11 +678,7 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id)
         switch (data) {
         case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
         {
-            ret = vfio_load_device_config_state(f, opaque);
-            if (ret) {
-                return ret;
-            }
-            break;
+            return vfio_load_device_config_state(f, opaque);
         }
         case VFIO_MIG_FLAG_DEV_SETUP_STATE:
         {
@@ -720,6 +724,7 @@ static SaveVMHandlers savevm_vfio_handlers = {
     .save_live_pending = vfio_save_pending,
     .save_live_iterate = vfio_save_iterate,
     .save_live_complete_precopy = vfio_save_complete_precopy,
+    .save_state = vfio_save_state,
     .load_setup = vfio_load_setup,
     .load_cleanup = vfio_load_cleanup,
     .load_state = vfio_load_state,
-- 
2.19.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/3] vfio: Set the priority of the VFIO VM state change handler explicitly
  2021-02-23  2:22 [PATCH v3 0/3] vfio: Some fixes and optimizations for VFIO migration Shenming Lu
  2021-02-23  2:22 ` [PATCH v3 1/3] vfio: Move the saving of the config space to the right place in " Shenming Lu
@ 2021-02-23  2:22 ` Shenming Lu
  2021-02-23  2:22 ` [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration Shenming Lu
  2021-03-02  5:58 ` [PATCH v3 0/3] vfio: Some fixes and optimizations for " Shenming Lu
  3 siblings, 0 replies; 9+ messages in thread
From: Shenming Lu @ 2021-02-23  2:22 UTC (permalink / raw)
  To: Alex Williamson, Kirti Wankhede, Cornelia Huck, qemu-devel, qemu-arm
  Cc: Lorenzo Pieralisi, Neo Jia, mst, Marc Zyngier,
	Dr . David Alan Gilbert, Eric Auger, yuzenghui, wanghaibin.wang,
	lushenming

In the VFIO VM state change handler when stopping the VM, the _RUNNING
bit in device_state is cleared which makes the VFIO device stop, including
no longer generating interrupts. Then we can save the pending states of
all interrupts in the GIC VM state change handler (on ARM).

So we have to set the priority of the VFIO VM state change handler
explicitly (like virtio devices) to ensure it is called before the
GIC's in saving.

Signed-off-by: Shenming Lu <lushenming@huawei.com>
Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/vfio/migration.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index f5bf67f642..b74982e3e6 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -862,7 +862,8 @@ static int vfio_migration_init(VFIODevice *vbasedev,
     register_savevm_live(id, VMSTATE_INSTANCE_ID_ANY, 1, &savevm_vfio_handlers,
                          vbasedev);
 
-    migration->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
+    migration->vm_state = qdev_add_vm_change_state_handler(vbasedev->dev,
+                                                           vfio_vmstate_change,
                                                            vbasedev);
     migration->migration_state.notify = vfio_migration_state_notifier;
     add_migration_state_change_notifier(&migration->migration_state);
-- 
2.19.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration
  2021-02-23  2:22 [PATCH v3 0/3] vfio: Some fixes and optimizations for VFIO migration Shenming Lu
  2021-02-23  2:22 ` [PATCH v3 1/3] vfio: Move the saving of the config space to the right place in " Shenming Lu
  2021-02-23  2:22 ` [PATCH v3 2/3] vfio: Set the priority of the VFIO VM state change handler explicitly Shenming Lu
@ 2021-02-23  2:22 ` Shenming Lu
  2021-03-02 21:26   ` Alex Williamson
  2021-03-02  5:58 ` [PATCH v3 0/3] vfio: Some fixes and optimizations for " Shenming Lu
  3 siblings, 1 reply; 9+ messages in thread
From: Shenming Lu @ 2021-02-23  2:22 UTC (permalink / raw)
  To: Alex Williamson, Kirti Wankhede, Cornelia Huck, qemu-devel, qemu-arm
  Cc: Lorenzo Pieralisi, Neo Jia, mst, Marc Zyngier,
	Dr . David Alan Gilbert, Eric Auger, yuzenghui, wanghaibin.wang,
	lushenming

In VFIO migration resume phase and some guest startups, there are
already unmasked vectors in the vector table when calling
vfio_msix_enable(). So in order to avoid inefficiently disabling
and enabling vectors repeatedly, let's allocate all needed vectors
first and then enable these unmasked vectors one by one without
disabling.

Signed-off-by: Shenming Lu <lushenming@huawei.com>
---
 hw/pci/msix.c         |  2 +-
 hw/vfio/pci.c         | 20 +++++++++++++++++---
 include/hw/pci/msix.h |  1 +
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index ae9331cd0b..e057958fcd 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -131,7 +131,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked)
     }
 }
 
-static bool msix_masked(PCIDevice *dev)
+bool msix_masked(PCIDevice *dev)
 {
     return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
 }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index f74be78209..088fd41926 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -569,6 +569,9 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
 
 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);
@@ -587,11 +590,22 @@ 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.
      */
-    vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
-    vfio_msix_vector_release(&vdev->pdev, 0);
+    if (!msix_masked(pdev)) {
+        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);
 
-    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
+    if (msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
                                   vfio_msix_vector_release, NULL)) {
         error_report("vfio: msix_set_vector_notifiers failed");
     }
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 4c4a60c739..b3cd88e262 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -28,6 +28,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f);
 
 int msix_enabled(PCIDevice *dev);
 int msix_present(PCIDevice *dev);
+bool msix_masked(PCIDevice *dev);
 
 bool msix_is_masked(PCIDevice *dev, unsigned vector);
 void msix_set_pending(PCIDevice *dev, unsigned vector);
-- 
2.19.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/3] vfio: Move the saving of the config space to the right place in VFIO migration
  2021-02-23  2:22 ` [PATCH v3 1/3] vfio: Move the saving of the config space to the right place in " Shenming Lu
@ 2021-03-01  5:57   ` Kirti Wankhede
  0 siblings, 0 replies; 9+ messages in thread
From: Kirti Wankhede @ 2021-03-01  5:57 UTC (permalink / raw)
  To: Shenming Lu, Alex Williamson, Cornelia Huck, qemu-devel, qemu-arm
  Cc: Lorenzo Pieralisi, Neo Jia, mst, Marc Zyngier,
	Dr . David Alan Gilbert, Eric Auger, yuzenghui, wanghaibin.wang


Reviewed-by: Kirti Wankhede <kwankhede@nvidia.com>

On 2/23/2021 7:52 AM, Shenming Lu wrote:
> On ARM64 the VFIO SET_IRQS ioctl is dependent on the VM interrupt
> setup, if the restoring of the VFIO PCI device config space is
> before the VGIC, an error might occur in the kernel.
> 
> So we move the saving of the config space to the non-iterable
> process, thus it will be called after the VGIC according to
> their priorities.
> 
> As for the possible dependence of the device specific migration
> data on it's config space, we can let the vendor driver to
> include any config info it needs in its own data stream.
> 
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>   hw/vfio/migration.c | 25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 00daa50ed8..f5bf67f642 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -575,11 +575,6 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>           return ret;
>       }
>   
> -    ret = vfio_save_device_config_state(f, opaque);
> -    if (ret) {
> -        return ret;
> -    }
> -
>       ret = vfio_update_pending(vbasedev);
>       if (ret) {
>           return ret;
> @@ -620,6 +615,19 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>       return ret;
>   }
>   
> +static void vfio_save_state(QEMUFile *f, void *opaque)
> +{
> +    VFIODevice *vbasedev = opaque;
> +    int ret;
> +
> +    ret = vfio_save_device_config_state(f, opaque);
> +    if (ret) {
> +        error_report("%s: Failed to save device config space",
> +                     vbasedev->name);
> +        qemu_file_set_error(f, ret);
> +    }
> +}
> +
>   static int vfio_load_setup(QEMUFile *f, void *opaque)
>   {
>       VFIODevice *vbasedev = opaque;
> @@ -670,11 +678,7 @@ static int vfio_load_state(QEMUFile *f, void *opaque, int version_id)
>           switch (data) {
>           case VFIO_MIG_FLAG_DEV_CONFIG_STATE:
>           {
> -            ret = vfio_load_device_config_state(f, opaque);
> -            if (ret) {
> -                return ret;
> -            }
> -            break;
> +            return vfio_load_device_config_state(f, opaque);
>           }
>           case VFIO_MIG_FLAG_DEV_SETUP_STATE:
>           {
> @@ -720,6 +724,7 @@ static SaveVMHandlers savevm_vfio_handlers = {
>       .save_live_pending = vfio_save_pending,
>       .save_live_iterate = vfio_save_iterate,
>       .save_live_complete_precopy = vfio_save_complete_precopy,
> +    .save_state = vfio_save_state,
>       .load_setup = vfio_load_setup,
>       .load_cleanup = vfio_load_cleanup,
>       .load_state = vfio_load_state,
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/3] vfio: Some fixes and optimizations for VFIO migration
  2021-02-23  2:22 [PATCH v3 0/3] vfio: Some fixes and optimizations for VFIO migration Shenming Lu
                   ` (2 preceding siblings ...)
  2021-02-23  2:22 ` [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration Shenming Lu
@ 2021-03-02  5:58 ` Shenming Lu
  3 siblings, 0 replies; 9+ messages in thread
From: Shenming Lu @ 2021-03-02  5:58 UTC (permalink / raw)
  To: Alex Williamson, Kirti Wankhede, Cornelia Huck, qemu-devel, qemu-arm
  Cc: Lorenzo Pieralisi, Neo Jia, mst, Marc Zyngier,
	Dr . David Alan Gilbert, Eric Auger, yuzenghui, wanghaibin.wang

Hi Alex,

Does this series need any further modification? Wish you can pick it up. :-)

On 2021/2/23 10:22, Shenming Lu wrote:
> This patch set includes two fixes and one optimization for VFIO migration
> as blew:
> 
> Patch 1-2:
> - Fix two ordering problems in migration.
> 
> Patch 3:
> - Optimize the enabling process of the MSI-X vectors in migration.
> 
> History:
> 
> v2 -> v3:
> - Nit fixes.
> - Set error in migration stream for migration to fail in Patch 1.
> - Tested Patch 3 with a Windows guest.
> 
> Thanks,
> Shenming
> 
> 
> Shenming Lu (3):
>   vfio: Move the saving of the config space to the right place in VFIO
>     migration
>   vfio: Set the priority of the VFIO VM state change handler explicitly
>   vfio: Avoid disabling and enabling vectors repeatedly in VFIO
>     migration
> 
>  hw/pci/msix.c         |  2 +-
>  hw/vfio/migration.c   | 28 +++++++++++++++++-----------
>  hw/vfio/pci.c         | 20 +++++++++++++++++---
>  include/hw/pci/msix.h |  1 +
>  4 files changed, 36 insertions(+), 15 deletions(-)
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration
  2021-02-23  2:22 ` [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration Shenming Lu
@ 2021-03-02 21:26   ` Alex Williamson
  2021-03-09  7:25     ` Shenming Lu
  0 siblings, 1 reply; 9+ messages in thread
From: Alex Williamson @ 2021-03-02 21:26 UTC (permalink / raw)
  To: Shenming Lu, mst, marcel.apfelbaum
  Cc: Lorenzo Pieralisi, Neo Jia, Marc Zyngier, Cornelia Huck,
	qemu-devel, Dr . David Alan Gilbert, Eric Auger, Kirti Wankhede,
	qemu-arm, yuzenghui, wanghaibin.wang


MST/Marcel,

Do you have an Ack or objection to exporting msix_masked() as below?
Thanks,

Alex

On Tue, 23 Feb 2021 10:22:25 +0800
Shenming Lu <lushenming@huawei.com> wrote:

> In VFIO migration resume phase and some guest startups, there are
> already unmasked vectors in the vector table when calling
> vfio_msix_enable(). So in order to avoid inefficiently disabling
> and enabling vectors repeatedly, let's allocate all needed vectors
> first and then enable these unmasked vectors one by one without
> disabling.
> 
> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> ---
>  hw/pci/msix.c         |  2 +-
>  hw/vfio/pci.c         | 20 +++++++++++++++++---
>  include/hw/pci/msix.h |  1 +
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index ae9331cd0b..e057958fcd 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -131,7 +131,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked)
>      }
>  }
>  
> -static bool msix_masked(PCIDevice *dev)
> +bool msix_masked(PCIDevice *dev)
>  {
>      return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
>  }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index f74be78209..088fd41926 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -569,6 +569,9 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>  
>  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);
> @@ -587,11 +590,22 @@ 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.
>       */
> -    vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
> -    vfio_msix_vector_release(&vdev->pdev, 0);
> +    if (!msix_masked(pdev)) {
> +        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);
>  
> -    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> +    if (msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
>                                    vfio_msix_vector_release, NULL)) {
>          error_report("vfio: msix_set_vector_notifiers failed");
>      }
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 4c4a60c739..b3cd88e262 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -28,6 +28,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f);
>  
>  int msix_enabled(PCIDevice *dev);
>  int msix_present(PCIDevice *dev);
> +bool msix_masked(PCIDevice *dev);
>  
>  bool msix_is_masked(PCIDevice *dev, unsigned vector);
>  void msix_set_pending(PCIDevice *dev, unsigned vector);



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration
  2021-03-02 21:26   ` Alex Williamson
@ 2021-03-09  7:25     ` Shenming Lu
  2021-03-09 18:12       ` Alex Williamson
  0 siblings, 1 reply; 9+ messages in thread
From: Shenming Lu @ 2021-03-09  7:25 UTC (permalink / raw)
  To: Alex Williamson, mst, marcel.apfelbaum
  Cc: Lorenzo Pieralisi, Neo Jia, Marc Zyngier, Cornelia Huck,
	qemu-devel, Dr . David Alan Gilbert, Eric Auger, Kirti Wankhede,
	qemu-arm, yuzenghui, wanghaibin.wang

On 2021/3/3 5:26, Alex Williamson wrote:
> 
> MST/Marcel,
> 
> Do you have an Ack or objection to exporting msix_masked() as below?

Or could we use msix_function_masked instead as below? :-)

if (!pdev->msix_function_masked) {
    for (nr = 0; nr < msix_nr_vectors_allocated(pdev); nr++) {
        if (!msix_is_masked(pdev, nr)) {
            max_vec = nr;
        }
    }
}

Thanks,
Shenming

> Thanks,
> 
> Alex
> 
> On Tue, 23 Feb 2021 10:22:25 +0800
> Shenming Lu <lushenming@huawei.com> wrote:
> 
>> In VFIO migration resume phase and some guest startups, there are
>> already unmasked vectors in the vector table when calling
>> vfio_msix_enable(). So in order to avoid inefficiently disabling
>> and enabling vectors repeatedly, let's allocate all needed vectors
>> first and then enable these unmasked vectors one by one without
>> disabling.
>>
>> Signed-off-by: Shenming Lu <lushenming@huawei.com>
>> ---
>>  hw/pci/msix.c         |  2 +-
>>  hw/vfio/pci.c         | 20 +++++++++++++++++---
>>  include/hw/pci/msix.h |  1 +
>>  3 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>> index ae9331cd0b..e057958fcd 100644
>> --- a/hw/pci/msix.c
>> +++ b/hw/pci/msix.c
>> @@ -131,7 +131,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked)
>>      }
>>  }
>>  
>> -static bool msix_masked(PCIDevice *dev)
>> +bool msix_masked(PCIDevice *dev)
>>  {
>>      return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
>>  }
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index f74be78209..088fd41926 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -569,6 +569,9 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
>>  
>>  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);
>> @@ -587,11 +590,22 @@ 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.
>>       */
>> -    vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
>> -    vfio_msix_vector_release(&vdev->pdev, 0);
>> +    if (!msix_masked(pdev)) {
>> +        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);
>>  
>> -    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
>> +    if (msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
>>                                    vfio_msix_vector_release, NULL)) {
>>          error_report("vfio: msix_set_vector_notifiers failed");
>>      }
>> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
>> index 4c4a60c739..b3cd88e262 100644
>> --- a/include/hw/pci/msix.h
>> +++ b/include/hw/pci/msix.h
>> @@ -28,6 +28,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f);
>>  
>>  int msix_enabled(PCIDevice *dev);
>>  int msix_present(PCIDevice *dev);
>> +bool msix_masked(PCIDevice *dev);
>>  
>>  bool msix_is_masked(PCIDevice *dev, unsigned vector);
>>  void msix_set_pending(PCIDevice *dev, unsigned vector);
> 
> .
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration
  2021-03-09  7:25     ` Shenming Lu
@ 2021-03-09 18:12       ` Alex Williamson
  0 siblings, 0 replies; 9+ messages in thread
From: Alex Williamson @ 2021-03-09 18:12 UTC (permalink / raw)
  To: Shenming Lu
  Cc: Lorenzo Pieralisi, Neo Jia, mst, Marc Zyngier, Cornelia Huck,
	qemu-devel, Dr . David Alan Gilbert, Eric Auger, Kirti Wankhede,
	qemu-arm, yuzenghui, wanghaibin.wang

On Tue, 9 Mar 2021 15:25:52 +0800
Shenming Lu <lushenming@huawei.com> wrote:

> On 2021/3/3 5:26, Alex Williamson wrote:
> > 
> > MST/Marcel,
> > 
> > Do you have an Ack or objection to exporting msix_masked() as below?  
> 
> Or could we use msix_function_masked instead as below? :-)
> 
> if (!pdev->msix_function_masked) {
>     for (nr = 0; nr < msix_nr_vectors_allocated(pdev); nr++) {
>         if (!msix_is_masked(pdev, nr)) {
>             max_vec = nr;
>         }
>     }
> }

Yeah, it looks like that should work.  I'll look for a v4 with that
change.  Thanks,

Alex

> > On Tue, 23 Feb 2021 10:22:25 +0800
> > Shenming Lu <lushenming@huawei.com> wrote:
> >   
> >> In VFIO migration resume phase and some guest startups, there are
> >> already unmasked vectors in the vector table when calling
> >> vfio_msix_enable(). So in order to avoid inefficiently disabling
> >> and enabling vectors repeatedly, let's allocate all needed vectors
> >> first and then enable these unmasked vectors one by one without
> >> disabling.
> >>
> >> Signed-off-by: Shenming Lu <lushenming@huawei.com>
> >> ---
> >>  hw/pci/msix.c         |  2 +-
> >>  hw/vfio/pci.c         | 20 +++++++++++++++++---
> >>  include/hw/pci/msix.h |  1 +
> >>  3 files changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >> index ae9331cd0b..e057958fcd 100644
> >> --- a/hw/pci/msix.c
> >> +++ b/hw/pci/msix.c
> >> @@ -131,7 +131,7 @@ static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked)
> >>      }
> >>  }
> >>  
> >> -static bool msix_masked(PCIDevice *dev)
> >> +bool msix_masked(PCIDevice *dev)
> >>  {
> >>      return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK;
> >>  }
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index f74be78209..088fd41926 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -569,6 +569,9 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
> >>  
> >>  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);
> >> @@ -587,11 +590,22 @@ 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.
> >>       */
> >> -    vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
> >> -    vfio_msix_vector_release(&vdev->pdev, 0);
> >> +    if (!msix_masked(pdev)) {
> >> +        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);
> >>  
> >> -    if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use,
> >> +    if (msix_set_vector_notifiers(pdev, vfio_msix_vector_use,
> >>                                    vfio_msix_vector_release, NULL)) {
> >>          error_report("vfio: msix_set_vector_notifiers failed");
> >>      }
> >> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> >> index 4c4a60c739..b3cd88e262 100644
> >> --- a/include/hw/pci/msix.h
> >> +++ b/include/hw/pci/msix.h
> >> @@ -28,6 +28,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f);
> >>  
> >>  int msix_enabled(PCIDevice *dev);
> >>  int msix_present(PCIDevice *dev);
> >> +bool msix_masked(PCIDevice *dev);
> >>  
> >>  bool msix_is_masked(PCIDevice *dev, unsigned vector);
> >>  void msix_set_pending(PCIDevice *dev, unsigned vector);  
> > 
> > .
> >   
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-09 19:44 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23  2:22 [PATCH v3 0/3] vfio: Some fixes and optimizations for VFIO migration Shenming Lu
2021-02-23  2:22 ` [PATCH v3 1/3] vfio: Move the saving of the config space to the right place in " Shenming Lu
2021-03-01  5:57   ` Kirti Wankhede
2021-02-23  2:22 ` [PATCH v3 2/3] vfio: Set the priority of the VFIO VM state change handler explicitly Shenming Lu
2021-02-23  2:22 ` [PATCH v3 3/3] vfio: Avoid disabling and enabling vectors repeatedly in VFIO migration Shenming Lu
2021-03-02 21:26   ` Alex Williamson
2021-03-09  7:25     ` Shenming Lu
2021-03-09 18:12       ` Alex Williamson
2021-03-02  5:58 ` [PATCH v3 0/3] vfio: Some fixes and optimizations for " Shenming Lu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.