[v11,05/11] vfio/migration: Block multiple devices migration

Commit Message

Avihai Horon Feb. 16, 2023, 2:36 p.m. UTC

Currently VFIO migration doesn't implement some kind of intermediate
quiescent state in which P2P DMAs are quiesced before stopping or
running the device. This can cause problems in multi-device migration
where the devices are doing P2P DMAs, since the devices are not stopped
together at the same time.

Until such support is added, block migration of multiple devices.

Signed-off-by: Avihai Horon <[e-mail]>
Reviewed-by: Cédric Le Goater <[e-mail]>
Reviewed-by: Juan Quintela <[e-mail]>
---
 include/hw/vfio/vfio-common.h |  2 ++
 hw/vfio/common.c              | 53 +++++++++++++++++++++++++++++++++++
 hw/vfio/migration.c           |  6 ++++
 3 files changed, 61 insertions(+)

Comments

Shameerali Kolothum Thodi May 16, 2023, 10:03 a.m. UTC | #1

> -----Original Message-----
> From:
> qemu-devel-bounces+shameerali.kolothum.thodi=[e-mail]
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nong
> nu.org] On Behalf Of Avihai Horon
> Sent: 16 February 2023 14:36
> To: [e-mail]
> Cc: Alex Williamson <[e-mail]>; Juan Quintela
> <[e-mail]>; Dr. David Alan Gilbert <[e-mail]>;
> Michael S. Tsirkin <[e-mail]>; Cornelia Huck <[e-mail]>;
> Paolo Bonzini <[e-mail]>; Vladimir Sementsov-Ogievskiy
> <[e-mail]>; Cédric Le Goater <[e-mail]>; Yishai
> Hadas <[e-mail]>; Jason Gunthorpe <[e-mail]>; Maor
> Gottlieb <[e-mail]>; Avihai Horon <[e-mail]>; Kirti
> Wankhede <[e-mail]>; Tarun Gupta <[e-mail]>;
> Joao Martins <[e-mail]>
> Subject: [PATCH v11 05/11] vfio/migration: Block multiple devices migration
> 
> Currently VFIO migration doesn't implement some kind of intermediate
> quiescent state in which P2P DMAs are quiesced before stopping or
> running the device. This can cause problems in multi-device migration
> where the devices are doing P2P DMAs, since the devices are not stopped
> together at the same time.
> 
> Until such support is added, block migration of multiple devices.

Missed this one. Currently this blocks even if the attached devices are not
capable of P2P DMAs. eg; HiSilicon ACC devices. These are integrated end point
devices without any P2P capability between them. Is it Ok to check for
VFIO_MIGRATION_P2P flag and allow if the devices are not supporting that?

I can sent a patch if that’s fine.

