[PATCH 0/3] qapi/vfio: Add VFIO device migration state change QAPI event

[PATCH 0/3] qapi/vfio: Add VFIO device migration state change 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.

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

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

-- 
2.26.3

[PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Avihai Horon]

Add a new QAPI event for VFIO device migration state change. 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.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 302b6fd00c..ef58a39d36 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2159,6 +2159,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..a38f26bccd
--- /dev/null
+++ b/qapi/vfio.json
@@ -0,0 +1,61 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+
+##
+# = VFIO devices
+##
+
+##
+# @VFIODeviceMigState:
+#
+# 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': 'VFIODeviceMigState',
+  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
+            'pre-copy', 'pre-copy-p2p' ],
+  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
+
+##
+# @VFIO_DEVICE_MIG_STATE_CHANGED:
+#
+# This event is emitted when a VFIO device migration state is changed.
+#
+# @device-id: The id of the VFIO device (final component of QOM path).
+#
+# @device-state: The new changed device migration state.
+#
+# Since: 9.1
+#
+# Example:
+#
+#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
+#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
+#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
+##
+{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
+  'data': {
+      'device-id': 'str',
+      'device-state': 'VFIODeviceMigState'
+  } }
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

[PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Avihai Horon]

Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
 hw/vfio/pci.c                 |  2 ++
 3 files changed, 47 insertions(+)

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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
     }
 }
 
+static VFIODeviceMigState
+mig_state_to_qapi_state(enum vfio_device_mig_state state)
+{
+    switch (state) {
+    case VFIO_DEVICE_STATE_STOP:
+        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
+    case VFIO_DEVICE_STATE_RUNNING:
+        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
+    case VFIO_DEVICE_STATE_STOP_COPY:
+        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
+    case VFIO_DEVICE_STATE_RESUMING:
+        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
+    case VFIO_DEVICE_STATE_RUNNING_P2P:
+        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
+    case VFIO_DEVICE_STATE_PRE_COPY:
+        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
+    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
+        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
+    default:
+        g_assert_not_reached();
+    }
+}
+
+static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
+{
+    VFIOMigration *migration = vbasedev->migration;
+    const char *id;
+    Object *obj;
+
+    if (!vbasedev->migration_events) {
+        return;
+    }
+
+    obj = vbasedev->ops->vfio_get_object(vbasedev);
+    id = object_get_canonical_path_component(obj);
+
+    qapi_event_send_vfio_device_mig_state_changed(
+        id, mig_state_to_qapi_state(migration->device_state));
+}
+
 static int vfio_migration_set_state(VFIODevice *vbasedev,
                                     enum vfio_device_mig_state new_state,
                                     enum vfio_device_mig_state recover_state)
@@ -126,11 +167,13 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
         }
 
         migration->device_state = recover_state;
+        vfio_migration_send_state_change_event(vbasedev);
 
         return ret;
     }
 
     migration->device_state = new_state;
+    vfio_migration_send_state_change_event(vbasedev);
     if (mig_state->data_fd != -1) {
         if (migration->data_fd != -1) {
             /*
@@ -157,6 +200,7 @@ reset_device:
     }
 
     migration->device_state = VFIO_DEVICE_STATE_RUNNING;
+    vfio_migration_send_state_change_event(vbasedev);
 
     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

[PATCH 3/3] vfio/migration: Don't emit STOP_COPY state change 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 migration state change QAPI
event, the STOP_COPY state change event is undesirably emitted twice.

Prevent this by conditionally transitioning to STOP_COPY state in
vfio_save_complete_precopy().

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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 6bbccf6545..30a2b2ea74 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -591,14 +591,17 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque)
 static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
 {
     VFIODevice *vbasedev = opaque;
+    VFIOMigration *migration = vbasedev->migration;
     ssize_t data_size;
     int ret;
 
     /* We reach here with device state STOP or STOP_COPY only */
-    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
-                                   VFIO_DEVICE_STATE_STOP);
-    if (ret) {
-        return ret;
+    if (migration->device_state == VFIO_DEVICE_STATE_STOP) {
+        ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
+                                       VFIO_DEVICE_STATE_STOP);
+        if (ret) {
+            return ret;
+        }
     }
 
     do {
-- 
2.26.3

Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Joao Martins]

On 30/04/2024 06:16, Avihai Horon wrote:
> Add a new QAPI event for VFIO device migration state change. 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.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>  MAINTAINERS           |  1 +
>  qapi/qapi-schema.json |  1 +
>  qapi/vfio.json        | 61 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/meson.build      |  1 +
>  4 files changed, 64 insertions(+)
>  create mode 100644 qapi/vfio.json
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 302b6fd00c..ef58a39d36 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2159,6 +2159,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..a38f26bccd
> --- /dev/null
> +++ b/qapi/vfio.json
> @@ -0,0 +1,61 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +
> +##
> +# = VFIO devices
> +##
> +
> +##
> +# @VFIODeviceMigState:
> +#
> +# 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': 'VFIODeviceMigState',
> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
> +            'pre-copy', 'pre-copy-p2p' ],
> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
> +

Considering MIG can also be interpreted as Multi Instance GPU elsewhere
unrelated to this shouldn't we be explicit here? i.e.

	VFIO_DEVICE_MIGRATION_STATE

... to avoid ambiguiosity.

> +##
> +# @VFIO_DEVICE_MIG_STATE_CHANGED:
> +#
> +# This event is emitted when a VFIO device migration state is changed.
> +#
> +# @device-id: The id of the VFIO device (final component of QOM path).
> +#
> +# @device-state: The new changed device migration state.
> +#
> +# Since: 9.1
> +#
> +# Example:
> +#
> +#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
> +#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
> +#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
> +##
> +{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
> +  'data': {
> +      'device-id': 'str',
> +      'device-state': 'VFIODeviceMigState'
> +  } }
> 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',
>  ]

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Joao Martins]

