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 <[email protected]>
>
> 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:[email protected]]
> Sent: Thursday, July 04, 2013 1:00 AM
> To: Jaeyong Yoo
> Cc: [email protected]; [email protected]; linux-
> [email protected]; 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 <[email protected]>
> >
> > 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