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

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

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.

> 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.

The rest of the patch is however looking fine.

~Andrew