On 2/4/2025 11:26 AM, Peter Xu wrote:
On Tue, Feb 04, 2025 at 01:40:34PM +0000, Peter Maydell wrote:
On Wed, 29 Jan 2025 at 16:11, Fabiano Rosas <faro...@suse.de> wrote:

From: Steve Sistare <steven.sist...@oracle.com>

Add the cpr-transfer migration mode, which allows the user to transfer
a guest to a new QEMU instance on the same host with minimal guest pause
time, by preserving guest RAM in place, albeit with new virtual addresses
in new QEMU, and by preserving device file descriptors.  Pages that were
locked in memory for DMA in old QEMU remain locked in new QEMU, because the
descriptor of the device that locked them remains open.

cpr-transfer preserves memory and devices descriptors by sending them to
new QEMU over a unix domain socket using SCM_RIGHTS.  Such CPR state cannot
be sent over the normal migration channel, because devices and backends
are created prior to reading the channel, so this mode sends CPR state
over a second "cpr" migration channel.  New QEMU reads the cpr channel
prior to creating devices or backends.  The user specifies the cpr channel
in the channel arguments on the outgoing side, and in a second -incoming
command-line parameter on the incoming side.

The user must start old QEMU with the the '-machine aux-ram-share=on' option,
which allows anonymous memory to be transferred in place to the new process
by transferring a memory descriptor for each ram block.  Memory-backend
objects must have the share=on attribute, but memory-backend-epc is not
supported.

The user starts new QEMU on the same host as old QEMU, with command-line
arguments to create the same machine, plus the -incoming option for the
main migration channel, like normal live migration.  In addition, the user
adds a second -incoming option with channel type "cpr".  This CPR channel
must support file descriptor transfer with SCM_RIGHTS, i.e. it must be a
UNIX domain socket.

To initiate CPR, the user issues a migrate command to old QEMU, adding
a second migration channel of type "cpr" in the channels argument.
Old QEMU stops the VM, saves state to the migration channels, and enters
the postmigrate state.  New QEMU mmap's memory descriptors, and execution
resumes.

The implementation splits qmp_migrate into start and finish functions.
Start sends CPR state to new QEMU, which responds by closing the CPR
channel.  Old QEMU detects the HUP then calls finish, which connects the
main migration channel.

In summary, the usage is:

   qemu-system-$arch -machine aux-ram-share=on ...

   start new QEMU with "-incoming <main-uri> -incoming <cpr-channel>"

   Issue commands to old QEMU:
     migrate_set_parameter mode cpr-transfer

     {"execute": "migrate", ...
         {"channel-type": "main"...}, {"channel-type": "cpr"...} ... }

Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
Reviewed-by: Peter Xu <pet...@redhat.com>
Acked-by: Markus Armbruster <arm...@redhat.com>
Link: 
https://lore.kernel.org/r/1736967650-129648-17-git-send-email-steven.sist...@oracle.com
Signed-off-by: Fabiano Rosas <faro...@suse.de>

Hi; this commit includes some code that has confused
Coverity (CID 1590980) and it also confused me, so maybe
it could be usefully made clearer?


  void qmp_migrate(const char *uri, bool has_channels,
                   MigrationChannelList *channels, bool has_detach, bool detach,
                   bool has_resume, bool resume, Error **errp)
@@ -2056,6 +2118,7 @@ void qmp_migrate(const char *uri, bool has_channels,
      g_autoptr(MigrationChannel) channel = NULL;
      MigrationAddress *addr = NULL;
      MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
+    MigrationChannel *cpr_channel = NULL;

      /*
       * Having preliminary checks for uri and channel
@@ -2076,6 +2139,7 @@ void qmp_migrate(const char *uri, bool has_channels,
              }
              channelv[type] = channels->value;
          }
+        cpr_channel = channelv[MIGRATION_CHANNEL_TYPE_CPR];
          addr = channelv[MIGRATION_CHANNEL_TYPE_MAIN]->addr;
          if (!addr) {
              error_setg(errp, "Channel list has no main entry");
@@ -2096,12 +2160,52 @@ void qmp_migrate(const char *uri, bool has_channels,
          return;
      }

+    if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) {
+        error_setg(errp, "missing 'cpr' migration channel");
+        return;
+    }

Here in qmp_migrate() we bail out if cpr_channel is NULL,
provided that s->parameters.mode is MIG_MODE_CPR_TRANSFER...

+
      resume_requested = has_resume && resume;
      if (!migrate_prepare(s, resume_requested, errp)) {
          /* Error detected, put into errp */
          return;
      }

+    if (cpr_state_save(cpr_channel, &local_err)) {

...but in cpr_state_save() when we decide whether to dereference
cpr_channel or not, we aren't checking s->parameters.mode,
we call migrate_mode() and check the result of that.
And migrate_mode() isn't completely trivial: it calls
cpr_get_incoming_mode(), so it's not obvious that it's
necessarily going to be the same value as s->parameters.mode.
So Coverity complains that it sees a code path where we
might dereference cpr_channel even when it's NULL.

Could this be made a bit clearer somehow, do you think?

That migrate_mode() is indeed tricky, and should only be needed for
incoming side QEMU to workaround current limitation that the migration
parameter "mode" cannot be set as early as when cpr_state_load() happens..

I think we could check s->parameters.mode here before doing
cpr_state_save(), it can also be more readable.

Steve, do you want to send a patch?

I am busy today but I will submit a patch tomorrow.  cpr_state_save
is only used on the outgoing side, so internally it can check
s->parameters.mode instead of migrate_mode().

- Steve


Reply via email to