Re: [PATCH 1/2] xen: refactor suspend pre/post hooks

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 05/08/2014 06:09 AM, David Vrabel wrote:
> New architectures currently have to provide implementations of 5 different
> functions: xen_arch_pre_suspend(), xen_arch_post_suspend(),
> xen_arch_hvm_post_suspend(), xen_mm_pin_all(), and xen_mm_unpin_all().
>
> Refactor the suspend code to only require xen_arch_pre_suspend() and
> xen_arch_post_suspend().
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

> ---
>   arch/x86/xen/suspend.c |   23 ++++++++++++++++++++---
>   arch/x86/xen/xen-ops.h |    2 ++
>   drivers/xen/manage.c   |   45 +++++++--------------------------------------
>   include/xen/xen-ops.h  |    4 ----
>   4 files changed, 29 insertions(+), 45 deletions(-)
>
> diff --git a/arch/x86/xen/suspend.c b/arch/x86/xen/suspend.c
> index 45329c8..c4df9db 100644
> --- a/arch/x86/xen/suspend.c
> +++ b/arch/x86/xen/suspend.c
> @@ -12,8 +12,10 @@
>   #include "xen-ops.h"
>   #include "mmu.h"
>   
> -void xen_arch_pre_suspend(void)
> +static void xen_pv_pre_suspend(void)
>   {
> +	xen_mm_pin_all();
> +
>   	xen_start_info->store_mfn = mfn_to_pfn(xen_start_info->store_mfn);
>   	xen_start_info->console.domU.mfn =
>   		mfn_to_pfn(xen_start_info->console.domU.mfn);
> @@ -26,7 +28,7 @@ void xen_arch_pre_suspend(void)
>   		BUG();
>   }
>   
> -void xen_arch_hvm_post_suspend(int suspend_cancelled)
> +static void xen_hvm_post_suspend(int suspend_cancelled)
>   {
>   #ifdef CONFIG_XEN_PVHVM
>   	int cpu;
> @@ -41,7 +43,7 @@ void xen_arch_hvm_post_suspend(int suspend_cancelled)
>   #endif
>   }
>   
> -void xen_arch_post_suspend(int suspend_cancelled)
> +static void xen_pv_post_suspend(int suspend_cancelled)
>   {
>   	xen_build_mfn_list_list();
>   
> @@ -60,6 +62,21 @@ void xen_arch_post_suspend(int suspend_cancelled)
>   		xen_vcpu_restore();
>   	}
>   
> +	xen_mm_unpin_all();
> +}
> +
> +void xen_arch_pre_suspend(void)
> +{
> +    if (xen_pv_domain())
> +        xen_pv_pre_suspend();
> +}
> +
> +void xen_arch_post_suspend(int cancelled)
> +{
> +    if (xen_pv_domain())
> +        xen_pv_post_suspend(cancelled);
> +    else
> +        xen_hvm_post_suspend(cancelled);
>   }
>   
>   static void xen_vcpu_notify_restore(void *data)
> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
> index 1cb6f4c..c834d4b 100644
> --- a/arch/x86/xen/xen-ops.h
> +++ b/arch/x86/xen/xen-ops.h
> @@ -31,6 +31,8 @@ void xen_setup_kernel_pagetable(pgd_t *pgd, unsigned long max_pfn);
>   void xen_reserve_top(void);
>   extern unsigned long xen_max_p2m_pfn;
>   
> +void xen_mm_pin_all(void);
> +void xen_mm_unpin_all(void);
>   void xen_set_pat(u64);
>   
>   char * __init xen_memory_setup(void);
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 32f9236..c3667b2 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -41,9 +41,6 @@ static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
>   
>   struct suspend_info {
>   	int cancelled;
> -	unsigned long arg; /* extra hypercall argument */
> -	void (*pre)(void);
> -	void (*post)(int cancelled);
>   };
>   
>   static RAW_NOTIFIER_HEAD(xen_resume_notifier);
> @@ -61,26 +58,6 @@ void xen_resume_notifier_unregister(struct notifier_block *nb)
>   EXPORT_SYMBOL_GPL(xen_resume_notifier_unregister);
>   
>   #ifdef CONFIG_HIBERNATE_CALLBACKS
> -static void xen_hvm_post_suspend(int cancelled)
> -{
> -	xen_arch_hvm_post_suspend(cancelled);
> -	gnttab_resume();
> -}
> -
> -static void xen_pre_suspend(void)
> -{
> -	xen_mm_pin_all();
> -	gnttab_suspend();
> -	xen_arch_pre_suspend();
> -}
> -
> -static void xen_post_suspend(int cancelled)
> -{
> -	xen_arch_post_suspend(cancelled);
> -	gnttab_resume();
> -	xen_mm_unpin_all();
> -}
> -
>   static int xen_suspend(void *data)
>   {
>   	struct suspend_info *si = data;
> @@ -94,18 +71,20 @@ static int xen_suspend(void *data)
>   		return err;
>   	}
>   
> -	if (si->pre)
> -		si->pre();
> +	gnttab_suspend();
> +	xen_arch_pre_suspend();
>   
>   	/*
>   	 * This hypercall returns 1 if suspend was cancelled
>   	 * or the domain was merely checkpointed, and 0 if it
>   	 * is resuming in a new domain.
>   	 */
> -	si->cancelled = HYPERVISOR_suspend(si->arg);
> +	si->cancelled = HYPERVISOR_suspend(xen_pv_domain()
> +                                           ? virt_to_mfn(xen_start_info)
> +                                           : 0);
>   
> -	if (si->post)
> -		si->post(si->cancelled);
> +	xen_arch_post_suspend(si->cancelled);
> +	gnttab_resume();
>   
>   	if (!si->cancelled) {
>   		xen_irq_resume();
> @@ -154,16 +133,6 @@ static void do_suspend(void)
>   
>   	si.cancelled = 1;
>   
> -	if (xen_hvm_domain()) {
> -		si.arg = 0UL;
> -		si.pre = NULL;
> -		si.post = &xen_hvm_post_suspend;
> -	} else {
> -		si.arg = virt_to_mfn(xen_start_info);
> -		si.pre = &xen_pre_suspend;
> -		si.post = &xen_post_suspend;
> -	}
> -
>   	err = stop_machine(xen_suspend, &si, cpumask_of(0));
>   
>   	raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 2cf4717..0b3149e 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -9,10 +9,6 @@ DECLARE_PER_CPU(struct vcpu_info *, xen_vcpu);
>   
>   void xen_arch_pre_suspend(void);
>   void xen_arch_post_suspend(int suspend_cancelled);
> -void xen_arch_hvm_post_suspend(int suspend_cancelled);
> -
> -void xen_mm_pin_all(void);
> -void xen_mm_unpin_all(void);
>   
>   void xen_timer_resume(void);
>   void xen_arch_resume(void);