Re: [PATCH v3 1/3] KVM: PPC: epapr: Factor out the epapr init

[Scott Wood <scottwood@freescale.com>]

On 02/10/2012 04:02 AM, Liu Yu wrote:
> from the kvm guest paravirt init code.
> 
> Signed-off-by: Liu Yu <yu.liu@freescale.com>
> ---
> v3:
> apply the epapr init for all ppc platform
> 
>  arch/powerpc/Kconfig                    |    4 +++
>  arch/powerpc/include/asm/epapr_hcalls.h |    8 +++++
>  arch/powerpc/kernel/Makefile            |    1 +
>  arch/powerpc/kernel/epapr_para.c        |   46 +++++++++++++++++++++++++++++++
>  arch/powerpc/kernel/kvm.c               |   13 +++------
>  arch/powerpc/kvm/Kconfig                |    1 +
>  6 files changed, 64 insertions(+), 9 deletions(-)
>  create mode 100644 arch/powerpc/kernel/epapr_para.c
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 47682b6..00bd508 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -196,6 +196,10 @@ config EPAPR_BOOT
>  	  Used to allow a board to specify it wants an ePAPR compliant wrapper.
>  	default n
>  
> +config EPAPR_PARA
> +	bool
> +	default n

EPAPR_PARAVIRT

>  config DEFAULT_UIMAGE
>  	bool
>  	help
> diff --git a/arch/powerpc/include/asm/epapr_hcalls.h b/arch/powerpc/include/asm/epapr_hcalls.h
> index f3b0c2c..c4b86e4 100644
> --- a/arch/powerpc/include/asm/epapr_hcalls.h
> +++ b/arch/powerpc/include/asm/epapr_hcalls.h
> @@ -148,6 +148,14 @@
>  #define EV_HCALL_CLOBBERS2 EV_HCALL_CLOBBERS3, "r5"
>  #define EV_HCALL_CLOBBERS1 EV_HCALL_CLOBBERS2, "r4"
>  
> +extern u32 *epapr_hcall_insts;
> +extern int epapr_hcall_insts_len;
> +
> +static inline void epapr_get_hcall_insts(u32 **instp, int *lenp)
> +{
> +	*instp = epapr_hcall_insts;
> +	*lenp = epapr_hcall_insts_len;
> +}

Why do we need a function for this?  Why is the public interface
anything other than "invoke a hypercall"?

> +static int __init epapr_para_init(void)
> +{
> +	struct device_node *hyper_node;
> +	u32 *insts;
> +	int len;
> +
> +	hyper_node = of_find_node_by_path("/hypervisor");
> +	if (!hyper_node)
> +		return -ENODEV;
> +
> +	insts = (u32*)of_get_property(hyper_node, "hcall-instructions", &len);

Do not cast away that const.

> @@ -535,18 +536,12 @@ EXPORT_SYMBOL_GPL(kvm_hypercall);
>  static int kvm_para_setup(void)
>  {
>  	extern u32 kvm_hypercall_start;
> -	struct device_node *hyper_node;
>  	u32 *insts;
>  	int len, i;
>  
> -	hyper_node = of_find_node_by_path("/hypervisor");
> -	if (!hyper_node)
> -		return -1;
> -
> -	insts = (u32*)of_get_property(hyper_node, "hcall-instructions", &len);
> -	if (len % 4)
> -		return -1;
> -	if (len > (4 * 4))
> +	insts = epapr_hcall_insts;
> +	len = epapr_hcall_insts_len;
> +	if (insts = NULL)
>  		return -1;
>  
>  	for (i = 0; i < (len / 4); i++)

Why are you still doing the patching inside kvm.c?

-Scott