Re: [PATCH] pstore: add a KHO backend

[Pasha Tatashin <pasha.tatashin@soleen.com>]

On 05-20 19:43, Michal Clapinski wrote:
> Up to this point to preserve late shutdown logs in memory, users had to
> predefine a memory region using ramoops. This commit changes this by
> preserving a buffer using kexec-handover.
> 
> For now it supports preserving only 1 dmesg buffer.

"only" sounds like a limitation, but I do not think it is. If userspace 
is configured properly after every boot, it can back up the dmesg from 
the previous kernel instance if needed.

> It gets replaced with the new buffer on every kexec, so the user has to
> copy the file out of pstore after every kexec.
> There is no erase() support.
> 
> Signed-off-by: Michal Clapinski <mclapinski@google.com>
> ---
>  fs/pstore/Kconfig      |   9 ++
>  fs/pstore/Makefile     |   2 +
>  fs/pstore/pstore_kho.c | 267 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 278 insertions(+)
>  create mode 100644 fs/pstore/pstore_kho.c
> 
> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> index 3acc38600cd1..70853799e211 100644
> --- a/fs/pstore/Kconfig
> +++ b/fs/pstore/Kconfig
> @@ -81,6 +81,15 @@ config PSTORE_RAM
>  
>  	  For more information, see Documentation/admin-guide/ramoops.rst.
>  
> +config PSTORE_KHO
> +	tristate "Preserve logs over kexec"
> +	depends on PSTORE
> +	depends on KEXEC_HANDOVER
> +	help
> +	  A pstore backend for preserving dmesg over KHO (kexec handover).
> +
> +	  It supports preservation of only 1 dmesg file.
> +
>  config PSTORE_ZONE
>  	tristate
>  	depends on PSTORE
> diff --git a/fs/pstore/Makefile b/fs/pstore/Makefile
> index c270467aeece..518cd408bf8e 100644
> --- a/fs/pstore/Makefile
> +++ b/fs/pstore/Makefile
> @@ -13,6 +13,8 @@ pstore-$(CONFIG_PSTORE_PMSG)	+= pmsg.o
>  ramoops-objs += ram.o ram_core.o
>  obj-$(CONFIG_PSTORE_RAM)	+= ramoops.o
>  
> +obj-$(CONFIG_PSTORE_KHO)	+= pstore_kho.o
> +
>  pstore_zone-objs += zone.o
>  obj-$(CONFIG_PSTORE_ZONE)	+= pstore_zone.o
>  
> diff --git a/fs/pstore/pstore_kho.c b/fs/pstore/pstore_kho.c
> new file mode 100644
> index 000000000000..3bc679273c8d
> --- /dev/null
> +++ b/fs/pstore/pstore_kho.c
> @@ -0,0 +1,267 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * KHO (Kexec Handover) backend for pstore
> + */

We need a proper Doc comment here explaining the benefits of using a 
KHO-based pstore. It has some very significant advantages, such as 
removing the need to manage a hardcoded memmap or rely on firmware 
support. Also, you can mention that pstore also provides the ability to 
preserve every single dmesg even after filesystem unmounts, without 
being limited by the console baud rate.

> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kexec_handover.h>
> +#include <linux/libfdt.h>
> +#include <linux/module.h>
> +#include <linux/pstore.h>
> +#include <linux/slab.h>
> +#include <linux/unaligned.h>
> +
> +#define KHO_PSTORE_FDT_NAME	"pstore-kho"
> +#define KHO_PSTORE_COMPAT	"pstore-kho-v1"
> +#define KHO_PSTORE_DATA		"pstore_kho_record"
> +
> +static const size_t record_max_size = 1 << CONFIG_LOG_BUF_SHIFT;

The above should be added to include/linux/kho/abi/pstore.h

Also, there is no need to add an FDT subtree; it is much cleaner to add 
a sub-blob and use a C-struct directly. Let's add something like struct 
pstore_ser to the pstore ABI header to act as an anchor for everything 
this code references.

Additionally, what happens if CONFIG_LOG_BUF_SHIFT changes between two 
kexec'd kernels? Will this still work? If not, then the buffer size 
needs to be explicitly defined as part of the ABI.

[...]

Pleae review 
https://sashiko.dev/#/patchset/20260520174332.921186-1-mclapinski%40google.com 
review comments.

Pasha