Xen devel - Jul 2013 - [PATCH RFC] xen/arm: domain kernel: Small fixes for making suspendable for arm

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

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";
 	};
 
 	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__);
+}
+
+void restore_processor_state(void)
+{
+	printk(KERN_ERR"%s: function not implemented\n", __func__);
+}
+
+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;
+}
+
+int pfn_is_nosave(unsigned long pfn)
+{
+	printk(KERN_ERR"%s: function not implemented\n", __func__);
+	return 0;
+}
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__);
+}
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__);
+}
+
+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");
+			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__);
+	}
+}
+
+void xen_arch_post_suspend(int suspend_cancelled)
+{
+	printk(KERN_ERR"%s: function not implemented\n", __func__);
+}
+
+static void xen_vcpu_notify_restore(void *data)
+{
+	printk(KERN_ERR"%s: function not implemented\n", __func__);
+}
+
+void xen_arch_resume(void)
+{
+	printk(KERN_ERR"%s: function not implemented\n", __func__);
+}
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__);
+}
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
 
 	if (si->post)
 		si->post(si->cancelled);
-- 
1.8.1.2

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

On Wed, 3 Jul 2013, 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...
> 
> 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 ++++
Be careful that xen for arm64 just went upstream and it''s just
recompiling the same Xen files under arm64.  See arch/arm64/xen.
The changes you make to c files under arch/arm/xen need to compile on
arm64 too.

>  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
> +
This could be an issue because if you introduce this symbol you allow
users to compile hibernation code on all arm platforms.
At the very least it should have "depends on XEN".


>  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";
>  	};
>  
>  	cpus {
please remove this change, this dts is just an example

> 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__);
> +}
> +
> +void restore_processor_state(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
> +
> +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;
> +}
> +
> +int pfn_is_nosave(unsigned long pfn)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +	return 0;
> +}
These functions are not Xen specific, they should not be under
arch/arm/xen.
Maybe we could put them under arch/arm/power or drivers/xen?


> 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__);
> +}
No need to print an error here, I would just add a comment saying "no
need to pin/unpin anything because we are always using second stage
translation".

> 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__);
> +}
if we don''t need to do anything, it''s not an error.

> +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");
> +			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__);
> +	}
> +}
It would be good to refactor the shared_info page setup on a separate
function that can be called from xen_guest_init and from
xen_arch_hvm_post_suspend, like we do on x86.

> +void xen_arch_post_suspend(int suspend_cancelled)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
> +
> +static void xen_vcpu_notify_restore(void *data)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
> +
> +void xen_arch_resume(void)
> +{
> +	printk(KERN_ERR"%s: function not implemented\n", __func__);
> +}
if don''t need to do anything, it''s not an error.

> 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__);
> +}
same 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
>  
>  	if (si->post)
>  		si->post(si->cancelled);
We should implement HYPERVISOR_suspend on ARM

On Wed, 2013-07-03 at 16:59 +0100, Stefano Stabellini wrote:
> > 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
> > +
> 
> This could be an issue because if you introduce this symbol you allow
> users to compile hibernation code on all arm platforms.
> At the very least it should have "depends on XEN".
> 
[...]
> > +void save_processor_state(void)
> > +{
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__);
> > +}
> > +
> > +void restore_processor_state(void)
> > +{
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__);
> > +}
> > +
> > +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;
> > +}
> > +
> > +int pfn_is_nosave(unsigned long pfn)
> > +{
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__);
> > +	return 0;
> > +}
> 
> These functions are not Xen specific, they should not be under
> arch/arm/xen.
> Maybe we could put them under arch/arm/power or drivers/xen?
Together with the spurious config symbol this suggests that perhaps the
hibernation interface is not the right one to be using for Xen on ARM.

How does this work on native ARM I wonder?

Ian.

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Thursday, July 04, 2013 1:00 AM
> To: Jaeyong Yoo
> Cc: xen-devel@lists.xen.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Will Deacon; Arnd Bergmann; Olof Johansson
> Subject: Re: [Xen-devel] [PATCH RFC] xen/arm: domain kernel: Small fixes
> for making suspendable for arm
> 
> On Wed, 3 Jul 2013, 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...
> >
> > 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 ++++
> 
> Be careful that xen for arm64 just went upstream and it''s just
recompiling
> the same Xen files under arm64.  See arch/arm64/xen.
> The changes you make to c files under arch/arm/xen need to compile on
> arm64 too.
OK.
> 
> 
> >  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
> > +
> 
> This could be an issue because if you introduce this symbol you allow
> users to compile hibernation code on all arm platforms.
> At the very least it should have "depends on XEN".
Got it! Thanks.
> 
> 
> 
> >  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";
> >  	};
> >
> >  	cpus {
> 
> please remove this change, this dts is just an example

OK
> 
> 
> > 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__); }
> > +
> > +void restore_processor_state(void)
> > +{
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__); }
> > +
> > +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;
> > +}
> > +
> > +int pfn_is_nosave(unsigned long pfn)
> > +{
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__);
> > +	return 0;
> > +}
> 
> These functions are not Xen specific, they should not be under
> arch/arm/xen.
> Maybe we could put them under arch/arm/power or drivers/xen?
Yes, that was my first thought, but I don''t want to put anything to
arch/arm/power.
Also, I''m not sure about drivers/xen either. Maybe we have to think
about
the 
whole power-related in arm.
> 
> 
> 
> > 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__); }
> 
> No need to print an error here, I would just add a comment saying "no
need
> to pin/unpin anything because we are always using second stage
> translation".
Got it.
> 
> 
> > 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__); }
> 
> if we don''t need to do anything, it''s not an error.
> 
OK
> 
> > +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");
> > +			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__);
> > +	}
> > +}
> 
> It would be good to refactor the shared_info page setup on a separate
> function that can be called from xen_guest_init and from
> xen_arch_hvm_post_suspend, like we do on x86.
OK.
> 
> 
> > +void xen_arch_post_suspend(int suspend_cancelled) {
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__); }
> > +
> > +static void xen_vcpu_notify_restore(void *data) {
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__); }
> > +
> > +void xen_arch_resume(void)
> > +{
> > +	printk(KERN_ERR"%s: function not implemented\n",
__func__); }
> 
> if don''t need to do anything, it''s not an error.
> 
> 
> > 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__); }
> 
> same 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
> >
> >  	if (si->post)
> >  		si->post(si->cancelled);
> 
> We should implement HYPERVISOR_suspend on ARM
Got it!

Jaeyong