From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Huang Subject: Re: [RFC v3 1/6] xen/arm: Add basic save/restore support for ARM Date: Thu, 08 May 2014 17:20:25 -0500 Message-ID: <536C0329.50901@samsung.com> References: <1399583908-21755-1-git-send-email-w1.huang@samsung.com> <1399583908-21755-2-git-send-email-w1.huang@samsung.com> <536C010A.8010509@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <536C010A.8010509@citrix.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: Andrew Cooper , xen-devel@lists.xen.org Cc: keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org, julien.grall@linaro.org, ian.jackson@eu.citrix.com, jaeyong.yoo@samsung.com, jbeulich@suse.com, yjhyun.yoo@samsung.com List-Id: xen-devel@lists.xenproject.org On 05/08/2014 05:11 PM, Andrew Cooper wrote: > On 08/05/2014 22:18, Wei Huang wrote: > > >> diff --git a/xen/include/asm-arm/hvm/support.h b/xen/include/asm-arm/hvm/support.h >> new file mode 100644 >> index 0000000..fa5fe75 >> --- /dev/null >> +++ b/xen/include/asm-arm/hvm/support.h >> @@ -0,0 +1,29 @@ >> +/* >> + * HVM support routines >> + * >> + * Copyright (c) 2014, Samsung Electronics. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple >> + * Place - Suite 330, Boston, MA 02111-1307 USA. >> + */ >> + >> +#ifndef __ASM_ARM_HVM_SUPPORT_H__ >> +#define __ASM_ARM_HVM_SUPPORT_H__ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#endif /* __ASM_ARM_HVM_SUPPORT_H__ */ > > This header file isn't touched by any subsequent patches, and just > having it as a list of includes is overkill. Can it be dropped? 5 > extra includes in a few .c files is hardly breaking the bank. Last time I tried it quickly, the compilation broke. Will try again. > >> diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h >> index 75b8e65..8312e7b 100644 >> --- a/xen/include/public/arch-arm/hvm/save.h >> +++ b/xen/include/public/arch-arm/hvm/save.h >> @@ -3,6 +3,7 @@ >> * be saved along with the domain's memory and device-model state. >> * >> * Copyright (c) 2012 Citrix Systems Ltd. >> + * Copyright (c) 2014 Samsung Electronics. >> * >> * Permission is hereby granted, free of charge, to any person obtaining a copy >> * of this software and associated documentation files (the "Software"), to >> @@ -26,6 +27,24 @@ >> #ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__ >> #define __XEN_PUBLIC_HVM_SAVE_ARM_H__ >> >> +#define HVM_ARM_FILE_MAGIC 0x92385520 >> +#define HVM_ARM_FILE_VERSION 0x00000001 >> + >> +/* Note: For compilation purpose hvm_save_header name is the same as x86, >> + * but layout is different. */ >> +struct hvm_save_header >> +{ >> + uint32_t magic; /* Must be HVM_ARM_FILE_MAGIC */ >> + uint32_t version; /* File format version */ >> + uint32_t cpuinfo; /* Record MIDR_EL1 info of saving machine */ >> +}; >> +DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header); >> + >> +/* >> + * Largest type-code in use >> + */ >> +#define HVM_SAVE_CODE_MAX 1 >> + >> #endif >> >> /* > > Hmm - it is quite poor to have this magically named "hvm_save_header". > > If you were to redefine arch_hvm{save,load} as: > > int arch_hvm_save(struct domain *d, struct hvm_domain_context_t *h); > int arch_hvm_load(struct domain *d, struct hvm_domain_context_t *h); > > and pushed the hvm_save_entry(HEADER, 0, h, &hdr) into arch_hvm_save(), > you can remove all trace of "struct hvm_save_header" from common code. > This then removes any requirements to have an identically named struct. > > It is probably worth having this all as a separate patch which just > cleans up the x86 and common code before introducing the ARM side of things. Reasonable. I will fix next revision. > > The rest of the patch is however looking fine. > > ~Andrew > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >