Re: [PATCH RFC] xen/arm: domain kernel: Small fixes for making suspendable for arm

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jul 03, 2013 at 06:16:28PM +0900, Jaeyong Yoo wrote:
> Modify makefile to compile driver/xen/manage.c for arm and implement
> resuming the shared page info. This patch is required for domu kernel
> to test the xen-on-arndale migration.
> 
> Since there are lot of missing functions for compiling hibernation mode,
> temporarily I put empty functions in xen/dummy.c, but they are originally
> belong to such as arch/arm/power directories (which is not existing).
> I think there would be any better way...

Since they are both shared by arm and arm64 and x86 could some of this be
in the drivers/xen ? This might mean moving some code from arch/x86?

> 
> Signed-off-by: Jaeyong Yoo <jaeyong.yoo@samsung.com>
> ---
>  arch/arm/Kconfig                |  3 ++
>  arch/arm/boot/dts/xenvm-4.2.dts |  2 +-
>  arch/arm/xen/Makefile           |  2 +-
>  arch/arm/xen/dummy.c            | 30 ++++++++++++++++
>  arch/arm/xen/mmu.c              | 12 +++++++
>  arch/arm/xen/suspend.c          | 76 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm/xen/time.c             |  7 ++++
>  drivers/xen/Makefile            |  2 +-
>  drivers/xen/manage.c            |  8 +++++
>  9 files changed, 139 insertions(+), 3 deletions(-)
>  create mode 100644 arch/arm/xen/dummy.c
>  create mode 100644 arch/arm/xen/mmu.c
>  create mode 100644 arch/arm/xen/suspend.c
>  create mode 100644 arch/arm/xen/time.c
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 2c3bdce..77309f7 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1469,6 +1469,9 @@ config ARCH_NO_VIRT_TO_BUS
>  config ISA_DMA_API
>  	bool
>  
> +config ARCH_HIBERNATION_POSSIBLE
> +        def_bool y
> +
>  config PCI
>  	bool "PCI support" if MIGHT_HAVE_PCI
>  	help
> diff --git a/arch/arm/boot/dts/xenvm-4.2.dts b/arch/arm/boot/dts/xenvm-4.2.dts
> index 2f4136b..33df5e6 100644
> --- a/arch/arm/boot/dts/xenvm-4.2.dts
> +++ b/arch/arm/boot/dts/xenvm-4.2.dts
> @@ -17,7 +17,7 @@
>  
>  	chosen {
>  		/* this field is going to be adjusted by the hypervisor */
> -		bootargs = "console=hvc0 root=/dev/xvda";
> +		bootargs = "console=hvc0 root=/dev/xvda1 rw init";

Stray patch perhaps?
>  	};
>  
>  	cpus {
> diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
> index 4384103..6fdc47a 100644
> --- a/arch/arm/xen/Makefile
> +++ b/arch/arm/xen/Makefile
> @@ -1 +1 @@
> -obj-y		:= enlighten.o hypercall.o grant-table.o
> +obj-y		:= enlighten.o hypercall.o grant-table.o suspend.o mmu.o time.o dummy.o
> diff --git a/arch/arm/xen/dummy.c b/arch/arm/xen/dummy.c
> new file mode 100644
> index 0000000..daa949c
> --- /dev/null
> +++ b/arch/arm/xen/dummy.c
> @@ -0,0 +1,30 @@
> +#include <linux/kernel.h>
> +#include <linux/printk.h>
> +
> +void save_processor_state(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);

Joe Perches made a valiant effort at converting all of the 'printk' to
pr_info/pr_err. Please do use that.

But a better option would be to do:

WARN(1);

As that would also produce a stack-trace to help in diagnosing this.

> +}
> +
> +void restore_processor_state(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}

But these functions are generic I think? Can't they also be used by
'baremetal' ARM in suspending/resuming? As such shouldn't they be in
a more generic layer?

> +
> +int swsusp_arch_suspend(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +	return 0;
> +}
> +
> +int swsusp_arch_resume(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +	return 0;
> +}

Ditto for that.
> +
> +int pfn_is_nosave(unsigned long pfn)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +	return 0;
> +}

And that.


And I think you would also need some EXPORT_SYMBOL_GPL.

> diff --git a/arch/arm/xen/mmu.c b/arch/arm/xen/mmu.c
> new file mode 100644
> index 0000000..cc0ccc9
> --- /dev/null
> +++ b/arch/arm/xen/mmu.c
> @@ -0,0 +1,12 @@
> +#include <linux/kernel.h>
> +#include <xen/xen.h>
> +
> +void xen_mm_pin_all(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
> +
> +void xen_mm_unpin_all(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}

A better approach is to make the in include/xen/xen-ops.h a bunch of

#ifdef CONFIG_ARM
void static inline xen_mm_pin_all(void) { }
#else
.. original code.
#endif

> diff --git a/arch/arm/xen/suspend.c b/arch/arm/xen/suspend.c
> new file mode 100644
> index 0000000..946a960
> --- /dev/null
> +++ b/arch/arm/xen/suspend.c
> @@ -0,0 +1,76 @@
> +#include <xen/xen.h>
> +#include <xen/events.h>
> +#include <xen/grant_table.h>
> +#include <xen/hvm.h>
> +#include <xen/interface/vcpu.h>
> +#include <xen/interface/xen.h>
> +#include <xen/interface/memory.h>
> +#include <xen/interface/hvm/params.h>
> +#include <xen/features.h>
> +#include <xen/platform_pci.h>
> +#include <xen/xenbus.h>
> +#include <xen/page.h>
> +#include <xen/xen-ops.h>
> +#include <asm/xen/hypervisor.h>
> +#include <asm/xen/hypercall.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqreturn.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +
> +#include <linux/mm.h>
> +
> +void xen_arch_pre_suspend(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
> +

Same thing. Move them in xen-ops.h without any implementation.

> +void xen_arch_hvm_post_suspend(int suspend_cancelled)
> +{
> +	if( !suspend_cancelled ) {
> +		int cpu;
> +		struct xen_add_to_physmap xatp;
> +		static struct shared_info *shared_info_page = 0;
> +
> +		if( !shared_info_page )
> +			shared_info_page = (struct shared_info *)
> +				get_zeroed_page(GFP_KERNEL);
> +		if (!shared_info_page) {
> +			pr_err("not enough memory\n");

Good. you are using pr_err here.

> +			return;
> +		}
> +
> +		xatp.domid = DOMID_SELF;
> +		xatp.idx = 0;
> +		xatp.space = XENMAPSPACE_shared_info;
> +		xatp.gpfn = __pa(shared_info_page) >> PAGE_SHIFT;
> +		if (HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp))
> +			BUG();
> +
> +		HYPERVISOR_shared_info = (struct shared_info *)shared_info_page;
> +
> +		/* xen_vcpu is a pointer to the vcpu_info struct in the shared_info
> +		 * page, we use it in the event channel upcall */
> +		for_each_online_cpu(cpu) {
> +			per_cpu(xen_vcpu, cpu) = &HYPERVISOR_shared_info->vcpu_info[cpu];
> +		}
> +		printk(KERN_ERR"%s: remmaping shared info...\n", __func__);

But here you are using KERN_ERR ? Why not 'pr_info'? Also this looks it
has been already implemented in the arch/arm already? Can you use the existing
code in there and just make it exported?

> +	}
> +}
> +
> +void xen_arch_post_suspend(int suspend_cancelled)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
Same thing. Move them in xen-ops.h without any implementation.
> +
> +static void xen_vcpu_notify_restore(void *data)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
Same thing. Move them in xen-ops.h without any implementation.
> +
> +void xen_arch_resume(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
Same thing. Move them in xen-ops.h without any implementation.
> diff --git a/arch/arm/xen/time.c b/arch/arm/xen/time.c
> new file mode 100644
> index 0000000..af90e53
> --- /dev/null
> +++ b/arch/arm/xen/time.c
> @@ -0,0 +1,7 @@
> +#include <linux/kernel.h>
> +#include <xen/xen.h>
> +
> +void xen_timer_resume(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}

I think you know what I am going to say here.

> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index eabd0ee..3d24a95 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -1,10 +1,10 @@
>  ifneq ($(CONFIG_ARM),y)
> -obj-y	+= manage.o
>  obj-$(CONFIG_HOTPLUG_CPU)		+= cpu_hotplug.o
>  endif
>  obj-$(CONFIG_X86)			+= fallback.o
>  obj-y	+= grant-table.o features.o events.o balloon.o
>  obj-y	+= xenbus/
> +obj-y	+= manage.o
>  
>  nostackp := $(call cc-option, -fno-stack-protector)
>  CFLAGS_features.o			:= $(nostackp)
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 412b96c..140c7a9 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -17,6 +17,7 @@
>  #include <xen/events.h>
>  #include <xen/hvc-console.h>
>  #include <xen/xen-ops.h>
> +#include <xen/interface/sched.h>
>  
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/page.h>
> @@ -86,7 +87,14 @@ static int xen_suspend(void *data)
>  	 * or the domain was merely checkpointed, and 0 if it
>  	 * is resuming in a new domain.
>  	 */
> +#ifdef CONFIG_ARM
> +	{
> +		struct sched_shutdown r = { .reason = SHUTDOWN_suspend };
> +		HYPERVISOR_sched_op(SCHEDOP_shutdown, &r);
> +	}
> +#else
>  	si->cancelled = HYPERVISOR_suspend(si->arg);
> +#endif

Please add the HYPERVISOR_suspend in arch/arm/include/asm/xen/hypercall.h
and implement it there.

>  
>  	if (si->post)
>  		si->post(si->cancelled);
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel