Re: [PATCH RFC 2/2] migration/cpr: Opt-in "mode" parameter for early boot access

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 28, 2026 at 08:24:23PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Make "mode" to be the first parameter to opt-in for early boot access.  CPR
>> > will start to consume this early boot parameter.  With this, we can remove
>> > the special "incoming_mode" variable together with cpr_get_incoming_mode().
>> >
>> 
>> Hmm, it's now possible to invoke QEMU with:
>> 
>> -incoming config:mode=cpr-exec,cpr-exec-command=-incoming config:
>> 
>> Seems like it could be an issue.
>
> Do you mean when being abused, or some issue that can happen with existing
> cpr-exec?
>

Abused. Or just overall user confusion over the syntax.

You imply in this patch that we want the users to eventually set the
mode in the -incoming option instead of relying in the automatic
detection at cpr_state_load(). Ok.

Then it's natural that the user will also want to set cpr-exec-command
via the -incoming option. But the definition of cpr-exec-command says
that it must include the -incoming option for the new machine. So now
the -incoming option has to deal with a potentially huge string. It's
also undefined what happens if the second commad line also contains
cpr-exec-command in it.

> For normal cpr-exec operation, we should only pass in -incoming file:*.  No
> config should be passed in.
>

Hmm, but there's no "should" here, is there? This patch allows the mode
to be set.

> But indeed if we want to decouple the implied "mode=" change for cpr-exec
> and cpr-transfer, we may want to also have "-incoming config:mode=cpr-exec"
> in the cpr-exec-command=.  But still, I don't see an issue so far.
>

Ok, keep it in mind though.

> If we think that's too much, we can still stick with the
> cpr_set_incoming_mode() implicitly setting mode for cpr-*.
>

Meh.. It's fine either way I think.

>> 
>> > One thing to mention is, to make cpr_is_incoming() work like before, we
>> > need to do extra check on INMIGRATE runstate to make sure it only returns
>> > true while during the incoming migration.
>> 
>> ...
>> 
>> > It used to be achieved by a
>> > pretty hackish "cpr_set_incoming_mode(MIG_MODE_NONE)" when incoming
>> > migration destroys.  Now we can remove it.
>> 
>> That's not it, I think. cpr_is_incoming() only returns true on incoming
>> migration because on outgoing there's no parsing of -incoming and
>> therefore cpr_state_load() doesn't call cpr_set_incoming_mode().
>
> Yes, I believe it's done by both these steps: cpr_set_incoming_mode() start
> to make it return true, then the other set of MIG_MODE_NONE (that I
> mentioned above) to explicitly turn it off.  Now with INMIGRATE check it
> should hopefully keep the exact same behavior.
>

Well, the new QEMU could be used to live update again, then the mode
should indeed be reset. Might as well have been made part of
migrate_prepare. I don't see the need to clear it like that aside from a
general cleanup action. Whatever, it's not relevant to this discussion.

