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<avih...@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),
