Date:   Mon, 20 Mar 2023 12:56:18 +0000
From:   Conor Dooley <conor.dooley@microchip.com>
To:     Sia Jee Heng <jeeheng.sia@starfivetech.com>
CC:     <paul.walmsley@sifive.com>, <palmer@dabbelt.com>,
        <aou@eecs.berkeley.edu>, <linux-riscv@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <leyfoon.tan@starfivetech.com>,
        <mason.huo@starfivetech.com>
Subject: Re: [PATCH v6 4/4] RISC-V: Add arch functions to support
 hibernation/suspend-to-disk
Message-ID: <ad8fcf05-0e3f-4350-b9ad-533cee984580@spud>
References: <20230314050316.31701-1-jeeheng.sia@starfivetech.com>
 <20230314050316.31701-5-jeeheng.sia@starfivetech.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="92iwmNY3MZHZxEZB"
Content-Disposition: inline
In-Reply-To: <20230314050316.31701-5-jeeheng.sia@starfivetech.com>
Precedence: bulk

--92iwmNY3MZHZxEZB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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.
>=20
> 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.
>=20
> To enable hibernation/suspend to disk into RISCV, the below config
> need to be enabled:
> - CONFIG_ARCH_HIBERNATION_HEADER
> - CONFIG_ARCH_HIBERNATION_POSSIBLE
>=20
> 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"
> =20
>  source "kernel/power/Kconfig"
> =20
> +config ARCH_HIBERNATION_POSSIBLE
> +	def_bool y
> +
> +config ARCH_HIBERNATION_HEADER
> +	def_bool HIBERNATION
> +
>  endmenu # "Power management options"
> =20
>  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 buil=
d 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.

--92iwmNY3MZHZxEZB
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZBhX8gAKCRB4tDGHoIJi
0l8vAP9Ee+Fz+1lL0amzFYBI/Hu68qUOkYzhS1WKahRNKBYm7wD/VHK4GaBsPUg4
KCiyt/fV6/UPkd/ICdIl2l/kmo6WUg0=
=Npkv
-----END PGP SIGNATURE-----

--92iwmNY3MZHZxEZB--