From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [RFC v2 1/6] xen/arm: Save and restore support with hvm context hypercalls Date: Thu, 17 Apr 2014 13:55:40 +0100 Message-ID: <534FCF4C.6040406@citrix.com> References: <1397595918-30419-1-git-send-email-w1.huang@samsung.com> <1397595918-30419-2-git-send-email-w1.huang@samsung.com> <534DC2D5.4050302@citrix.com> <534EFB08.1030608@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <534EFB08.1030608@samsung.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Huang , xen-devel@lists.xen.org Cc: yjhyun.yoo@samsung.com, julien.grall@linaro.org, ian.campbell@citrix.com, jaeyong.yoo@samsung.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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