On 30/04/2024 06:16, Avihai Horon wrote:
> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>  hw/vfio/pci.c                 |  2 ++
>  3 files changed, 47 insertions(+)
> 
> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>      }
>  }
>  
> +static VFIODeviceMigState
> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
> +{
> +    switch (state) {
> +    case VFIO_DEVICE_STATE_STOP:
> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
> +    case VFIO_DEVICE_STATE_RUNNING:
> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
> +    case VFIO_DEVICE_STATE_STOP_COPY:
> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
> +    case VFIO_DEVICE_STATE_RESUMING:
> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
> +    case VFIO_DEVICE_STATE_PRE_COPY:
> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +    const char *id;
> +    Object *obj;
> +
> +    if (!vbasedev->migration_events) {
> +        return;
> +    }
> +

Shouldn't this leap frog migrate_events() capability instead of introducing its
vfio equivalent i.e.

	if (!migrate_events()) {
	    return;
	}

?

Applications that don't understand the event string (migration related or not)
will just discard it (AIUI)

> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
> +    id = object_get_canonical_path_component(obj);
> +
> +    qapi_event_send_vfio_device_mig_state_changed(
> +        id, mig_state_to_qapi_state(migration->device_state));
> +}
> +
>  static int vfio_migration_set_state(VFIODevice *vbasedev,
>                                      enum vfio_device_mig_state new_state,
>                                      enum vfio_device_mig_state recover_state)
> @@ -126,11 +167,13 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>          }
>  
>          migration->device_state = recover_state;
> +        vfio_migration_send_state_change_event(vbasedev);
>  
>          return ret;
>      }
>  
>      migration->device_state = new_state;
> +    vfio_migration_send_state_change_event(vbasedev);
>      if (mig_state->data_fd != -1) {
>          if (migration->data_fd != -1) {
>              /*
> @@ -157,6 +200,7 @@ reset_device:
>      }
>  
>      migration->device_state = VFIO_DEVICE_STATE_RUNNING;
> +    vfio_migration_send_state_change_event(vbasedev);
>  
>      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 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Avihai Horon]

On 01/05/2024 14:50, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> On 30/04/2024 06:16, Avihai Horon wrote:
>> Add a new QAPI event for VFIO device migration state change. 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.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   MAINTAINERS           |  1 +
>>   qapi/qapi-schema.json |  1 +
>>   qapi/vfio.json        | 61 +++++++++++++++++++++++++++++++++++++++++++
>>   qapi/meson.build      |  1 +
>>   4 files changed, 64 insertions(+)
>>   create mode 100644 qapi/vfio.json
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 302b6fd00c..ef58a39d36 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2159,6 +2159,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..a38f26bccd
>> --- /dev/null
>> +++ b/qapi/vfio.json
>> @@ -0,0 +1,61 @@
>> +# -*- Mode: Python -*-
>> +# vim: filetype=python
>> +#
>> +
>> +##
>> +# = VFIO devices
>> +##
>> +
>> +##
>> +# @VFIODeviceMigState:
>> +#
>> +# 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': 'VFIODeviceMigState',
>> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
>> +            'pre-copy', 'pre-copy-p2p' ],
>> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
>> +
> Considering MIG can also be interpreted as Multi Instance GPU elsewhere
> unrelated to this shouldn't we be explicit here? i.e.
>
>          VFIO_DEVICE_MIGRATION_STATE
>
> ... to avoid ambiguiosity.

I used mig to avoid long names, but I don't mind changing it to 
migration if that's clearer.

Thanks.

>
>> +##
>> +# @VFIO_DEVICE_MIG_STATE_CHANGED:
>> +#
>> +# This event is emitted when a VFIO device migration state is changed.
>> +#
>> +# @device-id: The id of the VFIO device (final component of QOM path).
>> +#
>> +# @device-state: The new changed device migration state.
>> +#
>> +# Since: 9.1
>> +#
>> +# Example:
>> +#
>> +#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
>> +#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
>> +#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
>> +##
>> +{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
>> +  'data': {
>> +      'device-id': 'str',
>> +      'device-state': 'VFIODeviceMigState'
>> +  } }
>> 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',
>>   ]

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Avihai Horon]

On 01/05/2024 14:50, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> On 30/04/2024 06:16, Avihai Horon wrote:
>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>>   hw/vfio/pci.c                 |  2 ++
>>   3 files changed, 47 insertions(+)
>>
>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum vfio_device_mig_state state)
>>       }
>>   }
>>
>> +static VFIODeviceMigState
>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>> +{
>> +    switch (state) {
>> +    case VFIO_DEVICE_STATE_STOP:
>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
>> +    case VFIO_DEVICE_STATE_RUNNING:
>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
>> +    case VFIO_DEVICE_STATE_RESUMING:
>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
>> +
>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
>> +{
>> +    VFIOMigration *migration = vbasedev->migration;
>> +    const char *id;
>> +    Object *obj;
>> +
>> +    if (!vbasedev->migration_events) {
>> +        return;
>> +    }
>> +
> Shouldn't this leap frog migrate_events() capability instead of introducing its
> vfio equivalent i.e.
>
>          if (!migrate_events()) {
>              return;
>          }
>
> ?

I used a per VFIO device cap so the events can be fine tuned for each 
device (maybe one device should send events while the other not).
This gives the most flexibility and I don't think it complicates the 
configuration (one downside, though, is that it can't be 
enabled/disabled dynamically during runtime).

I don't think events add much overhead, so if you prefer a global cap, I 
can change it.
However, I'm not sure overloading the existing migrate_events() is valid?

>
> Applications that don't understand the event string (migration related or not)
> will just discard it (AIUI)
>
>> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
>> +    id = object_get_canonical_path_component(obj);
>> +
>> +    qapi_event_send_vfio_device_mig_state_changed(
>> +        id, mig_state_to_qapi_state(migration->device_state));
>> +}
>> +
>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>                                       enum vfio_device_mig_state new_state,
>>                                       enum vfio_device_mig_state recover_state)
>> @@ -126,11 +167,13 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>           }
>>
>>           migration->device_state = recover_state;
>> +        vfio_migration_send_state_change_event(vbasedev);
>>
>>           return ret;
>>       }
>>
>>       migration->device_state = new_state;
>> +    vfio_migration_send_state_change_event(vbasedev);
>>       if (mig_state->data_fd != -1) {
>>           if (migration->data_fd != -1) {
>>               /*
>> @@ -157,6 +200,7 @@ reset_device:
>>       }
>>
>>       migration->device_state = VFIO_DEVICE_STATE_RUNNING;
>> +    vfio_migration_send_state_change_event(vbasedev);
>>
>>       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 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Joao Martins]

On 01/05/2024 13:28, Avihai Horon wrote:
> 
> On 01/05/2024 14:50, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 30/04/2024 06:16, Avihai Horon wrote:
>>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>>>   hw/vfio/pci.c                 |  2 ++
>>>   3 files changed, 47 insertions(+)
>>>
>>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum
>>> vfio_device_mig_state state)
>>>       }
>>>   }
>>>
>>> +static VFIODeviceMigState
>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>> +{
>>> +    switch (state) {
>>> +    case VFIO_DEVICE_STATE_STOP:
>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
>>> +    default:
>>> +        g_assert_not_reached();
>>> +    }
>>> +}
>>> +
>>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
>>> +{
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +    const char *id;
>>> +    Object *obj;
>>> +
>>> +    if (!vbasedev->migration_events) {
>>> +        return;
>>> +    }
>>> +
>> Shouldn't this leap frog migrate_events() capability instead of introducing its
>> vfio equivalent i.e.
>>
>>          if (!migrate_events()) {
>>              return;
>>          }
>>
>> ?
> 
> I used a per VFIO device cap so the events can be fine tuned for each device
> (maybe one device should send events while the other not).
> This gives the most flexibility and I don't think it complicates the
> configuration (one downside, though, is that it can't be enabled/disabled
> dynamically during runtime).
> 
Right.

> I don't think events add much overhead, so if you prefer a global cap, I can
> change it.
> However, I'm not sure overloading the existing migrate_events() is valid?
> 

migration 'events' capability just means we will have some migration events
emited via QAPI monitor for: 1) general global status and 2) for each migration
pass (both with different event names=. So the suggestion was just what feels a
natural extension of that (...)

>>
>> Applications that don't understand the event string (migration related or not)
>> will just discard it (AIUI)
>>

(...) specially because of this as all these events have a different name.

But overloading might not make sense for others IDK ... it was just a suggestion
:) not a strong preference

>>> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
>>> +    id = object_get_canonical_path_component(obj);
>>> +
>>> +    qapi_event_send_vfio_device_mig_state_changed(
>>> +        id, mig_state_to_qapi_state(migration->device_state));
>>> +}
>>> +
>>>   static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>                                       enum vfio_device_mig_state new_state,
>>>                                       enum vfio_device_mig_state recover_state)
>>> @@ -126,11 +167,13 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>           }
>>>
>>>           migration->device_state = recover_state;
>>> +        vfio_migration_send_state_change_event(vbasedev);
>>>
>>>           return ret;
>>>       }
>>>
>>>       migration->device_state = new_state;
>>> +    vfio_migration_send_state_change_event(vbasedev);
>>>       if (mig_state->data_fd != -1) {
>>>           if (migration->data_fd != -1) {
>>>               /*
>>> @@ -157,6 +200,7 @@ reset_device:
>>>       }
>>>
>>>       migration->device_state = VFIO_DEVICE_STATE_RUNNING;
>>> +    vfio_migration_send_state_change_event(vbasedev);
>>>
>>>       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 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Markus Armbruster]

Avihai Horon <avihaih@nvidia.com> writes:

> Add a new QAPI event for VFIO device migration state change. 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.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

Can you explain briefly why this event makes sense only for VFIO devices?

> ---
>  MAINTAINERS           |  1 +
>  qapi/qapi-schema.json |  1 +
>  qapi/vfio.json        | 61 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/meson.build      |  1 +
>  4 files changed, 64 insertions(+)
>  create mode 100644 qapi/vfio.json
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 302b6fd00c..ef58a39d36 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2159,6 +2159,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..a38f26bccd
> --- /dev/null
> +++ b/qapi/vfio.json
> @@ -0,0 +1,61 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +
> +##
> +# = VFIO devices
> +##
> +
> +##
> +# @VFIODeviceMigState:
> +#
> +# 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': 'VFIODeviceMigState',
> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
> +            'pre-copy', 'pre-copy-p2p' ],
> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
> +
> +##
> +# @VFIO_DEVICE_MIG_STATE_CHANGED:
> +#
> +# This event is emitted when a VFIO device migration state is changed.
> +#
> +# @device-id: The id of the VFIO device (final component of QOM path).

Provide the full QOM path, please.  Feel free to additionally provide
its qdev ID.

Precedence: events MEMORY_DEVICE_SIZE_CHANGE, DEVICE_DELETED,
DEVICE_UNPLUG_GUEST_ERROR, ...

> +#
> +# @device-state: The new changed device migration state.
> +#
> +# Since: 9.1
> +#
> +# Example:
> +#
> +#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
> +#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
> +#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
> +##
> +{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
> +  'data': {
> +      'device-id': 'str',
> +      'device-state': 'VFIODeviceMigState'
> +  } }
> 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',
>  ]

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Avihai Horon]

On 02/05/2024 13:22, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> On 01/05/2024 13:28, Avihai Horon wrote:
>> On 01/05/2024 14:50, Joao Martins wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>>>>    hw/vfio/pci.c                 |  2 ++
>>>>    3 files changed, 47 insertions(+)
>>>>
>>>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum
>>>> vfio_device_mig_state state)
>>>>        }
>>>>    }
>>>>
>>>> +static VFIODeviceMigState
>>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>>> +{
>>>> +    switch (state) {
>>>> +    case VFIO_DEVICE_STATE_STOP:
>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
>>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
>>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
>>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
>>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
>>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
>>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>> +    }
>>>> +}
>>>> +
>>>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
>>>> +{
>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>> +    const char *id;
>>>> +    Object *obj;
>>>> +
>>>> +    if (!vbasedev->migration_events) {
>>>> +        return;
>>>> +    }
>>>> +
>>> Shouldn't this leap frog migrate_events() capability instead of introducing its
>>> vfio equivalent i.e.
>>>
>>>           if (!migrate_events()) {
>>>               return;
>>>           }
>>>
>>> ?
>> I used a per VFIO device cap so the events can be fine tuned for each device
>> (maybe one device should send events while the other not).
>> This gives the most flexibility and I don't think it complicates the
>> configuration (one downside, though, is that it can't be enabled/disabled
>> dynamically during runtime).
>>
> Right.
>
>> I don't think events add much overhead, so if you prefer a global cap, I can
>> change it.
>> However, I'm not sure overloading the existing migrate_events() is valid?
>>
> migration 'events' capability just means we will have some migration events
> emited via QAPI monitor for: 1) general global status and 2) for each migration
> pass (both with different event names=.

Yes, it's already overloaded.

In migration QAPI it says: "@events: generate events for each migration 
state change (since 2.4)".
This only refers to the MIGRATION event AFAIU.

Later on (in QEMU 2.6), MIGRATION_PASS event was added and the events 
cap was overloaded for the first time (without changing @events 
description).

Now we want to add yet another use for events capability, the VFIO 
migration state change events.

I think what bothers me is the @events description, which is not accurate.
Maybe it should be changed to "@events: generate migration related 
events (since 2.4)"? However, I'm not sure if it's OK to do this.

>   So the suggestion was just what feels a
> natural extension of that (...)
>
>>> Applications that don't understand the event string (migration related or not)
>>> will just discard it (AIUI)
>>>
> (...) specially because of this as all these events have a different name.
>
> But overloading might not make sense for others IDK ... it was just a suggestion
> :) not a strong preference

Yes, I get your rationale.
I don't have a strong opinion either, so maybe let's see what other 
people think.

Thanks.

>>>> +    obj = vbasedev->ops->vfio_get_object(vbasedev);
>>>> +    id = object_get_canonical_path_component(obj);
>>>> +
>>>> +    qapi_event_send_vfio_device_mig_state_changed(
>>>> +        id, mig_state_to_qapi_state(migration->device_state));
>>>> +}
>>>> +
>>>>    static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>>                                        enum vfio_device_mig_state new_state,
>>>>                                        enum vfio_device_mig_state recover_state)
>>>> @@ -126,11 +167,13 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>>>>            }
>>>>
>>>>            migration->device_state = recover_state;
>>>> +        vfio_migration_send_state_change_event(vbasedev);
>>>>
>>>>            return ret;
>>>>        }
>>>>
>>>>        migration->device_state = new_state;
>>>> +    vfio_migration_send_state_change_event(vbasedev);
>>>>        if (mig_state->data_fd != -1) {
>>>>            if (migration->data_fd != -1) {
>>>>                /*
>>>> @@ -157,6 +200,7 @@ reset_device:
>>>>        }
>>>>
>>>>        migration->device_state = VFIO_DEVICE_STATE_RUNNING;
>>>> +    vfio_migration_send_state_change_event(vbasedev);
>>>>
>>>>        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 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Avihai Horon]

On 02/05/2024 14:19, Markus Armbruster wrote:
> External email: Use caution opening links or attachments
>
>
> Avihai Horon <avihaih@nvidia.com> writes:
>
>> Add a new QAPI event for VFIO device migration state change. 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.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Can you explain briefly why this event makes sense only for VFIO devices?

VFIO devices have their own protocol for migration and a unique set of 
migration states.
This event holds info about these VFIO migration states, which I think 
cannot be described in the same accuracy by other events such as run 
state or migration states.

>
>> ---
>>   MAINTAINERS           |  1 +
>>   qapi/qapi-schema.json |  1 +
>>   qapi/vfio.json        | 61 +++++++++++++++++++++++++++++++++++++++++++
>>   qapi/meson.build      |  1 +
>>   4 files changed, 64 insertions(+)
>>   create mode 100644 qapi/vfio.json
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 302b6fd00c..ef58a39d36 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2159,6 +2159,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..a38f26bccd
>> --- /dev/null
>> +++ b/qapi/vfio.json
>> @@ -0,0 +1,61 @@
>> +# -*- Mode: Python -*-
>> +# vim: filetype=python
>> +#
>> +
>> +##
>> +# = VFIO devices
>> +##
>> +
>> +##
>> +# @VFIODeviceMigState:
>> +#
>> +# 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': 'VFIODeviceMigState',
>> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
>> +            'pre-copy', 'pre-copy-p2p' ],
>> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
>> +
>> +##
>> +# @VFIO_DEVICE_MIG_STATE_CHANGED:
>> +#
>> +# This event is emitted when a VFIO device migration state is changed.
>> +#
>> +# @device-id: The id of the VFIO device (final component of QOM path).
> Provide the full QOM path, please.  Feel free to additionally provide
> its qdev ID.

Sure, will do.

Thanks.

>
> Precedence: events MEMORY_DEVICE_SIZE_CHANGE, DEVICE_DELETED,
> DEVICE_UNPLUG_GUEST_ERROR, ...
>
>> +#
>> +# @device-state: The new changed device migration state.
>> +#
>> +# Since: 9.1
>> +#
>> +# Example:
>> +#
>> +#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
>> +#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
>> +#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
>> +##
>> +{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
>> +  'data': {
>> +      'device-id': 'str',
>> +      'device-state': 'VFIODeviceMigState'
>> +  } }
>> 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',
>>   ]

Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Markus Armbruster]

Avihai Horon <avihaih@nvidia.com> writes:

> On 02/05/2024 14:19, Markus Armbruster wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> Avihai Horon <avihaih@nvidia.com> writes:
>>
>>> Add a new QAPI event for VFIO device migration state change. 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.
>>>
>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>
>> Can you explain briefly why this event makes sense only for VFIO devices?
>
> VFIO devices have their own protocol for migration and a unique set of migration states.
> This event holds info about these VFIO migration states, which I think cannot be described in the same accuracy by other events such as run state or migration states.

Would it make sense to work this into the commit message?

[...]

Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Markus Armbruster]

Avihai Horon <avihaih@nvidia.com> writes:

> On 01/05/2024 14:50, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 30/04/2024 06:16, Avihai Horon wrote:
>>> Add a new QAPI event for VFIO device migration state change. 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.
>>>
>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>> ---
>>>   MAINTAINERS           |  1 +
>>>   qapi/qapi-schema.json |  1 +
>>>   qapi/vfio.json        | 61 +++++++++++++++++++++++++++++++++++++++++++
>>>   qapi/meson.build      |  1 +
>>>   4 files changed, 64 insertions(+)
>>>   create mode 100644 qapi/vfio.json
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 302b6fd00c..ef58a39d36 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2159,6 +2159,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..a38f26bccd
>>> --- /dev/null
>>> +++ b/qapi/vfio.json
>>> @@ -0,0 +1,61 @@
>>> +# -*- Mode: Python -*-
>>> +# vim: filetype=python
>>> +#
>>> +
>>> +##
>>> +# = VFIO devices
>>> +##
>>> +
>>> +##
>>> +# @VFIODeviceMigState:
>>> +#
>>> +# 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': 'VFIODeviceMigState',
>>> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
>>> +            'pre-copy', 'pre-copy-p2p' ],
>>> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }

Without 'prefix', you get VFIO_DEVICE_MIG_STATE_STOP and so forth.  Why
do you need a QAPI_ prefix?

>>> +
>>
>> Considering MIG can also be interpreted as Multi Instance GPU elsewhere
>> unrelated to this shouldn't we be explicit here? i.e.
>>
>>          VFIO_DEVICE_MIGRATION_STATE
>>
>> ... to avoid ambiguiosity.
>
> I used mig to avoid long names, but I don't mind changing it to migration if that's clearer.
>
> Thanks.

We generally avoid abbreviations in QAPI/QMP.

The event that reports general migration state change is called
MIGRATION, and its data type MigrationStatus.

We could call this one VFIO_MIGRATION, and its data type
VfioMigrationStatus.

>>> +##
>>> +# @VFIO_DEVICE_MIG_STATE_CHANGED:
>>> +#
>>> +# This event is emitted when a VFIO device migration state is changed.
>>> +#
>>> +# @device-id: The id of the VFIO device (final component of QOM path).
>>> +#
>>> +# @device-state: The new changed device migration state.
>>> +#
>>> +# Since: 9.1
>>> +#
>>> +# Example:
>>> +#
>>> +#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
>>> +#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
>>> +#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
>>> +##
>>> +{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
>>> +  'data': {
>>> +      'device-id': 'str',
>>> +      'device-state': 'VFIODeviceMigState'
>>> +  } }
>>> 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',
>>>  ]

The new code is in the intersection of VFIO and migration.  Putting it
into new vfio.json instead of existing migration.json lets you add it to
MAINTAINERS section VFIO instead of Migration.  Up to the maintainers
involved.

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Markus Armbruster]

Peter, Fabiano, I'd like to hear your opinion on the issue discussed
below.

Avihai Horon <avihaih@nvidia.com> writes:

> On 02/05/2024 13:22, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 01/05/2024 13:28, Avihai Horon wrote:
>>> On 01/05/2024 14:50, Joao Martins wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>>>>>    hw/vfio/pci.c                 |  2 ++
>>>>>    3 files changed, 47 insertions(+)
>>>>>
>>>>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum
>>>>> vfio_device_mig_state state)
>>>>>        }
>>>>>    }
>>>>>
>>>>> +static VFIODeviceMigState
>>>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>>>> +{
>>>>> +    switch (state) {
>>>>> +    case VFIO_DEVICE_STATE_STOP:
>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
>>>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
>>>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
>>>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
>>>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
>>>>> +    default:
>>>>> +        g_assert_not_reached();
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
>>>>> +{
>>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>>> +    const char *id;
>>>>> +    Object *obj;
>>>>> +
>>>>> +    if (!vbasedev->migration_events) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>> Shouldn't this leap frog migrate_events() capability instead of introducing its
>>>> vfio equivalent i.e.
>>>>
>>>>           if (!migrate_events()) {
>>>>               return;
>>>>           }
>>>>
>>>> ?
>>>
>>> I used a per VFIO device cap so the events can be fine tuned for each device
>>> (maybe one device should send events while the other not).
>>> This gives the most flexibility and I don't think it complicates the
>>> configuration (one downside, though, is that it can't be enabled/disabled
>>> dynamically during runtime).
>>>
>> Right.
>>
>>> I don't think events add much overhead, so if you prefer a global cap, I can
>>> change it.
>>> However, I'm not sure overloading the existing migrate_events() is valid?
>>>
>> migration 'events' capability just means we will have some migration events
>> emited via QAPI monitor for: 1) general global status and 2) for each migration
>> pass (both with different event names=.
>
> Yes, it's already overloaded.
>
> In migration QAPI it says: "@events: generate events for each migration state change (since 2.4)".
> This only refers to the MIGRATION event AFAIU.
>
> Later on (in QEMU 2.6), MIGRATION_PASS event was added and the events cap was overloaded for the first time (without changing @events description).
>
> Now we want to add yet another use for events capability, the VFIO migration state change events.
>
> I think what bothers me is the @events description, which is not accurate.
> Maybe it should be changed to "@events: generate migration related events (since 2.4)"? However, I'm not sure if it's OK to do this.
>
>>   So the suggestion was just what feels a
>> natural extension of that (...)
>>
>>>> Applications that don't understand the event string (migration related or not)
>>>> will just discard it (AIUI)
>>
>> (...) specially because of this as all these events have a different name.
>>
>> But overloading might not make sense for others IDK ... it was just a suggestion
>> :) not a strong preference
>
> Yes, I get your rationale.
> I don't have a strong opinion either, so maybe let's see what other people think.
>
> Thanks.

[...]

Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Avihai Horon]

On 06/05/2024 7:35, Markus Armbruster wrote:
> External email: Use caution opening links or attachments
>
>
> Avihai Horon <avihaih@nvidia.com> writes:
>
>> On 02/05/2024 14:19, Markus Armbruster wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> Avihai Horon <avihaih@nvidia.com> writes:
>>>
>>>> Add a new QAPI event for VFIO device migration state change. 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.
>>>>
>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>> Can you explain briefly why this event makes sense only for VFIO devices?
>> VFIO devices have their own protocol for migration and a unique set of migration states.
>> This event holds info about these VFIO migration states, which I think cannot be described in the same accuracy by other events such as run state or migration states.
> Would it make sense to work this into the commit message?

Sure, it wouldn't hurt. I will add it.

Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Avihai Horon]

On 06/05/2024 7:52, Markus Armbruster wrote:
> External email: Use caution opening links or attachments
>
>
> Avihai Horon <avihaih@nvidia.com> writes:
>
>> On 01/05/2024 14:50, Joao Martins wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>> Add a new QAPI event for VFIO device migration state change. 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.
>>>>
>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>>> ---
>>>>    MAINTAINERS           |  1 +
>>>>    qapi/qapi-schema.json |  1 +
>>>>    qapi/vfio.json        | 61 +++++++++++++++++++++++++++++++++++++++++++
>>>>    qapi/meson.build      |  1 +
>>>>    4 files changed, 64 insertions(+)
>>>>    create mode 100644 qapi/vfio.json
>>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 302b6fd00c..ef58a39d36 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -2159,6 +2159,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..a38f26bccd
>>>> --- /dev/null
>>>> +++ b/qapi/vfio.json
>>>> @@ -0,0 +1,61 @@
>>>> +# -*- Mode: Python -*-
>>>> +# vim: filetype=python
>>>> +#
>>>> +
>>>> +##
>>>> +# = VFIO devices
>>>> +##
>>>> +
>>>> +##
>>>> +# @VFIODeviceMigState:
>>>> +#
>>>> +# 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': 'VFIODeviceMigState',
>>>> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
>>>> +            'pre-copy', 'pre-copy-p2p' ],
>>>> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
> Without 'prefix', you get VFIO_DEVICE_MIG_STATE_STOP and so forth.  Why
> do you need a QAPI_ prefix?

VFIO uAPI already defines enum vfio_device_mig_state and its values 
VFIO_DEVICE_STATE_STOP, VFIO_DEVICE_STATE_RUNNING, etc.

I wanted to emphasize these are QAPI entities.

>
>>>> +
>>> Considering MIG can also be interpreted as Multi Instance GPU elsewhere
>>> unrelated to this shouldn't we be explicit here? i.e.
>>>
>>>           VFIO_DEVICE_MIGRATION_STATE
>>>
>>> ... to avoid ambiguiosity.
>> I used mig to avoid long names, but I don't mind changing it to migration if that's clearer.
>>
>> Thanks.
> We generally avoid abbreviations in QAPI/QMP.
>
> The event that reports general migration state change is called
> MIGRATION, and its data type MigrationStatus.
>
> We could call this one VFIO_MIGRATION, and its data type
> VfioMigrationStatus.

Sounds good, but how about VFIOMigrationState (I'd like it to relate to 
the VFIO state)?

>
>>>> +##
>>>> +# @VFIO_DEVICE_MIG_STATE_CHANGED:
>>>> +#
>>>> +# This event is emitted when a VFIO device migration state is changed.
>>>> +#
>>>> +# @device-id: The id of the VFIO device (final component of QOM path).
>>>> +#
>>>> +# @device-state: The new changed device migration state.
>>>> +#
>>>> +# Since: 9.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +#     <- {"timestamp": {"seconds": 1713771323, "microseconds": 212268},
>>>> +#         "event": "VFIO_DEVICE_MIG_STATE_CHANGED",
>>>> +#         "data": {"device-id": "vfio_dev1", "device-state": "stop"} }
>>>> +##
>>>> +{ 'event': 'VFIO_DEVICE_MIG_STATE_CHANGED',
>>>> +  'data': {
>>>> +      'device-id': 'str',
>>>> +      'device-state': 'VFIODeviceMigState'
>>>> +  } }
>>>> 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',
>>>>   ]
> The new code is in the intersection of VFIO and migration.  Putting it
> into new vfio.json instead of existing migration.json lets you add it to
> MAINTAINERS section VFIO instead of Migration.  Up to the maintainers
> involved.

Sure. I will keep it as is unless someone thinks otherwise.

Thanks.

Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Markus Armbruster]

Avihai Horon <avihaih@nvidia.com> writes:

> On 06/05/2024 7:52, Markus Armbruster wrote:
>> Avihai Horon <avihaih@nvidia.com> writes:
>>
>>> On 01/05/2024 14:50, Joao Martins wrote:
>>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>>> Add a new QAPI event for VFIO device migration state change. 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.
>>>>>
>>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

[...]

>>>>> diff --git a/qapi/vfio.json b/qapi/vfio.json
>>>>> new file mode 100644
>>>>> index 0000000000..a38f26bccd
>>>>> --- /dev/null
>>>>> +++ b/qapi/vfio.json
>>>>> @@ -0,0 +1,61 @@
>>>>> +# -*- Mode: Python -*-
>>>>> +# vim: filetype=python
>>>>> +#
>>>>> +
>>>>> +##
>>>>> +# = VFIO devices
>>>>> +##
>>>>> +
>>>>> +##
>>>>> +# @VFIODeviceMigState:
>>>>> +#
>>>>> +# 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': 'VFIODeviceMigState',
>>>>> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
>>>>> +            'pre-copy', 'pre-copy-p2p' ],
>>>>> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
>>
>> Without 'prefix', you get VFIO_DEVICE_MIG_STATE_STOP and so forth.  Why
>> do you need a QAPI_ prefix?
>
> VFIO uAPI already defines enum vfio_device_mig_state and its values 
> VFIO_DEVICE_STATE_STOP, VFIO_DEVICE_STATE_RUNNING, etc.
>
> I wanted to emphasize these are QAPI entities.

I see.

>>>>> +
>>>> Considering MIG can also be interpreted as Multi Instance GPU elsewhere
>>>> unrelated to this shouldn't we be explicit here? i.e.
>>>>
>>>>           VFIO_DEVICE_MIGRATION_STATE
>>>>
>>>> ... to avoid ambiguiosity.
>>>
>>> I used mig to avoid long names, but I don't mind changing it to migration if that's clearer.
>>>
>>> Thanks.
>>
>> We generally avoid abbreviations in QAPI/QMP.
>>
>> The event that reports general migration state change is called
>> MIGRATION, and its data type MigrationStatus.
>>
>> We could call this one VFIO_MIGRATION, and its data type
>> VfioMigrationStatus.
>
> Sounds good, but how about VFIOMigrationState (I'd like it to relate to 
> the VFIO state)?

No objection to "State" instead of "Status" then.

On VFIO vs. Vfio: several existing type names start with Vfio.

[...]

Re: [PATCH 1/3] qapi/vfio: Add VFIO device migration state change QAPI event

[Avihai Horon]

On 06/05/2024 13:36, Markus Armbruster wrote:
> External email: Use caution opening links or attachments
>
>
> Avihai Horon <avihaih@nvidia.com> writes:
>
>> On 06/05/2024 7:52, Markus Armbruster wrote:
>>> Avihai Horon <avihaih@nvidia.com> writes:
>>>
>>>> On 01/05/2024 14:50, Joao Martins wrote:
>>>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>>>> Add a new QAPI event for VFIO device migration state change. 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.
>>>>>>
>>>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> [...]
>
>>>>>> diff --git a/qapi/vfio.json b/qapi/vfio.json
>>>>>> new file mode 100644
>>>>>> index 0000000000..a38f26bccd
>>>>>> --- /dev/null
>>>>>> +++ b/qapi/vfio.json
>>>>>> @@ -0,0 +1,61 @@
>>>>>> +# -*- Mode: Python -*-
>>>>>> +# vim: filetype=python
>>>>>> +#
>>>>>> +
>>>>>> +##
>>>>>> +# = VFIO devices
>>>>>> +##
>>>>>> +
>>>>>> +##
>>>>>> +# @VFIODeviceMigState:
>>>>>> +#
>>>>>> +# 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': 'VFIODeviceMigState',
>>>>>> +  'data': [ 'stop', 'running', 'stop-copy', 'resuming', 'running-p2p',
>>>>>> +            'pre-copy', 'pre-copy-p2p' ],
>>>>>> +  'prefix': 'QAPI_VFIO_DEVICE_MIG_STATE' }
>>> Without 'prefix', you get VFIO_DEVICE_MIG_STATE_STOP and so forth.  Why
>>> do you need a QAPI_ prefix?
>> VFIO uAPI already defines enum vfio_device_mig_state and its values
>> VFIO_DEVICE_STATE_STOP, VFIO_DEVICE_STATE_RUNNING, etc.
>>
>> I wanted to emphasize these are QAPI entities.
> I see.
>
>>>>>> +
>>>>> Considering MIG can also be interpreted as Multi Instance GPU elsewhere
>>>>> unrelated to this shouldn't we be explicit here? i.e.
>>>>>
>>>>>            VFIO_DEVICE_MIGRATION_STATE
>>>>>
>>>>> ... to avoid ambiguiosity.
>>>> I used mig to avoid long names, but I don't mind changing it to migration if that's clearer.
>>>>
>>>> Thanks.
>>> We generally avoid abbreviations in QAPI/QMP.
>>>
>>> The event that reports general migration state change is called
>>> MIGRATION, and its data type MigrationStatus.
>>>
>>> We could call this one VFIO_MIGRATION, and its data type
>>> VfioMigrationStatus.
>> Sounds good, but how about VFIOMigrationState (I'd like it to relate to
>> the VFIO state)?
> No objection to "State" instead of "Status" then.
>
> On VFIO vs. Vfio: several existing type names start with Vfio.

Ah, yes, I see: BlockdevOptionsVirtioBlkVfioPci, VfioStats, 
VfioUserServerProperties.

I will go with the flow then, VfioMigrationState it is.

Thanks.

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Peter, Fabiano, I'd like to hear your opinion on the issue discussed
> below.
>
> Avihai Horon <avihaih@nvidia.com> writes:
>
>> On 02/05/2024 13:22, Joao Martins wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 01/05/2024 13:28, Avihai Horon wrote:
>>>> On 01/05/2024 14:50, Joao Martins wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>>>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>>>>>>    hw/vfio/pci.c                 |  2 ++
>>>>>>    3 files changed, 47 insertions(+)
>>>>>>
>>>>>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum
>>>>>> vfio_device_mig_state state)
>>>>>>        }
>>>>>>    }
>>>>>>
>>>>>> +static VFIODeviceMigState
>>>>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>>>>> +{
>>>>>> +    switch (state) {
>>>>>> +    case VFIO_DEVICE_STATE_STOP:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
>>>>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
>>>>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
>>>>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
>>>>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
>>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
>>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
>>>>>> +    default:
>>>>>> +        g_assert_not_reached();
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
>>>>>> +{
>>>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>>>> +    const char *id;
>>>>>> +    Object *obj;
>>>>>> +
>>>>>> +    if (!vbasedev->migration_events) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>> Shouldn't this leap frog migrate_events() capability instead of introducing its
>>>>> vfio equivalent i.e.
>>>>>
>>>>>           if (!migrate_events()) {
>>>>>               return;
>>>>>           }
>>>>>
>>>>> ?
>>>>
>>>> I used a per VFIO device cap so the events can be fine tuned for each device
>>>> (maybe one device should send events while the other not).
>>>> This gives the most flexibility and I don't think it complicates the
>>>> configuration (one downside, though, is that it can't be enabled/disabled
>>>> dynamically during runtime).
>>>>
>>> Right.
>>>
>>>> I don't think events add much overhead, so if you prefer a global cap, I can
>>>> change it.
>>>> However, I'm not sure overloading the existing migrate_events() is valid?
>>>>
>>> migration 'events' capability just means we will have some migration events
>>> emited via QAPI monitor for: 1) general global status and 2) for each migration
>>> pass (both with different event names=.
>>
>> Yes, it's already overloaded.
>>
>> In migration QAPI it says: "@events: generate events for each migration state change (since 2.4)".
>> This only refers to the MIGRATION event AFAIU.
>>
>> Later on (in QEMU 2.6), MIGRATION_PASS event was added and the events cap was overloaded for the first time (without changing @events description).
>>
>> Now we want to add yet another use for events capability, the VFIO migration state change events.
>>
>> I think what bothers me is the @events description, which is not accurate.
>> Maybe it should be changed to "@events: generate migration related events (since 2.4)"? However, I'm not sure if it's OK to do this.
>>
>>>   So the suggestion was just what feels a
>>> natural extension of that (...)
>>>
>>>>> Applications that don't understand the event string (migration related or not)
>>>>> will just discard it (AIUI)
>>>
>>> (...) specially because of this as all these events have a different name.
>>>
>>> But overloading might not make sense for others IDK ... it was just a suggestion
>>> :) not a strong preference
>>
>> Yes, I get your rationale.
>> I don't have a strong opinion either, so maybe let's see what other people think.

I don't see the need to tie this to the migration events
capability. Although there's overlap in the terms used, this seems more
like exposing a device feature via QEMU, then a migration event
per-se. The state changes also happen during moments unrelated to
migration (cover letter mentions start/stopping guest), so I assume we'd
like to keep those even if the management layer doesn't want to see
migration events.

Re: [PATCH 3/3] vfio/migration: Don't emit STOP_COPY state change event twice

[Cédric Le Goater]

Hello Avihai,

On 4/30/24 07:16, 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 migration state change QAPI
> event, the STOP_COPY state change event is undesirably emitted twice.
> 
> Prevent this by conditionally transitioning to STOP_COPY state in
> vfio_save_complete_precopy().
> 
> 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 | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 6bbccf6545..30a2b2ea74 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -591,14 +591,17 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque)
>   static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>   {
>       VFIODevice *vbasedev = opaque;
> +    VFIOMigration *migration = vbasedev->migration;
>       ssize_t data_size;
>       int ret;
>   
>       /* We reach here with device state STOP or STOP_COPY only */
> -    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
> -                                   VFIO_DEVICE_STATE_STOP);
> -    if (ret) {
> -        return ret;
> +    if (migration->device_state == VFIO_DEVICE_STATE_STOP) {

Shouldn't we handle no-op transitions in vfio_migration_set_state()
instead ?

Thanks,

C.



> +        ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP_COPY,
> +                                       VFIO_DEVICE_STATE_STOP);
> +        if (ret) {
> +            return ret;
> +        }
>       }
>   
>       do {

Re: [PATCH 3/3] vfio/migration: Don't emit STOP_COPY state change event twice

[Avihai Horon]

On 06/05/2024 17:39, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Avihai,
>
> On 4/30/24 07:16, 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 migration state change QAPI
>> event, the STOP_COPY state change event is undesirably emitted twice.
>>
>> Prevent this by conditionally transitioning to STOP_COPY state in
>> vfio_save_complete_precopy().
>>
>> 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 | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 6bbccf6545..30a2b2ea74 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -591,14 +591,17 @@ static int vfio_save_iterate(QEMUFile *f, void 
>> *opaque)
>>   static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>>   {
>>       VFIODevice *vbasedev = opaque;
>> +    VFIOMigration *migration = vbasedev->migration;
>>       ssize_t data_size;
>>       int ret;
>>
>>       /* We reach here with device state STOP or STOP_COPY only */
>> -    ret = vfio_migration_set_state(vbasedev, 
>> VFIO_DEVICE_STATE_STOP_COPY,
>> -                                   VFIO_DEVICE_STATE_STOP);
>> -    if (ret) {
>> -        return ret;
>> +    if (migration->device_state == VFIO_DEVICE_STATE_STOP) {
>
> Shouldn't we handle no-op transitions in vfio_migration_set_state()
> instead ?
>
Yes, you are right. It's better to handle the general case in 
vfio_migration_set_state().
Will change it.

Thanks.

> Thanks,
>
> C.
>
>
>
>> +        ret = vfio_migration_set_state(vbasedev, 
>> VFIO_DEVICE_STATE_STOP_COPY,
>> +                                       VFIO_DEVICE_STATE_STOP);
>> +        if (ret) {
>> +            return ret;
>> +        }
>>       }
>>
>>       do {
>

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Peter Xu]

On Mon, May 06, 2024 at 11:38:00AM -0300, Fabiano Rosas wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Peter, Fabiano, I'd like to hear your opinion on the issue discussed
> > below.
> >
> > Avihai Horon <avihaih@nvidia.com> writes:
> >
> >> On 02/05/2024 13:22, Joao Martins wrote:
> >>> External email: Use caution opening links or attachments
> >>>
> >>>
> >>> On 01/05/2024 13:28, Avihai Horon wrote:
> >>>> On 01/05/2024 14:50, Joao Martins wrote:
> >>>>> External email: Use caution opening links or attachments
> >>>>>
> >>>>>
> >>>>> On 30/04/2024 06:16, Avihai Horon wrote:
> >>>>>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
> >>>>>>    hw/vfio/pci.c                 |  2 ++
> >>>>>>    3 files changed, 47 insertions(+)
> >>>>>>
> >>>>>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum
> >>>>>> vfio_device_mig_state state)
> >>>>>>        }
> >>>>>>    }
> >>>>>>
> >>>>>> +static VFIODeviceMigState
> >>>>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
> >>>>>> +{
> >>>>>> +    switch (state) {
> >>>>>> +    case VFIO_DEVICE_STATE_STOP:
> >>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
> >>>>>> +    case VFIO_DEVICE_STATE_RUNNING:
> >>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
> >>>>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
> >>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
> >>>>>> +    case VFIO_DEVICE_STATE_RESUMING:
> >>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
> >>>>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
> >>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
> >>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
> >>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
> >>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
> >>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
> >>>>>> +    default:
> >>>>>> +        g_assert_not_reached();
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
> >>>>>> +{
> >>>>>> +    VFIOMigration *migration = vbasedev->migration;
> >>>>>> +    const char *id;
> >>>>>> +    Object *obj;
> >>>>>> +
> >>>>>> +    if (!vbasedev->migration_events) {
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +
> >>>>> Shouldn't this leap frog migrate_events() capability instead of introducing its
> >>>>> vfio equivalent i.e.
> >>>>>
> >>>>>           if (!migrate_events()) {
> >>>>>               return;
> >>>>>           }
> >>>>>
> >>>>> ?
> >>>>
> >>>> I used a per VFIO device cap so the events can be fine tuned for each device
> >>>> (maybe one device should send events while the other not).
> >>>> This gives the most flexibility and I don't think it complicates the
> >>>> configuration (one downside, though, is that it can't be enabled/disabled
> >>>> dynamically during runtime).
> >>>>
> >>> Right.
> >>>
> >>>> I don't think events add much overhead, so if you prefer a global cap, I can
> >>>> change it.
> >>>> However, I'm not sure overloading the existing migrate_events() is valid?
> >>>>
> >>> migration 'events' capability just means we will have some migration events
> >>> emited via QAPI monitor for: 1) general global status and 2) for each migration
> >>> pass (both with different event names=.
> >>
> >> Yes, it's already overloaded.
> >>
> >> In migration QAPI it says: "@events: generate events for each migration state change (since 2.4)".
> >> This only refers to the MIGRATION event AFAIU.
> >>
> >> Later on (in QEMU 2.6), MIGRATION_PASS event was added and the events cap was overloaded for the first time (without changing @events description).
> >>
> >> Now we want to add yet another use for events capability, the VFIO migration state change events.
> >>
> >> I think what bothers me is the @events description, which is not accurate.
> >> Maybe it should be changed to "@events: generate migration related events (since 2.4)"? However, I'm not sure if it's OK to do this.
> >>
> >>>   So the suggestion was just what feels a
> >>> natural extension of that (...)
> >>>
> >>>>> Applications that don't understand the event string (migration related or not)
> >>>>> will just discard it (AIUI)
> >>>
> >>> (...) specially because of this as all these events have a different name.
> >>>
> >>> But overloading might not make sense for others IDK ... it was just a suggestion
> >>> :) not a strong preference
> >>
> >> Yes, I get your rationale.
> >> I don't have a strong opinion either, so maybe let's see what other people think.
> 
> I don't see the need to tie this to the migration events
> capability. Although there's overlap in the terms used, this seems more
> like exposing a device feature via QEMU, then a migration event
> per-se. The state changes also happen during moments unrelated to
> migration (cover letter mentions start/stopping guest), so I assume we'd
> like to keep those even if the management layer doesn't want to see
> migration events.

Yeh makes sense to me to keep that per-device flag, as it's not a generic
migration state change.

  @events: generate events for each migration state change (since 2.4)

The other option is to emit event only if "migrate_events() &&
vfio_dev_send_event", but that sounds like an overkill to overload "events"
cap, and doesn't look like bring anything better than just keeping them
separate.

While at it, another trivial comment is maybe it's nice to have a helper to
both update the vfio migration state, plus emitting events when necessary.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Avihai Horon]

On 06/05/2024 18:07, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, May 06, 2024 at 11:38:00AM -0300, Fabiano Rosas wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Peter, Fabiano, I'd like to hear your opinion on the issue discussed
>>> below.
>>>
>>> Avihai Horon <avihaih@nvidia.com> writes:
>>>
>>>> On 02/05/2024 13:22, Joao Martins wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 01/05/2024 13:28, Avihai Horon wrote:
>>>>>> On 01/05/2024 14:50, Joao Martins wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> On 30/04/2024 06:16, Avihai Horon wrote:
>>>>>>>> Emit VFIO device migration state change 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           | 44 +++++++++++++++++++++++++++++++++++
>>>>>>>>     hw/vfio/pci.c                 |  2 ++
>>>>>>>>     3 files changed, 47 insertions(+)
>>>>>>>>
>>>>>>>> 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..6bbccf6545 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,46 @@ static const char *mig_state_to_str(enum
>>>>>>>> vfio_device_mig_state state)
>>>>>>>>         }
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +static VFIODeviceMigState
>>>>>>>> +mig_state_to_qapi_state(enum vfio_device_mig_state state)
>>>>>>>> +{
>>>>>>>> +    switch (state) {
>>>>>>>> +    case VFIO_DEVICE_STATE_STOP:
>>>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP;
>>>>>>>> +    case VFIO_DEVICE_STATE_RUNNING:
>>>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING;
>>>>>>>> +    case VFIO_DEVICE_STATE_STOP_COPY:
>>>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_STOP_COPY;
>>>>>>>> +    case VFIO_DEVICE_STATE_RESUMING:
>>>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RESUMING;
>>>>>>>> +    case VFIO_DEVICE_STATE_RUNNING_P2P:
>>>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_RUNNING_P2P;
>>>>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY:
>>>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY;
>>>>>>>> +    case VFIO_DEVICE_STATE_PRE_COPY_P2P:
>>>>>>>> +        return QAPI_VFIO_DEVICE_MIG_STATE_PRE_COPY_P2P;
>>>>>>>> +    default:
>>>>>>>> +        g_assert_not_reached();
>>>>>>>> +    }
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void vfio_migration_send_state_change_event(VFIODevice *vbasedev)
>>>>>>>> +{
>>>>>>>> +    VFIOMigration *migration = vbasedev->migration;
>>>>>>>> +    const char *id;
>>>>>>>> +    Object *obj;
>>>>>>>> +
>>>>>>>> +    if (!vbasedev->migration_events) {
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>> Shouldn't this leap frog migrate_events() capability instead of introducing its
>>>>>>> vfio equivalent i.e.
>>>>>>>
>>>>>>>            if (!migrate_events()) {
>>>>>>>                return;
>>>>>>>            }
>>>>>>>
>>>>>>> ?
>>>>>> I used a per VFIO device cap so the events can be fine tuned for each device
>>>>>> (maybe one device should send events while the other not).
>>>>>> This gives the most flexibility and I don't think it complicates the
>>>>>> configuration (one downside, though, is that it can't be enabled/disabled
>>>>>> dynamically during runtime).
>>>>>>
>>>>> Right.
>>>>>
>>>>>> I don't think events add much overhead, so if you prefer a global cap, I can
>>>>>> change it.
>>>>>> However, I'm not sure overloading the existing migrate_events() is valid?
>>>>>>
>>>>> migration 'events' capability just means we will have some migration events
>>>>> emited via QAPI monitor for: 1) general global status and 2) for each migration
>>>>> pass (both with different event names=.
>>>> Yes, it's already overloaded.
>>>>
>>>> In migration QAPI it says: "@events: generate events for each migration state change (since 2.4)".
>>>> This only refers to the MIGRATION event AFAIU.
>>>>
>>>> Later on (in QEMU 2.6), MIGRATION_PASS event was added and the events cap was overloaded for the first time (without changing @events description).
>>>>
>>>> Now we want to add yet another use for events capability, the VFIO migration state change events.
>>>>
>>>> I think what bothers me is the @events description, which is not accurate.
>>>> Maybe it should be changed to "@events: generate migration related events (since 2.4)"? However, I'm not sure if it's OK to do this.
>>>>
>>>>>    So the suggestion was just what feels a
>>>>> natural extension of that (...)
>>>>>
>>>>>>> Applications that don't understand the event string (migration related or not)
>>>>>>> will just discard it (AIUI)
>>>>> (...) specially because of this as all these events have a different name.
>>>>>
>>>>> But overloading might not make sense for others IDK ... it was just a suggestion
>>>>> :) not a strong preference
>>>> Yes, I get your rationale.
>>>> I don't have a strong opinion either, so maybe let's see what other people think.
>> I don't see the need to tie this to the migration events
>> capability. Although there's overlap in the terms used, this seems more
>> like exposing a device feature via QEMU, then a migration event
>> per-se. The state changes also happen during moments unrelated to
>> migration (cover letter mentions start/stopping guest), so I assume we'd
>> like to keep those even if the management layer doesn't want to see
>> migration events.
> Yeh makes sense to me to keep that per-device flag, as it's not a generic
> migration state change.
>
>    @events: generate events for each migration state change (since 2.4)
>
> The other option is to emit event only if "migrate_events() &&
> vfio_dev_send_event", but that sounds like an overkill to overload "events"
> cap, and doesn't look like bring anything better than just keeping them
> separate.

I agree, so I will keep it a per-device flag.

> While at it, another trivial comment is maybe it's nice to have a helper to
> both update the vfio migration state, plus emitting events when necessary.

I think vfio_migration_set_state() does exactly that, no?

Thanks.

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Peter Xu]

On Tue, May 07, 2024 at 10:47:13AM +0300, Avihai Horon wrote:
> > While at it, another trivial comment is maybe it's nice to have a helper to
> > both update the vfio migration state, plus emitting events when necessary.
> 
> I think vfio_migration_set_state() does exactly that, no?

Ah yes, looks so.  It's just that I saw some common patterns, like:

===8<===
@@ -126,11 +167,13 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
         }
 
         migration->device_state = recover_state;
