Re: [PATCH v4 0/9] Migration Stream v2

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 01/05/14 17:41, Konrad Rzeszutek Wilk wrote:
> On Wed, Apr 30, 2014 at 07:36:43PM +0100, Andrew Cooper wrote:
>> Hello,
>>
>> Presented here for review is v4 of the Migration Stream v2 work.  David,
>> Frediano and myself have worked together on the implementation, and we have
>> now managed to get PV and HVM migration working using the new format (even
>> when transparently inserted underneath Xapi).
>>
>> This series is based on staging, as 9f2f1298d0211962 is a prerequisite
>> hypercall for a bugfix.
>>
>> Draft F of the design document is available from
>>   xenbits.xen.org/people/andrewcoop/domain-save-format-F.pdf
> pg 8 says:
>
> body Record body of length body_length octects.
>
> It is a bit hard to parse. Perhaps:
> Data stream (body) of size defined in 'body_length' in multiples
> of octects.
> ?

Hmm - that is not very well written.  I shall clarify it somewhat,
although the length part is clear from the 'body_length' description.

>
> pg. 11
> Mentions 'XEN_DOMCTL_PFINFO_*' type. Should you at least point
> the reader to where those are defined in the code? Or in the spec?
> [edit: next page has it. You could mention that (following page defines
> them)

This is because of the way in which markdown split the page which is
hard to affect.  I shall clarify the first reference to mention
public/domctl.h

>
>
> 'page_data: "page_size octets of uncompressed page contents for each
> page set as present in the pfn array." I think you might want to
> replace: "..for each page set" as "corresponding to each element
> in the pfn array."
>
>
> Also oddly enough the picture shows 'page_data[N-1]' but for the
> pfn array it has 'pfn[C-1]'. I think you want s/N/C.
> [edit: And the next page explains why N != C].
>
> Perhaps for the 'page_data' definition you should mention
> that N != C as some of the pfn array entries skip the page_data
> array of data?

I will clarify that N is strictly <= C and can validly be 0.

>
>
> pg.17
> xfeature_mask: The xfeature_mask rom the hypercall body.
>
> I would replace body with structure?

This is rather more difficult.

As it turns out, xfeature_mask is not needed for migration.

It is the entire set of available xsave features on the saving cpu,
which is not relevant for the receiving cpu to validate against.  The
first and second uint64_t's of the vcpu_ctx are the xcr0 and xcr0_accum
which are the two masks relevant to validating the feature set and size
of context against the incoming data.

At the moment, the sole use for xfeature_mask is for the receiving cpu
to validate xcr0_accum against, but only after xcr0_accum has already
been validated, which makes the xfeature_mask check strictly redundant.

I have half a mind to submit a patch making Xen completely ignore it on
the "set" side, at which point it can be dropped from the migration
stream.  It does however have a use in "get" side for libxc deciding how
to set up the domain cpuid mask.

>
>
> pg.19
> TSC_MODE_* constant. You should probably list them. Or at least
> say what they are.
>
> Incarnation? Wiki says it means "embodied in flesh" Umm.

As a word, that is a valid meaning.

As for what it is doing in the code, I have no clue.  It is something to
do with how many times the toolstack has issued a set_tsc_info()
hypercall, but why Xen cant count this itself is beyond me, as is why
this even matters in the first place.

>
>
> pg. 20
>
> blob? How about payload? Data stream?

The public headers refer to it as a blob, and blob as in "binary blob"
is a fairly common term.

>
>
> pg. 21
>
> Would it make sense to point to the list of them at least?
> Say: "Can be found in XYZ file."

In my copious free time, I hope to remove some of them.  A large number
of them can be more appropriately done elsehow, such as domain creation
flags.

>
> pg. 22
>
> It says temporary - so... should it be ripped out of the toolstack?

Absolutely - it is gross layer violation.

As far as I am aware, the only time this record is actually used is for
libxl sending xenstore key/value information.  As xl/libxl already has
its own head & tail for the libxc migration stream, it can put xenstore
keys somewhere else.

It is currently included so "pseudo-v2" can transparently replace
xc_domain_save/restore() during development without also making invasive
changes to the users.

>
>
> pg. 23
>
> An x86 PV guest image will have in this order: ->
> An x86 PV guest image will have this order of records:

Ok

>
>
> pg. 23
> x86 HVM guest:
>  You should probably say "An x86 HVM guest will have.."
>
> but more interesingly - there is an "DEVICE_MODEL_STATE" which
> is not defined in the spec?
>

Another layer violation, although this slipup comes from my first
attempt to thing what HVM might need, and David's subsequent edit which
moved it back to reflecting the working code.

The device model state is also something which needs fixing.  Currently
on the sending side, libxl is responsible for writing it into the
stream, but on the receiving side, libxc is responsible for writing it
to a magic file in /var/run/xend/...

~Andrew