* [PATCH v6 0/2] hibernation support on ARM @ 2014-02-27 23:57 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-27 23:57 UTC (permalink / raw) To: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel Patches adding support for hibernation on ARM - ARM hibernation / suspend-to-disk - Change soft_restart to use non-tracing raw_local_irq_disable Patches based on v3.13 tag, verified hibernation on beaglebone black on a branch based on 3.13 merged with initial omap support from Russ Dill which can be found here (includes v1 patchset): http://git.linaro.org/git-ro/people/sebastian.capella/linux.git hibernation_3.13_russMerge [PATCH v6 1/2] ARM: avoid tracers in soft_restart arch/arm/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Use raw_local_irq_disable in place of local_irq_disable to avoid infinite abort recursion while tracing. (unchanged since v3) [PATCH v6 2/2] ARM hibernation / suspend-to-disk arch/arm/include/asm/memory.h | 1 + arch/arm/kernel/Makefile | 1 + arch/arm/kernel/hibernate.c | 113 +++++++++++++++++++++++++++++++++++++++++ arch/arm/mm/Kconfig | 5 ++ include/linux/suspend.h | 2 + 5 files changed, 122 insertions(+) Adds support for ARM based hibernation Additional notes: ----------------- There are two checkpatch warnings added by this patch. These follow behavior in existing hibernation implementations on other platforms. WARNING: externs should be avoided in .c files #116: FILE: arch/arm/kernel/hibernate.c:26: +extern const void __nosave_begin, __nosave_end; This extern is picking up the linker nosave region definitions, only used in hibernate. Follows same extern line used mips, powerpc, s390, sh, sparc, x86 & unicore32 WARNING: externs should be avoided in .c files #199: FILE: arch/arm/kernel/hibernate.c:109: +extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); This extern is used in the arch/arm/ in hibernate, process and bL_switcher Changes in v6: -------------- * Simplify static variable names Changes in v5: -------------- * Fixed checkpatch warning on trailing whitespace Changes in v4: -------------- * updated comment for soft_restart with review feedback * dropped freeze_processes patch which was queued separately to 3.14 by Rafael Wysocki: https://lkml.org/lkml/2014/2/25/683 Changes in v3: -------------- * added comment to use of soft_restart * drop irq disable soft_restart patch * add patch to avoid tracers in soft_restart by using raw_local_irq_* Changes in v2: -------------- * Removed unneeded flush_thread, use of __naked and cpu_init. * dropped Cyril Chemparathy <cyril@ti.com> from Cc: list as emails are bouncing. Thanks, Sebastian Capella ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 0/2] hibernation support on ARM @ 2014-02-27 23:57 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-27 23:57 UTC (permalink / raw) To: linux-arm-kernel Patches adding support for hibernation on ARM - ARM hibernation / suspend-to-disk - Change soft_restart to use non-tracing raw_local_irq_disable Patches based on v3.13 tag, verified hibernation on beaglebone black on a branch based on 3.13 merged with initial omap support from Russ Dill which can be found here (includes v1 patchset): http://git.linaro.org/git-ro/people/sebastian.capella/linux.git hibernation_3.13_russMerge [PATCH v6 1/2] ARM: avoid tracers in soft_restart arch/arm/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Use raw_local_irq_disable in place of local_irq_disable to avoid infinite abort recursion while tracing. (unchanged since v3) [PATCH v6 2/2] ARM hibernation / suspend-to-disk arch/arm/include/asm/memory.h | 1 + arch/arm/kernel/Makefile | 1 + arch/arm/kernel/hibernate.c | 113 +++++++++++++++++++++++++++++++++++++++++ arch/arm/mm/Kconfig | 5 ++ include/linux/suspend.h | 2 + 5 files changed, 122 insertions(+) Adds support for ARM based hibernation Additional notes: ----------------- There are two checkpatch warnings added by this patch. These follow behavior in existing hibernation implementations on other platforms. WARNING: externs should be avoided in .c files #116: FILE: arch/arm/kernel/hibernate.c:26: +extern const void __nosave_begin, __nosave_end; This extern is picking up the linker nosave region definitions, only used in hibernate. Follows same extern line used mips, powerpc, s390, sh, sparc, x86 & unicore32 WARNING: externs should be avoided in .c files #199: FILE: arch/arm/kernel/hibernate.c:109: +extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); This extern is used in the arch/arm/ in hibernate, process and bL_switcher Changes in v6: -------------- * Simplify static variable names Changes in v5: -------------- * Fixed checkpatch warning on trailing whitespace Changes in v4: -------------- * updated comment for soft_restart with review feedback * dropped freeze_processes patch which was queued separately to 3.14 by Rafael Wysocki: https://lkml.org/lkml/2014/2/25/683 Changes in v3: -------------- * added comment to use of soft_restart * drop irq disable soft_restart patch * add patch to avoid tracers in soft_restart by using raw_local_irq_* Changes in v2: -------------- * Removed unneeded flush_thread, use of __naked and cpu_init. * dropped Cyril Chemparathy <cyril@ti.com> from Cc: list as emails are bouncing. Thanks, Sebastian Capella ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 1/2] ARM: avoid tracers in soft_restart 2014-02-27 23:57 ` Sebastian Capella @ 2014-02-27 23:57 ` Sebastian Capella -1 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-27 23:57 UTC (permalink / raw) To: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel Cc: Sebastian Capella, Russell King, Andrew Morton, Thomas Gleixner, Will Deacon, Robin Holt, Lorenzo Pieralisi Use of tracers in local_irq_disable is causes recursive aborts when called with irqs disabled and using a temporary stack (hibernation). Replace local_irq_disable with raw_local_irq_disable instead to avoid tracers. Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Will Deacon <will.deacon@arm.com> Cc: Robin Holt <robin.m.holt@gmail.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- arch/arm/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c index 92f7b15..f58b723 100644 --- a/arch/arm/kernel/process.c +++ b/arch/arm/kernel/process.c @@ -100,7 +100,7 @@ void soft_restart(unsigned long addr) u64 *stack = soft_restart_stack + ARRAY_SIZE(soft_restart_stack); /* Disable interrupts first */ - local_irq_disable(); + raw_local_irq_disable(); local_fiq_disable(); /* Disable the L2 if we're the last man standing. */ -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v6 1/2] ARM: avoid tracers in soft_restart @ 2014-02-27 23:57 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-27 23:57 UTC (permalink / raw) To: linux-arm-kernel Use of tracers in local_irq_disable is causes recursive aborts when called with irqs disabled and using a temporary stack (hibernation). Replace local_irq_disable with raw_local_irq_disable instead to avoid tracers. Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Will Deacon <will.deacon@arm.com> Cc: Robin Holt <robin.m.holt@gmail.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- arch/arm/kernel/process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c index 92f7b15..f58b723 100644 --- a/arch/arm/kernel/process.c +++ b/arch/arm/kernel/process.c @@ -100,7 +100,7 @@ void soft_restart(unsigned long addr) u64 *stack = soft_restart_stack + ARRAY_SIZE(soft_restart_stack); /* Disable interrupts first */ - local_irq_disable(); + raw_local_irq_disable(); local_fiq_disable(); /* Disable the L2 if we're the last man standing. */ -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-27 23:57 ` Sebastian Capella @ 2014-02-27 23:57 ` Sebastian Capella -1 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-27 23:57 UTC (permalink / raw) To: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel Cc: Russ Dill, Rafael J. Wysocki, Sebastian Capella, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-König, Stephen Boyd, Lorenzo Pieralisi From: Russ Dill <Russ.Dill@ti.com> Enable hibernation for ARM architectures and provide ARM architecture specific calls used during hibernation. The swsusp hibernation framework depends on the platform first having functional suspend/resume. Then, in order to enable hibernation on a given platform, a platform_hibernation_ops structure may need to be registered with the system in order to save/restore any SoC-specific / cpu specific state needing (re)init over a suspend-to-disk/resume-from-disk cycle. For example: - "secure" SoCs that have different sets of control registers and/or different CR reg access patterns. - SoCs with L2 caches as the activation sequence there is SoC-dependent; a full off-on cycle for L2 is not done by the hibernation support code. - SoCs requiring steps on wakeup _before_ the "generic" parts done by cpu_suspend / cpu_resume can work correctly. - SoCs having persistent state which is maintained during suspend and resume, but will be lost during the power off cycle after suspend-to-disk. This is a rebase/rework of Frank Hofmann's v5 hibernation patchset. Acked-by: Russ Dill <Russ.Dill@ti.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> Acked-by: Pavel Machek <pavel@ucw.cz> Cc: Russell King <linux@arm.linux.org.uk> Cc: Len Brown <len.brown@intel.com> Cc: Nicolas Pitre <nico@linaro.org> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Jonathan Austin <jonathan.austin@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- arch/arm/include/asm/memory.h | 1 + arch/arm/kernel/Makefile | 1 + arch/arm/kernel/hibernate.c | 113 +++++++++++++++++++++++++++++++++++++++++ arch/arm/mm/Kconfig | 5 ++ include/linux/suspend.h | 2 + 5 files changed, 122 insertions(+) create mode 100644 arch/arm/kernel/hibernate.c diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- a/arch/arm/include/asm/memory.h +++ b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline void *phys_to_virt(phys_addr_t x) */ #define __pa(x) __virt_to_phys((unsigned long)(x)) #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) __pa(RELOC_HIDE((unsigned long)(x), 0)) #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) extern phys_addr_t (*arch_virt_to_idmap)(unsigned long x); diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index a30fc9b..8afa848 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_ARTHUR) += arthur.o obj-$(CONFIG_ISA_DMA) += dma-isa.o obj-$(CONFIG_PCI) += bios32.o isa.o obj-$(CONFIG_ARM_CPU_SUSPEND) += sleep.o suspend.o +obj-$(CONFIG_HIBERNATION) += hibernate.o obj-$(CONFIG_SMP) += smp.o ifdef CONFIG_MMU obj-$(CONFIG_SMP) += smp_tlb.o diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c new file mode 100644 index 0000000..a41e0e3 --- /dev/null +++ b/arch/arm/kernel/hibernate.c @@ -0,0 +1,113 @@ +/* + * Hibernation support specific for ARM + * + * Derived from work on ARM hibernation support by: + * + * Ubuntu project, hibernation support for mach-dove + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu) + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.) + * https://lkml.org/lkml/2010/6/18/4 + * https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html + * https://patchwork.kernel.org/patch/96442/ + * + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl> + * + * License terms: GNU General Public License (GPL) version 2 + */ + +#include <linux/mm.h> +#include <linux/suspend.h> +#include <asm/tlbflush.h> +#include <asm/cacheflush.h> +#include <asm/system_misc.h> +#include <asm/idmap.h> +#include <asm/suspend.h> + +extern const void __nosave_begin, __nosave_end; + +int pfn_is_nosave(unsigned long pfn) +{ + unsigned long nosave_begin_pfn = + __pa_symbol(&__nosave_begin) >> PAGE_SHIFT; + unsigned long nosave_end_pfn = + PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT; + + return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn); +} + +void notrace save_processor_state(void) +{ + WARN_ON(num_online_cpus() != 1); + local_fiq_disable(); +} + +void notrace restore_processor_state(void) +{ + local_fiq_enable(); +} + +/* + * Snapshot kernel memory and reset the system. + * + * swsusp_save() is executed in the suspend finisher so that the CPU + * context pointer and memory are part of the saved image, which is + * required by the resume kernel image to restart execution from + * swsusp_arch_suspend(). + * + * soft_restart is not technically needed, but is used to get success + * returned from cpu_suspend. + * + * When soft reboot completes, the hibernation snapshot is written out. + */ +static int notrace arch_save_image(unsigned long unused) +{ + int ret; + + ret = swsusp_save(); + if (ret == 0) + soft_restart(virt_to_phys(cpu_resume)); + return ret; +} + +/* + * Save the current CPU state before suspend / poweroff. + */ +int notrace swsusp_arch_suspend(void) +{ + return cpu_suspend(0, arch_save_image); +} + +/* + * The framework loads the hibernation image into a linked list anchored + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper + * destinations. + * + * To make this work if resume is triggered from initramfs, the + * pagetables need to be switched to allow writes to kernel mem. + */ +static void notrace arch_restore_image(void *unused) +{ + struct pbe *pbe; + + cpu_switch_mm(idmap_pgd, &init_mm); + for (pbe = restore_pblist; pbe; pbe = pbe->next) + copy_page(pbe->orig_address, pbe->address); + + soft_restart(virt_to_phys(cpu_resume)); +} + +static u8 resume_stack[PAGE_SIZE/2] __nosavedata; + +/* + * Resume from the hibernation image. + * Due to the kernel heap / data restore, stack contents change underneath + * and that would make function calls impossible; switch to a temporary + * stack within the nosave region to avoid that problem. + */ +int swsusp_arch_resume(void) +{ + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); + call_with_stack(arch_restore_image, 0, + resume_stack + sizeof(resume_stack)); + return 0; +} diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig index 1f8fed9..83707702 100644 --- a/arch/arm/mm/Kconfig +++ b/arch/arm/mm/Kconfig @@ -611,6 +611,11 @@ config CPU_USE_DOMAINS config IO_36 bool +config ARCH_HIBERNATION_POSSIBLE + bool + depends on MMU + default y if CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || CPU_XSCALE || CPU_XSC3 || CPU_V6 || CPU_V6K || CPU_V7 + comment "Processor Features" config ARM_LPAE diff --git a/include/linux/suspend.h b/include/linux/suspend.h index f73cabf..38bbf95 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -320,6 +320,8 @@ extern unsigned long get_safe_page(gfp_t gfp_mask); extern void hibernation_set_ops(const struct platform_hibernation_ops *ops); extern int hibernate(void); extern bool system_entering_hibernation(void); +asmlinkage int swsusp_save(void); +extern struct pbe *restore_pblist; #else /* CONFIG_HIBERNATION */ static inline void register_nosave_region(unsigned long b, unsigned long e) {} static inline void register_nosave_region_late(unsigned long b, unsigned long e) {} -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-27 23:57 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-27 23:57 UTC (permalink / raw) To: linux-arm-kernel From: Russ Dill <Russ.Dill@ti.com> Enable hibernation for ARM architectures and provide ARM architecture specific calls used during hibernation. The swsusp hibernation framework depends on the platform first having functional suspend/resume. Then, in order to enable hibernation on a given platform, a platform_hibernation_ops structure may need to be registered with the system in order to save/restore any SoC-specific / cpu specific state needing (re)init over a suspend-to-disk/resume-from-disk cycle. For example: - "secure" SoCs that have different sets of control registers and/or different CR reg access patterns. - SoCs with L2 caches as the activation sequence there is SoC-dependent; a full off-on cycle for L2 is not done by the hibernation support code. - SoCs requiring steps on wakeup _before_ the "generic" parts done by cpu_suspend / cpu_resume can work correctly. - SoCs having persistent state which is maintained during suspend and resume, but will be lost during the power off cycle after suspend-to-disk. This is a rebase/rework of Frank Hofmann's v5 hibernation patchset. Acked-by: Russ Dill <Russ.Dill@ti.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> Acked-by: Pavel Machek <pavel@ucw.cz> Cc: Russell King <linux@arm.linux.org.uk> Cc: Len Brown <len.brown@intel.com> Cc: Nicolas Pitre <nico@linaro.org> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Jonathan Austin <jonathan.austin@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: "Uwe Kleine-K?nig" <u.kleine-koenig@pengutronix.de> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- arch/arm/include/asm/memory.h | 1 + arch/arm/kernel/Makefile | 1 + arch/arm/kernel/hibernate.c | 113 +++++++++++++++++++++++++++++++++++++++++ arch/arm/mm/Kconfig | 5 ++ include/linux/suspend.h | 2 + 5 files changed, 122 insertions(+) create mode 100644 arch/arm/kernel/hibernate.c diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- a/arch/arm/include/asm/memory.h +++ b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline void *phys_to_virt(phys_addr_t x) */ #define __pa(x) __virt_to_phys((unsigned long)(x)) #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) __pa(RELOC_HIDE((unsigned long)(x), 0)) #define pfn_to_kaddr(pfn) __va((pfn) << PAGE_SHIFT) extern phys_addr_t (*arch_virt_to_idmap)(unsigned long x); diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile index a30fc9b..8afa848 100644 --- a/arch/arm/kernel/Makefile +++ b/arch/arm/kernel/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_ARTHUR) += arthur.o obj-$(CONFIG_ISA_DMA) += dma-isa.o obj-$(CONFIG_PCI) += bios32.o isa.o obj-$(CONFIG_ARM_CPU_SUSPEND) += sleep.o suspend.o +obj-$(CONFIG_HIBERNATION) += hibernate.o obj-$(CONFIG_SMP) += smp.o ifdef CONFIG_MMU obj-$(CONFIG_SMP) += smp_tlb.o diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c new file mode 100644 index 0000000..a41e0e3 --- /dev/null +++ b/arch/arm/kernel/hibernate.c @@ -0,0 +1,113 @@ +/* + * Hibernation support specific for ARM + * + * Derived from work on ARM hibernation support by: + * + * Ubuntu project, hibernation support for mach-dove + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu) + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.) + * https://lkml.org/lkml/2010/6/18/4 + * https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html + * https://patchwork.kernel.org/patch/96442/ + * + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl> + * + * License terms: GNU General Public License (GPL) version 2 + */ + +#include <linux/mm.h> +#include <linux/suspend.h> +#include <asm/tlbflush.h> +#include <asm/cacheflush.h> +#include <asm/system_misc.h> +#include <asm/idmap.h> +#include <asm/suspend.h> + +extern const void __nosave_begin, __nosave_end; + +int pfn_is_nosave(unsigned long pfn) +{ + unsigned long nosave_begin_pfn = + __pa_symbol(&__nosave_begin) >> PAGE_SHIFT; + unsigned long nosave_end_pfn = + PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT; + + return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn); +} + +void notrace save_processor_state(void) +{ + WARN_ON(num_online_cpus() != 1); + local_fiq_disable(); +} + +void notrace restore_processor_state(void) +{ + local_fiq_enable(); +} + +/* + * Snapshot kernel memory and reset the system. + * + * swsusp_save() is executed in the suspend finisher so that the CPU + * context pointer and memory are part of the saved image, which is + * required by the resume kernel image to restart execution from + * swsusp_arch_suspend(). + * + * soft_restart is not technically needed, but is used to get success + * returned from cpu_suspend. + * + * When soft reboot completes, the hibernation snapshot is written out. + */ +static int notrace arch_save_image(unsigned long unused) +{ + int ret; + + ret = swsusp_save(); + if (ret == 0) + soft_restart(virt_to_phys(cpu_resume)); + return ret; +} + +/* + * Save the current CPU state before suspend / poweroff. + */ +int notrace swsusp_arch_suspend(void) +{ + return cpu_suspend(0, arch_save_image); +} + +/* + * The framework loads the hibernation image into a linked list anchored + *@restore_pblist, for swsusp_arch_resume() to copy back to the proper + * destinations. + * + * To make this work if resume is triggered from initramfs, the + * pagetables need to be switched to allow writes to kernel mem. + */ +static void notrace arch_restore_image(void *unused) +{ + struct pbe *pbe; + + cpu_switch_mm(idmap_pgd, &init_mm); + for (pbe = restore_pblist; pbe; pbe = pbe->next) + copy_page(pbe->orig_address, pbe->address); + + soft_restart(virt_to_phys(cpu_resume)); +} + +static u8 resume_stack[PAGE_SIZE/2] __nosavedata; + +/* + * Resume from the hibernation image. + * Due to the kernel heap / data restore, stack contents change underneath + * and that would make function calls impossible; switch to a temporary + * stack within the nosave region to avoid that problem. + */ +int swsusp_arch_resume(void) +{ + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); + call_with_stack(arch_restore_image, 0, + resume_stack + sizeof(resume_stack)); + return 0; +} diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig index 1f8fed9..83707702 100644 --- a/arch/arm/mm/Kconfig +++ b/arch/arm/mm/Kconfig @@ -611,6 +611,11 @@ config CPU_USE_DOMAINS config IO_36 bool +config ARCH_HIBERNATION_POSSIBLE + bool + depends on MMU + default y if CPU_ARM920T || CPU_ARM926T || CPU_SA1100 || CPU_XSCALE || CPU_XSC3 || CPU_V6 || CPU_V6K || CPU_V7 + comment "Processor Features" config ARM_LPAE diff --git a/include/linux/suspend.h b/include/linux/suspend.h index f73cabf..38bbf95 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -320,6 +320,8 @@ extern unsigned long get_safe_page(gfp_t gfp_mask); extern void hibernation_set_ops(const struct platform_hibernation_ops *ops); extern int hibernate(void); extern bool system_entering_hibernation(void); +asmlinkage int swsusp_save(void); +extern struct pbe *restore_pblist; #else /* CONFIG_HIBERNATION */ static inline void register_nosave_region(unsigned long b, unsigned long e) {} static inline void register_nosave_region_late(unsigned long b, unsigned long e) {} -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-27 23:57 ` Sebastian Capella @ 2014-02-28 0:09 ` Stephen Boyd -1 siblings, 0 replies; 38+ messages in thread From: Stephen Boyd @ 2014-02-28 0:09 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-König, Lorenzo Pieralisi On 02/27/14 15:57, Sebastian Capella wrote: > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > index 8756e4b..1079ea8 100644 > --- a/arch/arm/include/asm/memory.h > +++ b/arch/arm/include/asm/memory.h > @@ -291,6 +291,7 @@ static inline void *phys_to_virt(phys_addr_t x) > */ > #define __pa(x) __virt_to_phys((unsigned long)(x)) > #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) > +#define __pa_symbol(x) __pa(RELOC_HIDE((unsigned long)(x), 0)) Just curious, is there a reason for the RELOC_HIDE() here? Or __pa_symbol() for that matter? It looks like only x86 uses this on the __nosave_{begin,end} symbol. Maybe it's copy-pasta? I also wonder if anyone has thought about making a __weak pfn_is_nosave() function so that architectures don't need to implement the same thing every time. Consolidating those shouldn't be part of this patch though. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 0:09 ` Stephen Boyd 0 siblings, 0 replies; 38+ messages in thread From: Stephen Boyd @ 2014-02-28 0:09 UTC (permalink / raw) To: linux-arm-kernel On 02/27/14 15:57, Sebastian Capella wrote: > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h > index 8756e4b..1079ea8 100644 > --- a/arch/arm/include/asm/memory.h > +++ b/arch/arm/include/asm/memory.h > @@ -291,6 +291,7 @@ static inline void *phys_to_virt(phys_addr_t x) > */ > #define __pa(x) __virt_to_phys((unsigned long)(x)) > #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x))) > +#define __pa_symbol(x) __pa(RELOC_HIDE((unsigned long)(x), 0)) Just curious, is there a reason for the RELOC_HIDE() here? Or __pa_symbol() for that matter? It looks like only x86 uses this on the __nosave_{begin,end} symbol. Maybe it's copy-pasta? I also wonder if anyone has thought about making a __weak pfn_is_nosave() function so that architectures don't need to implement the same thing every time. Consolidating those shouldn't be part of this patch though. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-28 0:09 ` Stephen Boyd (?) @ 2014-02-28 1:47 ` Russ Dill -1 siblings, 0 replies; 38+ messages in thread From: Russ Dill @ 2014-02-28 1:47 UTC (permalink / raw) To: Stephen Boyd, Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-König, Lorenzo Pieralisi On 02/27/2014 04:09 PM, Stephen Boyd wrote: > On 02/27/14 15:57, Sebastian Capella wrote: >> diff --git a/arch/arm/include/asm/memory.h >> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- >> a/arch/arm/include/asm/memory.h +++ >> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline >> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) >> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void >> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) >> __pa(RELOC_HIDE((unsigned long)(x), 0)) > > Just curious, is there a reason for the RELOC_HIDE() here? Or > __pa_symbol() for that matter? It looks like only x86 uses this on > the __nosave_{begin,end} symbol. Maybe it's copy-pasta? >From my understanding this needs to stick around so long as gcc 3.x is supported (did it get dropped yet?) on ARM Linux since it doesn't support -fno-strict-overflow. > I also wonder if anyone has thought about making a __weak > pfn_is_nosave() function so that architectures don't need to > implement the same thing every time. Consolidating those shouldn't > be part of this patch though. > Yes, I think just a couple of the architectures do anything besides checking if the address falls within the nosave section. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 1:47 ` Russ Dill 0 siblings, 0 replies; 38+ messages in thread From: Russ Dill @ 2014-02-28 1:47 UTC (permalink / raw) To: linux-arm-kernel On 02/27/2014 04:09 PM, Stephen Boyd wrote: > On 02/27/14 15:57, Sebastian Capella wrote: >> diff --git a/arch/arm/include/asm/memory.h >> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- >> a/arch/arm/include/asm/memory.h +++ >> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline >> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) >> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void >> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) >> __pa(RELOC_HIDE((unsigned long)(x), 0)) > > Just curious, is there a reason for the RELOC_HIDE() here? Or > __pa_symbol() for that matter? It looks like only x86 uses this on > the __nosave_{begin,end} symbol. Maybe it's copy-pasta? >From my understanding this needs to stick around so long as gcc 3.x is supported (did it get dropped yet?) on ARM Linux since it doesn't support -fno-strict-overflow. > I also wonder if anyone has thought about making a __weak > pfn_is_nosave() function so that architectures don't need to > implement the same thing every time. Consolidating those shouldn't > be part of this patch though. > Yes, I think just a couple of the architectures do anything besides checking if the address falls within the nosave section. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 1:47 ` Russ Dill 0 siblings, 0 replies; 38+ messages in thread From: Russ Dill @ 2014-02-28 1:47 UTC (permalink / raw) To: Stephen Boyd, Sebastian Capella Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin, linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Lorenzo Pieralisi, Santosh Shilimkar, Catalin Marinas, Uwe Kleine-König, linux-arm-kernel On 02/27/2014 04:09 PM, Stephen Boyd wrote: > On 02/27/14 15:57, Sebastian Capella wrote: >> diff --git a/arch/arm/include/asm/memory.h >> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- >> a/arch/arm/include/asm/memory.h +++ >> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline >> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) >> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void >> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) >> __pa(RELOC_HIDE((unsigned long)(x), 0)) > > Just curious, is there a reason for the RELOC_HIDE() here? Or > __pa_symbol() for that matter? It looks like only x86 uses this on > the __nosave_{begin,end} symbol. Maybe it's copy-pasta? >From my understanding this needs to stick around so long as gcc 3.x is supported (did it get dropped yet?) on ARM Linux since it doesn't support -fno-strict-overflow. > I also wonder if anyone has thought about making a __weak > pfn_is_nosave() function so that architectures don't need to > implement the same thing every time. Consolidating those shouldn't > be part of this patch though. > Yes, I think just a couple of the architectures do anything besides checking if the address falls within the nosave section. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-28 1:47 ` Russ Dill @ 2014-02-28 2:19 ` Stephen Boyd -1 siblings, 0 replies; 38+ messages in thread From: Stephen Boyd @ 2014-02-28 2:19 UTC (permalink / raw) To: Russ Dill Cc: Sebastian Capella, linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-König, Lorenzo Pieralisi On 02/27/14 17:47, Russ Dill wrote: > On 02/27/2014 04:09 PM, Stephen Boyd wrote: >> On 02/27/14 15:57, Sebastian Capella wrote: >>> diff --git a/arch/arm/include/asm/memory.h >>> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- >>> a/arch/arm/include/asm/memory.h +++ >>> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline >>> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) >>> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void >>> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) >>> __pa(RELOC_HIDE((unsigned long)(x), 0)) >> Just curious, is there a reason for the RELOC_HIDE() here? Or >> __pa_symbol() for that matter? It looks like only x86 uses this on >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta? > From my understanding this needs to stick around so long as gcc 3.x is > supported (did it get dropped yet?) on ARM Linux since it doesn't > support -fno-strict-overflow. I don't think it's been dropped yet but I wonder if anyone has tried recent kernels with such a compiler? Would the usage of &__pv_table_begin in arch/arm/mm/mmu.c also need the same treatment? Or the tagtable loop in atags_parse.c? Do the other architectures also need to be fixed? That link Sebastian points to says that ppc originally needed it but pfn_is_nosave() on ppc doesn't use RELOC_HIDE anywhere in their __pa() macro from what I can tell. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 2:19 ` Stephen Boyd 0 siblings, 0 replies; 38+ messages in thread From: Stephen Boyd @ 2014-02-28 2:19 UTC (permalink / raw) To: linux-arm-kernel On 02/27/14 17:47, Russ Dill wrote: > On 02/27/2014 04:09 PM, Stephen Boyd wrote: >> On 02/27/14 15:57, Sebastian Capella wrote: >>> diff --git a/arch/arm/include/asm/memory.h >>> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- >>> a/arch/arm/include/asm/memory.h +++ >>> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline >>> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) >>> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void >>> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) >>> __pa(RELOC_HIDE((unsigned long)(x), 0)) >> Just curious, is there a reason for the RELOC_HIDE() here? Or >> __pa_symbol() for that matter? It looks like only x86 uses this on >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta? > From my understanding this needs to stick around so long as gcc 3.x is > supported (did it get dropped yet?) on ARM Linux since it doesn't > support -fno-strict-overflow. I don't think it's been dropped yet but I wonder if anyone has tried recent kernels with such a compiler? Would the usage of &__pv_table_begin in arch/arm/mm/mmu.c also need the same treatment? Or the tagtable loop in atags_parse.c? Do the other architectures also need to be fixed? That link Sebastian points to says that ppc originally needed it but pfn_is_nosave() on ppc doesn't use RELOC_HIDE anywhere in their __pa() macro from what I can tell. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-28 2:19 ` Stephen Boyd @ 2014-02-28 10:20 ` Russell King - ARM Linux -1 siblings, 0 replies; 38+ messages in thread From: Russell King - ARM Linux @ 2014-02-28 10:20 UTC (permalink / raw) To: Stephen Boyd Cc: Russ Dill, Sebastian Capella, linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-König, Lorenzo Pieralisi On Thu, Feb 27, 2014 at 06:19:49PM -0800, Stephen Boyd wrote: > On 02/27/14 17:47, Russ Dill wrote: > > On 02/27/2014 04:09 PM, Stephen Boyd wrote: > >> On 02/27/14 15:57, Sebastian Capella wrote: > >>> diff --git a/arch/arm/include/asm/memory.h > >>> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- > >>> a/arch/arm/include/asm/memory.h +++ > >>> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline > >>> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) > >>> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void > >>> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) > >>> __pa(RELOC_HIDE((unsigned long)(x), 0)) > >> Just curious, is there a reason for the RELOC_HIDE() here? Or > >> __pa_symbol() for that matter? It looks like only x86 uses this on > >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta? > > From my understanding this needs to stick around so long as gcc 3.x is > > supported (did it get dropped yet?) on ARM Linux since it doesn't > > support -fno-strict-overflow. > > I don't think it's been dropped yet but I wonder if anyone has tried > recent kernels with such a compiler? > > Would the usage of &__pv_table_begin in arch/arm/mm/mmu.c also need the > same treatment? We've never had to play these kinds of games on ARM irrespective of compiler version. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 10:20 ` Russell King - ARM Linux 0 siblings, 0 replies; 38+ messages in thread From: Russell King - ARM Linux @ 2014-02-28 10:20 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 27, 2014 at 06:19:49PM -0800, Stephen Boyd wrote: > On 02/27/14 17:47, Russ Dill wrote: > > On 02/27/2014 04:09 PM, Stephen Boyd wrote: > >> On 02/27/14 15:57, Sebastian Capella wrote: > >>> diff --git a/arch/arm/include/asm/memory.h > >>> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- > >>> a/arch/arm/include/asm/memory.h +++ > >>> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline > >>> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) > >>> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void > >>> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) > >>> __pa(RELOC_HIDE((unsigned long)(x), 0)) > >> Just curious, is there a reason for the RELOC_HIDE() here? Or > >> __pa_symbol() for that matter? It looks like only x86 uses this on > >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta? > > From my understanding this needs to stick around so long as gcc 3.x is > > supported (did it get dropped yet?) on ARM Linux since it doesn't > > support -fno-strict-overflow. > > I don't think it's been dropped yet but I wonder if anyone has tried > recent kernels with such a compiler? > > Would the usage of &__pv_table_begin in arch/arm/mm/mmu.c also need the > same treatment? We've never had to play these kinds of games on ARM irrespective of compiler version. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ^ permalink raw reply [flat|nested] 38+ messages in thread
[parent not found: <20140228181731.29118.41809@capellas-linux>]
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk [not found] ` <20140228181731.29118.41809@capellas-linux> 2014-03-05 2:28 ` Sebastian Capella @ 2014-03-05 2:28 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-05 2:28 UTC (permalink / raw) To: Sebastian Capella, Russell King - ARM Linux, Stephen Boyd Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, "Uwe Kleine-König", Lorenzo Pieralisi, Russ Dill, Stephen Boyd Quoting Sebastian Capella (2014-02-28 10:17:31) > Quoting Russell King - ARM Linux (2014-02-28 02:20:18) > > On Thu, Feb 27, 2014 at 06:19:49PM -0800, Stephen Boyd wrote: > > > On 02/27/14 17:47, Russ Dill wrote: > > > > On 02/27/2014 04:09 PM, Stephen Boyd wrote: > > > >> On 02/27/14 15:57, Sebastian Capella wrote: > > > >>> diff --git a/arch/arm/include/asm/memory.h > > > >>> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- > > > >>> a/arch/arm/include/asm/memory.h +++ > > > >>> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline > > > >>> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) > > > >>> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void > > > >>> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) > > > >>> __pa(RELOC_HIDE((unsigned long)(x), 0)) > > > >> Just curious, is there a reason for the RELOC_HIDE() here? Or > > > >> __pa_symbol() for that matter? It looks like only x86 uses this on > > > >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta? > > > > From my understanding this needs to stick around so long as gcc 3.x is > > > > supported (did it get dropped yet?) on ARM Linux since it doesn't > > > > support -fno-strict-overflow. > > > > > > I don't think it's been dropped yet but I wonder if anyone has tried > > > recent kernels with such a compiler? > > > > > > Would the usage of &__pv_table_begin in arch/arm/mm/mmu.c also need the > > > same treatment? > > > > We've never had to play these kinds of games on ARM irrespective of > > compiler version. > > I am using gcc 4.6.3. I can try removing it but I suspect it will just > work without it. Let me see if I can get an older compiler and try both > ways. Hi, I've been struggling a bit to test 3.x compilers on this. I'm running an armv7 board, but the 3.x compilers I'm trying don't appear to suport armv7. Anyone have any suggestions? Is this a worthwhile effort? Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-03-05 2:28 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-05 2:28 UTC (permalink / raw) To: linux-arm-kernel Quoting Sebastian Capella (2014-02-28 10:17:31) > Quoting Russell King - ARM Linux (2014-02-28 02:20:18) > > On Thu, Feb 27, 2014 at 06:19:49PM -0800, Stephen Boyd wrote: > > > On 02/27/14 17:47, Russ Dill wrote: > > > > On 02/27/2014 04:09 PM, Stephen Boyd wrote: > > > >> On 02/27/14 15:57, Sebastian Capella wrote: > > > >>> diff --git a/arch/arm/include/asm/memory.h > > > >>> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- > > > >>> a/arch/arm/include/asm/memory.h +++ > > > >>> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline > > > >>> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) > > > >>> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void > > > >>> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) > > > >>> __pa(RELOC_HIDE((unsigned long)(x), 0)) > > > >> Just curious, is there a reason for the RELOC_HIDE() here? Or > > > >> __pa_symbol() for that matter? It looks like only x86 uses this on > > > >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta? > > > > From my understanding this needs to stick around so long as gcc 3.x is > > > > supported (did it get dropped yet?) on ARM Linux since it doesn't > > > > support -fno-strict-overflow. > > > > > > I don't think it's been dropped yet but I wonder if anyone has tried > > > recent kernels with such a compiler? > > > > > > Would the usage of &__pv_table_begin in arch/arm/mm/mmu.c also need the > > > same treatment? > > > > We've never had to play these kinds of games on ARM irrespective of > > compiler version. > > I am using gcc 4.6.3. I can try removing it but I suspect it will just > work without it. Let me see if I can get an older compiler and try both > ways. Hi, I've been struggling a bit to test 3.x compilers on this. I'm running an armv7 board, but the 3.x compilers I'm trying don't appear to suport armv7. Anyone have any suggestions? Is this a worthwhile effort? Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-03-05 2:28 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-05 2:28 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, "Uwe Kleine-König", Lorenzo Pieralisi, Russ Dill, Stephen Boyd Quoting Sebastian Capella (2014-02-28 10:17:31) > Quoting Russell King - ARM Linux (2014-02-28 02:20:18) > > On Thu, Feb 27, 2014 at 06:19:49PM -0800, Stephen Boyd wrote: > > > On 02/27/14 17:47, Russ Dill wrote: > > > > On 02/27/2014 04:09 PM, Stephen Boyd wrote: > > > >> On 02/27/14 15:57, Sebastian Capella wrote: > > > >>> diff --git a/arch/arm/include/asm/memory.h > > > >>> b/arch/arm/include/asm/memory.h index 8756e4b..1079ea8 100644 --- > > > >>> a/arch/arm/include/asm/memory.h +++ > > > >>> b/arch/arm/include/asm/memory.h @@ -291,6 +291,7 @@ static inline > > > >>> void *phys_to_virt(phys_addr_t x) */ #define __pa(x) > > > >>> __virt_to_phys((unsigned long)(x)) #define __va(x) ((void > > > >>> *)__phys_to_virt((phys_addr_t)(x))) +#define __pa_symbol(x) > > > >>> __pa(RELOC_HIDE((unsigned long)(x), 0)) > > > >> Just curious, is there a reason for the RELOC_HIDE() here? Or > > > >> __pa_symbol() for that matter? It looks like only x86 uses this on > > > >> the __nosave_{begin,end} symbol. Maybe it's copy-pasta? > > > > From my understanding this needs to stick around so long as gcc 3.x is > > > > supported (did it get dropped yet?) on ARM Linux since it doesn't > > > > support -fno-strict-overflow. > > > > > > I don't think it's been dropped yet but I wonder if anyone has tried > > > recent kernels with such a compiler? > > > > > > Would the usage of &__pv_table_begin in arch/arm/mm/mmu.c also need the > > > same treatment? > > > > We've never had to play these kinds of games on ARM irrespective of > > compiler version. > > I am using gcc 4.6.3. I can try removing it but I suspect it will just > work without it. Let me see if I can get an older compiler and try both > ways. Hi, I've been struggling a bit to test 3.x compilers on this. I'm running an armv7 board, but the 3.x compilers I'm trying don't appear to suport armv7. Anyone have any suggestions? Is this a worthwhile effort? Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-03-05 2:28 ` Sebastian Capella @ 2014-06-02 16:57 ` Sebastian Capella -1 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-06-02 16:57 UTC (permalink / raw) To: Sebastian Capella, Russell King - ARM Linux, Stephen Boyd, sebcape Cc: Linux Kernel, linux-pm, linaro-kernel, linux-arm-kernel, Rafael J. Wysocki, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-König, Lorenzo Pieralisi, Russ Dill Want to log my new email with this thread in case any questions arise later and people have trouble finding me. sebcape@gmail.com Thanks! Sebastian Capella ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-06-02 16:57 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-06-02 16:57 UTC (permalink / raw) To: linux-arm-kernel Want to log my new email with this thread in case any questions arise later and people have trouble finding me. sebcape at gmail.com Thanks! Sebastian Capella ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-27 23:57 ` Sebastian Capella (?) @ 2014-02-28 9:50 ` Lorenzo Pieralisi -1 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-02-28 9:50 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd On Thu, Feb 27, 2014 at 11:57:58PM +0000, Sebastian Capella wrote: [...] > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c > new file mode 100644 > index 0000000..a41e0e3 > --- /dev/null > +++ b/arch/arm/kernel/hibernate.c > @@ -0,0 +1,113 @@ > +/* > + * Hibernation support specific for ARM > + * > + * Derived from work on ARM hibernation support by: > + * > + * Ubuntu project, hibernation support for mach-dove > + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu) > + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.) > + * https://lkml.org/lkml/2010/6/18/4 > + * https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html > + * https://patchwork.kernel.org/patch/96442/ > + * > + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl> > + * > + * License terms: GNU General Public License (GPL) version 2 > + */ > + > +#include <linux/mm.h> > +#include <linux/suspend.h> > +#include <asm/tlbflush.h> > +#include <asm/cacheflush.h> You can drop tlbflush.h and cacheflush.h, they do not seem to be needed. > +#include <asm/system_misc.h> > +#include <asm/idmap.h> > +#include <asm/suspend.h> > + > +extern const void __nosave_begin, __nosave_end; > + > +int pfn_is_nosave(unsigned long pfn) > +{ > + unsigned long nosave_begin_pfn = > + __pa_symbol(&__nosave_begin) >> PAGE_SHIFT; > + unsigned long nosave_end_pfn = > + PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT; > + > + return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn); > +} > + > +void notrace save_processor_state(void) > +{ > + WARN_ON(num_online_cpus() != 1); > + local_fiq_disable(); > +} > + > +void notrace restore_processor_state(void) > +{ > + local_fiq_enable(); > +} > + > +/* > + * Snapshot kernel memory and reset the system. > + * > + * swsusp_save() is executed in the suspend finisher so that the CPU > + * context pointer and memory are part of the saved image, which is > + * required by the resume kernel image to restart execution from > + * swsusp_arch_suspend(). > + * > + * soft_restart is not technically needed, but is used to get success > + * returned from cpu_suspend. > + * > + * When soft reboot completes, the hibernation snapshot is written out. > + */ > +static int notrace arch_save_image(unsigned long unused) > +{ > + int ret; > + > + ret = swsusp_save(); > + if (ret == 0) > + soft_restart(virt_to_phys(cpu_resume)); > + return ret; > +} > + > +/* > + * Save the current CPU state before suspend / poweroff. > + */ > +int notrace swsusp_arch_suspend(void) > +{ > + return cpu_suspend(0, arch_save_image); > +} > + > +/* > + * The framework loads the hibernation image into a linked list anchored > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > + * destinations. > + * > + * To make this work if resume is triggered from initramfs, the > + * pagetables need to be switched to allow writes to kernel mem. > + */ Comment above needs updating. We are switching page tables to a set of page tables that are certain to live at the same location in the older kernel, that's the only reason, as we discussed. soft_restart will make sure (again) to switch to 1:1 page tables so that we can call cpu_resume with the MMU off. > +static void notrace arch_restore_image(void *unused) > +{ > + struct pbe *pbe; > + > + cpu_switch_mm(idmap_pgd, &init_mm); > + for (pbe = restore_pblist; pbe; pbe = pbe->next) > + copy_page(pbe->orig_address, pbe->address); > + > + soft_restart(virt_to_phys(cpu_resume)); > +} > + > +static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > + > +/* > + * Resume from the hibernation image. > + * Due to the kernel heap / data restore, stack contents change underneath > + * and that would make function calls impossible; switch to a temporary > + * stack within the nosave region to avoid that problem. > + */ > +int swsusp_arch_resume(void) > +{ > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > + call_with_stack(arch_restore_image, 0, > + resume_stack + sizeof(resume_stack)); This does not guarantee your stack is 8-byte aligned, that's not AAPCS compliant and might buy you trouble. Either you align the stack or you align the pointer you are passing. Please have a look at kernel/process.c Thanks, Lorenzo ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 9:50 ` Lorenzo Pieralisi 0 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-02-28 9:50 UTC (permalink / raw) To: linux-arm-kernel On Thu, Feb 27, 2014 at 11:57:58PM +0000, Sebastian Capella wrote: [...] > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c > new file mode 100644 > index 0000000..a41e0e3 > --- /dev/null > +++ b/arch/arm/kernel/hibernate.c > @@ -0,0 +1,113 @@ > +/* > + * Hibernation support specific for ARM > + * > + * Derived from work on ARM hibernation support by: > + * > + * Ubuntu project, hibernation support for mach-dove > + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu) > + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.) > + * https://lkml.org/lkml/2010/6/18/4 > + * https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html > + * https://patchwork.kernel.org/patch/96442/ > + * > + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl> > + * > + * License terms: GNU General Public License (GPL) version 2 > + */ > + > +#include <linux/mm.h> > +#include <linux/suspend.h> > +#include <asm/tlbflush.h> > +#include <asm/cacheflush.h> You can drop tlbflush.h and cacheflush.h, they do not seem to be needed. > +#include <asm/system_misc.h> > +#include <asm/idmap.h> > +#include <asm/suspend.h> > + > +extern const void __nosave_begin, __nosave_end; > + > +int pfn_is_nosave(unsigned long pfn) > +{ > + unsigned long nosave_begin_pfn = > + __pa_symbol(&__nosave_begin) >> PAGE_SHIFT; > + unsigned long nosave_end_pfn = > + PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT; > + > + return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn); > +} > + > +void notrace save_processor_state(void) > +{ > + WARN_ON(num_online_cpus() != 1); > + local_fiq_disable(); > +} > + > +void notrace restore_processor_state(void) > +{ > + local_fiq_enable(); > +} > + > +/* > + * Snapshot kernel memory and reset the system. > + * > + * swsusp_save() is executed in the suspend finisher so that the CPU > + * context pointer and memory are part of the saved image, which is > + * required by the resume kernel image to restart execution from > + * swsusp_arch_suspend(). > + * > + * soft_restart is not technically needed, but is used to get success > + * returned from cpu_suspend. > + * > + * When soft reboot completes, the hibernation snapshot is written out. > + */ > +static int notrace arch_save_image(unsigned long unused) > +{ > + int ret; > + > + ret = swsusp_save(); > + if (ret == 0) > + soft_restart(virt_to_phys(cpu_resume)); > + return ret; > +} > + > +/* > + * Save the current CPU state before suspend / poweroff. > + */ > +int notrace swsusp_arch_suspend(void) > +{ > + return cpu_suspend(0, arch_save_image); > +} > + > +/* > + * The framework loads the hibernation image into a linked list anchored > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > + * destinations. > + * > + * To make this work if resume is triggered from initramfs, the > + * pagetables need to be switched to allow writes to kernel mem. > + */ Comment above needs updating. We are switching page tables to a set of page tables that are certain to live at the same location in the older kernel, that's the only reason, as we discussed. soft_restart will make sure (again) to switch to 1:1 page tables so that we can call cpu_resume with the MMU off. > +static void notrace arch_restore_image(void *unused) > +{ > + struct pbe *pbe; > + > + cpu_switch_mm(idmap_pgd, &init_mm); > + for (pbe = restore_pblist; pbe; pbe = pbe->next) > + copy_page(pbe->orig_address, pbe->address); > + > + soft_restart(virt_to_phys(cpu_resume)); > +} > + > +static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > + > +/* > + * Resume from the hibernation image. > + * Due to the kernel heap / data restore, stack contents change underneath > + * and that would make function calls impossible; switch to a temporary > + * stack within the nosave region to avoid that problem. > + */ > +int swsusp_arch_resume(void) > +{ > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > + call_with_stack(arch_restore_image, 0, > + resume_stack + sizeof(resume_stack)); This does not guarantee your stack is 8-byte aligned, that's not AAPCS compliant and might buy you trouble. Either you align the stack or you align the pointer you are passing. Please have a look at kernel/process.c Thanks, Lorenzo ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 9:50 ` Lorenzo Pieralisi 0 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-02-28 9:50 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd On Thu, Feb 27, 2014 at 11:57:58PM +0000, Sebastian Capella wrote: [...] > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c > new file mode 100644 > index 0000000..a41e0e3 > --- /dev/null > +++ b/arch/arm/kernel/hibernate.c > @@ -0,0 +1,113 @@ > +/* > + * Hibernation support specific for ARM > + * > + * Derived from work on ARM hibernation support by: > + * > + * Ubuntu project, hibernation support for mach-dove > + * Copyright (C) 2010 Nokia Corporation (Hiroshi Doyu) > + * Copyright (C) 2010 Texas Instruments, Inc. (Teerth Reddy et al.) > + * https://lkml.org/lkml/2010/6/18/4 > + * https://lists.linux-foundation.org/pipermail/linux-pm/2010-June/027422.html > + * https://patchwork.kernel.org/patch/96442/ > + * > + * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl> > + * > + * License terms: GNU General Public License (GPL) version 2 > + */ > + > +#include <linux/mm.h> > +#include <linux/suspend.h> > +#include <asm/tlbflush.h> > +#include <asm/cacheflush.h> You can drop tlbflush.h and cacheflush.h, they do not seem to be needed. > +#include <asm/system_misc.h> > +#include <asm/idmap.h> > +#include <asm/suspend.h> > + > +extern const void __nosave_begin, __nosave_end; > + > +int pfn_is_nosave(unsigned long pfn) > +{ > + unsigned long nosave_begin_pfn = > + __pa_symbol(&__nosave_begin) >> PAGE_SHIFT; > + unsigned long nosave_end_pfn = > + PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT; > + > + return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn); > +} > + > +void notrace save_processor_state(void) > +{ > + WARN_ON(num_online_cpus() != 1); > + local_fiq_disable(); > +} > + > +void notrace restore_processor_state(void) > +{ > + local_fiq_enable(); > +} > + > +/* > + * Snapshot kernel memory and reset the system. > + * > + * swsusp_save() is executed in the suspend finisher so that the CPU > + * context pointer and memory are part of the saved image, which is > + * required by the resume kernel image to restart execution from > + * swsusp_arch_suspend(). > + * > + * soft_restart is not technically needed, but is used to get success > + * returned from cpu_suspend. > + * > + * When soft reboot completes, the hibernation snapshot is written out. > + */ > +static int notrace arch_save_image(unsigned long unused) > +{ > + int ret; > + > + ret = swsusp_save(); > + if (ret == 0) > + soft_restart(virt_to_phys(cpu_resume)); > + return ret; > +} > + > +/* > + * Save the current CPU state before suspend / poweroff. > + */ > +int notrace swsusp_arch_suspend(void) > +{ > + return cpu_suspend(0, arch_save_image); > +} > + > +/* > + * The framework loads the hibernation image into a linked list anchored > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > + * destinations. > + * > + * To make this work if resume is triggered from initramfs, the > + * pagetables need to be switched to allow writes to kernel mem. > + */ Comment above needs updating. We are switching page tables to a set of page tables that are certain to live at the same location in the older kernel, that's the only reason, as we discussed. soft_restart will make sure (again) to switch to 1:1 page tables so that we can call cpu_resume with the MMU off. > +static void notrace arch_restore_image(void *unused) > +{ > + struct pbe *pbe; > + > + cpu_switch_mm(idmap_pgd, &init_mm); > + for (pbe = restore_pblist; pbe; pbe = pbe->next) > + copy_page(pbe->orig_address, pbe->address); > + > + soft_restart(virt_to_phys(cpu_resume)); > +} > + > +static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > + > +/* > + * Resume from the hibernation image. > + * Due to the kernel heap / data restore, stack contents change underneath > + * and that would make function calls impossible; switch to a temporary > + * stack within the nosave region to avoid that problem. > + */ > +int swsusp_arch_resume(void) > +{ > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > + call_with_stack(arch_restore_image, 0, > + resume_stack + sizeof(resume_stack)); This does not guarantee your stack is 8-byte aligned, that's not AAPCS compliant and might buy you trouble. Either you align the stack or you align the pointer you are passing. Please have a look at kernel/process.c Thanks, Lorenzo ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-28 9:50 ` Lorenzo Pieralisi @ 2014-02-28 20:15 ` Sebastian Capella -1 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-28 20:15 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin, linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig, Russ Dill, Catalin Marinas, Santosh Shilimkar, Stephen Boyd, linux-arm-kernel Sorry, I appear to be having problems with my emails where list emails are bouncing back to me. I'm working on correcting this now. Quoting Lorenzo Pieralisi (2014-02-28 01:50:22) > On Thu, Feb 27, 2014 at 11:57:58PM +0000, Sebastian Capella wrote: > > [...] > > > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c > > new file mode 100644 > > index 0000000..a41e0e3 > > --- /dev/null > > +++ b/arch/arm/kernel/hibernate.c ... > > +#include <linux/suspend.h> > > +#include <asm/tlbflush.h> > > +#include <asm/cacheflush.h> > > You can drop tlbflush.h and cacheflush.h, they do not seem to be needed. Done! Thanks! > > + > > +/* > > + * The framework loads the hibernation image into a linked list anchored > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > > + * destinations. > > + * > > + * To make this work if resume is triggered from initramfs, the > > + * pagetables need to be switched to allow writes to kernel mem. > > + */ > > Comment above needs updating. We are switching page tables to a set of > page tables that are certain to live at the same location in the older > kernel, that's the only reason, as we discussed. soft_restart will make > sure (again) to switch to 1:1 page tables so that we can call cpu_resume > with the MMU off. How does this look? The framework loads as much of the hibernation image to final physical pages as possible. Any pages that were in use, will need to be restored prior to the soft_restart. The pages to restore are maintained in the list anchored at restore_pblist. At this point, we can swap the pages to their final location. We must switch the mapping to 1:1 to ensure that when we overwrite the page table physical pages we're using a known physical location (idmap_pgd) with known contents. > > +/* > > + * Resume from the hibernation image. > > + * Due to the kernel heap / data restore, stack contents change underneath > > + * and that would make function calls impossible; switch to a temporary > > + * stack within the nosave region to avoid that problem. > > + */ > > +int swsusp_arch_resume(void) > > +{ > > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > > + call_with_stack(arch_restore_image, 0, > > + resume_stack + sizeof(resume_stack)); > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > compliant and might buy you trouble. > > Either you align the stack or you align the pointer you are passing. > > Please have a look at kernel/process.c I've added this for now, do you see any issues? -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; - resume_stack + sizeof(resume_stack)); + resume_stack + ARRAY_SIZE(resume_stack)); Thanks Lorenzo! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 20:15 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-28 20:15 UTC (permalink / raw) To: linux-arm-kernel Sorry, I appear to be having problems with my emails where list emails are bouncing back to me. I'm working on correcting this now. Quoting Lorenzo Pieralisi (2014-02-28 01:50:22) > On Thu, Feb 27, 2014 at 11:57:58PM +0000, Sebastian Capella wrote: > > [...] > > > diff --git a/arch/arm/kernel/hibernate.c b/arch/arm/kernel/hibernate.c > > new file mode 100644 > > index 0000000..a41e0e3 > > --- /dev/null > > +++ b/arch/arm/kernel/hibernate.c ... > > +#include <linux/suspend.h> > > +#include <asm/tlbflush.h> > > +#include <asm/cacheflush.h> > > You can drop tlbflush.h and cacheflush.h, they do not seem to be needed. Done! Thanks! > > + > > +/* > > + * The framework loads the hibernation image into a linked list anchored > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > > + * destinations. > > + * > > + * To make this work if resume is triggered from initramfs, the > > + * pagetables need to be switched to allow writes to kernel mem. > > + */ > > Comment above needs updating. We are switching page tables to a set of > page tables that are certain to live at the same location in the older > kernel, that's the only reason, as we discussed. soft_restart will make > sure (again) to switch to 1:1 page tables so that we can call cpu_resume > with the MMU off. How does this look? The framework loads as much of the hibernation image to final physical pages as possible. Any pages that were in use, will need to be restored prior to the soft_restart. The pages to restore are maintained in the list anchored at restore_pblist. At this point, we can swap the pages to their final location. We must switch the mapping to 1:1 to ensure that when we overwrite the page table physical pages we're using a known physical location (idmap_pgd) with known contents. > > +/* > > + * Resume from the hibernation image. > > + * Due to the kernel heap / data restore, stack contents change underneath > > + * and that would make function calls impossible; switch to a temporary > > + * stack within the nosave region to avoid that problem. > > + */ > > +int swsusp_arch_resume(void) > > +{ > > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > > + call_with_stack(arch_restore_image, 0, > > + resume_stack + sizeof(resume_stack)); > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > compliant and might buy you trouble. > > Either you align the stack or you align the pointer you are passing. > > Please have a look at kernel/process.c I've added this for now, do you see any issues? -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; - resume_stack + sizeof(resume_stack)); + resume_stack + ARRAY_SIZE(resume_stack)); Thanks Lorenzo! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-28 20:15 ` Sebastian Capella (?) @ 2014-02-28 22:49 ` Lorenzo Pieralisi -1 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-02-28 22:49 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: [...] > > > + > > > +/* > > > + * The framework loads the hibernation image into a linked list anchored > > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > > > + * destinations. > > > + * > > > + * To make this work if resume is triggered from initramfs, the > > > + * pagetables need to be switched to allow writes to kernel mem. > > > + */ > > > > Comment above needs updating. We are switching page tables to a set of > > page tables that are certain to live at the same location in the older > > kernel, that's the only reason, as we discussed. soft_restart will make > > sure (again) to switch to 1:1 page tables so that we can call cpu_resume > > with the MMU off. > > How does this look? > > The framework loads as much of the hibernation image to final physical > pages as possible. Any pages that were in use, will need to be restored > prior to the soft_restart. The pages to restore are maintained in > the list anchored at restore_pblist. At this point, we can swap the > pages to their final location. We must switch the mapping to 1:1 to > ensure that when we overwrite the page table physical pages we're using > a known physical location (idmap_pgd) with known contents. It is ok, a tad too verbose. All I care about is a comment describing what's really needed, the existing one was confusing and wrong. > > > +/* > > > + * Resume from the hibernation image. > > > + * Due to the kernel heap / data restore, stack contents change underneath > > > + * and that would make function calls impossible; switch to a temporary > > > + * stack within the nosave region to avoid that problem. > > > + */ > > > +int swsusp_arch_resume(void) > > > +{ > > > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > > > + call_with_stack(arch_restore_image, 0, > > > + resume_stack + sizeof(resume_stack)); > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > compliant and might buy you trouble. > > > > Either you align the stack or you align the pointer you are passing. > > > > Please have a look at kernel/process.c > > I've added this for now, do you see any issues? > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > - resume_stack + sizeof(resume_stack)); > + resume_stack + ARRAY_SIZE(resume_stack)); I do not see why the stack depends on the PAGE_SIZE. I would be surprised if you need more than a few bytes (given that soft_restart switches stack again...), go through it with a debugger, it is easy to check the stack usage and allow for some extra buffer (but half a page is not needed). My main concern was alignment, and now that's fixed. Thanks ! Lorenzo ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 22:49 ` Lorenzo Pieralisi 0 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-02-28 22:49 UTC (permalink / raw) To: linux-arm-kernel On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: [...] > > > + > > > +/* > > > + * The framework loads the hibernation image into a linked list anchored > > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > > > + * destinations. > > > + * > > > + * To make this work if resume is triggered from initramfs, the > > > + * pagetables need to be switched to allow writes to kernel mem. > > > + */ > > > > Comment above needs updating. We are switching page tables to a set of > > page tables that are certain to live at the same location in the older > > kernel, that's the only reason, as we discussed. soft_restart will make > > sure (again) to switch to 1:1 page tables so that we can call cpu_resume > > with the MMU off. > > How does this look? > > The framework loads as much of the hibernation image to final physical > pages as possible. Any pages that were in use, will need to be restored > prior to the soft_restart. The pages to restore are maintained in > the list anchored at restore_pblist. At this point, we can swap the > pages to their final location. We must switch the mapping to 1:1 to > ensure that when we overwrite the page table physical pages we're using > a known physical location (idmap_pgd) with known contents. It is ok, a tad too verbose. All I care about is a comment describing what's really needed, the existing one was confusing and wrong. > > > +/* > > > + * Resume from the hibernation image. > > > + * Due to the kernel heap / data restore, stack contents change underneath > > > + * and that would make function calls impossible; switch to a temporary > > > + * stack within the nosave region to avoid that problem. > > > + */ > > > +int swsusp_arch_resume(void) > > > +{ > > > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > > > + call_with_stack(arch_restore_image, 0, > > > + resume_stack + sizeof(resume_stack)); > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > compliant and might buy you trouble. > > > > Either you align the stack or you align the pointer you are passing. > > > > Please have a look at kernel/process.c > > I've added this for now, do you see any issues? > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > - resume_stack + sizeof(resume_stack)); > + resume_stack + ARRAY_SIZE(resume_stack)); I do not see why the stack depends on the PAGE_SIZE. I would be surprised if you need more than a few bytes (given that soft_restart switches stack again...), go through it with a debugger, it is easy to check the stack usage and allow for some extra buffer (but half a page is not needed). My main concern was alignment, and now that's fixed. Thanks ! Lorenzo ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 22:49 ` Lorenzo Pieralisi 0 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-02-28 22:49 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: [...] > > > + > > > +/* > > > + * The framework loads the hibernation image into a linked list anchored > > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > > > + * destinations. > > > + * > > > + * To make this work if resume is triggered from initramfs, the > > > + * pagetables need to be switched to allow writes to kernel mem. > > > + */ > > > > Comment above needs updating. We are switching page tables to a set of > > page tables that are certain to live at the same location in the older > > kernel, that's the only reason, as we discussed. soft_restart will make > > sure (again) to switch to 1:1 page tables so that we can call cpu_resume > > with the MMU off. > > How does this look? > > The framework loads as much of the hibernation image to final physical > pages as possible. Any pages that were in use, will need to be restored > prior to the soft_restart. The pages to restore are maintained in > the list anchored at restore_pblist. At this point, we can swap the > pages to their final location. We must switch the mapping to 1:1 to > ensure that when we overwrite the page table physical pages we're using > a known physical location (idmap_pgd) with known contents. It is ok, a tad too verbose. All I care about is a comment describing what's really needed, the existing one was confusing and wrong. > > > +/* > > > + * Resume from the hibernation image. > > > + * Due to the kernel heap / data restore, stack contents change underneath > > > + * and that would make function calls impossible; switch to a temporary > > > + * stack within the nosave region to avoid that problem. > > > + */ > > > +int swsusp_arch_resume(void) > > > +{ > > > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > > > + call_with_stack(arch_restore_image, 0, > > > + resume_stack + sizeof(resume_stack)); > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > compliant and might buy you trouble. > > > > Either you align the stack or you align the pointer you are passing. > > > > Please have a look at kernel/process.c > > I've added this for now, do you see any issues? > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > - resume_stack + sizeof(resume_stack)); > + resume_stack + ARRAY_SIZE(resume_stack)); I do not see why the stack depends on the PAGE_SIZE. I would be surprised if you need more than a few bytes (given that soft_restart switches stack again...), go through it with a debugger, it is easy to check the stack usage and allow for some extra buffer (but half a page is not needed). My main concern was alignment, and now that's fixed. Thanks ! Lorenzo ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-28 22:49 ` Lorenzo Pieralisi @ 2014-02-28 23:38 ` Sebastian Capella -1 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-28 23:38 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin, linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig, Russ Dill, Catalin Marinas, Santosh Shilimkar, Stephen Boyd, linux-arm-kernel Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > [...] > > > > > + > > > > +/* > > > > + * The framework loads the hibernation image into a linked list anchored > > > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > > > > + * destinations. > > > > + * > > > > + * To make this work if resume is triggered from initramfs, the > > > > + * pagetables need to be switched to allow writes to kernel mem. > > > > + */ > > > > > > Comment above needs updating. We are switching page tables to a set of > > > page tables that are certain to live at the same location in the older > > > kernel, that's the only reason, as we discussed. soft_restart will make > > > sure (again) to switch to 1:1 page tables so that we can call cpu_resume > > > with the MMU off. > > > > How does this look? > > > > The framework loads as much of the hibernation image to final physical > > pages as possible. Any pages that were in use, will need to be restored > > prior to the soft_restart. The pages to restore are maintained in > > the list anchored at restore_pblist. At this point, we can swap the > > pages to their final location. We must switch the mapping to 1:1 to > > ensure that when we overwrite the page table physical pages we're using > > a known physical location (idmap_pgd) with known contents. > > It is ok, a tad too verbose. All I care about is a comment describing > what's really needed, the existing one was confusing and wrong. Maybe more like: Restore physical pages that were in use while loading hibernation image. Use idmap_pgd so our page tables use the same physical address as the hibernation image. Will be overwriten with the same contents. > > > > +/* > > > > + * Resume from the hibernation image. > > > > + * Due to the kernel heap / data restore, stack contents change underneath > > > > + * and that would make function calls impossible; switch to a temporary > > > > + * stack within the nosave region to avoid that problem. > > > > + */ > > > > +int swsusp_arch_resume(void) > > > > +{ > > > > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > > > > + call_with_stack(arch_restore_image, 0, > > > > + resume_stack + sizeof(resume_stack)); > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > compliant and might buy you trouble. > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > Please have a look at kernel/process.c > > > > I've added this for now, do you see any issues? > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > - resume_stack + sizeof(resume_stack)); > > + resume_stack + ARRAY_SIZE(resume_stack)); > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > if you need more than a few bytes (given that soft_restart switches stack > again...), go through it with a debugger, it is easy to check the stack > usage and allow for some extra buffer (but half a page is not needed). I assuming this is becase the no-save region is one page anyway (we skip restoring the no-save region physical page). So maybe 1/2 is a way to leave some room for whatever else may need to be here, but in any case the 4k is used for nosave. I think you're right that it can be much less. I'll check into this... Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-02-28 23:38 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-02-28 23:38 UTC (permalink / raw) To: linux-arm-kernel Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > [...] > > > > > + > > > > +/* > > > > + * The framework loads the hibernation image into a linked list anchored > > > > + * at restore_pblist, for swsusp_arch_resume() to copy back to the proper > > > > + * destinations. > > > > + * > > > > + * To make this work if resume is triggered from initramfs, the > > > > + * pagetables need to be switched to allow writes to kernel mem. > > > > + */ > > > > > > Comment above needs updating. We are switching page tables to a set of > > > page tables that are certain to live at the same location in the older > > > kernel, that's the only reason, as we discussed. soft_restart will make > > > sure (again) to switch to 1:1 page tables so that we can call cpu_resume > > > with the MMU off. > > > > How does this look? > > > > The framework loads as much of the hibernation image to final physical > > pages as possible. Any pages that were in use, will need to be restored > > prior to the soft_restart. The pages to restore are maintained in > > the list anchored at restore_pblist. At this point, we can swap the > > pages to their final location. We must switch the mapping to 1:1 to > > ensure that when we overwrite the page table physical pages we're using > > a known physical location (idmap_pgd) with known contents. > > It is ok, a tad too verbose. All I care about is a comment describing > what's really needed, the existing one was confusing and wrong. Maybe more like: Restore physical pages that were in use while loading hibernation image. Use idmap_pgd so our page tables use the same physical address as the hibernation image. Will be overwriten with the same contents. > > > > +/* > > > > + * Resume from the hibernation image. > > > > + * Due to the kernel heap / data restore, stack contents change underneath > > > > + * and that would make function calls impossible; switch to a temporary > > > > + * stack within the nosave region to avoid that problem. > > > > + */ > > > > +int swsusp_arch_resume(void) > > > > +{ > > > > + extern void call_with_stack(void (*fn)(void *), void *arg, void *sp); > > > > + call_with_stack(arch_restore_image, 0, > > > > + resume_stack + sizeof(resume_stack)); > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > compliant and might buy you trouble. > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > Please have a look at kernel/process.c > > > > I've added this for now, do you see any issues? > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > - resume_stack + sizeof(resume_stack)); > > + resume_stack + ARRAY_SIZE(resume_stack)); > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > if you need more than a few bytes (given that soft_restart switches stack > again...), go through it with a debugger, it is easy to check the stack > usage and allow for some extra buffer (but half a page is not needed). I assuming this is becase the no-save region is one page anyway (we skip restoring the no-save region physical page). So maybe 1/2 is a way to leave some room for whatever else may need to be here, but in any case the 4k is used for nosave. I think you're right that it can be much less. I'll check into this... Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-02-28 23:38 ` Sebastian Capella (?) @ 2014-03-04 9:55 ` Sebastian Capella -1 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-04 9:55 UTC (permalink / raw) To: Sebastian Capella, Lorenzo Pieralisi Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd Quoting Sebastian Capella (2014-02-28 15:38:54) > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > compliant and might buy you trouble. > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > Please have a look at kernel/process.c > > > > > > I've added this for now, do you see any issues? > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > - resume_stack + sizeof(resume_stack)); > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > if you need more than a few bytes (given that soft_restart switches stack > > again...), go through it with a debugger, it is easy to check the stack > > usage and allow for some extra buffer (but half a page is not needed). > > I assuming this is becase the no-save region is one page anyway (we skip > restoring the no-save region physical page). So maybe 1/2 is a way to > leave some room for whatever else may need to be here, but in any case > the 4k is used for nosave. I think you're right that it can be much less. Hi Lorenzo, Are you ok with this just being half a page? Or do you want me to try to reduce the stack size? I am at Connect without my debugger, so in that case it would have to wait until next week. The change for alignment is in as discussed. Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-03-04 9:55 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-04 9:55 UTC (permalink / raw) To: linux-arm-kernel Quoting Sebastian Capella (2014-02-28 15:38:54) > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > compliant and might buy you trouble. > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > Please have a look at kernel/process.c > > > > > > I've added this for now, do you see any issues? > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > - resume_stack + sizeof(resume_stack)); > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > if you need more than a few bytes (given that soft_restart switches stack > > again...), go through it with a debugger, it is easy to check the stack > > usage and allow for some extra buffer (but half a page is not needed). > > I assuming this is becase the no-save region is one page anyway (we skip > restoring the no-save region physical page). So maybe 1/2 is a way to > leave some room for whatever else may need to be here, but in any case > the 4k is used for nosave. I think you're right that it can be much less. Hi Lorenzo, Are you ok with this just being half a page? Or do you want me to try to reduce the stack size? I am at Connect without my debugger, so in that case it would have to wait until next week. The change for alignment is in as discussed. Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-03-04 9:55 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-04 9:55 UTC (permalink / raw) To: Sebastian Capella, Lorenzo Pieralisi Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd Quoting Sebastian Capella (2014-02-28 15:38:54) > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > compliant and might buy you trouble. > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > Please have a look at kernel/process.c > > > > > > I've added this for now, do you see any issues? > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > - resume_stack + sizeof(resume_stack)); > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > if you need more than a few bytes (given that soft_restart switches stack > > again...), go through it with a debugger, it is easy to check the stack > > usage and allow for some extra buffer (but half a page is not needed). > > I assuming this is becase the no-save region is one page anyway (we skip > restoring the no-save region physical page). So maybe 1/2 is a way to > leave some room for whatever else may need to be here, but in any case > the 4k is used for nosave. I think you're right that it can be much less. Hi Lorenzo, Are you ok with this just being half a page? Or do you want me to try to reduce the stack size? I am at Connect without my debugger, so in that case it would have to wait until next week. The change for alignment is in as discussed. Thanks! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-03-04 9:55 ` Sebastian Capella (?) @ 2014-03-04 11:17 ` Lorenzo Pieralisi -1 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-03-04 11:17 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd On Tue, Mar 04, 2014 at 09:55:31AM +0000, Sebastian Capella wrote: > Quoting Sebastian Capella (2014-02-28 15:38:54) > > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > > compliant and might buy you trouble. > > > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > > > Please have a look at kernel/process.c > > > > > > > > I've added this for now, do you see any issues? > > > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > > - resume_stack + sizeof(resume_stack)); > > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > > if you need more than a few bytes (given that soft_restart switches stack > > > again...), go through it with a debugger, it is easy to check the stack > > > usage and allow for some extra buffer (but half a page is not needed). > > > > I assuming this is becase the no-save region is one page anyway (we skip > > restoring the no-save region physical page). So maybe 1/2 is a way to > > leave some room for whatever else may need to be here, but in any case > > the 4k is used for nosave. I think you're right that it can be much less. > > Hi Lorenzo, > > Are you ok with this just being half a page? Or do you want me to try > to reduce the stack size? I am at Connect without my debugger, so in > that case it would have to wait until next week. I am ok, either you leave that as it is (that multiple division looks horrible but it is just nitpicking on my side) or define it as an u8 array, stick __attribute__((aligned(8)) to the definition (and explain why) and be done with it. You can add my: Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-03-04 11:17 ` Lorenzo Pieralisi 0 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-03-04 11:17 UTC (permalink / raw) To: linux-arm-kernel On Tue, Mar 04, 2014 at 09:55:31AM +0000, Sebastian Capella wrote: > Quoting Sebastian Capella (2014-02-28 15:38:54) > > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > > compliant and might buy you trouble. > > > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > > > Please have a look at kernel/process.c > > > > > > > > I've added this for now, do you see any issues? > > > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > > - resume_stack + sizeof(resume_stack)); > > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > > if you need more than a few bytes (given that soft_restart switches stack > > > again...), go through it with a debugger, it is easy to check the stack > > > usage and allow for some extra buffer (but half a page is not needed). > > > > I assuming this is becase the no-save region is one page anyway (we skip > > restoring the no-save region physical page). So maybe 1/2 is a way to > > leave some room for whatever else may need to be here, but in any case > > the 4k is used for nosave. I think you're right that it can be much less. > > Hi Lorenzo, > > Are you ok with this just being half a page? Or do you want me to try > to reduce the stack size? I am at Connect without my debugger, so in > that case it would have to wait until next week. I am ok, either you leave that as it is (that multiple division looks horrible but it is just nitpicking on my side) or define it as an u8 array, stick __attribute__((aligned(8)) to the definition (and explain why) and be done with it. You can add my: Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-03-04 11:17 ` Lorenzo Pieralisi 0 siblings, 0 replies; 38+ messages in thread From: Lorenzo Pieralisi @ 2014-03-04 11:17 UTC (permalink / raw) To: Sebastian Capella Cc: linux-kernel, linux-pm, linaro-kernel, linux-arm-kernel, Russ Dill, Rafael J. Wysocki, Russell King, Len Brown, Nicolas Pitre, Santosh Shilimkar, Will Deacon, Jonathan Austin, Catalin Marinas, Uwe Kleine-K?nig, Stephen Boyd On Tue, Mar 04, 2014 at 09:55:31AM +0000, Sebastian Capella wrote: > Quoting Sebastian Capella (2014-02-28 15:38:54) > > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > > compliant and might buy you trouble. > > > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > > > Please have a look at kernel/process.c > > > > > > > > I've added this for now, do you see any issues? > > > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > > - resume_stack + sizeof(resume_stack)); > > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > > if you need more than a few bytes (given that soft_restart switches stack > > > again...), go through it with a debugger, it is easy to check the stack > > > usage and allow for some extra buffer (but half a page is not needed). > > > > I assuming this is becase the no-save region is one page anyway (we skip > > restoring the no-save region physical page). So maybe 1/2 is a way to > > leave some room for whatever else may need to be here, but in any case > > the 4k is used for nosave. I think you're right that it can be much less. > > Hi Lorenzo, > > Are you ok with this just being half a page? Or do you want me to try > to reduce the stack size? I am at Connect without my debugger, so in > that case it would have to wait until next week. I am ok, either you leave that as it is (that multiple division looks horrible but it is just nitpicking on my side) or define it as an u8 array, stick __attribute__((aligned(8)) to the definition (and explain why) and be done with it. You can add my: Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk 2014-03-04 11:17 ` Lorenzo Pieralisi @ 2014-03-05 0:18 ` Sebastian Capella -1 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-05 0:18 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Len Brown, linaro-kernel, Russell King, Jonathan Austin, linux-pm, Will Deacon, Nicolas Pitre, Rafael J. Wysocki, linux-kernel, Uwe Kleine-K?nig, Russ Dill, Catalin Marinas, Santosh Shilimkar, Stephen Boyd, linux-arm-kernel Quoting Lorenzo Pieralisi (2014-03-04 03:17:46) > On Tue, Mar 04, 2014 at 09:55:31AM +0000, Sebastian Capella wrote: > > Quoting Sebastian Capella (2014-02-28 15:38:54) > > > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > > > compliant and might buy you trouble. > > > > > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > > > > > Please have a look at kernel/process.c > > > > > > > > > > I've added this for now, do you see any issues? > > > > > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > > > - resume_stack + sizeof(resume_stack)); > > > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > > > if you need more than a few bytes (given that soft_restart switches stack > > > > again...), go through it with a debugger, it is easy to check the stack > > > > usage and allow for some extra buffer (but half a page is not needed). > > > > > > I assuming this is becase the no-save region is one page anyway (we skip > > > restoring the no-save region physical page). So maybe 1/2 is a way to > > > leave some room for whatever else may need to be here, but in any case > > > the 4k is used for nosave. I think you're right that it can be much less. > > > > Hi Lorenzo, > > > > Are you ok with this just being half a page? Or do you want me to try > > to reduce the stack size? I am at Connect without my debugger, so in > > that case it would have to wait until next week. > > I am ok, either you leave that as it is (that multiple division looks > horrible but it is just nitpicking on my side) or define it as an u8 array, > stick __attribute__((aligned(8)) to the definition (and explain why) and be > done with it. > > You can add my: > > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Thanks Lorenzo! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v6 2/2] ARM hibernation / suspend-to-disk @ 2014-03-05 0:18 ` Sebastian Capella 0 siblings, 0 replies; 38+ messages in thread From: Sebastian Capella @ 2014-03-05 0:18 UTC (permalink / raw) To: linux-arm-kernel Quoting Lorenzo Pieralisi (2014-03-04 03:17:46) > On Tue, Mar 04, 2014 at 09:55:31AM +0000, Sebastian Capella wrote: > > Quoting Sebastian Capella (2014-02-28 15:38:54) > > > Quoting Lorenzo Pieralisi (2014-02-28 14:49:33) > > > > On Fri, Feb 28, 2014 at 08:15:57PM +0000, Sebastian Capella wrote: > > > > > > > > > > > > This does not guarantee your stack is 8-byte aligned, that's not AAPCS > > > > > > compliant and might buy you trouble. > > > > > > > > > > > > Either you align the stack or you align the pointer you are passing. > > > > > > > > > > > > Please have a look at kernel/process.c > > > > > > > > > > I've added this for now, do you see any issues? > > > > > > > > > > -static u8 resume_stack[PAGE_SIZE/2] __nosavedata; > > > > > +static u64 resume_stack[PAGE_SIZE/2/sizeof(u64)] __nosavedata; > > > > > - resume_stack + sizeof(resume_stack)); > > > > > + resume_stack + ARRAY_SIZE(resume_stack)); > > > > > > > > I do not see why the stack depends on the PAGE_SIZE. I would be surprised > > > > if you need more than a few bytes (given that soft_restart switches stack > > > > again...), go through it with a debugger, it is easy to check the stack > > > > usage and allow for some extra buffer (but half a page is not needed). > > > > > > I assuming this is becase the no-save region is one page anyway (we skip > > > restoring the no-save region physical page). So maybe 1/2 is a way to > > > leave some room for whatever else may need to be here, but in any case > > > the 4k is used for nosave. I think you're right that it can be much less. > > > > Hi Lorenzo, > > > > Are you ok with this just being half a page? Or do you want me to try > > to reduce the stack size? I am at Connect without my debugger, so in > > that case it would have to wait until next week. > > I am ok, either you leave that as it is (that multiple division looks > horrible but it is just nitpicking on my side) or define it as an u8 array, > stick __attribute__((aligned(8)) to the definition (and explain why) and be > done with it. > > You can add my: > > Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Thanks Lorenzo! Sebastian ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2014-06-02 16:57 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-27 23:57 [PATCH v6 0/2] hibernation support on ARM Sebastian Capella 2014-02-27 23:57 ` Sebastian Capella 2014-02-27 23:57 ` [PATCH v6 1/2] ARM: avoid tracers in soft_restart Sebastian Capella 2014-02-27 23:57 ` Sebastian Capella 2014-02-27 23:57 ` [PATCH v6 2/2] ARM hibernation / suspend-to-disk Sebastian Capella 2014-02-27 23:57 ` Sebastian Capella 2014-02-28 0:09 ` Stephen Boyd 2014-02-28 0:09 ` Stephen Boyd 2014-02-28 1:47 ` Russ Dill 2014-02-28 1:47 ` Russ Dill 2014-02-28 1:47 ` Russ Dill 2014-02-28 2:19 ` Stephen Boyd 2014-02-28 2:19 ` Stephen Boyd 2014-02-28 10:20 ` Russell King - ARM Linux 2014-02-28 10:20 ` Russell King - ARM Linux [not found] ` <20140228181731.29118.41809@capellas-linux> 2014-03-05 2:28 ` Sebastian Capella 2014-03-05 2:28 ` Sebastian Capella 2014-03-05 2:28 ` Sebastian Capella 2014-06-02 16:57 ` Sebastian Capella 2014-06-02 16:57 ` Sebastian Capella 2014-02-28 9:50 ` Lorenzo Pieralisi 2014-02-28 9:50 ` Lorenzo Pieralisi 2014-02-28 9:50 ` Lorenzo Pieralisi 2014-02-28 20:15 ` Sebastian Capella 2014-02-28 20:15 ` Sebastian Capella 2014-02-28 22:49 ` Lorenzo Pieralisi 2014-02-28 22:49 ` Lorenzo Pieralisi 2014-02-28 22:49 ` Lorenzo Pieralisi 2014-02-28 23:38 ` Sebastian Capella 2014-02-28 23:38 ` Sebastian Capella 2014-03-04 9:55 ` Sebastian Capella 2014-03-04 9:55 ` Sebastian Capella 2014-03-04 9:55 ` Sebastian Capella 2014-03-04 11:17 ` Lorenzo Pieralisi 2014-03-04 11:17 ` Lorenzo Pieralisi 2014-03-04 11:17 ` Lorenzo Pieralisi 2014-03-05 0:18 ` Sebastian Capella 2014-03-05 0:18 ` Sebastian Capella
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.