+        vfio_migration_send_state_change_event(vbasedev);
 
         return ret;
     }
 
     migration->device_state = new_state;
+    vfio_migration_send_state_change_event(vbasedev);
     if (mig_state->data_fd != -1) {
         if (migration->data_fd != -1) {
             /*
@@ -157,6 +200,7 @@ reset_device:
     }
 
     migration->device_state = VFIO_DEVICE_STATE_RUNNING;
+    vfio_migration_send_state_change_event(vbasedev);
 
     return ret;
 }
===8<===

So maybe some more internal helpers?  It doesn't look like to cover all
updates to device_state, but I really didn't read into it.  Not a huge deal
really, feel free to keep it as-is if maintainers are happy.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/3] vfio/migration: Emit VFIO device migration state change QAPI event

[Avihai Horon]

On 07/05/2024 18:51, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, May 07, 2024 at 10:47:13AM +0300, Avihai Horon wrote:
>>> While at it, another trivial comment is maybe it's nice to have a helper to
>>> both update the vfio migration state, plus emitting events when necessary.
>> I think vfio_migration_set_state() does exactly that, no?
> Ah yes, looks so.  It's just that I saw some common patterns, like:
>
> ===8<===
> @@ -126,11 +167,13 @@ static int vfio_migration_set_state(VFIODevice *vbasedev,
>           }
>
>           migration->device_state = recover_state;
> +        vfio_migration_send_state_change_event(vbasedev);
>
>           return ret;
>       }
>
>       migration->device_state = new_state;
> +    vfio_migration_send_state_change_event(vbasedev);
>       if (mig_state->data_fd != -1) {
>           if (migration->data_fd != -1) {
>               /*
> @@ -157,6 +200,7 @@ reset_device:
>       }
>
>       migration->device_state = VFIO_DEVICE_STATE_RUNNING;
> +    vfio_migration_send_state_change_event(vbasedev);
>
>       return ret;
>   }
> ===8<===
>
> So maybe some more internal helpers?  It doesn't look like to cover all
> updates to device_state, but I really didn't read into it.  Not a huge deal
> really, feel free to keep it as-is if maintainers are happy.

Oh, I see what you mean.

Cedric, would you like me to add the following?

===8<===

--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -122,6 +122,14 @@ static void vfio_migration_send_event(VFIODevice 
*vbasedev)
          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)
@@ -171,14 +179,12 @@ static int vfio_migration_set_state(VFIODevice 
*vbasedev,
              goto reset_device;
          }

-        migration->device_state = recover_state;
-        vfio_migration_send_event(vbasedev);
+        set_state(vbasedev, recover_state);

          return ret;
      }

-    migration->device_state = new_state;
-    vfio_migration_send_event(vbasedev);
+    set_state(vbasedev, new_state);
      if (mig_state->data_fd != -1) {
          if (migration->data_fd != -1) {
              /*
@@ -204,8 +210,7 @@ reset_device:
                   strerror(errno));
      }

-    migration->device_state = VFIO_DEVICE_STATE_RUNNING;
-    vfio_migration_send_event(vbasedev);
+    set_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);

      return ret;
  }

===8<===