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

[Jaeyong Yoo <jaeyong.yoo@samsung.com>]

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