[PATCH v2 0/3] qapi/vfio: Add VFIO migration QAPI event

[PATCH v2 0/3] qapi/vfio: Add VFIO migration QAPI event

[Avihai Horon]

Hello,

This series adds a new QAPI event for VFIO device migration state 
change. This event will be emitted when a VFIO device changes its
state, for example, during migration or when stopping/starting the
guest.

This event can be used by management applications to get updates on the
current state of the VFIO device for their own purposes.

A new per VFIO device capability, "migration-events", is added so events
can be enabled only for the required devices. It is disabled by default.

Feedback/comments are appreciated,

Thanks.

Changes from v1 [1]:
* Added more info to patch #1 commit mesasge. (Markus)
* Renamed VFIODeviceMigState to VfioMigrationState and
  VFIO_DEVICE_MIG_STATE_CHANGED to VFIO_MIGRATION. (Joao, Markus)
* Added qom-path and qdev id to VFIO_MIGRATION event data. (Markus)
* Handled no-op state transitions in vfio_migration_set_state().
  (Cedric)
* Added helper to set VFIO state and emit VFIO event. (Peter)

[1]
https://lore.kernel.org/qemu-devel/20240430051621.19597-1-avihaih@nvidia.com/

Avihai Horon (3):
  qapi/vfio: Add VFIO migration QAPI event
  vfio/migration: Emit VFIO migration QAPI event
  vfio/migration: Don't emit STOP_COPY VFIO migration QAPI event twice

 MAINTAINERS                   |  1 +
 qapi/qapi-schema.json         |  1 +
 qapi/vfio.json                | 67 +++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-common.h |  1 +
 hw/vfio/migration.c           | 60 +++++++++++++++++++++++++++++--
 hw/vfio/pci.c                 |  2 ++
 qapi/meson.build              |  1 +
 7 files changed, 130 insertions(+), 3 deletions(-)
 create mode 100644 qapi/vfio.json

-- 
2.26.3

[PATCH v2 1/3] qapi/vfio: Add VFIO migration QAPI event

[Avihai Horon]

Add a new QAPI event for VFIO migration. This event will be emitted when
a VFIO device changes its migration state, for example, during migration
or when stopping/starting the guest.

This event can be used by management applications to get updates on the
current state of the VFIO device for their own purposes.

Note that this new event is introduced since VFIO devices have a unique
set of migration states which cannot be described as accurately by other
existing events such as run state or migration status.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 MAINTAINERS           |  1 +
 qapi/qapi-schema.json |  1 +
 qapi/vfio.json        | 67 +++++++++++++++++++++++++++++++++++++++++++
 qapi/meson.build      |  1 +
 4 files changed, 70 insertions(+)
 create mode 100644 qapi/vfio.json