Thanks,
Shameer
> 
> Signed-off-by: Avihai Horon <[e-mail]>
> Reviewed-by: Cédric Le Goater <[e-mail]>
> Reviewed-by: Juan Quintela <[e-mail]>
> ---
>  include/hw/vfio/vfio-common.h |  2 ++
>  hw/vfio/common.c              | 53
> +++++++++++++++++++++++++++++++++++
>  hw/vfio/migration.c           |  6 ++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h
> b/include/hw/vfio/vfio-common.h
> index e573f5a9f1..56b1683824 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -218,6 +218,8 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup)
> VFIOGroupList;
>  extern VFIOGroupList vfio_group_list;
> 
>  bool vfio_mig_active(void);
> +int vfio_block_multiple_devices_migration(Error **errp);
> +void vfio_unblock_multiple_devices_migration(void);
>  int64_t vfio_mig_bytes_transferred(void);
> 
>  #ifdef CONFIG_LINUX
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 3a35f4afad..fe80ccf914 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -41,6 +41,7 @@
>  #include "qapi/error.h"
>  #include "migration/migration.h"
>  #include "migration/misc.h"
> +#include "migration/blocker.h"
>  #include "sysemu/tpm.h"
> 
>  VFIOGroupList vfio_group_list =
> @@ -337,6 +338,58 @@ bool vfio_mig_active(void)
>      return true;
>  }
> 
> +static Error *multiple_devices_migration_blocker;
> +
> +static unsigned int vfio_migratable_device_num(void)
> +{
> +    VFIOGroup *group;
> +    VFIODevice *vbasedev;
> +    unsigned int device_num = 0;
> +
> +    QLIST_FOREACH(group, &vfio_group_list, next) {
> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +            if (vbasedev->migration) {
> +                device_num++;
> +            }
> +        }
> +    }
> +
> +    return device_num;
> +}
> +
> +int vfio_block_multiple_devices_migration(Error **errp)
> +{
> +    int ret;
> +
> +    if (multiple_devices_migration_blocker ||
> +        vfio_migratable_device_num() <= 1) {
> +        return 0;
> +    }
> +
> +    error_setg(&multiple_devices_migration_blocker,
> +               "Migration is currently not supported with multiple "
> +               "VFIO devices");
> +    ret = migrate_add_blocker(multiple_devices_migration_blocker, errp);
> +    if (ret < 0) {
> +        error_free(multiple_devices_migration_blocker);
> +        multiple_devices_migration_blocker = NULL;
> +    }
> +
> +    return ret;
> +}
> +
> +void vfio_unblock_multiple_devices_migration(void)
> +{
> +    if (!multiple_devices_migration_blocker ||
> +        vfio_migratable_device_num() > 1) {
> +        return;
> +    }
> +
> +    migrate_del_blocker(multiple_devices_migration_blocker);
> +    error_free(multiple_devices_migration_blocker);
> +    multiple_devices_migration_blocker = NULL;
> +}
> +
>  static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>  {
>      VFIOGroup *group;
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index e56eef1ee8..8e96999669 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -878,6 +878,11 @@ int vfio_migration_probe(VFIODevice *vbasedev,
> Error **errp)
>          goto add_blocker;
>      }
> 
> +    ret = vfio_block_multiple_devices_migration(errp);
> +    if (ret) {
> +        return ret;
> +    }
> +
>      trace_vfio_migration_probe(vbasedev->name, info->index);
>      g_free(info);
>      return 0;
> @@ -904,6 +909,7 @@ void vfio_migration_finalize(VFIODevice *vbasedev)
>          qemu_del_vm_change_state_handler(migration->vm_state);
>          unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio",
> vbasedev);
>          vfio_migration_exit(vbasedev);
> +        vfio_unblock_multiple_devices_migration();
>      }
> 
>      if (vbasedev->migration_blocker) {
> --
> 2.26.3
>

Jason Gunthorpe May 16, 2023, 11:59 a.m. UTC | #2

On Tue, May 16, 2023 at 10:03:54AM +0000, Shameerali Kolothum Thodi wrote:

> > Currently VFIO migration doesn't implement some kind of intermediate
> > quiescent state in which P2P DMAs are quiesced before stopping or
> > running the device. This can cause problems in multi-device migration
> > where the devices are doing P2P DMAs, since the devices are not stopped
> > together at the same time.
> > 
> > Until such support is added, block migration of multiple devices.
> 
> Missed this one. Currently this blocks even if the attached devices are not
> capable of P2P DMAs. eg; HiSilicon ACC devices. These are integrated end point
> devices without any P2P capability between them. Is it Ok to check for
> VFIO_MIGRATION_P2P flag and allow if the devices are not supporting that?

Lacking VFIO_MIGRATION_P2P doesn't mean the device is incapable of
P2P, it means the migration can't support P2P.

We'd need some kind of new flag to check and such devices should be
blocked from creating P2P mappings. Basically we don't currently
fully support devices that are incapable of P2P operations.

What happens on your platform if a guest tries to do P2P? Does the
platform crash?

Jason

Shameerali Kolothum Thodi May 16, 2023, 1:57 p.m. UTC | #3

> -----Original Message-----
> From: Jason Gunthorpe [mailto:[e-mail]]
> Sent: 16 May 2023 13:00
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: Avihai Horon <[e-mail]>; [e-mail]; Alex
> Williamson <[e-mail]>; Juan Quintela
> <[e-mail]>; Dr. David Alan Gilbert <[e-mail]>;
> Michael S. Tsirkin <[e-mail]>; Cornelia Huck <[e-mail]>;
> Paolo Bonzini <[e-mail]>; Vladimir Sementsov-Ogievskiy
> <[e-mail]>; Cédric Le Goater <[e-mail]>; Yishai
> Hadas <[e-mail]>; Maor Gottlieb <[e-mail]>; Kirti
> Wankhede <[e-mail]>; Tarun Gupta <[e-mail]>;
> Joao Martins <[e-mail]>
> Subject: Re: [PATCH v11 05/11] vfio/migration: Block multiple devices
> migration
> 
> On Tue, May 16, 2023 at 10:03:54AM +0000, Shameerali Kolothum Thodi
> wrote:
> 
> > > Currently VFIO migration doesn't implement some kind of intermediate
> > > quiescent state in which P2P DMAs are quiesced before stopping or
> > > running the device. This can cause problems in multi-device migration
> > > where the devices are doing P2P DMAs, since the devices are not stopped
> > > together at the same time.
> > >
> > > Until such support is added, block migration of multiple devices.
> >
> > Missed this one. Currently this blocks even if the attached devices are not
> > capable of P2P DMAs. eg; HiSilicon ACC devices. These are integrated end
> point
> > devices without any P2P capability between them. Is it Ok to check for
> > VFIO_MIGRATION_P2P flag and allow if the devices are not supporting
> that?
> 
> Lacking VFIO_MIGRATION_P2P doesn't mean the device is incapable of
> P2P, it means the migration can't support P2P.
> 
> We'd need some kind of new flag to check and such devices should be
> blocked from creating P2P mappings. Basically we don't currently
> fully support devices that are incapable of P2P operations.

Ok. I will take a look.

> What happens on your platform if a guest tries to do P2P? Does the
> platform crash?

I am not sure. Since the devices are behind SMMU, I was under the assumption
that we do have the guarantee of isolation here(grouping). Or this is something
we are worried only during migration?

Thanks,
Shameer

Jason Gunthorpe May 16, 2023, 2:04 p.m. UTC | #4

On Tue, May 16, 2023 at 01:57:22PM +0000, Shameerali Kolothum Thodi wrote:

> > What happens on your platform if a guest tries to do P2P? Does the
> > platform crash?
> 
> I am not sure. Since the devices are behind SMMU, I was under the assumption
> that we do have the guarantee of isolation here(grouping).

That is not the case. The SMMU will contain an IOPTE with the the CPU
address of the P2P memory and when the SMMU executes that operation
the system does .. ??

Hopefully it responds back with error and keeps going, or it
crashes..

Jason

Alex Williamson May 16, 2023, 2:27 p.m. UTC | #5

On Tue, 16 May 2023 13:57:22 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> > -----Original Message-----
> > From: Jason Gunthorpe [mailto:[e-mail]]
> > Sent: 16 May 2023 13:00
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: Avihai Horon <[e-mail]>; [e-mail]; Alex
> > Williamson <[e-mail]>; Juan Quintela
> > <[e-mail]>; Dr. David Alan Gilbert <[e-mail]>;
> > Michael S. Tsirkin <[e-mail]>; Cornelia Huck <[e-mail]>;
> > Paolo Bonzini <[e-mail]>; Vladimir Sementsov-Ogievskiy
> > <[e-mail]>; Cédric Le Goater <[e-mail]>; Yishai
> > Hadas <[e-mail]>; Maor Gottlieb <[e-mail]>; Kirti
> > Wankhede <[e-mail]>; Tarun Gupta <[e-mail]>;
> > Joao Martins <[e-mail]>
> > Subject: Re: [PATCH v11 05/11] vfio/migration: Block multiple devices
> > migration
> > 
> > On Tue, May 16, 2023 at 10:03:54AM +0000, Shameerali Kolothum Thodi
> > wrote:
> >   
> > > > Currently VFIO migration doesn't implement some kind of intermediate
> > > > quiescent state in which P2P DMAs are quiesced before stopping or
> > > > running the device. This can cause problems in multi-device migration
> > > > where the devices are doing P2P DMAs, since the devices are not stopped
> > > > together at the same time.
> > > >
> > > > Until such support is added, block migration of multiple devices.  
> > >
> > > Missed this one. Currently this blocks even if the attached devices are not
> > > capable of P2P DMAs. eg; HiSilicon ACC devices. These are integrated end  
> > point  
> > > devices without any P2P capability between them. Is it Ok to check for
> > > VFIO_MIGRATION_P2P flag and allow if the devices are not supporting  
> > that?
> > 
> > Lacking VFIO_MIGRATION_P2P doesn't mean the device is incapable of
> > P2P, it means the migration can't support P2P.
> > 
> > We'd need some kind of new flag to check and such devices should be
> > blocked from creating P2P mappings. Basically we don't currently
> > fully support devices that are incapable of P2P operations.  
> 
> Ok. I will take a look.
> 
> > What happens on your platform if a guest tries to do P2P? Does the
> > platform crash?  
> 
> I am not sure. Since the devices are behind SMMU, I was under the assumption
> that we do have the guarantee of isolation here(grouping). Or this is something
> we are worried only during migration?

Grouping doesn't guarantee that mappings cannot be created through the
SMMU between devices.  When we refer to devices being isolated between
groups, that only excludes internal P2P between devices, for example
across the internal link between functions with implementation specific
routing.  For group isolation, the guarantee is that DMA is always
routed upstream, not that the ultimate target cannot be another device.
To guarantee lack of P2P the SMMU would need to reject non-memory
translation targets.  Thanks,

Alex

Shameerali Kolothum Thodi May 16, 2023, 2:35 p.m. UTC | #6

> -----Original Message-----
> From: Alex Williamson [mailto:[e-mail]]
> Sent: 16 May 2023 15:28
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: Jason Gunthorpe <[e-mail]>; Avihai Horon <[e-mail]>;
> [e-mail]; Juan Quintela <[e-mail]>; Dr. David
> Alan Gilbert <[e-mail]>; Michael S. Tsirkin <[e-mail]>;
> Cornelia Huck <[e-mail]>; Paolo Bonzini
> <[e-mail]>; Vladimir Sementsov-Ogievskiy
> <[e-mail]>; Cédric Le Goater <[e-mail]>; Yishai
> Hadas <[e-mail]>; Maor Gottlieb <[e-mail]>; Kirti
> Wankhede <[e-mail]>; Tarun Gupta <[e-mail]>;
> Joao Martins <[e-mail]>
> Subject: Re: [PATCH v11 05/11] vfio/migration: Block multiple devices
> migration
> 
> On Tue, 16 May 2023 13:57:22 +0000
> Shameerali Kolothum Thodi <[e-mail]>
> wrote:
> 
> > > -----Original Message-----
> > > From: Jason Gunthorpe [mailto:[e-mail]]
> > > Sent: 16 May 2023 13:00
> > > To: Shameerali Kolothum Thodi
> <[e-mail]>
> > > Cc: Avihai Horon <[e-mail]>; [e-mail]; Alex
> > > Williamson <[e-mail]>; Juan Quintela
> > > <[e-mail]>; Dr. David Alan Gilbert <[e-mail]>;
> > > Michael S. Tsirkin <[e-mail]>; Cornelia Huck
> <[e-mail]>;
> > > Paolo Bonzini <[e-mail]>; Vladimir Sementsov-Ogievskiy
> > > <[e-mail]>; Cédric Le Goater <[e-mail]>;
> Yishai
> > > Hadas <[e-mail]>; Maor Gottlieb <[e-mail]>; Kirti
> > > Wankhede <[e-mail]>; Tarun Gupta
> <[e-mail]>;
> > > Joao Martins <[e-mail]>
> > > Subject: Re: [PATCH v11 05/11] vfio/migration: Block multiple devices
> > > migration
> > >
> > > On Tue, May 16, 2023 at 10:03:54AM +0000, Shameerali Kolothum Thodi
> > > wrote:
> > >
> > > > > Currently VFIO migration doesn't implement some kind of
> intermediate
> > > > > quiescent state in which P2P DMAs are quiesced before stopping or
> > > > > running the device. This can cause problems in multi-device migration
> > > > > where the devices are doing P2P DMAs, since the devices are not
> stopped
> > > > > together at the same time.
> > > > >
> > > > > Until such support is added, block migration of multiple devices.
> > > >
> > > > Missed this one. Currently this blocks even if the attached devices are
> not
> > > > capable of P2P DMAs. eg; HiSilicon ACC devices. These are integrated
> end
> > > point
> > > > devices without any P2P capability between them. Is it Ok to check for
> > > > VFIO_MIGRATION_P2P flag and allow if the devices are not supporting
> > > that?
> > >
> > > Lacking VFIO_MIGRATION_P2P doesn't mean the device is incapable of
> > > P2P, it means the migration can't support P2P.
> > >
> > > We'd need some kind of new flag to check and such devices should be
> > > blocked from creating P2P mappings. Basically we don't currently
> > > fully support devices that are incapable of P2P operations.
> >
> > Ok. I will take a look.
> >
> > > What happens on your platform if a guest tries to do P2P? Does the
> > > platform crash?
> >
> > I am not sure. Since the devices are behind SMMU, I was under the
> assumption
> > that we do have the guarantee of isolation here(grouping). Or this is
> something
> > we are worried only during migration?
> 
> Grouping doesn't guarantee that mappings cannot be created through the
> SMMU between devices.  When we refer to devices being isolated between
> groups, that only excludes internal P2P between devices, for example
> across the internal link between functions with implementation specific
> routing.  For group isolation, the guarantee is that DMA is always
> routed upstream, not that the ultimate target cannot be another device.
> To guarantee lack of P2P the SMMU would need to reject non-memory
> translation targets.  Thanks,

Ok. Got it. So it depends on what SMMU does for that mapping and is not
related to migration per se and has the potential to crash the system if 
SMMU go ahead with that memory access. Isn't it a more generic problem
then when we have multiple devices attached to the VM? I need to check if 
there is anything in SMMU spec that forbids this access.

Thanks,
Shameer

Jason Gunthorpe May 16, 2023, 4:11 p.m. UTC | #7

On Tue, May 16, 2023 at 02:35:21PM +0000, Shameerali Kolothum Thodi wrote:

> Ok. Got it. So it depends on what SMMU does for that mapping and is not
> related to migration per se and has the potential to crash the system if 
> SMMU go ahead with that memory access. Isn't it a more generic problem
> then when we have multiple devices attached to the VM? 

It is, and I am hoping to solve it when I get P2P support into iommufd

> I need to check if there is anything in SMMU spec that forbids this
> access.

There isn't, it is up to the integration how it handles 'hairpin'
traffic. A good implementation will work correctly as Intel and other
CPUs do. A very bad implementation will crash. Medium is to return
error tlps.

Jason

Patch

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index e573f5a9f1..56b1683824 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -218,6 +218,8 @@  typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
 extern VFIOGroupList vfio_group_list;
 
 bool vfio_mig_active(void);
+int vfio_block_multiple_devices_migration(Error **errp);
+void vfio_unblock_multiple_devices_migration(void);
 int64_t vfio_mig_bytes_transferred(void);
 
 #ifdef CONFIG_LINUX
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3a35f4afad..fe80ccf914 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -41,6 +41,7 @@ 
 #include "qapi/error.h"
 #include "migration/migration.h"
 #include "migration/misc.h"
+#include "migration/blocker.h"
 #include "sysemu/tpm.h"
 
 VFIOGroupList vfio_group_list =
@@ -337,6 +338,58 @@  bool vfio_mig_active(void)
     return true;
 }
 
+static Error *multiple_devices_migration_blocker;
+
+static unsigned int vfio_migratable_device_num(void)
+{
+    VFIOGroup *group;
+    VFIODevice *vbasedev;
+    unsigned int device_num = 0;
+
+    QLIST_FOREACH(group, &vfio_group_list, next) {
+        QLIST_FOREACH(vbasedev, &group->device_list, next) {
+            if (vbasedev->migration) {
+                device_num++;
+            }
+        }
+    }
+
+    return device_num;
+}
+
+int vfio_block_multiple_devices_migration(Error **errp)
+{
+    int ret;
+
+    if (multiple_devices_migration_blocker ||
+        vfio_migratable_device_num() <= 1) {
+        return 0;
+    }
+
+    error_setg(&multiple_devices_migration_blocker,
+               "Migration is currently not supported with multiple "
+               "VFIO devices");
+    ret = migrate_add_blocker(multiple_devices_migration_blocker, errp);
+    if (ret < 0) {
+        error_free(multiple_devices_migration_blocker);
+        multiple_devices_migration_blocker = NULL;
+    }
+
+    return ret;
+}
+
+void vfio_unblock_multiple_devices_migration(void)
+{
+    if (!multiple_devices_migration_blocker ||
+        vfio_migratable_device_num() > 1) {
+        return;
+    }
+
+    migrate_del_blocker(multiple_devices_migration_blocker);
+    error_free(multiple_devices_migration_blocker);
+    multiple_devices_migration_blocker = NULL;
+}
+
 static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
 {
     VFIOGroup *group;
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index e56eef1ee8..8e96999669 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -878,6 +878,11 @@  int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
         goto add_blocker;
     }
 
+    ret = vfio_block_multiple_devices_migration(errp);
+    if (ret) {
+        return ret;
+    }
+
     trace_vfio_migration_probe(vbasedev->name, info->index);
     g_free(info);
     return 0;
@@ -904,6 +909,7 @@  void vfio_migration_finalize(VFIODevice *vbasedev)
         qemu_del_vm_change_state_handler(migration->vm_state);
         unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev);
         vfio_migration_exit(vbasedev);
+        vfio_unblock_multiple_devices_migration();
     }
 
     if (vbasedev->migration_blocker) {