* Re: [Xen-devel] [PATCH RFC] xen/arm: domain kernel: Small fixes for making suspendable for arm
2013-07-03 15:59 ` [Xen-devel] [PATCH RFC] xen/arm: domain kernel: Small fixes for making suspendable for arm Stefano Stabellini
@ 2013-07-03 16:04 ` Ian Campbell
2013-07-04 1:34 ` Jaeyong Yoo
1 sibling, 0 replies; 3+ messages in thread
From: Ian Campbell @ 2013-07-03 16:04 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Jaeyong Yoo, Arnd Bergmann, Will Deacon, linux-kernel, xen-devel,
Olof Johansson, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [Xen-devel] [PATCH RFC] xen/arm: domain kernel: Small fixes for making suspendable for arm
2013-07-03 15:59 ` [Xen-devel] [PATCH RFC] xen/arm: domain kernel: Small fixes for making suspendable for arm Stefano Stabellini
2013-07-03 16:04 ` Ian Campbell
@ 2013-07-04 1:34 ` Jaeyong Yoo
1 sibling, 0 replies; 3+ messages in thread
From: Jaeyong Yoo @ 2013-07-04 1:34 UTC (permalink / raw)
To: 'Stefano Stabellini'
Cc: xen-devel, linux-arm-kernel, linux-kernel, 'Will Deacon',
'Arnd Bergmann', 'Olof Johansson'
> -----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
^ permalink raw reply [flat|nested] 3+ messages in thread