Re: [RFC v3 1/6] xen/arm: Add basic save/restore support for ARM

[Wei Huang <w1.huang@samsung.com>]

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 <xen/types.h>
>> +#include <public/hvm/ioreq.h>
>> +#include <xen/sched.h>
>> +#include <xen/hvm/save.h>
>> +#include <asm/processor.h>
>> +
>> +#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
>