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 <arm...@redhat.com> writes:
Peter, Fabiano, I'd like to hear your opinion on the issue discussed
below.
Avihai Horon <avih...@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<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.
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.
