On 8/30/23 09:01, Avihai Horon wrote:
On 29/08/2023 21:27, Peter Xu wrote:
External email: Use caution opening links or attachments
On Tue, Aug 29, 2023 at 07:20:47PM +0300, Avihai Horon wrote:
On 29/08/2023 17:53, Peter Xu wrote:
External email: Use caution opening links or attachments
On Mon, Aug 28, 2023 at 06:18:41PM +0300, Avihai Horon wrote:
diff --git a/migration/options.c b/migration/options.c
index 1d1e1321b0..e201053563 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -499,6 +499,11 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
Error **errp)
error_setg(errp, "Postcopy is not yet compatible with multifd");
return false;
}
+
+ if (migration_vfio_mig_active()) {
+ error_setg(errp, "Postcopy is not compatible with VFIO migration");
+ return false;
+ }
Hmm.. this will add yet another vfio hard-coded line into migration/..
What will happen if the vfio device is hot plugged after enabling
postcopy-ram here?
In that case a migration blocker will be added.
Is it possible to do it in a generic way?
What comes to my mind is to let devices register a handler for a "caps
change" notification and allow them to object.
But maybe that's a bit of an overkill.
This one also sounds better than hard-codes to me.
I was thinking the only unified place to do such check is when migration
starts, as long as we switch to SETUP all caps are locked and doesn't allow
any change until it finishes or fails.
So, can we do this check inside vfio_save_setup(), allow vfio_save_setup()
to fail the whole migration early? For example, maybe we should have an
Error** passed in, then if it fails it calls migrate_set_error, so
reflected in query-migrate later too.
Yes, I think this could work and it will simplify things because we could
also drop the VFIO migration blockers code.
The downside is that the user will know migration is blocked only when he
tries to migrate, and migrate_caps_check() will not block setting postcopy
when a VFIO device is already attached.
I don't have a strong opinion here, so if it's fine by you and everyone
else, I could change that to what you suggested.
Failing later would be fine in this case to me; my expectation is VFIO
users should be advanced already anyway (as the whole solution is still
pretty involved comparing to a generic VM migration) and shouldn't try to
trigger that at all in real life. IOW I'd expect this check will be there
just for sanity, rather than being relied on to let people be aware of it
by the error message.
Yes, I agree with you.
Meanwhile the blocker + caps check is slightly complicated to me to guard
both sides. So I'd vote for failing at the QMP command. But we can wait
and see whether there's other votes.
Sure.
So I will do the checking in vfio_save_setup(), unless someone else has a
better idea.
Just to recap for my understanding,
vfio_save_setup() would test migrate_postcopy_ram() and update a new
'Error *err' parameter of the .save_setup() op which would be taken
into account in qemu_savevm_state_setup(). Is that correct ?
Thanks,
C.
