Re: [Xen-devel] [RFC v2 1/6] xen/arm: Save and restore support with hvm context hypercalls

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/04/2014 22:50, Wei Huang wrote:
> On 04/15/2014 06:37 PM, Andrew Cooper wrote:
>>> --- a/xen/include/public/arch-arm/hvm/save.h
>>> +++ b/xen/include/public/arch-arm/hvm/save.h
>>> @@ -26,6 +26,136 @@
>>>   #ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__
>>>   #define __XEN_PUBLIC_HVM_SAVE_ARM_H__
>>>
>>> +#define HVM_FILE_MAGIC   0x92385520
>>> +#define HVM_FILE_VERSION 0x00000001
>>> +
>>> +struct hvm_save_header
>>> +{
>>> +    uint32_t magic;             /* Must be HVM_FILE_MAGIC */
>>> +    uint32_t version;           /* File format version */
>>> +    uint64_t changeset;         /* Version of Xen that saved this
>>> file */
>>> +    uint32_t cpuid;             /* MIDR_EL1 on the saving machine */
>>
>> This looks needlessly copied from x86, which is far from ideal.
>>
>> On x86, Xen tries to parse the mercural revision number from its compile
>> time information and fails now that the underlying codebase has moved
>> from hg to git.  As a result, the value is now generally -1.
>>
>> cpuid is also an x86ism as far as I am aware.
>>
>> I also wonder about the wisdom of having identically named structures
>> like this in arch code without an arch_ prefix?  We should make it as
>> hard as possible for things like this to accidentally get referenced in
>> common code.
>>
> It is tricky for hvm_save_header. This is a struct used in common code
> (xen/common/hvm/save.c). Instead of making it arch_ specific, I would
> move it to common code (include/public/hvm/save.h), with the following
> modification:
> 1. Re-define "cpuid" of hvm_save_header as cpu_id. hvm_save_header
> will be shared by both x86 and ARM.
> 2. Rename HVM_FILE_MAGIC to HVM_ARM_FILE_MAGIC. Still keep it in
> arch-arm/hvm/save.h file. This is used in arch- specific code, so we
> won't get confused. The same applies to HVM_FILE_MAGIC in x86.
> 3. Other struct in arch-arm/hvm/save.h will remain in the same file.
> Those structs are arch specific anyway.
>
> -Wei
>

Eugh.  Having a more thorough look through all of this code, it is in
need of improvement.

For the sake of hdr.{magic,version,changeset}, it is not worth keeping
some common save logic for the header.  arch_hvm_save() should be
updated to be given the hvm context and should construct & write the
entire structure.  This also matches the current semantics of
arch_hvm_load() where the arch handler deals with the entire structure.

The currently existing hvm_save_header is quite silly.  'changeset'
conveys no useful information since the switch from hg to git.  'cpuid'
is used for the sole purpose a printk(), and 'gtsc_khz' is
unconditionally repeated later in the migration record with the rest of
the tsc information.

Everything currently in arch-x86/hvm/save.h should be renamed to
identify them as hvm_x86.  This can be done in a backwards compatible
manner by using some __XEN_INTERFACE_VERSION__ ifdefary

Everything new in arch-arm/hvm/save.h should be identified as hvm_arm
right from the outset.

Beyond that, the only key point should need to be that
HVM_$arch_FILE_MAGIC need be different for each $arch, but that appears
already in hand.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel