Re: [PATCH v2 3/3] RISC-V: Add arch functions to support hibernation/suspend-to-disk

[Andrew Jones]

On Tue, Jan 10, 2023 at 08:37:01AM +0000, JeeHeng Sia wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> > Sent: Tuesday, 10 January, 2023 3:36 AM
> > To: JeeHeng Sia <jeeheng.sia@xxxxxxxxxxxxxxxx>
> > Cc: paul.walmsley@xxxxxxxxxx; palmer@xxxxxxxxxxx;
> > aou@xxxxxxxxxxxxxxxxx; linux-riscv@xxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; Leyfoon Tan <leyfoon.tan@xxxxxxxxxxxxxxxx>;
> > Mason Huo <mason.huo@xxxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH v2 3/3] RISC-V: Add arch functions to support
> > hibernation/suspend-to-disk
> > 
> > On Mon, Jan 09, 2023 at 02:24:07PM +0800, Sia Jee Heng wrote:
> > > Low level Arch functions were created to support hibernation.
> > > swsusp_arch_suspend() relies code from __cpu_suspend_enter() to write
> > > cpu state onto the stack, then calling swsusp_save() to save the memory
> > > image.
> > >
> > > arch_hibernation_header_restore() and arch_hibernation_header_save()
> > > functions are implemented to prevent kernel crash when resume,
> > > the kernel built version is saved into the hibernation image header
> > > to making sure only the same kernel is restore when resume.
> > 
> > Why does it crash with the generic version check?
> The kernel could have different device driver enabled and the device drivers would find itself running in a different address space if restore with different kernel version.

That part I understood, but I was wondering why the generic check which
compares kernel version and more wasn't sufficient. I answered my own
question though. Extra data needs to be stored to the header (hartid,
satp, etc.), so we need an arch-specific hibernation header. However we
*still* need the version for the version check, so some of the generic
check is reproduced in the arch-specific header. Please update the commit
message with this more detailed explanation.

...
> > > +extern int in_suspend;
> > 
> > This declaration could be in arch/riscv/kernel/hibernate.c
> Can't declare it to the .c file because checkpatch will report error if we do so.

Error or warning? It makes more sense to me to be where it needs to be,
but it doesn't matter too much either way.

...
> > > +	/* The below code will restore the hibernated image. */
> > > +	ld	a1, HIBERN_PBE_ADDR(s4)
> > > +	ld	a0, HIBERN_PBE_ORIG(s4)
> > > +
> > > +	lui     a4, 0x1
> > > +	add     a4, a4, a0
> > > +copy:	ld      a5, 0(a1)
> > 
> > copy label should get its own line and how about changing it,
> ok
> > loop, and done to local symbol names, e.g. .Lcopy?
> I think better to use two local symbol names, i.e .Lcopy and .Ldone

Two? You currently have three labels which all serve purposes.

BTW, maybe unrolling the copy loop a bit as arm64 does with its copy_page
macro would be a good idea.

...
> > > +		sleep_cpu = -EINVAL;
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +#ifdef CONFIG_SMP
> > 
> > The #ifdef shouldn't be necessary.
> We need the #ifdef CONFIG_SMP is because the bringup_hibernate_cpu() is defined under the guardian of ifdef CONFIG_SMP.
> One will face compile error if the CONFIG_SMP was disabled for a single cpu testing.

Ah, you're right. It's interesting that arm64 doesn't appear to guard that
call.

Thanks,
drew