>> 
>> >
>> > Another good side effect is, we can finally drop MIG_MODE_NONE: it was
>> > never a valid value, only used by the temp global variable.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  include/migration/cpr.h |  3 ---
>> >  migration/migration.h   |  3 +++
>> >  migration/cpr.c         | 18 +++++++++---------
>> >  migration/migration.c   | 37 ++++++++++++++++++++++++++++++++++++-
>> >  migration/options.c     | 10 +++-------
>> >  5 files changed, 51 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>> > index 56fb67e6b4..27061ad629 100644
>> > --- a/include/migration/cpr.h
>> > +++ b/include/migration/cpr.h
>> > @@ -12,8 +12,6 @@
>> >  #include "io/channel.h"
>> >  #include "qemu/queue.h"
>> >  
>> > -#define MIG_MODE_NONE           -1
>> > -
>> >  #define QEMU_CPR_FILE_MAGIC     0x51435052
>> >  #define QEMU_CPR_FILE_VERSION   0x00000001
>> >  #define CPR_STATE "CprState"
>> > @@ -38,7 +36,6 @@ int cpr_open_fd(const char *path, int flags, const char *name, int id,
>> >  typedef bool (*cpr_walk_fd_cb)(int fd);
>> >  bool cpr_walk_fd(cpr_walk_fd_cb cb);
>> >  
>> > -MigMode cpr_get_incoming_mode(void);
>> >  void cpr_set_incoming_mode(MigMode mode);
>> >  bool cpr_is_incoming(void);
>> >  
>> > diff --git a/migration/migration.h b/migration/migration.h
>> > index 841f49b215..9fa97f6d9a 100644
>> > --- a/migration/migration.h
>> > +++ b/migration/migration.h
>> > @@ -609,4 +609,7 @@ void migration_bitmap_sync_precopy(bool last_stage);
>> >  void dirty_bitmap_mig_init(void);
>> >  bool should_send_vmdesc(void);
>> >  
>> > +void migration_parameters_boot_set_mode(MigMode mode);
>> > +MigrationParameters *migration_get_parameters(void);
>> > +
>> >  #endif
>> > diff --git a/migration/cpr.c b/migration/cpr.c
>> > index bca43e9bf3..1f49afe109 100644
>> > --- a/migration/cpr.c
>> > +++ b/migration/cpr.c
>> > @@ -16,6 +16,7 @@
>> >  #include "migration/qemu-file.h"
>> >  #include "migration/savevm.h"
>> >  #include "migration/vmstate.h"
>> > +#include "migration/migration.h"
>> >  #include "monitor/monitor.h"
>> >  #include "system/runstate.h"
>> >  #include "trace.h"
>> > @@ -239,21 +240,20 @@ QIOChannel *cpr_state_ioc(void)
>> >      return qemu_file_get_ioc(cpr_state_file);
>> >  }
>> >  
>> > -static MigMode incoming_mode = MIG_MODE_NONE;
>> > -
>> > -MigMode cpr_get_incoming_mode(void)
>> > -{
>> > -    return incoming_mode;
>> > -}
>> > -
>> >  void cpr_set_incoming_mode(MigMode mode)
>> >  {
>> > -    incoming_mode = mode;
>> > +    migration_parameters_boot_set_mode(mode);
>> >  }
>> >  
>> >  bool cpr_is_incoming(void)
>> >  {
>> > -    return incoming_mode != MIG_MODE_NONE;
>> > +    MigMode mode = migrate_mode();
>> > +
>> > +    if (!runstate_check(RUN_STATE_INMIGRATE)) {
>> > +        return false;
>> > +    }
>> > +
>> > +    return mode == MIG_MODE_CPR_EXEC || mode == MIG_MODE_CPR_TRANSFER;
>> >  }
>> >  
>> >  bool cpr_state_save(MigrationChannel *channel, Error **errp)
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index d918be7a44..d7591571ed 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -145,6 +145,42 @@ static void migration_parameters_boot_apply(void)
>> >      }
>> >  }
>> >  
>> > +static void migration_parameters_boot_init(void)
>> > +{
>> > +    if (!mig_boot_params) {
>> > +        mig_boot_params = g_new0(MigrationParameters, 1);
>> > +    }
>> > +}
>> > +
>> > +/*
>> > + * This should only be used during boot by CPR.  NOTE: This is only needed
>> > + * to be compatible with old CPR use case, if we decide to have users
>> > + * switch to -incoming config:mode=cpr-* then this can be removed.
>> 
>> This new scheme does create some usability weirdness of allowing a
>> parameter to be set in -incoming and then to be changed later with
>> migrate-set-parameters. Hopefully it's ok.
>
> I assume it's OK?  It's the same as -global migration.* in this case,
> updates to global parameters.
>
>> 
>> > + */
>> > +void migration_parameters_boot_set_mode(MigMode mode)
>> > +{
>> > +    assert(!current_migration);
>> > +    migration_parameters_boot_init();
>> > +    mig_boot_params->has_mode = true;
>> > +    mig_boot_params->mode = mode;
>> 
>> I bet there's some visitor trick that could make this function generic
>> for all parameters.
>
> Yeah, that's why I relied on the qmp_migrate_set_parameters() API to apply
> these configs in patch 1, waiting for you to introduce that trick in your
> other series to not hard-code has_* checks. :)
>
> But IMHO it's not needed here: we shouldn't even need this helper if we
> don't implicitly set mode..  The plan is we should never need another
> parameter for such helper.. hence I made it directly accessing it.
>

ok

