[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.