From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists1p.gnu.org (lists1p.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E6166C98314 for ; Thu, 24 Sep 2026 12:39:12 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists1p.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1x9ij6-0002Bc-Vf; Thu, 24 Sep 2026 08:39:01 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists1p.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1x9ij5-0002Ax-7Y for qemu-devel@nongnu.org; Thu, 24 Sep 2026 08:38:59 -0400 Received: from smtp-out1.suse.de ([2a07:de40:b251:101:10:150:64:1]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1x9ij1-0000TS-Pl for qemu-devel@nongnu.org; Thu, 24 Sep 2026 08:38:58 -0400 Received: from imap1.dmz-prg2.suse.org (unknown [10.150.64.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 1D5E521AE5; Thu, 24 Sep 2026 12:38:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1790253528; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=o5v/WItKUJ7GZEcUTVswQiwU8anCqeSzc/WozXuKiHI=; b=lEMFbprwPFn6JeCRyx0pFyDSoDw5lcbglzlQSLTUPgzlxHds3x1l6brAltM/lTRHIp+iE0 qvIdmPU35r16KxKKenf8P7TzY0XBGQDtN9Mhr1d5N7+IfZziwP0QhTvkXJTZ8MJAATEmnj ReNM4mrwGNvGqPGQoJbdV/Afb6mRTbY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1790253528; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=o5v/WItKUJ7GZEcUTVswQiwU8anCqeSzc/WozXuKiHI=; b=+0xQgUS4MSS5KSiJ0kC1PDS1FZSaZgKNpwcn9+WRaxB7Crk1YlMztB92Heo3k0hQePRRQH o6tIOOd1Z1lpYIBA== Authentication-Results: smtp-out1.suse.de; none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1790253524; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=o5v/WItKUJ7GZEcUTVswQiwU8anCqeSzc/WozXuKiHI=; b=F5CPjwlS+2+WA77yvW+/d2QoI7V87gszQxr+YmXLrbnMysCqicEqVvL8U/vL/9wqVR2VKp r+gj5pvmlY+uRY6lZ1BH8K9w3Vwaa2oHNPhYvpCg9o72W7AnKeIAmITOqXXCRNDGx22l7t xm/f/2QJ+OUct4EPWo9U0bK3qiytWqM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1790253524; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=o5v/WItKUJ7GZEcUTVswQiwU8anCqeSzc/WozXuKiHI=; b=A6OGEM0blCYEnP2KpvKRIhjGbotTCE0HWgw12SASZUoq3BnfJufYHZdvVZHThGc9zK6GP4 9+dCQHkK3f96ccBQ== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 61EB9136ED; Thu, 24 Sep 2026 12:38:43 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id eNPvAdMZtWpmcgAAD6G6ig (envelope-from ); Thu, 24 Sep 2026 12:38:43 +0000 From: Fabiano Rosas To: Akihiko Odaki , qemu-devel@nongnu.org Cc: xen-devel@lists.xenproject.org, Stefan Berger , "Michael S. Tsirkin" , Stefano Garzarella , Paolo Bonzini , Fam Zheng , Peter Xu , Richard Henderson , Stefano Stabellini , Anthony PERARD , "Edgar E. Iglesias" , Philippe =?utf-8?Q?Mathieu-Daud=C3=A9?= Subject: Re: [PATCH 2/2] migration: Implement Xen migration blocker In-Reply-To: References: <20260912-xen-v1-0-ebd6a2aa519f@rsg.ci.i.u-tokyo.ac.jp> <20260912-xen-v1-2-ebd6a2aa519f@rsg.ci.i.u-tokyo.ac.jp> <87v77x9x8u.fsf@suse.de> <78984db5-6de9-40c7-abf4-884c539232da@rsg.ci.i.u-tokyo.ac.jp> <87fqyzabx7.fsf@suse.de> Date: Thu, 24 Sep 2026 09:38:36 -0300 Message-ID: <877bkaakoj.fsf@suse.de> MIME-Version: 1.0 Content-Type: text/plain X-Spamd-Result: default: False [-2.80 / 50.00]; BAYES_HAM(-3.00)[100.00%]; SUSPICIOUS_RECIPS(1.50)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.20)[-1.000]; MIME_GOOD(-0.10)[text/plain]; FREEMAIL_ENVRCPT(0.00)[gmail.com]; TO_MATCH_ENVRCPT_ALL(0.00)[]; URIBL_BLOCKED(0.00)[suse.de:mid,imap1.dmz-prg2.suse.org:helo,xen.org:url,u-tokyo.ac.jp:email]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; MIME_TRACE(0.00)[0:+]; ARC_NA(0.00)[]; RCPT_COUNT_TWELVE(0.00)[14]; FREEMAIL_CC(0.00)[lists.xenproject.org,linux.vnet.ibm.com,redhat.com,euphon.net,linaro.org,kernel.org,xenproject.org,gmail.com,oss.qualcomm.com]; RCVD_TLS_ALL(0.00)[]; MISSING_XM_UA(0.00)[]; FROM_EQ_ENVFROM(0.00)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; TAGGED_RCPT(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; DBL_BLOCKED_OPENRESOLVER(0.00)[xen.org:url, suse.de:mid, imap1.dmz-prg2.suse.org:helo, u-tokyo.ac.jp:email] Received-SPF: pass client-ip=2a07:de40:b251:101:10:150:64:1; envelope-from=farosas@suse.de; helo=smtp-out1.suse.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: qemu development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Akihiko Odaki writes: > On 2026/09/24 6:35, Fabiano Rosas wrote: >> Akihiko Odaki writes: >> >>> On 2026/09/22 23:28, Fabiano Rosas wrote: >>>> Akihiko Odaki 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 >>>>> --- >>>>> 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 > > Good idea. That will also fix the documentation comments, along with the > variable and function names. I will include this change in the next version. > >> >>>> >>>>> * >>>>> * @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(); >> ... > > I agree. We can also add a comment here explaining that > -only-migratable needs to be enforced at this point. > See if you can structure these changes in a way that we first change the behavior regarding when only_migratable is enforced, then later add the Xen support. So we make it clear in the commit log how this affects the normal mode. >> >>>> >>>>> + >>>>> 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? > > enforcing_only_migratable is initially false, allowing blockers to be > registered without immediate enforcement. > > Once enforce_only_migratable() is called, the existing blockers are > validated, and this flag becomes true. Any blocker added later are then > checked immediately. Together, this ensures all blockers are > consistently enforced at runtime. > I see, otherwise we'll reject a blocker for MIG_MODE_NORMAL when in reality the vm could be running Xen and therefore not want to reject that blocker. >> >>>>> >>>>> 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; > > blocker_mode() returns an index, whereas ignore_only_migratable is a > bitmask, so we'll need to adapt the bitwise operations accordingly to > handle them properly. > Well spotted. >> >> 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)) { > > It would be safer to use assert(phase_check(PHASE_ACCEL_CREATED)); here; > otherwise, the returned value may not be valid. > If we want to allow blockers to be registered before accel being configured, then we cannot assert. >> if (xen_enabled()) { >> return MIG_BLOCK_XEN; >> } >> } >> >> return migrate_mode(); >> } > > Aside from the minor adjustments mentioned above, explicitly > special-casing CPR looks good to me overall. > >> >>>>> 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; >> } > > This might be bikeshedding, but I'd rather extract the logic to check > migration_blockers to a new function, perhaps named > has_migration_blockers(). > > I feel can_migrate() is a bit confusing since migrate_prepare() actually > has more checks and it does not clearly differentiate itself from > migration_is_blocked() well. Updating migration/savevm.c would also be > slightly cumbersome. > > While has_migration_blockers() doesn't perfectly differentiate itself > from migration_is_blocked() either, aligning it with the existing > "migration_blockers" variable clarifies its exact intent. > Sure, go ahead. Maybe migration_active_blockers or some other word to indicate the blockers affect the mode/accel currently in effect. >> >>>>> + >>>>> 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. > > That makes sense to me. I'll incorporate this approach in the next version. > Thanks! See you in v2. > Regards, > Akihiko Odaki > >> >> 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; >>>>> }