Re: [Qemu-devel] [PATCH v5 18/45] MIG_CMD_PACKAGED: Send a packaged chunk of migration stream

[David Gibson]

On Fri, Mar 13, 2015 at 11:51:42AM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (address@hidden) wrote:
> > On Wed, Feb 25, 2015 at 04:51:41PM +0000, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > 
> > > MIG_CMD_PACKAGED is a migration command that allows a chunk
> > > of migration stream to be sent in one go, and be received by
> > > a separate instance of the loadvm loop while not interacting
> > > with the migration stream.
> > 
> > Hrm.  I'd be more comfortable if the semantics of CMD_PACKAGED were
> > defined in terms of visible effects on the other end, rather than in
> > terms of how it's implemented internally.
> > 
> > > This is used by postcopy to load device state (from the package)
> > > while loading memory pages from the main stream.
> > 
> > Which makes the above paragraph a bit misleading - the whole point
> > here is that loading the package data *does* interact with the
> > migration stream - just that it's the migration stream after the end
> > of the package.
> 
> Hmm, how about:
> 
> 
> MIG_CMD_PACKAGED is a migration command that wraps a chunk of migration
> stream inside a package whose length can be determined purely by reading
> its header.  The destination guarantees that the whole MIG_CMD_PACKAGED is
> read off the stream prior to parsing the contents.
> 
> This is used by postcopy to load device state (from the package)
> while leaving the main stream free to receive memory pages.

It's an improvement.

I'm still a bit concerned that the semantics of CMD_PACKAGED are
unhealthily bound up with hw the current implementation works.

[snip]
> > > +/* Immediately following this command is a blob of data containing an 
> > > embedded
> > > + * chunk of migration stream; read it and load it.
> > > + */
> > > +static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis,
> > > +                                      uint32_t length)
> > > +{
> > > +    int ret;
> > > +    uint8_t *buffer;
> > > +    QEMUSizedBuffer *qsb;
> > > +
> > > +    trace_loadvm_handle_cmd_packaged(length);
> > > +
> > > +    if (length > MAX_VM_CMD_PACKAGED_SIZE) {
> > > +        error_report("Unreasonably large packaged state: %u", length);
> > > +        return -1;
> > 
> > It would be a good idea to check this on the send side as well as
> > receive, wouldn't it?
> 
> Yes, I had been doing that in the postcopy code that called the
> send code; but I've now moved it down into savevm_send_packaged.

Good, better symmetry that way.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpeagjAznuVT.pgp
Description: PGP signature