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?
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.
> 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.
> *
> * @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.
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?
> +
> 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;
>
> 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;
> +
> 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.
> 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.
> 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?
> + 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;
> +}
> +
> 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];
> +
> + 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;
> }