diff --git a/MAINTAINERS b/MAINTAINERS
index 84391777db..b5f1de459e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2160,6 +2160,7 @@ F: hw/vfio/*
 F: include/hw/vfio/
 F: docs/igd-assign.txt
 F: docs/devel/migration/vfio.rst
+F: qapi/vfio.json
 
 vfio-ccw
 M: Eric Farman <farman@linux.ibm.com>
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 5e33da7228..b1581988e4 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -78,5 +78,6 @@
 { 'include': 'pci.json' }
 { 'include': 'stats.json' }
 { 'include': 'virtio.json' }
+{ 'include': 'vfio.json' }
 { 'include': 'cryptodev.json' }
 { 'include': 'cxl.json' }
diff --git a/qapi/vfio.json b/qapi/vfio.json
new file mode 100644
index 0000000000..a0e5013188
--- /dev/null
+++ b/qapi/vfio.json
@@ -0,0 +1,67 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+
+##
+# = VFIO devices
+##
+
+##
+# @VfioMigrationState:
+#
+# An enumeration of the VFIO device migration states.
+#
+# @stop: The device is stopped.
+#
+# @running: The device is running.
+#
+# @stop-copy: The device is stopped and its internal state is available
+#     for reading.
+#
+# @resuming: The device is stopped and its internal state is available
+#     for writing.
+#
+# @running-p2p: The device is running in the P2P quiescent state.
+#
+# @pre-copy: The device is running, tracking its internal state and its
+#     internal state is available for reading.
+#
+# @pre-copy-p2p: The device is running in the P2P quiescent state,
+#     tracking its internal state and its internal state is available
+#     for reading.
+#
+# Since: 9.1
+##
+{ 'enum': 'VfioMigrationState',
+  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
+            'pre-copy', 'pre-copy-p2p' ],
+  'prefix': 'QAPI_VFIO_MIGRATION_STATE' }
+
+##
+# @VFIO_MIGRATION:
+#
+# This event is emitted when a VFIO device migration state is changed.
+#
+# @device-id: The device's id, if it has one.
+#
+# @qom-path: The device's QOM path.
+#
+# @device-state: The new changed device migration state.
+#
+# Since: 9.1
+#
+# Example:
+#
+#     <- { "timestamp": { "seconds": 1713771323, "microseconds": 212268 },
+#          "event": "VFIO_MIGRATION",
+#          "data": {
+#              "device-id": "vfio_dev1",
+#              "qom-path": "/machine/peripheral/vfio_dev1",
+#              "device-state": "stop" } }
+##
+{ 'event': 'VFIO_MIGRATION',
+  'data': {
+      'device-id': 'str',
+      'qom-path': 'str',
+      'device-state': 'VfioMigrationState'
+  } }
diff --git a/qapi/meson.build b/qapi/meson.build
index c92af6e063..e7bc54e5d0 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -52,6 +52,7 @@ qapi_all_modules = [
   'stats',
   'trace',
   'transaction',
+  'vfio',
   'virtio',
   'yank',
 ]
-- 
2.26.3

Re: [PATCH v2 1/3] qapi/vfio: Add VFIO migration QAPI event

[Cédric Le Goater]

On 5/9/24 11:09, Avihai Horon wrote:
> Add a new QAPI event for VFIO migration. This event will be emitted when
> a VFIO device changes its migration state, for example, during migration
> or when stopping/starting the guest.
> 
> This event can be used by management applications to get updates on the
> current state of the VFIO device for their own purposes.
> 
> Note that this new event is introduced since VFIO devices have a unique
> set of migration states which cannot be described as accurately by other
> existing events such as run state or migration status.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

LGTM,

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   MAINTAINERS           |  1 +
>   qapi/qapi-schema.json |  1 +
>   qapi/vfio.json        | 67 +++++++++++++++++++++++++++++++++++++++++++
>   qapi/meson.build      |  1 +
>   4 files changed, 70 insertions(+)
>   create mode 100644 qapi/vfio.json
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 84391777db..b5f1de459e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2160,6 +2160,7 @@ F: hw/vfio/*
>   F: include/hw/vfio/
>   F: docs/igd-assign.txt
>   F: docs/devel/migration/vfio.rst
> +F: qapi/vfio.json
>   
>   vfio-ccw
>   M: Eric Farman <farman@linux.ibm.com>
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index 5e33da7228..b1581988e4 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -78,5 +78,6 @@
>   { 'include': 'pci.json' }
>   { 'include': 'stats.json' }
>   { 'include': 'virtio.json' }
> +{ 'include': 'vfio.json' }
>   { 'include': 'cryptodev.json' }
>   { 'include': 'cxl.json' }
> diff --git a/qapi/vfio.json b/qapi/vfio.json
> new file mode 100644
> index 0000000000..a0e5013188
> --- /dev/null
> +++ b/qapi/vfio.json
> @@ -0,0 +1,67 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +
> +##
> +# = VFIO devices
> +##
> +
> +##
> +# @VfioMigrationState:
> +#
> +# An enumeration of the VFIO device migration states.
> +#
> +# @stop: The device is stopped.
> +#
> +# @running: The device is running.
> +#
> +# @stop-copy: The device is stopped and its internal state is available
> +#     for reading.
> +#
> +# @resuming: The device is stopped and its internal state is available
> +#     for writing.
> +#
> +# @running-p2p: The device is running in the P2P quiescent state.
> +#
> +# @pre-copy: The device is running, tracking its internal state and its
> +#     internal state is available for reading.
> +#
> +# @pre-copy-p2p: The device is running in the P2P quiescent state,
> +#     tracking its internal state and its internal state is available
> +#     for reading.
> +#
> +# Since: 9.1
> +##
> +{ 'enum': 'VfioMigrationState',
> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
> +            'pre-copy', 'pre-copy-p2p' ],
> +  'prefix': 'QAPI_VFIO_MIGRATION_STATE' }
> +
> +##
> +# @VFIO_MIGRATION:
> +#
> +# This event is emitted when a VFIO device migration state is changed.
> +#
> +# @device-id: The device's id, if it has one.
> +#
> +# @qom-path: The device's QOM path.
> +#
> +# @device-state: The new changed device migration state.
> +#
> +# Since: 9.1
> +#
> +# Example:
> +#
> +#     <- { "timestamp": { "seconds": 1713771323, "microseconds": 212268 },
> +#          "event": "VFIO_MIGRATION",
> +#          "data": {
> +#              "device-id": "vfio_dev1",
> +#              "qom-path": "/machine/peripheral/vfio_dev1",
> +#              "device-state": "stop" } }
> +##
> +{ 'event': 'VFIO_MIGRATION',
> +  'data': {
> +      'device-id': 'str',
> +      'qom-path': 'str',
> +      'device-state': 'VfioMigrationState'
> +  } }
> diff --git a/qapi/meson.build b/qapi/meson.build
> index c92af6e063..e7bc54e5d0 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -52,6 +52,7 @@ qapi_all_modules = [
>     'stats',
>     'trace',
>     'transaction',
> +  'vfio',
>     'virtio',
>     'yank',
>   ]

[PATCH v2 2/3] vfio/migration: Emit VFIO migration QAPI event

[Avihai Horon]

Emit VFIO migration QAPI event when a VFIO device changes its migration
state. This can be used by management applications to get updates on the
current state of the VFIO device for their own purposes.

A new per VFIO device capability, "migration-events", is added so events
can be enabled only for the required devices. It is disabled by default.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 include/hw/vfio/vfio-common.h |  1 +
 hw/vfio/migration.c           | 56 +++++++++++++++++++++++++++++++++--
 hw/vfio/pci.c                 |  2 ++
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b9da6c08ef..3ec5f2425e 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -115,6 +115,7 @@ typedef struct VFIODevice {
     bool no_mmap;
     bool ram_block_discard_allowed;
     OnOffAuto enable_migration;
+    bool migration_events;
     VFIODeviceOps *ops;
     unsigned int num_irqs;
     unsigned int num_regions;
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 06ae40969b..5a359c4c78 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -24,6 +24,7 @@
 #include "migration/register.h"
 #include "migration/blocker.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-vfio.h"
 #include "exec/ramlist.h"
 #include "exec/ram_addr.h"
 #include "pci.h"
@@ -80,6 +81,55 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
     }
 }
 
+static VfioMigrationState
+mig_state_to_qapi_state(enum vfio_device_mig_state state)
+{
+    switch (state) {
+    case VFIO_DEVICE_STATE_STOP:
+        return QAPI_VFIO_MIGRATION_STATE_STOP;
+    case VFIO_DEVICE_STATE_RUNNING:
+        return QAPI_VFIO_MIGRATION_STATE_RUNNING;
+    case VFIO_DEVICE_STATE_STOP_COPY:
+        return QAPI_VFIO_MIGRATION_STATE_STOP_COPY;
+    case VFIO_DEVICE_STATE_RESUMING:
+        return QAPI_VFIO_MIGRATION_STATE_RESUMING;
+    case VFIO_DEVICE_STATE_RUNNING_P2P:
+        return QAPI_VFIO_MIGRATION_STATE_RUNNING_P2P;
+    case VFIO_DEVICE_STATE_PRE_COPY:
+        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY;
+    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
+        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY_P2P;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static void vfio_migration_send_event(VFIODevice *vbasedev)
+{
+    VFIOMigration *migration = vbasedev->migration;
+    DeviceState *dev = vbasedev->dev;
+    g_autofree char *qom_path = NULL;
+    Object *obj;
+
+    if (!vbasedev->migration_events) {
+        return;
+    }
+
+    obj = vbasedev->ops->vfio_get_object(vbasedev);
+    qom_path = object_get_canonical_path(obj);
+
+    qapi_event_send_vfio_migration(
+        dev->id, qom_path, mig_state_to_qapi_state(migration->device_state));
+}
+
+static void set_state(VFIODevice *vbasedev, enum vfio_device_mig_state state)
+{
+    VFIOMigration *migration = vbasedev->migration;
+
+    migration->device_state = state;
+    vfio_migration_send_event(vbasedev);
+}
+
 static int vfio_migration_set_state(VFIODevice *vbasedev,
                                     enum vfio_device_mig_state new_state,
                                     enum vfio_device_mig_state recover_state)
@@ -125,12 +175,12 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
             goto reset_device;
         }
 
-        migration->device_state = recover_state;
+        set_state(vbasedev, recover_state);
 
         return ret;
     }
 
-    migration->device_state = new_state;
+    set_state(vbasedev, new_state);
     if (mig_state->data_fd != -1) {
         if (migration->data_fd != -1) {
             /*
@@ -156,7 +206,7 @@ reset_device:
                  strerror(errno));
     }
 
-    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
+    set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);
 
     return ret;
 }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 64780d1b79..8840602c50 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3362,6 +3362,8 @@ static Property vfio_pci_dev_properties[] = {
                     VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
     DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
                             vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
+                     vbasedev.migration_events, false),
     DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
     DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
                      vbasedev.ram_block_discard_allowed, false),
-- 
2.26.3

Re: [PATCH v2 2/3] vfio/migration: Emit VFIO migration QAPI event

[Cédric Le Goater]

On 5/9/24 11:09, Avihai Horon wrote:
> Emit VFIO migration QAPI event when a VFIO device changes its migration
> state. This can be used by management applications to get updates on the
> current state of the VFIO device for their own purposes.
> 
> A new per VFIO device capability, "migration-events", is added so events
> can be enabled only for the required devices. It is disabled by default.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   include/hw/vfio/vfio-common.h |  1 +
>   hw/vfio/migration.c           | 56 +++++++++++++++++++++++++++++++++--
>   hw/vfio/pci.c                 |  2 ++
>   3 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef..3ec5f2425e 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -115,6 +115,7 @@ typedef struct VFIODevice {
>       bool no_mmap;
>       bool ram_block_discard_allowed;
>       OnOffAuto enable_migration;
> +    bool migration_events;
>       VFIODeviceOps *ops;
>       unsigned int num_irqs;
>       unsigned int num_regions;
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 06ae40969b..5a359c4c78 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -24,6 +24,7 @@
>   #include "migration/register.h"
>   #include "migration/blocker.h"
>   #include "qapi/error.h"
> +#include "qapi/qapi-events-vfio.h"
>   #include "exec/ramlist.h"
>   #include "exec/ram_addr.h"
>   #include "pci.h"
> @@ -80,6 +81,55 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>       }
>   }
>   
> +static VfioMigrationState
> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
> +{
> +    switch (state) {
> +    case VFIO_DEVICE_STATE_STOP:
> +        return QAPI_VFIO_MIGRATION_STATE_STOP;
> +    case VFIO_DEVICE_STATE_RUNNING:
> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING;
> +    case VFIO_DEVICE_STATE_STOP_COPY:
> +        return QAPI_VFIO_MIGRATION_STATE_STOP_COPY;
> +    case VFIO_DEVICE_STATE_RESUMING:
> +        return QAPI_VFIO_MIGRATION_STATE_RESUMING;
> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING_P2P;
> +    case VFIO_DEVICE_STATE_PRE_COPY:
> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY;
> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY_P2P;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static void vfio_migration_send_event(VFIODevice *vbasedev)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +    DeviceState *dev = vbasedev->dev;
> +    g_autofree char *qom_path = NULL;
> +    Object *obj;
> +
> +    if (!vbasedev->migration_events) {
> +        return;
> +    }

I would add an assert on vbasedev->ops->vfio_get_object

> +    obj = vbasedev->ops->vfio_get_object(vbasedev);

and another assert on obj.

> +    qom_path = object_get_canonical_path(obj);
> +
> +    qapi_event_send_vfio_migration(
> +        dev->id, qom_path, mig_state_to_qapi_state(migration->device_state));
> +}
> +
> +static void set_state(VFIODevice *vbasedev, enum vfio_device_mig_state state)

to avoid the conflict with vfio_migration_set_state(), let's call it :
vfio_migration_set_device_state() ? We want a 'vfio_migration_' prefix.


Thanks,

C.




> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +
> +    migration->device_state = state;
> +    vfio_migration_send_event(vbasedev);
> +}
> +
>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>                                       enum vfio_device_mig_state new_state,
>                                       enum vfio_device_mig_state recover_state)
> @@ -125,12 +175,12 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>               goto reset_device;
>           }
>   
> -        migration->device_state = recover_state;
> +        set_state(vbasedev, recover_state);
>   
>           return ret;
>       }
>   
> -    migration->device_state = new_state;
> +    set_state(vbasedev, new_state);
>       if (mig_state->data_fd != -1) {
>           if (migration->data_fd != -1) {
>               /*
> @@ -156,7 +206,7 @@ reset_device:
>                    strerror(errno));
>       }
>   
> -    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
> +    set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);
>   
>       return ret;
>   }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 64780d1b79..8840602c50 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3362,6 +3362,8 @@ static Property vfio_pci_dev_properties[] = {
>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>       DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>                               vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
> +                     vbasedev.migration_events, false),
>       DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
>       DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
>                        vbasedev.ram_block_discard_allowed, false),

Re: [PATCH v2 2/3] vfio/migration: Emit VFIO migration QAPI event

[Avihai Horon]

On 13/05/2024 17:01, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/9/24 11:09, Avihai Horon wrote:
>> Emit VFIO migration QAPI event when a VFIO device changes its migration
>> state. This can be used by management applications to get updates on the
>> current state of the VFIO device for their own purposes.
>>
>> A new per VFIO device capability, "migration-events", is added so events
>> can be enabled only for the required devices. It is disabled by default.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   include/hw/vfio/vfio-common.h |  1 +
>>   hw/vfio/migration.c           | 56 +++++++++++++++++++++++++++++++++--
>>   hw/vfio/pci.c                 |  2 ++
>>   3 files changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h 
>> b/include/hw/vfio/vfio-common.h
>> index b9da6c08ef..3ec5f2425e 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -115,6 +115,7 @@ typedef struct VFIODevice {
>>       bool no_mmap;
>>       bool ram_block_discard_allowed;
>>       OnOffAuto enable_migration;
>> +    bool migration_events;
>>       VFIODeviceOps *ops;
>>       unsigned int num_irqs;
>>       unsigned int num_regions;
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 06ae40969b..5a359c4c78 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -24,6 +24,7 @@
>>   #include "migration/register.h"
>>   #include "migration/blocker.h"
>>   #include "qapi/error.h"
>> +#include "qapi/qapi-events-vfio.h"
>>   #include "exec/ramlist.h"
>>   #include "exec/ram_addr.h"
>>   #include "pci.h"
>> @@ -80,6 +81,55 @@ static const char *mig_state_to_str(enum 
>> vfio_device_mig_state state)
>>       }
>>   }
>>
>> +static VfioMigrationState
>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>> +{
>> +    switch (state) {
>> +    case VFIO_DEVICE_STATE_STOP:
>> +        return QAPI_VFIO_MIGRATION_STATE_STOP;
>> +    case VFIO_DEVICE_STATE_RUNNING:
>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING;
>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>> +        return QAPI_VFIO_MIGRATION_STATE_STOP_COPY;
>> +    case VFIO_DEVICE_STATE_RESUMING:
>> +        return QAPI_VFIO_MIGRATION_STATE_RESUMING;
>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING_P2P;
>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY;
>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY_P2P;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
>> +
>> +static void vfio_migration_send_event(VFIODevice *vbasedev)
>> +{
>> +    VFIOMigration *migration = vbasedev->migration;
>> +    DeviceState *dev = vbasedev->dev;
>> +    g_autofree char *qom_path = NULL;
>> +    Object *obj;
>> +
>> +    if (!vbasedev->migration_events) {
>> +        return;
>> +    }
>
> I would add an assert on vbasedev->ops->vfio_get_object
>
>> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
>
> and another assert on obj.

vfio_migration_init() already checks these:

     if (!vbasedev->ops->vfio_get_object) {
         return -EINVAL;
     }

     obj = vbasedev->ops->vfio_get_object(vbasedev);
     if (!obj) {
         return -EINVAL;
     }

Do you think these checks in migration init are enough?

>
>> +    qom_path = object_get_canonical_path(obj);
>> +
>> +    qapi_event_send_vfio_migration(
>> +        dev->id, qom_path, 
>> mig_state_to_qapi_state(migration->device_state));
>> +}
>> +
>> +static void set_state(VFIODevice *vbasedev, enum 
>> vfio_device_mig_state state)
>
> to avoid the conflict with vfio_migration_set_state(), let's call it :
> vfio_migration_set_device_state() ? We want a 'vfio_migration_' prefix.

Sure, I will rename to that.

Thanks.

>
>
> Thanks,
>
> C.
>
>
>
>
>> +{
>> +    VFIOMigration *migration = vbasedev->migration;
>> +
>> +    migration->device_state = state;
>> +    vfio_migration_send_event(vbasedev);
>> +}
>> +
>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>                                       enum vfio_device_mig_state 
>> new_state,
>>                                       enum vfio_device_mig_state 
>> recover_state)
>> @@ -125,12 +175,12 @@ static int vfio_migration_set_state(VFIODevice 
>> *vbasedev,
>>               goto reset_device;
>>           }
>>
>> -        migration->device_state = recover_state;
>> +        set_state(vbasedev, recover_state);
>>
>>           return ret;
>>       }
>>
>> -    migration->device_state = new_state;
>> +    set_state(vbasedev, new_state);
>>       if (mig_state->data_fd != -1) {
>>           if (migration->data_fd != -1) {
>>               /*
>> @@ -156,7 +206,7 @@ reset_device:
>>                    strerror(errno));
>>       }
>>
>> -    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
>> +    set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);
>>
>>       return ret;
>>   }
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 64780d1b79..8840602c50 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3362,6 +3362,8 @@ static Property vfio_pci_dev_properties[] = {
>>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>>       DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>>                               vbasedev.enable_migration, 
>> ON_OFF_AUTO_AUTO),
>> +    DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>> +                     vbasedev.migration_events, false),
>>       DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, 
>> false),
>>       DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
>>                        vbasedev.ram_block_discard_allowed, false),
>

Re: [PATCH v2 2/3] vfio/migration: Emit VFIO migration QAPI event

[Cédric Le Goater]

On 5/13/24 16:34, Avihai Horon wrote:
> 
> On 13/05/2024 17:01, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 5/9/24 11:09, Avihai Horon wrote:
>>> Emit VFIO migration QAPI event when a VFIO device changes its migration
>>> state. This can be used by management applications to get updates on the
>>> current state of the VFIO device for their own purposes.
>>>
>>> A new per VFIO device capability, "migration-events", is added so events
>>> can be enabled only for the required devices. It is disabled by default.
>>>
>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>> ---
>>>   include/hw/vfio/vfio-common.h |  1 +
>>>   hw/vfio/migration.c           | 56 +++++++++++++++++++++++++++++++++--
>>>   hw/vfio/pci.c                 |  2 ++
>>>   3 files changed, 56 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index b9da6c08ef..3ec5f2425e 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -115,6 +115,7 @@ typedef struct VFIODevice {
>>>       bool no_mmap;
>>>       bool ram_block_discard_allowed;
>>>       OnOffAuto enable_migration;
>>> +    bool migration_events;
>>>       VFIODeviceOps *ops;
>>>       unsigned int num_irqs;
>>>       unsigned int num_regions;
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 06ae40969b..5a359c4c78 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -24,6 +24,7 @@
>>>   #include "migration/register.h"
>>>   #include "migration/blocker.h"
>>>   #include "qapi/error.h"
>>> +#include "qapi/qapi-events-vfio.h"
>>>   #include "exec/ramlist.h"
>>>   #include "exec/ram_addr.h"
>>>   #include "pci.h"
>>> @@ -80,6 +81,55 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>>>       }
>>>   }
>>>
>>> +static VfioMigrationState
>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>> +{
>>> +    switch (state) {
>>> +    case VFIO_DEVICE_STATE_STOP:
>>> +        return QAPI_VFIO_MIGRATION_STATE_STOP;
>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING;
>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>> +        return QAPI_VFIO_MIGRATION_STATE_STOP_COPY;
>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>> +        return QAPI_VFIO_MIGRATION_STATE_RESUMING;
>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING_P2P;
>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY;
>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY_P2P;
>>> +    default:
>>> +        g_assert_not_reached();
>>> +    }
>>> +}
>>> +
>>> +static void vfio_migration_send_event(VFIODevice *vbasedev)
>>> +{
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +    DeviceState *dev = vbasedev->dev;
>>> +    g_autofree char *qom_path = NULL;
>>> +    Object *obj;
>>> +
>>> +    if (!vbasedev->migration_events) {
>>> +        return;
>>> +    }
>>
>> I would add an assert on vbasedev->ops->vfio_get_object
>>
>>> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
>>
>> and another assert on obj.
> 
> vfio_migration_init() already checks these:
> 
>      if (!vbasedev->ops->vfio_get_object) {
>          return -EINVAL;
>      }
> 
>      obj = vbasedev->ops->vfio_get_object(vbasedev);
>      if (!obj) {
>          return -EINVAL;
>      }
> 
> Do you think these checks in migration init are enough?

I am sure they are today. These extra asserts are to avoid issues if
the code is moved around or if anyone finds inspiration by reading
vfio_migration_send_event().

Thanks,

C.



> 
>>
>>> +    qom_path = object_get_canonical_path(obj);
>>> +
>>> +    qapi_event_send_vfio_migration(
>>> +        dev->id, qom_path, mig_state_to_qapi_state(migration->device_state));
>>> +}
>>> +
>>> +static void set_state(VFIODevice *vbasedev, enum vfio_device_mig_state state)
>>
>> to avoid the conflict with vfio_migration_set_state(), let's call it :
>> vfio_migration_set_device_state() ? We want a 'vfio_migration_' prefix.
> 
> Sure, I will rename to that.
> 
> Thanks.
> 
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>
>>> +{
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +
>>> +    migration->device_state = state;
>>> +    vfio_migration_send_event(vbasedev);
>>> +}
>>> +
>>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>                                       enum vfio_device_mig_state new_state,
>>>                                       enum vfio_device_mig_state recover_state)
>>> @@ -125,12 +175,12 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>               goto reset_device;
>>>           }
>>>
>>> -        migration->device_state = recover_state;
>>> +        set_state(vbasedev, recover_state);
>>>
>>>           return ret;
>>>       }
>>>
>>> -    migration->device_state = new_state;
>>> +    set_state(vbasedev, new_state);
>>>       if (mig_state->data_fd != -1) {
>>>           if (migration->data_fd != -1) {
>>>               /*
>>> @@ -156,7 +206,7 @@ reset_device:
>>>                    strerror(errno));
>>>       }
>>>
>>> -    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
>>> +    set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);
>>>
>>>       return ret;
>>>   }
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 64780d1b79..8840602c50 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3362,6 +3362,8 @@ static Property vfio_pci_dev_properties[] = {
>>>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>>>       DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>>>                               vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>>> +    DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>>> +                     vbasedev.migration_events, false),
>>>       DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
>>>       DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
>>>                        vbasedev.ram_block_discard_allowed, false),
>>
> 

Re: [PATCH v2 2/3] vfio/migration: Emit VFIO migration QAPI event

[Avihai Horon]

On 13/05/2024 17:43, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/13/24 16:34, Avihai Horon wrote:
>>
>> On 13/05/2024 17:01, Cédric Le Goater wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 5/9/24 11:09, Avihai Horon wrote:
>>>> Emit VFIO migration QAPI event when a VFIO device changes its 
>>>> migration
>>>> state. This can be used by management applications to get updates 
>>>> on the
>>>> current state of the VFIO device for their own purposes.
>>>>
>>>> A new per VFIO device capability, "migration-events", is added so 
>>>> events
>>>> can be enabled only for the required devices. It is disabled by 
>>>> default.
>>>>
>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>>> ---
>>>>   include/hw/vfio/vfio-common.h |  1 +
>>>>   hw/vfio/migration.c           | 56 
>>>> +++++++++++++++++++++++++++++++++--
>>>>   hw/vfio/pci.c                 |  2 ++
>>>>   3 files changed, 56 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h 
>>>> b/include/hw/vfio/vfio-common.h
>>>> index b9da6c08ef..3ec5f2425e 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -115,6 +115,7 @@ typedef struct VFIODevice {
>>>>       bool no_mmap;
>>>>       bool ram_block_discard_allowed;
>>>>       OnOffAuto enable_migration;
>>>> +    bool migration_events;
>>>>       VFIODeviceOps *ops;
>>>>       unsigned int num_irqs;
>>>>       unsigned int num_regions;
>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>> index 06ae40969b..5a359c4c78 100644
>>>> --- a/hw/vfio/migration.c
>>>> +++ b/hw/vfio/migration.c
>>>> @@ -24,6 +24,7 @@
>>>>   #include "migration/register.h"
>>>>   #include "migration/blocker.h"
>>>>   #include "qapi/error.h"
>>>> +#include "qapi/qapi-events-vfio.h"
>>>>   #include "exec/ramlist.h"
>>>>   #include "exec/ram_addr.h"
>>>>   #include "pci.h"
>>>> @@ -80,6 +81,55 @@ static const char *mig_state_to_str(enum 
>>>> vfio_device_mig_state state)
>>>>       }
>>>>   }
>>>>
>>>> +static VfioMigrationState
>>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>>> +{
>>>> +    switch (state) {
>>>> +    case VFIO_DEVICE_STATE_STOP:
>>>> +        return QAPI_VFIO_MIGRATION_STATE_STOP;
>>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING;
>>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>>> +        return QAPI_VFIO_MIGRATION_STATE_STOP_COPY;
>>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>>> +        return QAPI_VFIO_MIGRATION_STATE_RESUMING;
>>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>>> +        return QAPI_VFIO_MIGRATION_STATE_RUNNING_P2P;
>>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY;
>>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>>> +        return QAPI_VFIO_MIGRATION_STATE_PRE_COPY_P2P;
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>> +    }
>>>> +}
>>>> +
>>>> +static void vfio_migration_send_event(VFIODevice *vbasedev)
>>>> +{
>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>> +    DeviceState *dev = vbasedev->dev;
>>>> +    g_autofree char *qom_path = NULL;
>>>> +    Object *obj;
>>>> +
>>>> +    if (!vbasedev->migration_events) {
>>>> +        return;
>>>> +    }
>>>
>>> I would add an assert on vbasedev->ops->vfio_get_object
>>>
>>>> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
>>>
>>> and another assert on obj.
>>
>> vfio_migration_init() already checks these:
>>
>>      if (!vbasedev->ops->vfio_get_object) {
>>          return -EINVAL;
>>      }
>>
>>      obj = vbasedev->ops->vfio_get_object(vbasedev);
>>      if (!obj) {
>>          return -EINVAL;
>>      }
>>
>> Do you think these checks in migration init are enough?
>
> I am sure they are today. These extra asserts are to avoid issues if
> the code is moved around or if anyone finds inspiration by reading
> vfio_migration_send_event().
>
Ah, I see your point.

I will add the asserts then.

Thanks.

> Thanks,
>
> C.
>
>
>
>>
>>>
>>>> +    qom_path = object_get_canonical_path(obj);
>>>> +
>>>> +    qapi_event_send_vfio_migration(
>>>> +        dev->id, qom_path, 
>>>> mig_state_to_qapi_state(migration->device_state));
>>>> +}
>>>> +
>>>> +static void set_state(VFIODevice *vbasedev, enum 
>>>> vfio_device_mig_state state)
>>>
>>> to avoid the conflict with vfio_migration_set_state(), let's call it :
>>> vfio_migration_set_device_state() ? We want a 'vfio_migration_' prefix.
>>
>> Sure, I will rename to that.
>>
>> Thanks.
>>
>>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>>
>>>
>>>> +{
>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>> +
>>>> +    migration->device_state = state;
>>>> +    vfio_migration_send_event(vbasedev);
>>>> +}
>>>> +
>>>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>>                                       enum vfio_device_mig_state 
>>>> new_state,
>>>>                                       enum vfio_device_mig_state 
>>>> recover_state)
>>>> @@ -125,12 +175,12 @@ static int 
>>>> vfio_migration_set_state(VFIODevice *vbasedev,
>>>>               goto reset_device;
>>>>           }
>>>>
>>>> -        migration->device_state = recover_state;
>>>> +        set_state(vbasedev, recover_state);
>>>>
>>>>           return ret;
>>>>       }
>>>>
>>>> -    migration->device_state = new_state;
>>>> +    set_state(vbasedev, new_state);
>>>>       if (mig_state->data_fd != -1) {
>>>>           if (migration->data_fd != -1) {
>>>>               /*
>>>> @@ -156,7 +206,7 @@ reset_device:
>>>>                    strerror(errno));
>>>>       }
>>>>
>>>> -    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
>>>> +    set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);
>>>>
>>>>       return ret;
>>>>   }
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index 64780d1b79..8840602c50 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -3362,6 +3362,8 @@ static Property vfio_pci_dev_properties[] = {
>>>>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>>>>       DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>>>>                               vbasedev.enable_migration, 
>>>> ON_OFF_AUTO_AUTO),
>>>> +    DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>>>> +                     vbasedev.migration_events, false),
>>>>       DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, 
>>>> vbasedev.no_mmap, false),
>>>>       DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
>>>>                        vbasedev.ram_block_discard_allowed, false),
>>>
>>
>

[PATCH v2 3/3] vfio/migration: Don't emit STOP_COPY VFIO migration QAPI event twice

[Avihai Horon]

When migrating a VFIO device that supports pre-copy, it is transitioned
to STOP_COPY twice: once in vfio_vmstate_change() and second time in
vfio_save_complete_precopy().

The second transition is harmless, as it's a STOP_COPY->STOP_COPY no-op
transition. However, with the newly added VFIO migration QAPI event, the
STOP_COPY event is undesirably emitted twice.

Prevent this by returning early in vfio_migration_set_state() if
new_state is the same as current device state.

Note that the STOP_COPY transition in vfio_save_complete_precopy() is
essential for VFIO devices that don't support pre-copy, for migrating an
already stopped guest and for snapshots.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 hw/vfio/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 5a359c4c78..14ef9c924e 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -143,6 +143,10 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
         (struct vfio_device_feature_mig_state *)feature->data;
     int ret;
 
+    if (new_state == migration->device_state) {
+        return 0;
+    }
+
     feature->argsz = sizeof(buf);
     feature->flags =
         VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE;
-- 
2.26.3

Re: [PATCH v2 3/3] vfio/migration: Don't emit STOP_COPY VFIO migration QAPI event twice

[Cédric Le Goater]

On 5/9/24 11:09, Avihai Horon wrote:
> When migrating a VFIO device that supports pre-copy, it is transitioned
> to STOP_COPY twice: once in vfio_vmstate_change() and second time in
> vfio_save_complete_precopy().
> 
> The second transition is harmless, as it's a STOP_COPY->STOP_COPY no-op
> transition. However, with the newly added VFIO migration QAPI event, the
> STOP_COPY event is undesirably emitted twice.
> 
> Prevent this by returning early in vfio_migration_set_state() if
> new_state is the same as current device state.
> 
> Note that the STOP_COPY transition in vfio_save_complete_precopy() is
> essential for VFIO devices that don't support pre-copy, for migrating an
> already stopped guest and for snapshots.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   hw/vfio/migration.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 5a359c4c78..14ef9c924e 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -143,6 +143,10 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>           (struct vfio_device_feature_mig_state *)feature->data;
>       int ret;
>   

I wonder if we should improve the trace events a little to track better
the state transitions. May be move trace_vfio_migration_set_state()
at the beginning of vfio_migration_set_state() and introduce a new
event for the currently named routine set_state() ?

This can come with followups.


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> +    if (new_state == migration->device_state) {
> +        return 0;
> +    }
> +
>       feature->argsz = sizeof(buf);
>       feature->flags =
>           VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE;

Re: [PATCH v2 3/3] vfio/migration: Don't emit STOP_COPY VFIO migration QAPI event twice

[Avihai Horon]

On 13/05/2024 17:13, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/9/24 11:09, Avihai Horon wrote:
>> When migrating a VFIO device that supports pre-copy, it is transitioned
>> to STOP_COPY twice: once in vfio_vmstate_change() and second time in
>> vfio_save_complete_precopy().
>>
>> The second transition is harmless, as it's a STOP_COPY->STOP_COPY no-op
>> transition. However, with the newly added VFIO migration QAPI event, the
>> STOP_COPY event is undesirably emitted twice.
>>
>> Prevent this by returning early in vfio_migration_set_state() if
>> new_state is the same as current device state.
>>
>> Note that the STOP_COPY transition in vfio_save_complete_precopy() is
>> essential for VFIO devices that don't support pre-copy, for migrating an
>> already stopped guest and for snapshots.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   hw/vfio/migration.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 5a359c4c78..14ef9c924e 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -143,6 +143,10 @@ static int vfio_migration_set_state(VFIODevice 
>> *vbasedev,
>>           (struct vfio_device_feature_mig_state *)feature->data;
>>       int ret;
>>
>
> I wonder if we should improve the trace events a little to track better
> the state transitions. May be move trace_vfio_migration_set_state()
> at the beginning of vfio_migration_set_state() and introduce a new
> event for the currently named routine set_state() ?
>
> This can come with followups.
>
Yes, this sounds good.

Thanks.

>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>
> Thanks,
>
> C.
>
>
>> +    if (new_state == migration->device_state) {
>> +        return 0;
>> +    }
>> +
>>       feature->argsz = sizeof(buf);
>>       feature->flags =
>>           VFIO_DEVICE_FEATURE_SET | 
>> VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE;
>