Akihiko Odaki <[email protected]> writes:
> On 2026/09/22 23:28, Fabiano Rosas wrote:
>> Akihiko Odaki <[email protected]> writes:
>>
>>> xen-save-devices-state saves device VMState outside QEMU's normal
>>> migration-mode machinery, so it currently ignores migration blockers.
>>> Add a separate blocker class for this command.
>>>
>>> Blockers registered with migrate_add_blocker() or
>>> migrate_add_blocker_internal() now include this class. Existing
>>> mode-specific blocker masks remain unchanged, except that vhost-scsi's
>>> default blocker now covers normal migration and Xen device-state
>>> saving. Its CPR exemption assumes that the replacement QEMU runs on
>>> the same host and not concurrently with the old QEMU; Xen save and
>>> restore do not guarantee those conditions. Users can set
>>> migratable=true when shared storage or orchestrator-migrated target
>>> state makes migration safe.
>>>
>>
>> So one thing is rejecting the xen-save-devices-state command if there
>> are blockers. This could very well just use the normal mode (default),
>> no? Or all modes, like qmp_dump_guest_memory() does?
>
> In general, blockers registered via migrate_add_blocker_normal() are
> specific to standard QEMU live migration and do not apply to Xen. For
> example, block formats like qcow or vhd register normal migration
> blockers, but Xen explicitly supports migration with them [1].
> Unconditionally applying normal-mode blockers to Xen would cause
> regressions. vhost-scsi is the exception here and is handled specifically.
>
> Checking every mode would lead to the same regression. Note that
> qmp_dump_guest_memory() registers an all-mode blocker during execution;
> it does not check all existing blocker lists when invoked.
>
> [1] https://xenbits.xen.org/docs/4.22-testing/SUPPORT.html#blkback
>
>>
>> Another thing (complementary, could be in the same patch) is rejecting
>> the command line if there are blocked devices, including any ones used
>> along with Xen acceleration. Again, why does this need a new blocker
>> category? I'm not seeing it.
> You can use -only-migratable.
>
>>
>>> Fixes: a7ae8355b446 ("Introduce "xen-save-devices-state"")
>>> Signed-off-by: Akihiko Odaki <[email protected]>
>>> ---
>>> include/migration/blocker.h | 10 ++++++-
>>> include/qemu/accel.h | 2 +-
>>> migration/migration.h | 2 ++
>>> accel/accel-system.c | 25 +++++++++++++-----
>>> hw/scsi/vhost-scsi.c | 5 +++-
>>> migration/migration.c | 63
>>> ++++++++++++++++++++++++++++++++++++++-------
>>> migration/savevm.c | 4 +++
>>> system/vl.c | 9 ++++---
>>> 8 files changed, 99 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/migration/blocker.h b/include/migration/blocker.h
>>> index 80b75ad5cbdb..0fbaaac83ce8 100644
>>> --- a/include/migration/blocker.h
>>> +++ b/include/migration/blocker.h
>>> @@ -16,6 +16,14 @@
>>>
>>> #include "qapi/qapi-types-migration.h"
>>>
>>> +/**
>>> + * define MIG_XEN - identifier for Xen migration
>>
>> Hm.
>>
>>> + *
>>> + * Xen migration is distinct from those expressed with &typedef MigMode.
>>> + * Use this for migration blockers that affect Xen.
>>> + */
>>> +#define MIG_XEN MIG_MODE__MAX
>>
>> This could be fine, Xen migration is pre-existing and 'mode' is part of
>> QAPI, we don't want to create a new mode for Xen.
>>
>>> +
>>> /**
>>> * @migrate_add_blocker - prevent all modes of migration from proceeding
>>> *
>>> @@ -80,7 +88,7 @@ int migrate_add_blocker_normal(Error **reasonp, Error
>>> **errp);
>>> *
>>> * @reasonp - address of an error to be returned whenever migration is
>>> attempted
>>> *
>>> - * @modes - the migration modes to be blocked, a bit set of MigMode
>>> + * @modes - the migration modes to be blocked, a bit set of MigMode and
>>> %MIG_XEN
>>
>> But it's not a mode, so we shouldn't re-use @modes. We just want a place
>> to store the Xen blockers, let's not introduce the confusion that this
>> could be a mode somehow.
>>
>> We could just add migrate_add_blocker_xen and call add_blockers directly
>> since you're changing that function to use the array size instead of
>> MIG_MODE__MAX.
>
> I would prefer to keep a single function for adding blockers across all
> migration types so that the full scope of a blocker can be seen at a
> single call site (see the vhost-scsi changes for an example).
>
Fair.
> Practically speaking, I think the risk of reusing @modes is minimal
> because there is no alternative selective-blocking API to misuse. It
> would indeed be cleaner to have a parameter name that encompasses both
> MigMode and MIG_XEN, but I haven't come up with a better name yet;
> suggestions are welcome.
>
Keep the @mode, but let's do away with any mentions of MigMode. This is
a "blocker mode", not "migration mode". wink wink
>>
>>> *
>>> * @errp - [out] The reason (if any) we cannot block migration right now.
>>> *
>>> diff --git a/include/qemu/accel.h b/include/qemu/accel.h
>>> index 6f65f1cb89a0..b6b456a82fc3 100644
>>> --- a/include/qemu/accel.h
>>> +++ b/include/qemu/accel.h
>>> @@ -47,7 +47,7 @@ const char *current_accel_name(void);
>>>
>>> void accel_init_interfaces(AccelClass *ac);
>>>
>>> -int accel_init_machine(AccelState *accel, MachineState *ms);
>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp);
>>>
>>> /* Called just before os_setup_post (ie just before drop OS privs) */
>>> void accel_setup_post(MachineState *ms);
>>> diff --git a/migration/migration.h b/migration/migration.h
>>> index 683bc7bdd521..f53fdb2df107 100644
>>> --- a/migration/migration.h
>>> +++ b/migration/migration.h
>>> @@ -568,7 +568,9 @@ void migration_start_incoming(void);
>>> int migration_call_notifiers(MigrationEventType type, Error **errp);
>>>
>>> int migrate_init(MigrationState *s, Error **errp);
>>> +bool enforce_only_migratable(Error **errp);
>>> bool migration_is_blocked(Error **errp);
>>> +bool xen_migration_is_blocked(Error **errp);
>>> /* True if outgoing migration has entered postcopy phase */
>>> bool migration_in_postcopy(void);
>>> bool migration_postcopy_is_alive(MigrationStatus state);
>>> diff --git a/accel/accel-system.c b/accel/accel-system.c
>>> index 1325b23864c8..75a691cb3cfe 100644
>>> --- a/accel/accel-system.c
>>> +++ b/accel/accel-system.c
>>> @@ -26,7 +26,9 @@
>>> #include "qemu/osdep.h"
>>> #include "qemu/accel.h"
>>> #include "qom/compat-properties.h"
>>> +#include "qapi/error.h"
>>> #include "qapi/qapi-commands-accelerator.h"
>>> +#include "migration/migration.h"
>>> #include "monitor/monitor.h"
>>> #include "monitor/hmp.h"
>>> #include "hw/core/boards.h"
>>> @@ -37,20 +39,31 @@
>>> #include "qemu/error-report.h"
>>> #include "accel-internal.h"
>>>
>>> -int accel_init_machine(AccelState *accel, MachineState *ms)
>>> +int accel_init_machine(AccelState *accel, MachineState *ms, Error **errp)
>>> {
>>> AccelClass *acc = ACCEL_GET_CLASS(accel);
>>> int ret;
>>> ms->accelerator = accel;
>>> *(acc->allowed) = true;
>>> +
>>> + if (!enforce_only_migratable(errp)) {
>>> + ret = -EACCES;
>>> + goto fail;
>>> + }
>>
>> Here I think you want to fail the initialization if the accelerator is
>> Xen, -only-migratable is set and there are blockers. At this point is
>> not clear whether these blockers really need a xen-specific category.
>
> The category reflects the specific requirements of the blocker itself.
> Looking at vhost-scsi (which registers a Xen-affecting blocker)
> clarifies why this distinction is necessary; this code merely enforces
> registered blockers.
>
>>
>> Isn't this too soon anyway? What blockers will be registered at this
>> point and if there are blockers on the list, why the
>> is_only_migratable() check at migrate_add_blocker_modes() hasn't already
>> rejected the cmdline?
>
> This check is placed here because qemu_create_early_backends() runs
> before configure_accelerators(). Command-line backends (such as qcow or
> vmdk) can register blockers before xen_enabled() accurately reflects the
> selected accelerator.
>
Ok, so we want to validate that the blockers registered so far don't
affect the current accel. This information is important to have in a
comment somewhere.
Now, in the bad case we're going to fail the startup anyway, so wouldn't
it be better to avoid all the error handling and just make this a
generic check after configure_accelerators?
void qemu_init(int argc, char **argv)
{
configure_accelerators(argv[0]);
phase_advance(PHASE_ACCEL_CREATED);
enforce_only_migratable();
...
>>
>>> +
>>> ret = acc->init_machine(accel, ms);
>>> if (ret < 0) {
>>> - ms->accelerator = NULL;
>>> - *(acc->allowed) = false;
>>> - object_unref(OBJECT(accel));
>>> - } else {
>>> - object_set_accelerator_compat_props(acc->compat_props);
>>> + error_setg(errp, "%s", strerror(-ret));
>>> + goto fail;
>>> }
>>> +
>>> + object_set_accelerator_compat_props(acc->compat_props);
>>> + return ret;
>>> +
>>> +fail:
>>> + ms->accelerator = NULL;
>>> + *(acc->allowed) = false;
>>> + object_unref(OBJECT(accel));
>>> return ret;
>>> }
>>>
>>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>>> index 657403cad011..f9847b5e7096 100644
>>> --- a/hw/scsi/vhost-scsi.c
>>> +++ b/hw/scsi/vhost-scsi.c
>>> @@ -262,12 +262,15 @@ static void vhost_scsi_realize(DeviceState *dev,
>>> Error **errp)
>>> }
>>>
>>> if (!vsc->migratable) {
>>> + unsigned int modes = BIT(MIG_MODE_NORMAL) | BIT(MIG_XEN);
>>> +
>>> error_setg(&vsc->migration_blocker,
>>> "vhost-scsi does not support migration in all cases. "
>>> "When external environment supports it (Orchestrator
>>> migrates "
>>> "target SCSI device state or use shared storage over
>>> network), "
>>> "set 'migratable' property to true to enable migration.");
>>> - if (migrate_add_blocker_normal(&vsc->migration_blocker, errp) < 0)
>>> {
>>> + if (migrate_add_blocker_modes(&vsc->migration_blocker,
>>> + modes, errp) < 0) {
>>> goto free_virtio;
>>> }
>>> }
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index d0b864a76066..c177dc52a1c1 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -23,6 +23,7 @@
>>> #include "system/runstate.h"
>>> #include "system/system.h"
>>> #include "system/cpu-throttle.h"
>>> +#include "system/xen.h"
>>> #include "ram.h"
>>> #include "migration/cpr.h"
>>> #include "migration/global_state.h"
>>> @@ -94,7 +95,8 @@ enum mig_rp_message_type {
>>> static MigrationState *current_migration;
>>> static MigrationIncomingState *current_incoming;
>>>
>>> -static GSList *migration_blockers[MIG_MODE__MAX];
>>> +static GSList *migration_blockers[MIG_MODE__MAX + 1];
>>> +static bool enforcing_only_migratable;
I think I missed the need for this variable. Isn't only_migratable
enough?
>>>
>>> static bool migration_object_check(MigrationState *ms, Error **errp);
>>> static bool migration_switchover_start(MigrationState *s, Error **errp);
>>> @@ -1760,15 +1762,22 @@ static bool is_busy(Error **reasonp, Error **errp)
>>> return false;
>>> }
>>>
>>> +static void propagate_disallowed_blocker(Error **errp, Error **reasonp)
>>> +{
>>> + error_propagate_prepend(errp, *reasonp,
>>> + "disallowing migration blocker "
>>> + "(--only-migratable) for: ");
>>> + *reasonp = NULL;
>>> +}
>>> +
>>> static bool is_only_migratable(Error **reasonp, unsigned modes, Error
>>> **errp)
>>> {
>>> + unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>> +
Hmm, the original code is actually special casing CPR isn't it? I'd
prefer to be explicit instead:
unsigned int ignore_only_migratable = (BIT(MIG_MODE_CPR_TRANSFER) |
BIT(MIG_MODE_CPR_EXEC) |
BIT(MIG_MODE_CPR_REBOOT));
unsigned int cur_mode = blocker_mode() & ~ignore_only_migratable;
Where blocker_mode is:
/*
* Migration blockers are scoped by migration mode. Nonetheless it's
* still possible for an accelerator to have particularities that
* require blocking migration, independently of mode. Use this helper
* when indexing into migration_blockers to ensure the accelerator is
* taken into account.
*/
static unsigned int *blocker_mode(void)
{
if (phase_check(PHASE_ACCEL_CREATED)) {
if (xen_enabled()) {
return MIG_BLOCK_XEN;
}
}
return migrate_mode();
}
>>> ERRP_GUARD();
>>>
>>> - if (only_migratable && (modes & BIT(MIG_MODE_NORMAL))) {
>>> - error_propagate_prepend(errp, *reasonp,
>>> - "disallowing migration blocker "
>>> - "(--only-migratable) for: ");
>>> - *reasonp = NULL;
>>> + if (enforcing_only_migratable && (modes & BIT(mode))) {
>>> + propagate_disallowed_blocker(errp, reasonp);
>>> return true;
>>> }
>>> return false;
>>> @@ -1776,7 +1785,7 @@ static bool is_only_migratable(Error **reasonp,
>>> unsigned modes, Error **errp)
>>>
>>> static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>>> {
>>> - for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>> + for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers); mode++) {
>>
>> Not a MigMode anymore.
>
> Good catch. I will fix the type in the next version.
>
>>
>>> if (modes & BIT(mode)) {
>>> assert(g_slist_index(migration_blockers[mode],
>>> *reasonp) == -1);
>>> @@ -1809,7 +1818,7 @@ int migrate_add_blocker_modes(Error **reasonp,
>>> unsigned modes, Error **errp)
>>>
>>> int migrate_add_blocker_internal(Error **reasonp, Error **errp)
>>> {
>>> - unsigned modes = BIT(MIG_MODE__MAX) - 1;
>>> + unsigned modes = BIT(ARRAY_SIZE(migration_blockers)) - 1;
>>>
>>> if (is_busy(reasonp, errp)) {
>>> return -EBUSY;
>>> @@ -1820,7 +1829,7 @@ int migrate_add_blocker_internal(Error **reasonp,
>>> Error **errp)
>>> void migrate_del_blocker(Error **reasonp)
>>> {
>>> if (*reasonp) {
>>> - for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>>> + for (MigMode mode = 0; mode < ARRAY_SIZE(migration_blockers);
>>> mode++) {
>>
>> Same here, just use int i.
>
> Will update the variable name and type accordingly.
>
>>
>>> migration_blockers[mode] =
>>> g_slist_remove(migration_blockers[mode],
>>> *reasonp);
>>> }
>>> @@ -1948,6 +1957,26 @@ void qmp_migrate_pause(Error **errp)
>>> "during postcopy-active or postcopy-recover state");
>>> }
>>>
>>> +bool enforce_only_migratable(Error **errp)
>>> +{
>>> + unsigned mode = xen_enabled() ? MIG_XEN : MIG_MODE_NORMAL;
>>> + GSList *blockers = migration_blockers[mode];
>>
>> For Xen, what blockers are those? Is it the case that existing blockers
>> are simply not taking effect for Xen because the QMP command never
>> checks blockers? Or do you intend to add xen-specific blockers?
>
> The former.
>
> Existing migration blockers currently do not take effect for Xen, which
> this patch fixes. However, applying all blockers from an existing
> MigMode could cause a regression for the reasons stated above.
> Therefore, this patch ensures that only blockers relevant to Xen are
> applied.
>
> Specifically, it enforces the blockers added via migrate_add_blocker()
> and the vhost-scsi blocker. The remaining blockers have not been proven
> incompatible with Xen migration, so applying them unconditionally could
> cause regressions.
>
> Regards,
> Akihiko Odaki
>
>>
>>> + Error *reason;
>>> +
>>> + if (!only_migratable) {
>>> + return true;
>>> + }
>>> +
>>> + if (blockers) {
>>> + reason = error_copy(blockers->data);
>>> + propagate_disallowed_blocker(errp, &reason);
>>> + return false;
>>> + }
>>> +
>>> + enforcing_only_migratable = true;
>>> + return true;
>>> +}
We could reduce the scope of migration_is_blocked so
enforce_only_migrate can call it instead of checking the blockers
itself:
// replaces migration_is_blocked
bool can_migrate(Error **errp)
{
if (qemu_savevm_state_blocked(errp)) {
return false;
}
if (migration_is_blocked(errp) {
return false;
}
return true;
}
static bool migration_is_blocked(Error **errp)
{
GSList *blockers;
assert(phase_check(PHASE_ACCEL_CREATED));
blockers = migration_blockers[blocker_mode()];
if (blockers) {
error_propagate(errp, error_copy(blockers->data));
return true;
}
return false;
}
>>> +
>>> bool migration_is_blocked(Error **errp)
>>> {
>>> GSList *blockers = migration_blockers[migrate_mode()];
>>> @@ -1964,6 +1993,22 @@ bool migration_is_blocked(Error **errp)
>>> return false;
>>> }
>>>
>>> +bool xen_migration_is_blocked(Error **errp)
>>> +{
>>> + GSList *blockers = migration_blockers[MIG_XEN];
Does this need to use MIG_XEN? If we use something like the
blocker_mode() I suggested, then this function can be replaced with
can_migrate() and we make all the points that start migration uniform.
What do you think? Let me know if I missed any details.
Thanks
>>> +
>>> + if (qemu_savevm_state_blocked(errp)) {
>>> + return true;
>>> + }
>>> +
>>> + if (blockers) {
>>> + error_propagate(errp, error_copy(blockers->data));
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> /* Returns true if continue to migrate, or false if error detected */
>>> static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>> {
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index 4b590ea672d2..4e5e1af1b202 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -3418,6 +3418,10 @@ void qmp_xen_save_devices_state(const char
>>> *filename, bool has_live, bool live,
>>> int saved_vm_running;
>>> int ret;
>>>
>>> + if (xen_migration_is_blocked(errp)) {
>>> + return;
>>> + }
>>> +
>>> if (!has_live) {
>>> /* live default to true so old version of Xen tool stack can have
>>> a
>>> * successful live migration */
>>> diff --git a/system/vl.c b/system/vl.c
>>> index 9bd7664b85c7..21fa810637c3 100644
>>> --- a/system/vl.c
>>> +++ b/system/vl.c
>>> @@ -2392,6 +2392,7 @@ static int accelerator_set_property(void *opaque,
>>>
>>> static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error
>>> **errp)
>>> {
>>> + Error *local_err = NULL;
>>> bool *p_init_failed = opaque;
>>> const char *acc = qemu_opt_get(opts, "accel");
>>> AccelClass *ac = accel_find(acc);
>>> @@ -2418,10 +2419,12 @@ static int do_configure_accelerator(void *opaque,
>>> QemuOpts *opts, Error **errp)
>>> accel,
>>> &error_fatal);
>>>
>>> - ret = accel_init_machine(accel, current_machine);
>>> + ret = accel_init_machine(accel, current_machine, &local_err);
>>> if (ret < 0) {
>>> - if (!qtest_with_kvm || ret != -ENOENT) {
>>> - error_report("failed to initialize %s: %s", acc,
>>> strerror(-ret));
>>> + if (qtest_with_kvm && ret == -ENOENT) {
>>> + error_free(local_err);
>>> + } else {
>>> + error_reportf_err(local_err, "failed to initialize %s: ", acc);
>>> }
>>> goto bad;
>>> }
