On 06-05 14:10, 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.
>
> pstore_kho supports preserving only 1 dmesg buffer.
> 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 <[email protected]>
> ---
> v2:
> - Added a comment explaining the benefits of pstore_kho.
> - Created include/linux/kho/abi/pstore.h.
> - Got rid of the KHO subtree.
> - Made sure never to free incoming kho data.
> This way the module can be safely reloaded.
> - Sashiko complained that I trust the data coming from the old kernel.
> I ignored it. LMK if I shouldn't trust the old kernel.
> ---
> fs/pstore/Kconfig | 10 ++
> fs/pstore/Makefile | 2 +
> fs/pstore/pstore_kho.c | 230 +++++++++++++++++++++++++++++++++
> include/linux/kho/abi/pstore.h | 27 ++++
> 4 files changed, 269 insertions(+)
> create mode 100644 fs/pstore/pstore_kho.c
> create mode 100644 include/linux/kho/abi/pstore.h
>
> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> index 3acc38600cd1..455790fec955 100644
> --- a/fs/pstore/Kconfig
> +++ b/fs/pstore/Kconfig
> @@ -81,6 +81,16 @@ 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 does not require any additional cmdline params to work.
> +
> + 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..6d4187d91642
> --- /dev/null
> +++ b/fs/pstore/pstore_kho.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KHO (Kexec Handover) backend for pstore.
No need to spelll it out.
> + *
> + * KHO-based pstore provides a mechanism to hand over pstore data
> (specifically
> + * dmesg logs) from one kernel to another across a kexec reboot using the
> + * Kexec Handover (KHO) framework.
> + *
> + * Key advantages of KHO-based pstore include:
> + * - No hardcoded memmap: Unlike ramoops, it does not require reserving a
> static
> + * memory region in the bootloader or device tree. Memory is allocated
> + * dynamically and handed over to the next kernel.
> + * - Firmware independence: It does not rely on platform firmware support
> (like
> + * ACPI ERST or UEFI variable storage) to preserve logs across reboots.
> + * - High throughput: It avoids the performance bottlenecks of serial
> consoles,
> + * not being limited by console baud rates.
> + * - Complete log preservation: It preserves all dmesg logs, including those
> + * generated late in the reboot cycle after filesystems have been
> unmounted,
> + * up to the point of the kexec jump.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kho/abi/pstore.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kexec_handover.h>
> +#include <linux/module.h>
> +#include <linux/pstore.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +/*
> + * The in and out buffers are separate and they need not be the same size.
> + * Therefore, this is not part of ABI.
> + */
> +#define RECORD_MAX_SIZE (1 << CONFIG_LOG_BUF_SHIFT)
This does not sound right. I think, we should enforce size through
ABI. Or make it flexible so
> +
> +struct pstore_kho_context {
> + struct pstore_info pstore;
> + bool read_done;
> +};
> +
> +static struct pstore_ser *kho_ser_in;
> +static struct pstore_ser *kho_ser_out;
> +
> +static int pstore_kho_open(struct pstore_info *psi)
> +{
> + struct pstore_kho_context *cxt = psi->data;
> +
> + cxt->read_done = false;
> + return 0;
> +}
> +
> +static ssize_t pstore_kho_read(struct pstore_record *record)
> +{
> + struct pstore_kho_context *cxt = record->psi->data;
> + struct pstore_kho_record *kho_data_in;
> +
> + if (cxt->read_done || !kho_ser_in)
> + return 0;
> +
> + cxt->read_done = true;
> + kho_data_in = &kho_ser_in->record;
> +
> + record->buf = kmemdup(kho_data_in->buf, kho_data_in->size, GFP_KERNEL);
> + if (!record->buf)
> + return -ENOMEM;
> +
> + record->type = PSTORE_TYPE_DMESG;
> + record->id = 0;
> + record->size = kho_data_in->size;
> + record->time.tv_sec = kho_data_in->time_sec;
> + record->time.tv_nsec = kho_data_in->time_nsec;
> + record->count = kho_data_in->count;
> + record->reason = kho_data_in->reason;
> + record->part = kho_data_in->part;
> + record->compressed = kho_data_in->compressed;
> +
> + return record->size;
> +}
> +
> +static int pstore_kho_write(struct pstore_record *record)
> +{
> + struct pstore_kho_record *kho_data_out = &kho_ser_out->record;
> +
> + if (record->type != PSTORE_TYPE_DMESG)
> + return -EINVAL;
> +
> + if (kho_data_out->size != 0) {
> + pr_err("pstore kho already contains a record\n");
> + return -ENOSPC;
> + }
> +
> + if (record->size > RECORD_MAX_SIZE) {
> + pr_err("dmesg record too big, record size: %lu, available
> space: %d\n",
> + record->size, RECORD_MAX_SIZE);
> + return -ENOSPC;
> + }
> +
> + memcpy(kho_data_out->buf, record->buf, record->size);
> + kho_data_out->size = record->size;
> + kho_data_out->time_sec = record->time.tv_sec;
> + kho_data_out->time_nsec = record->time.tv_nsec;
> + kho_data_out->count = record->count;
> + kho_data_out->reason = record->reason;
> + kho_data_out->part = record->part;
> + kho_data_out->compressed = record->compressed;
> +
> + return 0;
> +}
> +
> +static struct pstore_kho_context pstore_kho_cxt = {
> + .pstore = {
> + .owner = THIS_MODULE,
> + .name = "kho",
> + .bufsize = RECORD_MAX_SIZE,
Let's make this ABI for simplicity.
> + .flags = PSTORE_FLAGS_DMESG,
> + .max_reason = KMSG_DUMP_SHUTDOWN,
In all other places, the default is KMSG_DUMP_OOPS, and it is increased
or decreased based on user-provided parameters. Should we not do the
same here?
> + .open = pstore_kho_open,
> + .read = pstore_kho_read,
> + .write = pstore_kho_write,
> + },
> +};
> +
> +static void __init kho_setup_incoming(void)
> +{
> + phys_addr_t kho_ser_phys;
> + int err;
> +
> + err = kho_retrieve_subtree(KHO_PSTORE_FDT_NAME, &kho_ser_phys);
> + if (err) {
> + if (err != -ENOENT)
> + pr_err("failed to retrieve KHO data %s: %d\n",
> + KHO_PSTORE_FDT_NAME, err);
> + return;
> + }
> +
> + kho_ser_in = phys_to_virt(kho_ser_phys);
> +
> + if (kho_ser_in->version != KHO_PSTORE_VERSION) {
> + pr_err("unsupported KHO pstore version: %d\n",
> kho_ser_in->version);
> + kho_ser_in = NULL;
> + return;
> + }
> +
> + pr_info("successfully restored preserved data\n");
> +}
> +
> +static int __init kho_setup_outgoing(void)
> +{
> + int err;
> + size_t total_size = sizeof(struct pstore_ser) + RECORD_MAX_SIZE;
Please use the reverse-christmas-tree order for variable declarations.
> +
> + kho_ser_out = kho_alloc_preserve(total_size);
RECORD_MAX_SIZE is not part of the ABI, yet it is statically configured
during kho_setup_outgoing(). We need to either make it dynamic, setting
up preserved pages as we go based on the amount of used memory (i.e use
something like KHO linked-blocks), or make this part of the ABI.
> + if (IS_ERR(kho_ser_out)) {
> + pr_err("failed to allocate pstore kho ser anchor\n");
> + return PTR_ERR(kho_ser_out);
> + }
> + memset(kho_ser_out, 0, total_size);
> + kho_ser_out->version = KHO_PSTORE_VERSION;
> +
> + err = kho_add_subtree(KHO_PSTORE_FDT_NAME, kho_ser_out);
> + if (err) {
> + pr_err("failed to add KHO data\n");
> + goto err_free_ser;
> + }
> +
> + return 0;
> +
> +err_free_ser:
> + kho_unpreserve_free(kho_ser_out);
> + return err;
> +}
> +
> +static int __init pstore_kho_init(void)
> +{
> + int err;
> + struct pstore_kho_context *cxt = &pstore_kho_cxt;
RCT order please.
> +
> + if (!kho_is_enabled()) {
> + pr_info("KHO is disabled, pstore_kho cannot start\n");
> + return -ENODEV;
> + }
> +
> + kho_setup_incoming();
> + err = kho_setup_outgoing();
> + if (err) {
> + pr_err("failed to setup outgoing KHO\n");
> + return err;
Although the outgoing failed, can we still retrieve incoming messages?
> + }
> +
> + cxt->pstore.data = cxt;
> + cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
> + if (!cxt->pstore.buf) {
> + err = -ENOMEM;
> + goto err_free_outgoing;
> + }
> +
> + err = pstore_register(&cxt->pstore);
> + if (err) {
> + pr_err("failed to register with pstore\n");
> + goto err_free_pstore_buf;
> + }
> +
> + return 0;
> +
> +err_free_pstore_buf:
> + kfree(cxt->pstore.buf);
> +
> +err_free_outgoing:
> + kho_remove_subtree(kho_ser_out);
> + kho_unpreserve_free(kho_ser_out);
> +
> + return err;
> +}
> +module_init(pstore_kho_init);
> +
> +static void __exit pstore_kho_exit(void)
> +{
> + pstore_unregister(&pstore_kho_cxt.pstore);
> + kfree(pstore_kho_cxt.pstore.buf);
> +
> + kho_remove_subtree(kho_ser_out);
> + kho_unpreserve_free(kho_ser_out);
> +}
> +module_exit(pstore_kho_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Pstore backend for dmesg preservation over kexec");
> diff --git a/include/linux/kho/abi/pstore.h b/include/linux/kho/abi/pstore.h
> new file mode 100644
> index 000000000000..743ec64d67fc
> --- /dev/null
> +++ b/include/linux/kho/abi/pstore.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _LINUX_KHO_ABI_PSTORE_H
> +#define _LINUX_KHO_ABI_PSTORE_H
> +
> +#include <linux/types.h>
Please use the header comment in other ABI files as a template for what
should be stated here. Please also include it in the documentation,
consistent with all other ABI headers.
> +
> +#define KHO_PSTORE_FDT_NAME "pstore-kho"
> +#define KHO_PSTORE_VERSION 1
> +
> +struct pstore_kho_record {
I would prefer: pstore_record_ser
> + s64 size;
> + s64 time_sec;
> + u32 time_nsec;
> + s32 count;
> + u32 reason;
> + u32 part;
> + u32 compressed;
> + char buf[];
> +};
> +
> +struct pstore_ser {
> + u32 version;
> + struct pstore_kho_record record;
> +};
> +
> +#endif /* _LINUX_KHO_ABI_PSTORE_H */
> --
> 2.54.0.1032.g2f8565e1d1-goog
>