>> 
>> > +}
>> > +
>> > +/*
>> > + * Get the effective migration parameter object.
>> > + *
>> > + * Three possibilities:
>> > + * - Migration object has been initialized, always use it, or,
>> > + * - Migration boot parameters are initialized, then use it, or,
>> > + * - return NULL
>> 
>> Hmm, can't we create the early parameters unconditionally so there's no
>> NULL case here? A bit annoying having to check for NULL for (eventually)
>> every parameter, even on the source side.
>
> We could do that, but remember the default values for parameters are only
> applied so far until migration objects init (migration_properties, when
> qdev walks it over; unfortunately still a TYPE_DEVICE).
>

Sorry, you kind of lost me. What do the defaults have to do with
allocating mig_boot_params? Do you mean mig_boot_params cannot really be
used interchangeably with s->parameters because it doesn't have any
defaults set?

> If you want me to finish that part first I can try too; If we plan to merge
> the other incoming-fds to TAP then there's also no rush for this work, we
> can add some pre-requisite changes.
>
> Or, this series can also be the minimum to drop TAP's incoming-fds, then we
> cleanup this NULL case later.
>

Sure, let's help with Vladimir's work. But "migration_get_parameters"
returning something other than a valid MigrationParameters is prone to
cause issues down the line. It's not obvious that the early parameters
are only valid on the incoming side and also not obvious that the
function might be called too early even for early parameters.

>> 
>> > + *
>> > + * Callers should always check non-NULL pointer first before use.
>> > + */
>> > +MigrationParameters *migration_get_parameters(void)
>> > +{
>> > +    return current_migration ?
>> > +        &current_migration->parameters : mig_boot_params;
>> > +}
>> 
>> Feels like time to drop the global parameters already. It's a weird
>> cognitive load having to grasp that there's two MigrationParameters,
>> both used in outgoing and incoming, but one can only be set in
>> incoming. The one stored in current_migration might be used
>> interchangeably with the one that's flying around. One lives inside and
>> shares lifetime with MigrationState, the other is dynamically allocated
>> and has a shorter lifetime.
>
> The idea is making mig_boot_params so temprary so nobody notices.
>
> Note that we have at least these layers of values to apply on top of the
> parameters:
>
> a) default value (via migration_properties, discussed above)
> b) machine type compat value (via instance_post_init() of mig obj)
> c) "-incoming config:*" that this series introduced
>
> IIUC, the sane way to prioritize these values is having 3) higher priority
> than 1) and 2).  In this case, we will need a buffering mechanism anyway
> when:
>
> - take early migration parameters, we make sure parameters available almost
>   since the start
>
> - ... qemu initializes machine compat properties ...
>
> - migration object init, apply machine compat properties
>
> When applying machine compat properties, the migration parameter object
> cannot be the same that persisted the boot values, so we likely always need
> two of them so that after apply compat properties we use boot parameters to
> overwrite them.
>

Or we move MigrationParameters out of MigrationState and use it however
we want. At migration_instance_init() add a:

params->zero_page_detection = ms->zero_page_detection;

and that's it.

Of course, it disables the -global usage for setting random
parameters. But the compat properties remain in migration_properties
working like today.

I understand it's a whole can of worms by itself, but don't you think
it's worth it? It would allow to use a single MigrationParameters object
all over.

>> 
>> > +
>> >  static void migration_downtime_start(MigrationState *s)
>> >  {
>> >      trace_vmstate_downtime_checkpoint("src-downtime-start");
>> > @@ -546,7 +582,6 @@ void migration_incoming_state_destroy(void)
>> >          mis->postcopy_qemufile_dst = NULL;
>> >      }
>> >  
>> > -    cpr_set_incoming_mode(MIG_MODE_NONE);
>> >      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>> >  }
>> >  
>> > diff --git a/migration/options.c b/migration/options.c
>> > index 5cbfd29099..c1209cbec3 100644
>> > --- a/migration/options.c
>> > +++ b/migration/options.c
>> > @@ -886,16 +886,12 @@ uint64_t migrate_max_postcopy_bandwidth(void)
>> >      return s->parameters.max_postcopy_bandwidth;
>> >  }
>> >  
>> > +/* Opt-in "mode" parameter to be available even during boot */
>> >  MigMode migrate_mode(void)
>> >  {
>> > -    MigMode mode = cpr_get_incoming_mode();
>> > +    MigrationParameters *params = migration_get_parameters();
>> >  
>> > -    if (mode == MIG_MODE_NONE) {
>> > -        mode = migrate_get_current()->parameters.mode;
>> > -    }
>> > -
>> > -    assert(mode >= 0 && mode < MIG_MODE__MAX);
>> > -    return mode;
>> > +    return params ? params->mode : MIG_MODE_NORMAL;
>> >  }
>> >  
>> >  int migrate_multifd_channels(void)
>>