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

[Conor Dooley <conor.dooley@microchip.com>]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

On Tue, Mar 14, 2023 at 01:03:16PM +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 specific hibernation header is implemented and is utilized by the
> arch_hibernation_header_restore() and arch_hibernation_header_save()
> functions. The arch specific hibernation header consists of satp, hartid,
> and the cpu_resume address. The kernel built version is also need to be
> saved into the hibernation image header to making sure only the same
> kernel is restore when resume.
> 
> swsusp_arch_resume() creates a temporary page table that covering only
> the linear map. It copies the restore code to a 'safe' page, then start
> to restore the memory image. Once completed, it restores the original
> kernel's page table. It then calls into __hibernate_cpu_resume()
> to restore the CPU context. Finally, it follows the normal hibernation
> path back to the hibernation core.
> 
> To enable hibernation/suspend to disk into RISCV, the below config
> need to be enabled:
> - CONFIG_ARCH_HIBERNATION_HEADER
> - CONFIG_ARCH_HIBERNATION_POSSIBLE
> 
> Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> Reviewed-by: Ley Foon Tan <leyfoon.tan@starfivetech.com>
> Reviewed-by: Mason Huo <mason.huo@starfivetech.com>

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index c5e42cc37604..473c2a1a6884 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -707,6 +707,12 @@  menu "Power management options"
>  
>  source "kernel/power/Kconfig"
>  
> +config ARCH_HIBERNATION_POSSIBLE
> +	def_bool y
> +
> +config ARCH_HIBERNATION_HEADER
> +	def_bool HIBERNATION
> +
>  endmenu # "Power management options"
>  
>  menu "CPU Power Management"

Since these options are not user-selectable, how come these are
def_bools, rather than just selecting them in the main `config RISCV`
section? Is there some sort of dependency that goes wrong when
structured that way?

> diff --git a/arch/riscv/kernel/hibernate.c b/arch/riscv/kernel/hibernate.c
> new file mode 100644
> index 000000000000..f11be60b0668
> --- /dev/null
> +++ b/arch/riscv/kernel/hibernate.c

> +/**
> + * struct arch_hibernate_hdr_invariants - container to store kernel build version.
> + * @uts_version: to save the build number and date so that the we do not resume with

nit: s/so that the we/so that we/

I noted previously that the page table stuff is beyond me, so modulo
that, I'm happy with how this looks now.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks for adding hibernation support!
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]