Re: [PATCH v6 2/2] ARM hibernation / suspend-to-disk

[Sebastian Capella <sebastian.capella@linaro.org>]

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