[RFC][PATCH 00/14] introduce kmemdump

[RFC][PATCH 00/14] introduce kmemdump

[Eugen Hristev]

kmemdump is a mechanism which allows the kernel to mark specific memory
areas for dumping or specific backend usage.
Once regions are marked, kmemdump keeps an internal list with the regions
and registers them in the backend.
Further, depending on the backend driver, these regions can be dumped using
firmware or different hardware block.
Regions being marked beforehand, when the system is up and running, there
is no need nor dependency on a panic handler, or a working kernel that can
dump the debug information.
The kmemdump approach works when pstore, kdump, or another mechanism do not.
Pstore relies on persistent storage, a dedicated RAM area or flash, which
has the disadvantage of having the memory reserved all the time, or another
specific non volatile memory. Some devices cannot keep the RAM contents on
reboot so ramoops does not work. Some devices do not allow kexec to run
another kernel to debug the crashed one.
For such devices, that have another mechanism to help debugging, like
firmware, kmemdump is a viable solution.

kmemdump can create a core image, similar with /proc/vmcore, with only
the registered regions included. This can be loaded into crash tool/gdb and
analyzed.
To have this working, specific information from the kernel is registered,
and this is done at kmemdump init time, no need for the kmemdump user to
do anything.

The implementation is based on the initial Pstore/directly mapped zones
published as an RFC here:
https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/

The back-end implementation for qcom_smem is based on the minidump
patch series and driver written by Mukesh Ojha, thanks:
https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/

I appreciate the feedback on this series, I know it is a longshot, and there
is a lot to improve, but I hope I am on the right track.

Thanks,
Eugen

PS. Here is how crash tool reports the dump:

     KERNEL: /home/eugen/linux-minidump/vmlinux  [TAINTED]
    DUMPFILE: /home/eugen/eee
        CPUS: 8 [OFFLINE: 7]
        DATE: Thu Jan  1 02:00:00 EET 1970
      UPTIME: 00:00:28
    NODENAME: qemuarm64
     RELEASE: 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty
     VERSION: #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
     MACHINE: aarch64  (unknown Mhz)
      MEMORY: 0
       PANIC: ""

crash> log
[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd4b2]
[    0.000000] Linux version 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty (eugen@eugen-station) (aarch64-none-linux-gnu-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614, GNU ld (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 2.42.0.20240614) #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
[    0.000000] KASLR enabled
[...]

Eugen Hristev (14):
  Documentation: add kmemdump
  kmemdump: introduce kmemdump
  kmemdump: introduce qcom-md backend driver
  soc: qcom: smem: add minidump device
  Documentation: kmemdump: add section for coreimage ELF
  kmemdump: add coreimage ELF layer
  printk: add kmsg_kmemdump_register
  kmemdump: coreimage: add kmsg registration
  genirq: add irq_kmemdump_register
  kmemdump: coreimage: add irq registration
  panic: add panic_kmemdump_register
  kmemdump: coreimage: add panic registration
  sched: add sched_kmemdump_register
  kmemdump: coreimage: add sched registration

 Documentation/debug/index.rst      |  17 ++
 Documentation/debug/kmemdump.rst   |  83 +++++
 drivers/Kconfig                    |   2 +
 drivers/Makefile                   |   2 +
 drivers/debug/Kconfig              |  39 +++
 drivers/debug/Makefile             |   5 +
 drivers/debug/kmemdump.c           | 197 ++++++++++++
 drivers/debug/kmemdump_coreimage.c | 293 ++++++++++++++++++
 drivers/debug/qcom_md.c            | 467 +++++++++++++++++++++++++++++
 drivers/soc/qcom/smem.c            |  10 +
 include/linux/irqnr.h              |   1 +
 include/linux/kmemdump.h           |  77 +++++
 include/linux/kmsg_dump.h          |   6 +
 include/linux/panic.h              |   1 +
 include/linux/sched.h              |   1 +
 kernel/irq/irqdesc.c               |   7 +
 kernel/panic.c                     |   8 +
 kernel/printk/printk.c             |  13 +
 kernel/sched/core.c                |   7 +
 19 files changed, 1236 insertions(+)
 create mode 100644 Documentation/debug/index.rst
 create mode 100644 Documentation/debug/kmemdump.rst
 create mode 100644 drivers/debug/Kconfig
 create mode 100644 drivers/debug/Makefile
 create mode 100644 drivers/debug/kmemdump.c
 create mode 100644 drivers/debug/kmemdump_coreimage.c
 create mode 100644 drivers/debug/qcom_md.c
 create mode 100644 include/linux/kmemdump.h

-- 
2.43.0

[RFC][PATCH 01/14] Documentation: add kmemdump

[Eugen Hristev]

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 Documentation/debug/index.rst    | 17 +++++++
 Documentation/debug/kmemdump.rst | 77 ++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100644 Documentation/debug/index.rst
 create mode 100644 Documentation/debug/kmemdump.rst

diff --git a/Documentation/debug/index.rst b/Documentation/debug/index.rst
new file mode 100644
index 000000000000..9a9365c62f02
--- /dev/null
+++ b/Documentation/debug/index.rst
@@ -0,0 +1,17 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===
+kmemdump
+===
+
+.. toctree::
+   :maxdepth: 1
+
+   kmemdump
+
+.. only::  subproject and html
+
+   Indices
+   =======
+
+   * :ref:`genindex`
diff --git a/Documentation/debug/kmemdump.rst b/Documentation/debug/kmemdump.rst
new file mode 100644
index 000000000000..dfee755a1be1
--- /dev/null
+++ b/Documentation/debug/kmemdump.rst
@@ -0,0 +1,77 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==========================
+kmemdump
+==========================
+
+This document provides information about the kmemdump feature.
+
+Overview
+========
+
+kmemdump is a mechanism that allows any driver or producer to register a
+chunk of memory into kmemdump, to be used at a later time for a specific
+purpose like debugging or memory dumping.
+
+kmemdump allows a backend to be connected, this backend interfaces a
+specific hardware that can debug or dump the memory registered into
+kmemdump.
+
+kmemdump Internals
+=============
+
+API
+----
+
+A memory region is being registered with a call to `kmemdump_register` which
+takes as parameters the name of the region, a pointer to the virtual memory
+start address and the size. If successful, this call returns an unique ID for
+the allocated zone.
+
+The region would be registered with a call to `kmemdump_unregister` which
+takes the id as a parameter.
+
+Backend
+-------
+
+Backend is represented by a `struct kmemdump_backend` which has to be filled
+in by the backend driver. Further, this struct is being passed to kmemdump
+with a `backend_register` call. `backend_unregister` will remove the backend
+from kmemdump.
+
+Once a backend is being registered, all previously registered regions are
+being sent to the backend for registration.
+
+When the backend is being removed, all regions are being first deregistered
+from the backend.
+
+kmemdump will request the backend to register a region with `register_region`
+call, and deregister a region with `unregister_region` call. These two
+functions are mandatory to be provided by a backend at registration time.
+
+Data structures
+---------------
+
+`struct kmemdump_backend` represents the kmemdump backend and it has two
+function pointers, one called `register_region` and the other
+`unregister_region`.
+
+Each region is being stored into a cyclic array of unique ids called
+`kmemdump_idr`.
+
+kmemdump Initialization
+------------------
+
+After system boots, kmemdump will be ready to accept region registration
+from producer drivers. However, the backend may not be registered yet.
+These regions are being added to the internal list and pending backend
+initialization. Once the backend is up and running, all the regions are
+registered into the backend. If, for example, the backend becomes unavailable
+and is removed, all the regions are unregistered and kmemdump waits for
+another backend to become available.
+
+backend functionality
+-----------------
+
+kmemdump backend can keep it's own list of regions and use the specific
+hardware available to dump the memory regions or use them for debugging.
-- 
2.43.0

[RFC][PATCH 02/14] kmemdump: introduce kmemdump

[Eugen Hristev]

Kmemdump mechanism allows any driver to mark a specific memory area
for later dumping purpose, depending on the functionality
of the attached backend. The backend would interface any hardware
mechanism that will allow dumping to complete regardless of the
state of the kernel (running, frozen, crashed, or any particular
state).

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 drivers/Kconfig          |   2 +
 drivers/Makefile         |   2 +
 drivers/debug/Kconfig    |  16 ++++
 drivers/debug/Makefile   |   3 +
 drivers/debug/kmemdump.c | 185 +++++++++++++++++++++++++++++++++++++++
 include/linux/kmemdump.h |  52 +++++++++++
 6 files changed, 260 insertions(+)
 create mode 100644 drivers/debug/Kconfig
 create mode 100644 drivers/debug/Makefile
 create mode 100644 drivers/debug/kmemdump.c
 create mode 100644 include/linux/kmemdump.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index 7bdad836fc62..ef56588f559e 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -245,4 +245,6 @@ source "drivers/cdx/Kconfig"
 
 source "drivers/dpll/Kconfig"
 
+source "drivers/debug/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 45d1c3e630f7..cf544a405007 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -195,3 +195,5 @@ obj-$(CONFIG_CDX_BUS)		+= cdx/
 obj-$(CONFIG_DPLL)		+= dpll/
 
 obj-$(CONFIG_S390)		+= s390/
+
+obj-y				+= debug/
diff --git a/drivers/debug/Kconfig b/drivers/debug/Kconfig
new file mode 100644
index 000000000000..22348608d187
--- /dev/null
+++ b/drivers/debug/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0
+menu "Generic Driver Debug Options"
+
+config DRIVER_KMEMDUMP
+	bool "Allow drivers to register memory for dumping"
+	help
+	  Kmemdump mechanism allows any driver to mark a specific memory area
+	  for later dumping purpose, depending on the functionality
+	  of the attached backend. The backend would interface any hardware
+	  mechanism that will allow dumping to complete regardless of the
+	  state of the kernel (running, frozen, crashed, or any particular
+	  state).
+
+	  Note that modules using this feature must be rebuilt if option
+	  changes.
+endmenu
diff --git a/drivers/debug/Makefile b/drivers/debug/Makefile
new file mode 100644
index 000000000000..cc14dea250e3
--- /dev/null
+++ b/drivers/debug/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_DRIVER_KMEMDUMP) += kmemdump.o
diff --git a/drivers/debug/kmemdump.c b/drivers/debug/kmemdump.c
new file mode 100644
index 000000000000..a685c0863e25
--- /dev/null
+++ b/drivers/debug/kmemdump.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/kmemdump.h>
+#include <linux/idr.h>
+
+#define MAX_ZONES 512
+
+static struct kmemdump_backend *backend;
+static DEFINE_IDR(kmemdump_idr);
+static DEFINE_MUTEX(kmemdump_lock);
+static LIST_HEAD(kmemdump_list);
+
+/**
+ * kmemdump_register() - Register region into kmemdump.
+ * @handle: string of maximum 8 chars that identifies this region
+ * @zone: pointer to the zone of memory
+ * @size: region size
+ *
+ * Return: On success, it returns an allocated unique id that can be used
+ *	at a later point to identify the region. On failure, it returns
+ *	negative error value.
+ */
+int kmemdump_register(char *handle, void *zone, size_t size)
+{
+	struct kmemdump_zone *z = kzalloc(sizeof(*z), GFP_KERNEL);
+	int id;
+
+	if (!z)
+		return -ENOMEM;
+
+	mutex_lock(&kmemdump_lock);
+
+	id = idr_alloc_cyclic(&kmemdump_idr, z, 0, MAX_ZONES, GFP_KERNEL);
+	if (id < 0) {
+		mutex_unlock(&kmemdump_lock);
+		return id;
+	}
+
+	if (!backend)
+		pr_debug("kmemdump backend not available yet, waiting...\n");
+
+	z->zone = zone;
+	z->size = size;
+	z->id = id;
+
+	if (handle)
+		strscpy(z->handle, handle, 8);
+
+	if (backend) {
+		int ret;
+
+		ret = backend->register_region(id, handle, zone, size);
+		if (ret) {
+			mutex_unlock(&kmemdump_lock);
+			return ret;
+		}
+		z->registered = true;
+	}
+
+	mutex_unlock(&kmemdump_lock);
+	return id;
+}
+EXPORT_SYMBOL_GPL(kmemdump_register);
+
+/**
+ * kmemdump_unregister() - Unregister region from kmemdump.
+ * @id: unique id that was returned when this region was successfully
+ *	registered initially.
+ *
+ * Return: None
+ */
+void kmemdump_unregister(int id)
+{
+	struct kmemdump_zone *z;
+
+	mutex_lock(&kmemdump_lock);
+
+	z = idr_find(&kmemdump_idr, id);
+	if (!z)
+		return;
+	if (z->registered && backend)
+		backend->unregister_region(z->id);
+
+	idr_remove(&kmemdump_idr, id);
+	kfree(z);
+
+	mutex_unlock(&kmemdump_lock);
+}
+EXPORT_SYMBOL_GPL(kmemdump_unregister);
+
+static int kmemdump_register_fn(int id, void *p, void *data)
+{
+	struct kmemdump_zone *z = p;
+	int ret;
+
+	if (z->registered)
+		return 0;
+
+	ret = backend->register_region(z->id, z->handle, z->zone, z->size);
+	if (ret)
+		return ret;
+	z->registered = true;
+
+	return 0;
+}
+
+/**
+ * kmemdump_register_backend() - Register a backend into kmemdump.
+ * Only one backend is supported at a time.
+ * @be: Pointer to a driver allocated backend. This backend must have
+ *	two callbacks for registering and deregistering a zone from the
+ *	backend.
+ *
+ * Return: On success, it returns 0, negative error value otherwise.
+ */
+int kmemdump_register_backend(struct kmemdump_backend *be)
+{
+	mutex_lock(&kmemdump_lock);
+
+	if (backend)
+		return -EALREADY;
+
+	if (!be || !be->register_region || !be->unregister_region)
+		return -EINVAL;
+
+	backend = be;
+	pr_info("kmemdump backend %s registered successfully.\n",
+		backend->name);
+
+	/* Try to call the backend for all previously requested zones */
+	idr_for_each(&kmemdump_idr, kmemdump_register_fn, NULL);
+
+	mutex_unlock(&kmemdump_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kmemdump_register_backend);
+
+static int kmemdump_unregister_fn(int id, void *p, void *data)
+{
+	int ret;
+	struct kmemdump_zone *z = p;
+
+	if (!z->registered)
+		return 0;
+
+	ret = backend->unregister_region(z->id);
+	if (ret)
+		return ret;
+	z->registered = false;
+
+	return 0;
+}
+
+/**
+ * kmemdump_register_backend() - Unregister the backend from kmemdump.
+ * Only one backend is supported at a time.
+ * Before deregistering, this will call the backend to unregister all the
+ * previously registered zones.
+ * @be: Pointer to a driver allocated backend. This backend must match
+ *	the initially registered backend.
+ *
+ * Return: None
+ */
+void kmemdump_unregister_backend(struct kmemdump_backend *be)
+{
+	mutex_lock(&kmemdump_lock);
+
+	if (backend != be) {
+		mutex_unlock(&kmemdump_lock);
+		return;
+	}
+
+	/* Try to call the backend for all previously requested zones */
+	idr_for_each(&kmemdump_idr, kmemdump_unregister_fn, NULL);
+
+	backend = NULL;
+	pr_info("kmemdump backend %s removed successfully.\n", be->name);
+
+	mutex_unlock(&kmemdump_lock);
+}
+EXPORT_SYMBOL_GPL(kmemdump_unregister_backend);
diff --git a/include/linux/kmemdump.h b/include/linux/kmemdump.h
new file mode 100644
index 000000000000..b55b15c295ac
--- /dev/null
+++ b/include/linux/kmemdump.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _KMEMDUMP_H
+#define _KMEMDUMP_H
+
+#define KMEMDUMP_ZONE_MAX_HANDLE 8
+/**
+ * struct kmemdump_zone - region mark zone information
+ * @id: unique id for this zone
+ * @zone: pointer to the memory area for this zone
+ * @size: size of the memory area of this zone
+ * @registered: bool indicating whether this zone is registered into the
+ *	backend or not.
+ * @handle: a string representing this region
+ */
+struct kmemdump_zone {
+	int id;
+	void *zone;
+	size_t size;
+	bool registered;
+	char handle[KMEMDUMP_ZONE_MAX_HANDLE];
+};
+
+#define KMEMDUMP_BACKEND_MAX_NAME 128
+/**
+ * struct kmemdump_backend - region mark backend information
+ * @name: the name of the backend
+ * @register_region: callback to register region in the backend
+ * @unregister_region: callback to unregister region in the backend
+ */
+struct kmemdump_backend {
+	char name[KMEMDUMP_BACKEND_MAX_NAME];
+	int (*register_region)(unsigned int id, char *, void *, size_t);
+	int (*unregister_region)(unsigned int id);
+};
+
+#ifdef CONFIG_DRIVER_KMEMDUMP
+int kmemdump_register(char *handle, void *zone, size_t size);
+void kmemdump_unregister(int id);
+#else
+static inline int kmemdump_register(char *handle, void *area, size_t size)
+{
+	return 0;
+}
+
+static inline void kmemdump_unregister(int id)
+{
+}
+#endif
+
+int kmemdump_register_backend(struct kmemdump_backend *backend);
+void kmemdump_unregister_backend(struct kmemdump_backend *backend);
+#endif
-- 
2.43.0

[RFC][PATCH 03/14] kmemdump: introduce qcom-md backend driver

[Eugen Hristev]

Qualcomm Minidump is a backend driver for kmemdump.
Regions are being registered into the shared memory on Qualcomm platforms
and into the table of contents.
Further, the firmware can read the table of contents and dump the memory
accordingly.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
Co-developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/debug/Kconfig   |  13 ++
 drivers/debug/Makefile  |   1 +
 drivers/debug/qcom_md.c | 467 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 481 insertions(+)
 create mode 100644 drivers/debug/qcom_md.c

diff --git a/drivers/debug/Kconfig b/drivers/debug/Kconfig
index 22348608d187..72a906487e02 100644
--- a/drivers/debug/Kconfig
+++ b/drivers/debug/Kconfig
@@ -13,4 +13,17 @@ config DRIVER_KMEMDUMP
 
 	  Note that modules using this feature must be rebuilt if option
 	  changes.
+
+config QCOM_MD_KMEMDUMP_BACKEND
+	tristate "Qualcomm Minidump kmemdump backend driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on DRIVER_KMEMDUMP
+	help
+	  Say y here to enable the Qualcomm Minidump kmemdump backend
+	  driver.
+	  With this backend, the registered regions are being linked
+	  into the minidump table of contents. Further on, the firmware
+	  will be able to read the table of contents and extract the
+	  memory regions on case-by-case basis.
+
 endmenu
diff --git a/drivers/debug/Makefile b/drivers/debug/Makefile
index cc14dea250e3..d8a9db29cd15 100644
--- a/drivers/debug/Makefile
+++ b/drivers/debug/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_DRIVER_KMEMDUMP) += kmemdump.o
+obj-$(CONFIG_QCOM_MD_KMEMDUMP_BACKEND) += qcom_md.o
diff --git a/drivers/debug/qcom_md.c b/drivers/debug/qcom_md.c
new file mode 100644
index 000000000000..1aff28e18230
--- /dev/null
+++ b/drivers/debug/qcom_md.c
@@ -0,0 +1,467 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/socinfo.h>
+#include <linux/kmemdump.h>
+
+/*
+ * In some of the Old Qualcomm devices, boot firmware statically allocates 300
+ * as total number of supported region (including all co-processors) in
+ * minidump table out of which linux was using 201. In future, this limitation
+ * from boot firmware might get removed by allocating the region dynamically.
+ * So, keep it compatible with older devices, we can keep the current limit for
+ * Linux to 201.
+ */
+#define MAX_NUM_ENTRIES	  201
+
+#define MAX_NUM_OF_SS           10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID	602
+#define MINIDUMP_REGION_VALID	   ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_SS_ENCR_DONE	   ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENABLED	   ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+#define MINIDUMP_SS_ENCR_NOTREQ	   (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
+
+#define MINIDUMP_APSS_DESC	   0
+
+/**
+ * struct minidump - Minidump driver data information
+ * @apss_data: APSS driver data
+ * @md_lock: Lock to protect access to APSS minidump table
+ */
+struct minidump {
+	struct device		*dev;
+	struct minidump_ss_data	*apss_data;
+	struct mutex		md_lock;
+};
+
+/**
+ * struct minidump_region - Minidump region
+ * @name		: Name of the region to be dumped
+ * @seq_num:		: Use to differentiate regions with same name.
+ * @valid		: This entry to be dumped (if set to 1)
+ * @address		: Physical address of region to be dumped
+ * @size		: Size of the region
+ */
+struct minidump_region {
+	char	name[MAX_REGION_NAME_LENGTH];
+	__le32	seq_num;
+	__le32	valid;
+	__le64	address;
+	__le64	size;
+};
+
+/**
+ * struct minidump_subsystem - Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : if set to 1, this region would be copied during coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @region_count : Number of regions added in this subsystem toc
+ * @regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem {
+	__le32	status;
+	__le32	enabled;
+	__le32	encryption_status;
+	__le32	encryption_required;
+	__le32	region_count;
+	__le64	regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc - Global Table of Content
+ * @status : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @enabled : Minidump enable status
+ * @subsystems : Array of subsystems toc
+ */
+struct minidump_global_toc {
+	__le32				status;
+	__le32				md_revision;
+	__le32				enabled;
+	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
+};
+
+/**
+ * struct minidump_ss_data - Minidump subsystem private data
+ * @md_ss_toc: Application Subsystem TOC pointer
+ * @md_regions: Application Subsystem region base pointer
+ */
+struct minidump_ss_data {
+	struct minidump_subsystem *md_ss_toc;
+	struct minidump_region	  *md_regions;
+};
+
+#define MINIDUMP_MAX_NAME_LENGTH	12
+/**
+ * struct qcom_minidump_region - Minidump region information
+ *
+ * @name:	Minidump region name
+ * @virt_addr:  Virtual address of the entry.
+ * @phys_addr:	Physical address of the entry to dump.
+ * @size:	Number of bytes to dump from @address location,
+ *		and it should be 4 byte aligned.
+ */
+struct qcom_minidump_region {
+	char		name[MINIDUMP_MAX_NAME_LENGTH];
+	void		*virt_addr;
+	phys_addr_t	phys_addr;
+	size_t		size;
+	unsigned int	id;
+};
+
+static LIST_HEAD(apss_md_rlist);
+
+/**
+ * struct md_region_list - Minidump region list struct
+ *
+ * @md_region:	associated minidump region
+ * @list:  list head entry
+ */
+struct md_region_list {
+	struct qcom_minidump_region md_region;
+	struct list_head list;
+};
+
+static struct minidump *md;
+
+/**
+ * qcom_md_add_region() - Register region in APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: None
+ */
+static void qcom_md_add_region(const struct qcom_minidump_region *region)
+{
+	struct minidump_subsystem *mdss_toc = md->apss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int region_cnt;
+
+	region_cnt = le32_to_cpu(mdss_toc->region_count);
+	mdr = &md->apss_data->md_regions[region_cnt];
+	strscpy(mdr->name, region->name, sizeof(mdr->name));
+	mdr->address = cpu_to_le64(region->phys_addr);
+	mdr->size = cpu_to_le64(region->size);
+	mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
+	region_cnt++;
+	mdss_toc->region_count = cpu_to_le32(region_cnt);
+}
+
+/**
+ * qcom_md_get_region_index() - Lookup minidump region by name
+ * @mdss_data: minidump subsystem data
+ * @region: minidump region.
+ *
+ * Return: On success, it returns the region index, on failure, returns
+ *	negative error value
+ */
+static int qcom_md_get_region_index(struct minidump_ss_data *mdss_data,
+				    const struct qcom_minidump_region *region)
+{
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int i;
+	unsigned int count;
+
+	count = le32_to_cpu(mdss_toc->region_count);
+	for (i = 0; i < count; i++) {
+		mdr = &mdss_data->md_regions[i];
+		if (!strcmp(mdr->name, region->name))
+			return i;
+	}
+
+	return -ENOENT;
+}
+
+/**
+ * qcom_md_region_unregister() - Unregister region from APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+static int qcom_md_region_unregister(const struct qcom_minidump_region *region)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int region_cnt;
+	unsigned int idx;
+	int ret;
+
+	ret = qcom_md_get_region_index(mdss_data, region);
+	if (ret < 0) {
+		dev_err(md->dev, "%s region is not present\n", region->name);
+		return ret;
+	}
+
+	idx = ret;
+	mdr = &mdss_data->md_regions[0];
+	region_cnt = le32_to_cpu(mdss_toc->region_count);
+	/*
+	 * Left shift all the regions exist after this removed region
+	 * index by 1 to fill the gap and zero out the last region
+	 * present at the end.
+	 */
+	memmove(&mdr[idx], &mdr[idx + 1], (region_cnt - idx - 1) * sizeof(*mdr));
+	memset(&mdr[region_cnt - 1], 0, sizeof(*mdr));
+	region_cnt--;
+	mdss_toc->region_count = cpu_to_le32(region_cnt);
+
+	return 0;
+}
+
+/**
+ * qcom_md_region_register() - Register region in APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+static int qcom_md_region_register(const struct qcom_minidump_region *region)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	unsigned int num_region;
+	int ret;
+
+	ret = qcom_md_get_region_index(mdss_data, region);
+	if (ret >= 0) {
+		dev_info(md->dev, "%s region is already registered\n", region->name);
+		return -EEXIST;
+	}
+
+	/* Check if there is a room for a new entry */
+	num_region = le32_to_cpu(mdss_toc->region_count);
+	if (num_region >= MAX_NUM_ENTRIES) {
+		dev_err(md->dev, "maximum region limit %u reached\n", num_region);
+		return -ENOSPC;
+	}
+
+	qcom_md_add_region(region);
+
+	return 0;
+}
+
+/**
+ * qcom_minidump_valid_region() - Checks if region is valid
+ * @region: minidump region.
+ *
+ * Return: true if region is valid, false otherwise.
+ */
+static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region)
+{
+	return region &&
+		strnlen(region->name, MINIDUMP_MAX_NAME_LENGTH) < MINIDUMP_MAX_NAME_LENGTH &&
+			region->virt_addr &&
+			region->size &&
+			IS_ALIGNED(region->size, 4);
+}
+
+/**
+ * qcom_minidump_region_register() - Register region in APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+static int qcom_minidump_region_register(const struct qcom_minidump_region *region)
+{
+	int ret;
+
+	if (!qcom_minidump_valid_region(region))
+		return -EINVAL;
+
+	mutex_lock(&md->md_lock);
+	ret = qcom_md_region_register(region);
+
+	mutex_unlock(&md->md_lock);
+	return ret;
+}
+
+/**
+ * qcom_minidump_region_unregister() - Unregister region from APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+static int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
+{
+	int ret;
+
+	if (!qcom_minidump_valid_region(region))
+		return -EINVAL;
+
+	mutex_lock(&md->md_lock);
+	ret = qcom_md_region_unregister(region);
+
+	mutex_unlock(&md->md_lock);
+	return ret;
+}
+
+/**
+ * qcom_apss_md_table_init() - Initialize the minidump table
+ * @mdss_toc: minidump subsystem table of contents
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+static int qcom_apss_md_table_init(struct minidump_subsystem *mdss_toc)
+{
+	struct minidump_ss_data *mdss_data;
+
+	mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);
+	if (!mdss_data)
+		return -ENOMEM;
+
+	mdss_data->md_ss_toc = mdss_toc;
+	mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
+					     sizeof(*mdss_data->md_regions),
+					     GFP_KERNEL);
+	if (!mdss_data->md_regions)
+		return -ENOMEM;
+
+	mdss_toc = mdss_data->md_ss_toc;
+	mdss_toc->regions_baseptr = cpu_to_le64(virt_to_phys(mdss_data->md_regions));
+	mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
+	mdss_toc->status = cpu_to_le32(1);
+	mdss_toc->region_count = cpu_to_le32(0);
+
+	/* Tell bootloader not to encrypt the regions of this subsystem */
+	mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
+	mdss_toc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
+
+	md->apss_data = mdss_data;
+
+	return 0;
+}
+
+/**
+ * register_md_region() - Register a new minidump region
+ * @id: unique id to identify the region
+ * @name: name of the region
+ * @vaddr: virtual memory address of the region start
+ * @size: size of the region
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+static int register_md_region(unsigned int id, char *name, void *vaddr,
+			      size_t size)
+{
+	struct qcom_minidump_region *md_region;
+	int ret;
+
+	struct md_region_list *mdr_list =
+		kzalloc(sizeof(*mdr_list), GFP_KERNEL);
+	if (!mdr_list)
+		return -ENOMEM;
+	md_region = &mdr_list->md_region;
+
+	scnprintf(md_region->name, sizeof(md_region->name), "K%d%.8s", id, name);
+	md_region->virt_addr = vaddr;
+	md_region->phys_addr = virt_to_phys(vaddr);
+	md_region->size = ALIGN(size, 4);
+	md_region->id = id;
+
+	ret = qcom_minidump_region_register(md_region);
+	if (ret < 0) {
+		pr_err("failed to register region in minidump %d %s: err: %d\n",
+		       id, name, ret);
+		return ret;
+	}
+
+	list_add(&mdr_list->list, &apss_md_rlist);
+	return 0;
+}
+
+/**
+ * unregister_md_region() - Unregister a previously registered minidump region
+ * @id: unique id to identify the region
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+static int unregister_md_region(unsigned int id)
+{
+	int ret = -ENOENT;
+	struct md_region_list *mdr_list;
+	struct md_region_list *tmp;
+
+	list_for_each_entry_safe(mdr_list, tmp, &apss_md_rlist, list) {
+		struct qcom_minidump_region *region;
+
+		region = &mdr_list->md_region;
+		if (region->id == id) {
+			ret = qcom_minidump_region_unregister(region);
+			list_del(&mdr_list->list);
+			return ret;
+		}
+	}
+
+	pr_err("failed to unregister region from minidump %d\n", ret);
+
+	return ret;
+}
+
+static struct kmemdump_backend qcom_md_backend = {
+	.name = "qcom_md",
+	.register_region = register_md_region,
+	.unregister_region = unregister_md_region,
+};
+
+static int qcom_md_probe(struct platform_device *pdev)
+{
+	struct minidump_global_toc *mdgtoc;
+	size_t size;
+	int ret;
+
+	md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);
+
+	md->dev = &pdev->dev;
+
+	mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
+	if (IS_ERR(mdgtoc)) {
+		ret = PTR_ERR(mdgtoc);
+		dev_err(md->dev, "Couldn't find minidump smem item %d\n", ret);
+	}
+
+	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
+		dev_err(md->dev, "minidump table is not initialized %d\n", ret);
+		return -ENAVAIL;
+	}
+
+	mutex_init(&md->md_lock);
+
+	ret = qcom_apss_md_table_init(&mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
+	if (ret) {
+		dev_err(md->dev, "apss minidump initialization failed %d\n", ret);
+		return ret;
+	}
+
+	return kmemdump_register_backend(&qcom_md_backend);
+}
+
+static void qcom_md_remove(struct platform_device *pdev)
+{
+	kmemdump_unregister_backend(&qcom_md_backend);
+}
+
+static struct platform_driver qcom_md_driver = {
+	.probe = qcom_md_probe,
+	.remove = qcom_md_remove,
+	.driver  = {
+		.name = "qcom-md",
+	},
+};
+
+module_platform_driver(qcom_md_driver);
+
+MODULE_AUTHOR("Eugen Hristev <eugen.hristev@linaro.org>");
+MODULE_AUTHOR("Mukesh Ojha <quic_mojha@quicinc.com>");
+MODULE_DESCRIPTION("Qualcomm kmemdump minidump backend driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

[RFC][PATCH 04/14] soc: qcom: smem: add minidump device

[Eugen Hristev]

Add a minidump platform device.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 drivers/soc/qcom/smem.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 592819701809..985147b563f8 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -270,6 +270,7 @@ struct smem_region {
  * @partitions: list of partitions of current processor/host
  * @item_count: max accepted item number
  * @socinfo:	platform device pointer
+ * @mdinfo:	minidump device pointer
  * @num_regions: number of @regions
  * @regions:	list of the memory regions defining the shared memory
  */
@@ -280,6 +281,7 @@ struct qcom_smem {
 
 	u32 item_count;
 	struct platform_device *socinfo;
+	struct platform_device *mdinfo;
 	struct smem_ptable *ptable;
 	struct smem_partition global_partition;
 	struct smem_partition partitions[SMEM_HOST_COUNT];
@@ -1236,12 +1238,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	if (IS_ERR(smem->socinfo))
 		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
 
+	smem->mdinfo = platform_device_register_data(&pdev->dev, "qcom-md",
+						     PLATFORM_DEVID_AUTO, NULL,
+						     0);
+	if (IS_ERR(smem->mdinfo))
+		dev_err(&pdev->dev, "failed to register platform md device\n");
+
 	return 0;
 }
 
 static void qcom_smem_remove(struct platform_device *pdev)
 {
 	platform_device_unregister(__smem->socinfo);
+	if (!IS_ERR(__smem->mdinfo))
+		platform_device_unregister(__smem->mdinfo);
 
 	hwspin_lock_free(__smem->hwlock);
 	__smem = NULL;
-- 
2.43.0

[RFC][PATCH 05/14] Documentation: kmemdump: add section for coreimage ELF

[Eugen Hristev]

Add section describing the utility of coreimage ELF generation for kmemdump.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 Documentation/debug/kmemdump.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/debug/kmemdump.rst b/Documentation/debug/kmemdump.rst
index dfee755a1be1..b67dc166905a 100644
--- a/Documentation/debug/kmemdump.rst
+++ b/Documentation/debug/kmemdump.rst
@@ -17,6 +17,12 @@ kmemdump allows a backend to be connected, this backend interfaces a
 specific hardware that can debug or dump the memory registered into
 kmemdump.
 
+kmemdump can also prepare specific regions of the kernel that can be
+put together to form a minimal core image file. To achieve this, the first
+region is an ELF header with program headers for each region, and specific
+ELF NOTE section with vmcoreinfo. To enable this feature, use
+`CONFIG_DRIVER_KMEMDUMP_COREIMAGE`.
+
 kmemdump Internals
 =============
 
-- 
2.43.0

[RFC][PATCH 06/14] kmemdump: add coreimage ELF layer

[Eugen Hristev]

Implement kmemdumping into an ELF coreimage.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
Co-developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/debug/Kconfig              |  10 +
 drivers/debug/Makefile             |   1 +
 drivers/debug/kmemdump.c           |  12 ++
 drivers/debug/kmemdump_coreimage.c | 285 +++++++++++++++++++++++++++++
 include/linux/kmemdump.h           |  25 +++
 5 files changed, 333 insertions(+)
 create mode 100644 drivers/debug/kmemdump_coreimage.c

diff --git a/drivers/debug/Kconfig b/drivers/debug/Kconfig
index 72a906487e02..cd73f299e5cd 100644
--- a/drivers/debug/Kconfig
+++ b/drivers/debug/Kconfig
@@ -14,6 +14,16 @@ config DRIVER_KMEMDUMP
 	  Note that modules using this feature must be rebuilt if option
 	  changes.
 
+config DRIVER_KMEMDUMP_COREIMAGE
+	depends on DRIVER_KMEMDUMP
+	bool "Assemble memory regions into a coredump readable with debuggers"
+	help
+	  Enabling this will assemble all the memory regions into a
+	  core ELF file. The first region will include program headers for
+	  all the regions, together with vmcoreinfo and specific coredump
+	  structures. The coredump file can then be loaded into GDB or crash
+	  tool and further inspected.
+
 config QCOM_MD_KMEMDUMP_BACKEND
 	tristate "Qualcomm Minidump kmemdump backend driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/debug/Makefile b/drivers/debug/Makefile
index d8a9db29cd15..95ecc315b376 100644
--- a/drivers/debug/Makefile
+++ b/drivers/debug/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_DRIVER_KMEMDUMP) += kmemdump.o
+obj-$(CONFIG_DRIVER_KMEMDUMP_COREIMAGE) += kmemdump_coreimage.o
 obj-$(CONFIG_QCOM_MD_KMEMDUMP_BACKEND) += qcom_md.o
diff --git a/drivers/debug/kmemdump.c b/drivers/debug/kmemdump.c
index a685c0863e25..be96b39a136a 100644
--- a/drivers/debug/kmemdump.c
+++ b/drivers/debug/kmemdump.c
@@ -58,6 +58,7 @@ int kmemdump_register(char *handle, void *zone, size_t size)
 			return ret;
 		}
 		z->registered = true;
+		update_elfheader(z);
 	}
 
 	mutex_unlock(&kmemdump_lock);
@@ -84,6 +85,7 @@ void kmemdump_unregister(int id)
 	if (z->registered && backend)
 		backend->unregister_region(z->id);
 
+	clear_elfheader(z);
 	idr_remove(&kmemdump_idr, id);
 	kfree(z);
 
@@ -103,6 +105,7 @@ static int kmemdump_register_fn(int id, void *p, void *data)
 	if (ret)
 		return ret;
 	z->registered = true;
+	update_elfheader(z);
 
 	return 0;
 }
@@ -130,6 +133,14 @@ int kmemdump_register_backend(struct kmemdump_backend *be)
 	pr_info("kmemdump backend %s registered successfully.\n",
 		backend->name);
 
+	init_elfheader(backend);
+
+	mutex_unlock(&kmemdump_lock);
+
+	register_coreinfo();
+
+	mutex_lock(&kmemdump_lock);
+
 	/* Try to call the backend for all previously requested zones */
 	idr_for_each(&kmemdump_idr, kmemdump_register_fn, NULL);
 
@@ -151,6 +162,7 @@ static int kmemdump_unregister_fn(int id, void *p, void *data)
 	if (ret)
 		return ret;
 	z->registered = false;
+	clear_elfheader(z);
 
 	return 0;
 }
diff --git a/drivers/debug/kmemdump_coreimage.c b/drivers/debug/kmemdump_coreimage.c
new file mode 100644
index 000000000000..59630adf5dd2
--- /dev/null
+++ b/drivers/debug/kmemdump_coreimage.c
@@ -0,0 +1,285 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/init.h>
+#include <linux/elfcore.h>
+#include <linux/kmemdump.h>
+#include <linux/utsname.h>
+#include <linux/sched/stat.h>
+#include <linux/vmcore_info.h>
+
+#define CORE_STR "CORE"
+#define KMEMDUMP_ELF_ID 0
+
+#define MAX_NUM_ENTRIES	  201
+
+/**
+ * struct kmemdump_elfhdr - Kmemdump table elf header
+ * @ehdr: elf main header
+ * @phdr: Program header
+ * @elf_offset: Section offset in elf
+ */
+struct kmemdump_elfhdr {
+	struct elfhdr	 *ehdr;
+	struct elf_phdr	 *phdr;
+	size_t		 elf_offset;
+} elf;
+
+extern char kernel_config_data;
+
+/**
+ * register_coreinfo() - Register vital kernel information into the coreimage
+ *
+ * Return: None
+ */
+void register_coreinfo(void)
+{
+	/*
+	 * Right before kernel_config_data there is a marker string,
+	 * capture it as well.
+	 * Debug tools don't care about config size, they just try 64K and
+	 * if it's not enough, double it.
+	 */
+	kmemdump_register("config", (void *)&kernel_config_data - 8,
+			  128 * 1024);
+	kmemdump_register("utsn", (void *)&init_uts_ns,
+			  sizeof(init_uts_ns));
+	kmemdump_register("banner", (void *)&linux_banner,
+			  strlen(linux_banner));
+	kmemdump_register("percpu", (void *)&__per_cpu_offset,
+			  sizeof(__per_cpu_offset));
+	kmemdump_register("init_mm", (void *)&init_mm,
+			  sizeof(init_mm));
+	kmemdump_register("mmpgd", (void *)init_mm.pgd,
+			  sizeof(*init_mm.pgd));
+	kmemdump_register("highmem", (void *)&high_memory,
+			  sizeof(&high_memory));
+	kmemdump_register("memsect", (void *)*mem_section,
+			  sizeof(struct mem_section) * NR_SECTION_ROOTS);
+	kmemdump_register("memsect", (void *)&mem_section,
+			  sizeof(&mem_section));
+	kmemdump_register("totalram", (void *)&_totalram_pages,
+			  sizeof(_totalram_pages));
+	kmemdump_register("node_states", (void *)&node_states,
+			  sizeof(node_states));
+	kmemdump_register("nr_threads", (void *)&nr_threads,
+			  sizeof(nr_threads));
+	kmemdump_register("cpu_po", (void *)cpu_possible_mask,
+			  sizeof(cpumask_t));
+	kmemdump_register("cpu_pr", (void *)cpu_present_mask,
+			  sizeof(cpumask_t));
+	kmemdump_register("cpu_on", (void *)cpu_online_mask,
+			  sizeof(cpumask_t));
+	kmemdump_register("jiffies", (void *)&jiffies_64,
+			  sizeof(jiffies_64));
+}
+
+static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
+{
+	struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
+
+	return &ephdr[idx];
+}
+
+/**
+ * clear_elfheader() - Remove the program header for a specific memory zone
+ * @z: pointer to the kmemdump zone
+ *
+ * Return: On success, it returns 0, errno otherwise
+ */
+int clear_elfheader(const struct kmemdump_zone *z)
+{
+	struct elfhdr *ehdr = elf.ehdr;
+	struct elf_phdr *phdr;
+	struct elf_phdr *tmp_phdr;
+	unsigned int phidx;
+	unsigned int i;
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		phdr = elf_phdr_entry_addr(ehdr, i);
+		if (phdr->p_paddr == virt_to_phys(z->zone) &&
+		    phdr->p_memsz == ALIGN(z->size, 4))
+			break;
+	}
+
+	if (i == ehdr->e_phnum) {
+		pr_err("Cannot find program header entry in elf\n");
+		return -EINVAL;
+	}
+
+	phidx = i;
+
+	/* Clear program header */
+	tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
+	for (i = phidx; i < ehdr->e_phnum - 1; i++) {
+		tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
+		phdr = elf_phdr_entry_addr(ehdr, i);
+		memcpy(phdr, tmp_phdr, sizeof(*phdr));
+		phdr->p_offset = phdr->p_offset - ALIGN(z->size, 4);
+	}
+	memset(tmp_phdr, 0, sizeof(*tmp_phdr));
+	ehdr->e_phnum--;
+
+	elf.elf_offset -= ALIGN(z->size, 4);
+
+	return 0;
+}
+
+/**
+ * update_elfheader() - Add the program header for a specific memory zone
+ * @z: pointer to the kmemdump zone
+ *
+ * Return: None
+ */
+void update_elfheader(const struct kmemdump_zone *z)
+{
+	struct elfhdr *ehdr = elf.ehdr;
+	struct elf_phdr *phdr;
+
+	phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
+
+	phdr->p_type = PT_LOAD;
+	phdr->p_offset = elf.elf_offset;
+	phdr->p_vaddr = (elf_addr_t)z->zone;
+	phdr->p_paddr = (elf_addr_t)virt_to_phys(z->zone);
+	phdr->p_filesz = phdr->p_memsz = ALIGN(z->size, 4);
+	phdr->p_flags = PF_R | PF_W;
+
+	elf.elf_offset += ALIGN(z->size, 4);
+}
+
+static void append_kcore_note(char *notes, size_t *i, const char *name,
+			      unsigned int type, const void *desc,
+			      size_t descsz)
+{
+	struct elf_note *note = (struct elf_note *)&notes[*i];
+
+	note->n_namesz = strlen(name) + 1;
+	note->n_descsz = descsz;
+	note->n_type = type;
+	*i += sizeof(*note);
+	memcpy(&notes[*i], name, note->n_namesz);
+	*i = ALIGN(*i + note->n_namesz, 4);
+	memcpy(&notes[*i], desc, descsz);
+	*i = ALIGN(*i + descsz, 4);
+}
+
+static void append_kcore_note_nodesc(char *notes, size_t *i, const char *name,
+				     unsigned int type, size_t descsz)
+{
+	struct elf_note *note = (struct elf_note *)&notes[*i];
+
+	note->n_namesz = strlen(name) + 1;
+	note->n_descsz = descsz;
+	note->n_type = type;
+	*i += sizeof(*note);
+	memcpy(&notes[*i], name, note->n_namesz);
+	*i = ALIGN(*i + note->n_namesz, 4);
+}
+
+/**
+ * init_elfheader() - Prepare coreinfo elf header
+ *		This function prepares the elf header for the coredump image.
+ *		Initially there is a single program header for the elf NOTE.
+ *		The note contains the usual core dump information, and the
+ *		vmcoreinfo.
+ * @z: pointer to the kmemdump zone
+ *
+ * Return: 0 on success, errno otherwise
+ */
+int init_elfheader(struct kmemdump_backend *be)
+{
+	struct elfhdr *ehdr;
+	struct elf_phdr *phdr;
+	void *notes;
+	unsigned int elfh_size;
+	unsigned int phdr_off;
+	size_t note_len, i = 0;
+
+	struct elf_prstatus prstatus = {};
+	struct elf_prpsinfo prpsinfo = {
+		.pr_sname = 'R',
+		.pr_fname = "vmlinux",
+	};
+
+	/*
+	 * Header buffer contains:
+	 * ELF header, Note entry with PR status, PR ps info, and vmcoreinfo
+	 * MAX_NUM_ENTRIES Program headers,
+	 */
+	elfh_size = sizeof(*ehdr);
+	elfh_size += sizeof(struct elf_prstatus);
+	elfh_size += sizeof(struct elf_prpsinfo);
+	elfh_size += sizeof(VMCOREINFO_NOTE_NAME);
+	elfh_size += ALIGN(vmcoreinfo_size, 4);
+	elfh_size += (sizeof(*phdr)) * (MAX_NUM_ENTRIES);
+
+	elfh_size = ALIGN(elfh_size, 4);
+
+	elf.ehdr = kzalloc(elfh_size, GFP_KERNEL);
+	if (!elf.ehdr)
+		return -ENOMEM;
+
+	ehdr = elf.ehdr;
+	/* Assign Program headers offset, it's right after the elf header. */
+	elf.phdr = phdr = (struct elf_phdr *)(ehdr + 1);
+	phdr_off = sizeof(*ehdr);
+
+	memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
+	ehdr->e_ident[EI_CLASS] = ELF_CLASS;
+	ehdr->e_ident[EI_DATA] = ELF_DATA;
+	ehdr->e_ident[EI_VERSION] = EV_CURRENT;
+	ehdr->e_ident[EI_OSABI] = ELF_OSABI;
+	ehdr->e_type = ET_CORE;
+	ehdr->e_machine  = ELF_ARCH;
+	ehdr->e_version = EV_CURRENT;
+	ehdr->e_ehsize = sizeof(*ehdr);
+	ehdr->e_phentsize = sizeof(*phdr);
+
+	elf.elf_offset = elfh_size;
+
+	notes = (void *)(((char *)elf.ehdr) + elf.elf_offset);
+
+	/* we have a single program header now */
+	ehdr->e_phnum = 1;
+
+	/* Length of the note is made of :
+	 * 3 elf notes structs (prstatus, prpsinfo, vmcoreinfo)
+	 * 3 notes names (2 core strings, 1 vmcoreinfo name)
+	 * sizeof each note
+	 */
+	note_len = (3 * sizeof(struct elf_note) +
+		    2 * ALIGN(sizeof(CORE_STR), 4) +
+		    VMCOREINFO_NOTE_NAME_BYTES +
+		    ALIGN(sizeof(struct elf_prstatus), 4) +
+		    ALIGN(sizeof(struct elf_prpsinfo), 4) +
+		    ALIGN(vmcoreinfo_size, 4));
+
+	phdr->p_type = PT_NOTE;
+	phdr->p_offset = elf.elf_offset;
+	phdr->p_filesz = note_len;
+
+	/* advance elf offset */
+	elf.elf_offset += note_len;
+
+	strscpy(prpsinfo.pr_psargs, saved_command_line,
+		sizeof(prpsinfo.pr_psargs));
+
+	append_kcore_note(notes, &i, CORE_STR, NT_PRSTATUS, &prstatus,
+			  sizeof(prstatus));
+	append_kcore_note(notes, &i, CORE_STR, NT_PRPSINFO, &prpsinfo,
+			  sizeof(prpsinfo));
+	append_kcore_note_nodesc(notes, &i, VMCOREINFO_NOTE_NAME, 0,
+				 ALIGN(vmcoreinfo_size, 4));
+
+	ehdr->e_phoff = phdr_off;
+
+	/* This is the first kmemdump region, the ELF header */
+	be->register_region(KMEMDUMP_ELF_ID, "ELF", elf.ehdr,
+			    elfh_size + note_len - ALIGN(vmcoreinfo_size, 4));
+
+	/* The second region is the vmcoreinfo, which goes right after */
+	be->register_region(KMEMDUMP_ELF_ID, "vmcoreinfo", vmcoreinfo_data,
+			    ALIGN(vmcoreinfo_size, 4));
+
+	return 0;
+}
diff --git a/include/linux/kmemdump.h b/include/linux/kmemdump.h
index b55b15c295ac..13aabd72d17a 100644
--- a/include/linux/kmemdump.h
+++ b/include/linux/kmemdump.h
@@ -49,4 +49,29 @@ static inline void kmemdump_unregister(int id)
 
 int kmemdump_register_backend(struct kmemdump_backend *backend);
 void kmemdump_unregister_backend(struct kmemdump_backend *backend);
+
+#ifdef CONFIG_DRIVER_KMEMDUMP_COREIMAGE
+int init_elfheader(struct kmemdump_backend *be);
+void update_elfheader(const struct kmemdump_zone *z);
+int clear_elfheader(const struct kmemdump_zone *z);
+void register_coreinfo(void);
+#else
+static inline int init_elfheader(struct kmemdump_backend *be)
+{
+	return 0;
+}
+
+static inline void update_elfheader(const struct kmemdump_zone *z)
+{
+}
+
+static inline int clear_elfheader(const struct kmemdump_zone *z)
+{
+	return 0;
+}
+
+static inline void register_coreinfo(void)
+{
+}
+#endif
 #endif
-- 
2.43.0

[RFC][PATCH 07/14] printk: add kmsg_kmemdump_register

[Eugen Hristev]

Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
to kmemdump.
This will allow kmemdump to be able to dump specific log buffer areas on
demand.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 include/linux/kmsg_dump.h |  6 ++++++
 kernel/printk/printk.c    | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
index 6055fc969877..cbe76c94710b 100644
--- a/include/linux/kmsg_dump.h
+++ b/include/linux/kmsg_dump.h
@@ -80,6 +80,8 @@ int kmsg_dump_register(struct kmsg_dumper *dumper);
 
 int kmsg_dump_unregister(struct kmsg_dumper *dumper);
 
+void kmsg_kmemdump_register(void);
+
 const char *kmsg_dump_reason_str(enum kmsg_dump_reason reason);
 #else
 static inline void kmsg_dump_desc(enum kmsg_dump_reason reason, const char *desc)
@@ -112,6 +114,10 @@ static inline int kmsg_dump_unregister(struct kmsg_dumper *dumper)
 	return -EINVAL;
 }
 
+static inline void kmsg_kmemdump_register(void)
+{
+}
+
 static inline const char *kmsg_dump_reason_str(enum kmsg_dump_reason reason)
 {
 	return "Disabled";
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 057db78876cd..cf4aa86ef7af 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -48,6 +48,7 @@
 #include <linux/sched/clock.h>
 #include <linux/sched/debug.h>
 #include <linux/sched/task_stack.h>
+#include <linux/kmemdump.h>
 
 #include <linux/uaccess.h>
 #include <asm/sections.h>
@@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
 }
 EXPORT_SYMBOL_GPL(kmsg_dump_register);
 
+void kmsg_kmemdump_register(void)
+{
+	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
+	kmemdump_register("prb", (void *)&prb, sizeof(prb));
+	kmemdump_register("prb", (void *)prb, sizeof(*prb));
+	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
+			  sizeof(_printk_rb_static_descs));
+	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
+			  sizeof(_printk_rb_static_infos));
+}
+EXPORT_SYMBOL_GPL(kmsg_kmemdump_register);
+
 /**
  * kmsg_dump_unregister - unregister a kmsg dumper.
  * @dumper: pointer to the kmsg_dumper structure
-- 
2.43.0

[RFC][PATCH 08/14] kmemdump: coreimage: add kmsg registration

[Eugen Hristev]

Register log buffer into kmemdump coreimage

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 drivers/debug/kmemdump_coreimage.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/debug/kmemdump_coreimage.c b/drivers/debug/kmemdump_coreimage.c
index 59630adf5dd2..077b65cae4cb 100644
--- a/drivers/debug/kmemdump_coreimage.c
+++ b/drivers/debug/kmemdump_coreimage.c
@@ -3,6 +3,7 @@
 #include <linux/init.h>
 #include <linux/elfcore.h>
 #include <linux/kmemdump.h>
+#include <linux/kmsg_dump.h>
 #include <linux/utsname.h>
 #include <linux/sched/stat.h>
 #include <linux/vmcore_info.h>
@@ -71,6 +72,8 @@ void register_coreinfo(void)
 			  sizeof(cpumask_t));
 	kmemdump_register("jiffies", (void *)&jiffies_64,
 			  sizeof(jiffies_64));
+
+	kmsg_kmemdump_register();
 }
 
 static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
-- 
2.43.0

[RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Eugen Hristev]

Add function to register irq info into kmemdump.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 include/linux/irqnr.h | 1 +
 kernel/irq/irqdesc.c  | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/irqnr.h b/include/linux/irqnr.h
index e97206c721a0..136f0572ad78 100644
--- a/include/linux/irqnr.h
+++ b/include/linux/irqnr.h
@@ -9,6 +9,7 @@ unsigned int irq_get_nr_irqs(void) __pure;
 unsigned int irq_set_nr_irqs(unsigned int nr);
 extern struct irq_desc *irq_to_desc(unsigned int irq);
 unsigned int irq_get_next_irq(unsigned int offset);
+void irq_kmemdump_register(void);
 
 #define for_each_irq_desc(irq, desc)                                      \
 	for (unsigned int __nr_irqs__ = irq_get_nr_irqs(); __nr_irqs__;   \
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 287830739783..ae29165b1f1f 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -12,6 +12,7 @@
 #include <linux/export.h>
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
+#include <linux/kmemdump.h>
 #include <linux/maple_tree.h>
 #include <linux/irqdomain.h>
 #include <linux/sysfs.h>
@@ -164,6 +165,12 @@ unsigned int irq_set_nr_irqs(unsigned int nr)
 }
 EXPORT_SYMBOL_GPL(irq_set_nr_irqs);
 
+void irq_kmemdump_register(void)
+{
+	kmemdump_register("irq", (void *)&nr_irqs, sizeof(nr_irqs));
+}
+EXPORT_SYMBOL_GPL(irq_kmemdump_register);
+
 static DEFINE_MUTEX(sparse_irq_lock);
 static struct maple_tree sparse_irqs = MTREE_INIT_EXT(sparse_irqs,
 					MT_FLAGS_ALLOC_RANGE |
-- 
2.43.0

[RFC][PATCH 10/14] kmemdump: coreimage: add irq registration

[Eugen Hristev]

Register irq info into kmemdump

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 drivers/debug/kmemdump_coreimage.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/debug/kmemdump_coreimage.c b/drivers/debug/kmemdump_coreimage.c
index 077b65cae4cb..2144edcc6157 100644
--- a/drivers/debug/kmemdump_coreimage.c
+++ b/drivers/debug/kmemdump_coreimage.c
@@ -2,6 +2,7 @@
 
 #include <linux/init.h>
 #include <linux/elfcore.h>
+#include <linux/irqnr.h>
 #include <linux/kmemdump.h>
 #include <linux/kmsg_dump.h>
 #include <linux/utsname.h>
@@ -74,6 +75,7 @@ void register_coreinfo(void)
 			  sizeof(jiffies_64));
 
 	kmsg_kmemdump_register();
+	irq_kmemdump_register();
 }
 
 static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
-- 
2.43.0

[RFC][PATCH 11/14] panic: add panic_kmemdump_register

[Eugen Hristev]

Add function to register panic info into kmemdump

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 include/linux/panic.h | 1 +
 kernel/panic.c        | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/linux/panic.h b/include/linux/panic.h
index 54d90b6c5f47..42a24bae218c 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -96,5 +96,6 @@ extern const char *print_tainted_verbose(void);
 extern void add_taint(unsigned flag, enum lockdep_ok);
 extern int test_taint(unsigned flag);
 extern unsigned long get_taint(void);
+extern void panic_kmemdump_register(void);
 
 #endif	/* _LINUX_PANIC_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index d8635d5cecb2..a26c9581e70a 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -38,6 +38,7 @@
 #include <linux/seq_buf.h>
 #include <trace/events/error_report.h>
 #include <asm/sections.h>
+#include <linux/kmemdump.h>
 
 #define PANIC_TIMER_STEP 100
 #define PANIC_BLINK_SPD 18
@@ -589,6 +590,13 @@ unsigned long get_taint(void)
 	return tainted_mask;
 }
 
+void panic_kmemdump_register(void)
+{
+	kmemdump_register("taint", (void *)&tainted_mask, sizeof(tainted_mask));
+	kmemdump_register("taintflags", (void *)&taint_flags, sizeof(taint_flags));
+}
+EXPORT_SYMBOL_GPL(panic_kmemdump_register);
+
 /**
  * add_taint: add a taint flag if not already set.
  * @flag: one of the TAINT_* constants.
-- 
2.43.0

[RFC][PATCH 12/14] kmemdump: coreimage: add panic registration

[Eugen Hristev]

Call panic registration for the core image.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 drivers/debug/kmemdump_coreimage.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/debug/kmemdump_coreimage.c b/drivers/debug/kmemdump_coreimage.c
index 2144edcc6157..7f3b089b5739 100644
--- a/drivers/debug/kmemdump_coreimage.c
+++ b/drivers/debug/kmemdump_coreimage.c
@@ -5,6 +5,7 @@
 #include <linux/irqnr.h>
 #include <linux/kmemdump.h>
 #include <linux/kmsg_dump.h>
+#include <linux/panic.h>
 #include <linux/utsname.h>
 #include <linux/sched/stat.h>
 #include <linux/vmcore_info.h>
@@ -76,6 +77,7 @@ void register_coreinfo(void)
 
 	kmsg_kmemdump_register();
 	irq_kmemdump_register();
+	panic_kmemdump_register();
 }
 
 static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
-- 
2.43.0

[RFC][PATCH 13/14] sched: add sched_kmemdump_register

[Eugen Hristev]

Add function to register scheduler info into kmemdump

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 include/linux/sched.h | 1 +
 kernel/sched/core.c   | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9c15365a30c0..441f5a6099f6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1820,6 +1820,7 @@ extern int cpuset_cpumask_can_shrink(const struct cpumask *cur, const struct cpu
 extern int task_can_attach(struct task_struct *p);
 extern int dl_bw_alloc(int cpu, u64 dl_bw);
 extern void dl_bw_free(int cpu, u64 dl_bw);
+extern void sched_kmemdump_register(void);
 #ifdef CONFIG_SMP
 
 /* do_set_cpus_allowed() - consider using set_cpus_allowed_ptr() instead */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 00ea6f31973c..8615fd02d4db 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -66,6 +66,7 @@
 #include <linux/vtime.h>
 #include <linux/wait_api.h>
 #include <linux/workqueue_api.h>
+#include <linux/kmemdump.h>
 
 #ifdef CONFIG_PREEMPT_DYNAMIC
 # ifdef CONFIG_GENERIC_ENTRY
@@ -8672,6 +8673,12 @@ void __init sched_init(void)
 	scheduler_running = 1;
 }
 
+void sched_kmemdump_register(void)
+{
+	kmemdump_register("runqueues", (void *)&runqueues, sizeof(runqueues));
+}
+EXPORT_SYMBOL_GPL(sched_kmemdump_register);
+
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
 void __might_sleep(const char *file, int line)
-- 
2.43.0

[RFC][PATCH 14/14] kmemdump: coreimage: add sched registration

[Eugen Hristev]

Register sched info into kmemdump coreimage

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 drivers/debug/kmemdump_coreimage.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/debug/kmemdump_coreimage.c b/drivers/debug/kmemdump_coreimage.c
index 7f3b089b5739..2be396ca0495 100644
--- a/drivers/debug/kmemdump_coreimage.c
+++ b/drivers/debug/kmemdump_coreimage.c
@@ -78,6 +78,7 @@ void register_coreinfo(void)
 	kmsg_kmemdump_register();
 	irq_kmemdump_register();
 	panic_kmemdump_register();
+	sched_kmemdump_register();
 }
 
 static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
-- 
2.43.0

Re: [RFC][PATCH 00/14] introduce kmemdump

[Trilok Soni]

On 4/22/2025 4:31 AM, Eugen Hristev wrote:
> kmemdump is a mechanism which allows the kernel to mark specific memory
> areas for dumping or specific backend usage.
> Once regions are marked, kmemdump keeps an internal list with the regions
> and registers them in the backend.
> Further, depending on the backend driver, these regions can be dumped using
> firmware or different hardware block.
> Regions being marked beforehand, when the system is up and running, there
> is no need nor dependency on a panic handler, or a working kernel that can
> dump the debug information.
> The kmemdump approach works when pstore, kdump, or another mechanism do not.
> Pstore relies on persistent storage, a dedicated RAM area or flash, which
> has the disadvantage of having the memory reserved all the time, or another
> specific non volatile memory. Some devices cannot keep the RAM contents on
> reboot so ramoops does not work. Some devices do not allow kexec to run
> another kernel to debug the crashed one.
> For such devices, that have another mechanism to help debugging, like
> firmware, kmemdump is a viable solution.
> 
> kmemdump can create a core image, similar with /proc/vmcore, with only
> the registered regions included. This can be loaded into crash tool/gdb and
> analyzed.
> To have this working, specific information from the kernel is registered,
> and this is done at kmemdump init time, no need for the kmemdump user to
> do anything.
> 
> The implementation is based on the initial Pstore/directly mapped zones
> published as an RFC here:
> https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/
> 
> The back-end implementation for qcom_smem is based on the minidump
> patch series and driver written by Mukesh Ojha, thanks:
> https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/
> 
> I appreciate the feedback on this series, I know it is a longshot, and there
> is a lot to improve, but I hope I am on the right track.


Is there any way to demonstrate this framework on non-Qualcomm device? Like any
other ARM device from TI, NXP etc; x86/RISC-V based device is also fine.

-- 
---Trilok Soni

Re: [RFC][PATCH 07/14] printk: add kmsg_kmemdump_register

[Petr Mladek]

On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
> to kmemdump.
> This will allow kmemdump to be able to dump specific log buffer areas on
> demand.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>  }
>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
>  
> +void kmsg_kmemdump_register(void)
> +{
> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));

This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
It should be enough to register the memory with the structure.

> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
> +			  sizeof(_printk_rb_static_descs));
> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
> +			  sizeof(_printk_rb_static_infos));

Also this looks wrong. These are static buffers which are used during
early boot. They might later be replaced by dynamically allocated
buffers when a bigger buffer is requested by "log_buf_len" command
line parameter.

I think that we need to register the memory of the structure
and 3 more buffers. See how the bigger buffer is allocated in
setup_log_buf().

I would expect something like:

	unsigned int descs_count;
	unsigned long data_size;

	descs_count = 2 << prb->desc_ring.count_bits;
	data_size = 2 << prb->data_ring.size_bits;

	kmemdump_register("prb", (void *)prb, sizeof(*prb));
	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
			  descs_count * sizeof(struct prb_desc));
	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
			  descs_count * sizeof(struct printk_info));
	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);


But I wonder if this is enough. The current crash dump code also needs
to export the format of the used structures, see
log_buf_vmcoreinfo_setup().

Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?

> +}
> +EXPORT_SYMBOL_GPL(kmsg_kmemdump_register);
> +

Best Regards,
Petr

Re: [RFC][PATCH 07/14] printk: add kmsg_kmemdump_register

[Eugen Hristev]

Hello Petr,

Thank you for your review.

On 5/5/25 18:25, Petr Mladek wrote:
> On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
>> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
>> to kmemdump.
>> This will allow kmemdump to be able to dump specific log buffer areas on
>> demand.
>>
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>>  }
>>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
>>  
>> +void kmsg_kmemdump_register(void)
>> +{
>> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
>> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
>> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> 
> This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
> It should be enough to register the memory with the structure.

Yes, from my perspective this should be also enough. However, when
loading the generated core dump into crash tool , the tool first looks
for the prb pointer itself, and then stops if the pointer is not readable.
After the prb pointer is being found, the crash tool dereferences it ,
and looks at the indicated address for the actual memory.
That is why the pointer is also saved as a kmemdump region in my proof
of concept.

> 
>> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
>> +			  sizeof(_printk_rb_static_descs));
>> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
>> +			  sizeof(_printk_rb_static_infos));
> 
> Also this looks wrong. These are static buffers which are used during
> early boot. They might later be replaced by dynamically allocated
> buffers when a bigger buffer is requested by "log_buf_len" command
> line parameter.
> 

I will double check whether the crash tool looks for these symbols or
only the memory, and come back with an answer

> I think that we need to register the memory of the structure
> and 3 more buffers. See how the bigger buffer is allocated in
> setup_log_buf().
> 
> I would expect something like:
> 
> 	unsigned int descs_count;
> 	unsigned long data_size;
> 
> 	descs_count = 2 << prb->desc_ring.count_bits;
> 	data_size = 2 << prb->data_ring.size_bits;
> 
> 	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> 	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
> 			  descs_count * sizeof(struct prb_desc));
> 	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
> 			  descs_count * sizeof(struct printk_info));
> 	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);
> 
> 
Thank you. It may be that in my test case, the buffer was not
extended/reallocated with a bigger one.

> But I wonder if this is enough. The current crash dump code also needs
> to export the format of the used structures, see
> log_buf_vmcoreinfo_setup().

It appears that crash tool looks for the structures into vmlinux
symbols. It can be that this information is not available to some tools,
or vmlinux not available, in which case all the used structures format
and sizes need to be exported. But right now, the crash tool does not
work without vmlinux.

> 
> Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?

I believe CONFIG_KMEMDUMP_COREIMAGE should select CONFIG_VMCORE_INFO
indeed, which is not done in my patches. Or I have not fully understood
your question ?


Eugen
> 
>> +}
>> +EXPORT_SYMBOL_GPL(kmsg_kmemdump_register);
>> +
> 
> Best Regards,
> Petr

Re: [RFC][PATCH 07/14] printk: add kmsg_kmemdump_register

[Petr Mladek]

On Mon 2025-05-05 18:51:19, Eugen Hristev wrote:
> Hello Petr,
> 
> Thank you for your review.
> 
> On 5/5/25 18:25, Petr Mladek wrote:
> > On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
> >> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
> >> to kmemdump.
> >> This will allow kmemdump to be able to dump specific log buffer areas on
> >> demand.
> >>
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
> >>  }
> >>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
> >>  
> >> +void kmsg_kmemdump_register(void)
> >> +{
> >> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
> >> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
> >> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> > 
> > This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
> > It should be enough to register the memory with the structure.
>
> Yes, from my perspective this should be also enough. However, when
> loading the generated core dump into crash tool , the tool first looks
> for the prb pointer itself, and then stops if the pointer is not readable.
> After the prb pointer is being found, the crash tool dereferences it ,
> and looks at the indicated address for the actual memory.
> That is why the pointer is also saved as a kmemdump region in my proof
> of concept.

I see. It makes perfect sense to store the pointer as well after all.

> >> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
> >> +			  sizeof(_printk_rb_static_descs));
> >> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
> >> +			  sizeof(_printk_rb_static_infos));
> > 
> > Also this looks wrong. These are static buffers which are used during
> > early boot. They might later be replaced by dynamically allocated
> > buffers when a bigger buffer is requested by "log_buf_len" command
> > line parameter.
> > 
> 
> I will double check whether the crash tool looks for these symbols or
> only the memory, and come back with an answer
> 
> > I think that we need to register the memory of the structure
> > and 3 more buffers. See how the bigger buffer is allocated in
> > setup_log_buf().
> > 
> > I would expect something like:
> > 
> > 	unsigned int descs_count;
> > 	unsigned long data_size;
> > 
> > 	descs_count = 2 << prb->desc_ring.count_bits;
> > 	data_size = 2 << prb->data_ring.size_bits;
> > 
> > 	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> > 	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
> > 			  descs_count * sizeof(struct prb_desc));
> > 	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
> > 			  descs_count * sizeof(struct printk_info));
> > 	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);
> > 
> > 
> Thank you. It may be that in my test case, the buffer was not
> extended/reallocated with a bigger one.

I guess so. A bigger buffer is allocated either when explicitly
requested by "log_buf_len=" command line option. Or when the kernel
is running on a huge system with many CPUs and log_buf_add_cpu()
decides that the default buffer is not big enough for backtraces from
all CPUs.

> > But I wonder if this is enough. The current crash dump code also needs
> > to export the format of the used structures, see
> > log_buf_vmcoreinfo_setup().
> 
> It appears that crash tool looks for the structures into vmlinux
> symbols. It can be that this information is not available to some tools,
> or vmlinux not available, in which case all the used structures format
> and sizes need to be exported. But right now, the crash tool does not
> work without vmlinux.
> > 
> > Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?
> 
> I believe CONFIG_KMEMDUMP_COREIMAGE should select CONFIG_VMCORE_INFO
> indeed, which is not done in my patches. Or I have not fully understood
> your question ?

I do not see CONFIG_VMCORE_INFO selected in drivers/debug/Kconfig.
But maybe the dependency is defined another way.

Honestly, I did not study all these details. I focused primary on
the printk-related interface and commented what came to my mind.

Also I was not sure how the dumped memory can be analyzed. I expected
that it should be readable by the "crash" tool. But I did not see it explained
in Documentation/debug/kmemdump.rst.

Best Regards,
Petr

Re: [RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Thomas Gleixner]

$Subject: ... See

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

On Tue, Apr 22 2025 at 14:31, Eugen Hristev wrote:
> Add function to register irq info into kmemdump.

What is irq info? Please explain explicitly which information is exposed
and why.

>  
> +void irq_kmemdump_register(void)
> +{
> +	kmemdump_register("irq", (void *)&nr_irqs, sizeof(nr_irqs));
> +}
> +EXPORT_SYMBOL_GPL(irq_kmemdump_register);

Are you going to slap a gazillion of those register a single variable
functions all over the place?

That's a really horrible idea.

The obvious way to deal with that is to annotate the variable:

static unsigned int nr_irqs = NR_IRQS;
KMEMDUMP_VAR(nr_irqs);

Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
kmemdump specific section and then kmemdump can just walk that section
and dump stuff. No magic register functions and no extra storage
management for static/global variables.

No?

Thanks,

        tglx

Re: [RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Eugen Hristev]

On 5/7/25 13:18, Thomas Gleixner wrote:
> 
> $Subject: ... See
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes
> 
> On Tue, Apr 22 2025 at 14:31, Eugen Hristev wrote:
>> Add function to register irq info into kmemdump.
> 
> What is irq info? Please explain explicitly which information is exposed
> and why.
> 
>>  
>> +void irq_kmemdump_register(void)
>> +{
>> +	kmemdump_register("irq", (void *)&nr_irqs, sizeof(nr_irqs));
>> +}
>> +EXPORT_SYMBOL_GPL(irq_kmemdump_register);
> 
> Are you going to slap a gazillion of those register a single variable
> functions all over the place?
> 
> That's a really horrible idea.
> 
> The obvious way to deal with that is to annotate the variable:
> 
> static unsigned int nr_irqs = NR_IRQS;
> KMEMDUMP_VAR(nr_irqs);
> 
> Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
> kmemdump specific section and then kmemdump can just walk that section
> and dump stuff. No magic register functions and no extra storage
> management for static/global variables.
> 
> No?

Thank you very much for your review ! I will try it out.

Eugen
> 
> Thanks,
> 
>         tglx

Re: [RFC][PATCH 00/14] introduce kmemdump

[Bjorn Andersson]

On Tue, Apr 22, 2025 at 02:31:42PM +0300, Eugen Hristev wrote:
> kmemdump is a mechanism which allows the kernel to mark specific memory
> areas for dumping or specific backend usage.
> Once regions are marked, kmemdump keeps an internal list with the regions
> and registers them in the backend.
> Further, depending on the backend driver, these regions can be dumped using
> firmware or different hardware block.
> Regions being marked beforehand, when the system is up and running, there
> is no need nor dependency on a panic handler, or a working kernel that can
> dump the debug information.
> The kmemdump approach works when pstore, kdump, or another mechanism do not.
> Pstore relies on persistent storage, a dedicated RAM area or flash, which
> has the disadvantage of having the memory reserved all the time, or another
> specific non volatile memory. Some devices cannot keep the RAM contents on
> reboot so ramoops does not work. Some devices do not allow kexec to run
> another kernel to debug the crashed one.
> For such devices, that have another mechanism to help debugging, like
> firmware, kmemdump is a viable solution.
> 
> kmemdump can create a core image, similar with /proc/vmcore, with only
> the registered regions included. This can be loaded into crash tool/gdb and
> analyzed.
> To have this working, specific information from the kernel is registered,
> and this is done at kmemdump init time, no need for the kmemdump user to
> do anything.
> 
> The implementation is based on the initial Pstore/directly mapped zones
> published as an RFC here:
> https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/
> 
> The back-end implementation for qcom_smem is based on the minidump
> patch series and driver written by Mukesh Ojha, thanks:
> https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/
> 
> I appreciate the feedback on this series, I know it is a longshot, and there
> is a lot to improve, but I hope I am on the right track.
> 
> Thanks,
> Eugen
> 
> PS. Here is how crash tool reports the dump:
> 
>      KERNEL: /home/eugen/linux-minidump/vmlinux  [TAINTED]
>     DUMPFILE: /home/eugen/eee

Can you please describe the steps taken to get acquire/generate this
file and how to invoke crash?

Regards,
Bjorn

>         CPUS: 8 [OFFLINE: 7]
>         DATE: Thu Jan  1 02:00:00 EET 1970
>       UPTIME: 00:00:28
>     NODENAME: qemuarm64
>      RELEASE: 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty
>      VERSION: #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>      MACHINE: aarch64  (unknown Mhz)
>       MEMORY: 0
>        PANIC: ""
> 
> crash> log
> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd4b2]
> [    0.000000] Linux version 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty (eugen@eugen-station) (aarch64-none-linux-gnu-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614, GNU ld (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 2.42.0.20240614) #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
> [    0.000000] KASLR enabled
> [...]
> 
> Eugen Hristev (14):
>   Documentation: add kmemdump
>   kmemdump: introduce kmemdump
>   kmemdump: introduce qcom-md backend driver
>   soc: qcom: smem: add minidump device
>   Documentation: kmemdump: add section for coreimage ELF
>   kmemdump: add coreimage ELF layer
>   printk: add kmsg_kmemdump_register
>   kmemdump: coreimage: add kmsg registration
>   genirq: add irq_kmemdump_register
>   kmemdump: coreimage: add irq registration
>   panic: add panic_kmemdump_register
>   kmemdump: coreimage: add panic registration
>   sched: add sched_kmemdump_register
>   kmemdump: coreimage: add sched registration
> 
>  Documentation/debug/index.rst      |  17 ++
>  Documentation/debug/kmemdump.rst   |  83 +++++
>  drivers/Kconfig                    |   2 +
>  drivers/Makefile                   |   2 +
>  drivers/debug/Kconfig              |  39 +++
>  drivers/debug/Makefile             |   5 +
>  drivers/debug/kmemdump.c           | 197 ++++++++++++
>  drivers/debug/kmemdump_coreimage.c | 293 ++++++++++++++++++
>  drivers/debug/qcom_md.c            | 467 +++++++++++++++++++++++++++++
>  drivers/soc/qcom/smem.c            |  10 +
>  include/linux/irqnr.h              |   1 +
>  include/linux/kmemdump.h           |  77 +++++
>  include/linux/kmsg_dump.h          |   6 +
>  include/linux/panic.h              |   1 +
>  include/linux/sched.h              |   1 +
>  kernel/irq/irqdesc.c               |   7 +
>  kernel/panic.c                     |   8 +
>  kernel/printk/printk.c             |  13 +
>  kernel/sched/core.c                |   7 +
>  19 files changed, 1236 insertions(+)
>  create mode 100644 Documentation/debug/index.rst
>  create mode 100644 Documentation/debug/kmemdump.rst
>  create mode 100644 drivers/debug/Kconfig
>  create mode 100644 drivers/debug/Makefile
>  create mode 100644 drivers/debug/kmemdump.c
>  create mode 100644 drivers/debug/kmemdump_coreimage.c
>  create mode 100644 drivers/debug/qcom_md.c
>  create mode 100644 include/linux/kmemdump.h
> 
> -- 
> 2.43.0
> 

Re: [RFC][PATCH 04/14] soc: qcom: smem: add minidump device

[Bjorn Andersson]

On Tue, Apr 22, 2025 at 02:31:46PM +0300, Eugen Hristev wrote:
> Add a minidump platform device.

Please provide a bit more context here.

> 
> Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
> ---
>  drivers/soc/qcom/smem.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 592819701809..985147b563f8 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -270,6 +270,7 @@ struct smem_region {
>   * @partitions: list of partitions of current processor/host
>   * @item_count: max accepted item number
>   * @socinfo:	platform device pointer
> + * @mdinfo:	minidump device pointer
>   * @num_regions: number of @regions
>   * @regions:	list of the memory regions defining the shared memory
>   */
> @@ -280,6 +281,7 @@ struct qcom_smem {
>  
>  	u32 item_count;
>  	struct platform_device *socinfo;
> +	struct platform_device *mdinfo;
>  	struct smem_ptable *ptable;
>  	struct smem_partition global_partition;
>  	struct smem_partition partitions[SMEM_HOST_COUNT];
> @@ -1236,12 +1238,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  	if (IS_ERR(smem->socinfo))
>  		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>  
> +	smem->mdinfo = platform_device_register_data(&pdev->dev, "qcom-md",

Let's spell out "minidump" here.

Regards,
Bjorn

> +						     PLATFORM_DEVID_AUTO, NULL,
> +						     0);
> +	if (IS_ERR(smem->mdinfo))
> +		dev_err(&pdev->dev, "failed to register platform md device\n");
> +
>  	return 0;
>  }
>  
>  static void qcom_smem_remove(struct platform_device *pdev)
>  {
>  	platform_device_unregister(__smem->socinfo);
> +	if (!IS_ERR(__smem->mdinfo))
> +		platform_device_unregister(__smem->mdinfo);
>  
>  	hwspin_lock_free(__smem->hwlock);
>  	__smem = NULL;
> -- 
> 2.43.0
> 

Re: [RFC][PATCH 00/14] introduce kmemdump

[Eugen Hristev]

Hello Bjorn,

On 5/7/25 19:54, Bjorn Andersson wrote:
> On Tue, Apr 22, 2025 at 02:31:42PM +0300, Eugen Hristev wrote:
>> kmemdump is a mechanism which allows the kernel to mark specific memory
>> areas for dumping or specific backend usage.
>> Once regions are marked, kmemdump keeps an internal list with the regions
>> and registers them in the backend.
>> Further, depending on the backend driver, these regions can be dumped using
>> firmware or different hardware block.
>> Regions being marked beforehand, when the system is up and running, there
>> is no need nor dependency on a panic handler, or a working kernel that can
>> dump the debug information.
>> The kmemdump approach works when pstore, kdump, or another mechanism do not.
>> Pstore relies on persistent storage, a dedicated RAM area or flash, which
>> has the disadvantage of having the memory reserved all the time, or another
>> specific non volatile memory. Some devices cannot keep the RAM contents on
>> reboot so ramoops does not work. Some devices do not allow kexec to run
>> another kernel to debug the crashed one.
>> For such devices, that have another mechanism to help debugging, like
>> firmware, kmemdump is a viable solution.
>>
>> kmemdump can create a core image, similar with /proc/vmcore, with only
>> the registered regions included. This can be loaded into crash tool/gdb and
>> analyzed.
>> To have this working, specific information from the kernel is registered,
>> and this is done at kmemdump init time, no need for the kmemdump user to
>> do anything.
>>
>> The implementation is based on the initial Pstore/directly mapped zones
>> published as an RFC here:
>> https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/
>>
>> The back-end implementation for qcom_smem is based on the minidump
>> patch series and driver written by Mukesh Ojha, thanks:
>> https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/
>>
>> I appreciate the feedback on this series, I know it is a longshot, and there
>> is a lot to improve, but I hope I am on the right track.
>>
>> Thanks,
>> Eugen
>>
>> PS. Here is how crash tool reports the dump:
>>
>>      KERNEL: /home/eugen/linux-minidump/vmlinux  [TAINTED]
>>     DUMPFILE: /home/eugen/eee
> 
> Can you please describe the steps taken to get acquire/generate this
> file and how to invoke crash?
> 

Thank you for looking into this.

Next week, on 16th of May, on Friday, there will be a talk related to
this patch series at Linaro Connect in Lisbon. In that talk I will also
show a demo in which all the process of acquiring the core dump and
crash will be covered.
I will be traveling the following days, if I get the time I will submit
the steps as a reply to this email, if not, then for sure I will submit
them after the talk in Lisbon.

Eugen

> Regards,
> Bjorn
> 
>>         CPUS: 8 [OFFLINE: 7]
>>         DATE: Thu Jan  1 02:00:00 EET 1970
>>       UPTIME: 00:00:28
>>     NODENAME: qemuarm64
>>      RELEASE: 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty
>>      VERSION: #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>>      MACHINE: aarch64  (unknown Mhz)
>>       MEMORY: 0
>>        PANIC: ""
>>
>> crash> log
>> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd4b2]
>> [    0.000000] Linux version 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty (eugen@eugen-station) (aarch64-none-linux-gnu-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614, GNU ld (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 2.42.0.20240614) #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>> [    0.000000] KASLR enabled
>> [...]
>>
>> Eugen Hristev (14):
>>   Documentation: add kmemdump
>>   kmemdump: introduce kmemdump
>>   kmemdump: introduce qcom-md backend driver
>>   soc: qcom: smem: add minidump device
>>   Documentation: kmemdump: add section for coreimage ELF
>>   kmemdump: add coreimage ELF layer
>>   printk: add kmsg_kmemdump_register
>>   kmemdump: coreimage: add kmsg registration
>>   genirq: add irq_kmemdump_register
>>   kmemdump: coreimage: add irq registration
>>   panic: add panic_kmemdump_register
>>   kmemdump: coreimage: add panic registration
>>   sched: add sched_kmemdump_register
>>   kmemdump: coreimage: add sched registration
>>
>>  Documentation/debug/index.rst      |  17 ++
>>  Documentation/debug/kmemdump.rst   |  83 +++++
>>  drivers/Kconfig                    |   2 +
>>  drivers/Makefile                   |   2 +
>>  drivers/debug/Kconfig              |  39 +++
>>  drivers/debug/Makefile             |   5 +
>>  drivers/debug/kmemdump.c           | 197 ++++++++++++
>>  drivers/debug/kmemdump_coreimage.c | 293 ++++++++++++++++++
>>  drivers/debug/qcom_md.c            | 467 +++++++++++++++++++++++++++++
>>  drivers/soc/qcom/smem.c            |  10 +
>>  include/linux/irqnr.h              |   1 +
>>  include/linux/kmemdump.h           |  77 +++++
>>  include/linux/kmsg_dump.h          |   6 +
>>  include/linux/panic.h              |   1 +
>>  include/linux/sched.h              |   1 +
>>  kernel/irq/irqdesc.c               |   7 +
>>  kernel/panic.c                     |   8 +
>>  kernel/printk/printk.c             |  13 +
>>  kernel/sched/core.c                |   7 +
>>  19 files changed, 1236 insertions(+)
>>  create mode 100644 Documentation/debug/index.rst
>>  create mode 100644 Documentation/debug/kmemdump.rst
>>  create mode 100644 drivers/debug/Kconfig
>>  create mode 100644 drivers/debug/Makefile
>>  create mode 100644 drivers/debug/kmemdump.c
>>  create mode 100644 drivers/debug/kmemdump_coreimage.c
>>  create mode 100644 drivers/debug/qcom_md.c
>>  create mode 100644 include/linux/kmemdump.h
>>
>> -- 
>> 2.43.0
>>

Re: [RFC][PATCH 01/14] Documentation: add kmemdump

[Trilok Soni]

On 4/22/2025 4:31 AM, Eugen Hristev wrote:
> Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
> ---
>  Documentation/debug/index.rst    | 17 +++++++
>  Documentation/debug/kmemdump.rst | 77 ++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>  create mode 100644 Documentation/debug/index.rst
>  create mode 100644 Documentation/debug/kmemdump.rst
> 
> diff --git a/Documentation/debug/index.rst b/Documentation/debug/index.rst
> new file mode 100644
> index 000000000000..9a9365c62f02
> --- /dev/null
> +++ b/Documentation/debug/index.rst
> @@ -0,0 +1,17 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===
> +kmemdump
> +===
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   kmemdump
> +
> +.. only::  subproject and html
> +
> +   Indices
> +   =======
> +
> +   * :ref:`genindex`
> diff --git a/Documentation/debug/kmemdump.rst b/Documentation/debug/kmemdump.rst
> new file mode 100644
> index 000000000000..dfee755a1be1
> --- /dev/null
> +++ b/Documentation/debug/kmemdump.rst
> @@ -0,0 +1,77 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==========================
> +kmemdump
> +==========================
> +
> +This document provides information about the kmemdump feature.
> +
> +Overview
> +========
> +
> +kmemdump is a mechanism that allows any driver or producer to register a
> +chunk of memory into kmemdump, to be used at a later time for a specific
> +purpose like debugging or memory dumping.
> +
> +kmemdump allows a backend to be connected, this backend interfaces a
> +specific hardware that can debug or dump the memory registered into
> +kmemdump.
> +
> +kmemdump Internals
> +=============

I feel that we are missing to explain "what you need from your firmware"
to support kmemdump on your platform. We should add that section and 
given an example on how Qualcomm does it in their firmware etc; 


-- 
---Trilok Soni

Re: [RFC][PATCH 02/14] kmemdump: introduce kmemdump

[Bjorn Andersson]

On Tue, Apr 22, 2025 at 02:31:44PM +0300, Eugen Hristev wrote:
> Kmemdump mechanism allows any driver to mark a specific memory area

I know naming is a hard problem, but "kmemdump" sounds to me like a
mechanism where the kernel does the memory dumping - and in contrast to
existing mechanisms, that's not what this does.

> for later dumping purpose, depending on the functionality
> of the attached backend. The backend would interface any hardware
> mechanism that will allow dumping to complete regardless of the
> state of the kernel (running, frozen, crashed, or any particular
> state).
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
> ---
>  drivers/Kconfig          |   2 +
>  drivers/Makefile         |   2 +
>  drivers/debug/Kconfig    |  16 ++++
>  drivers/debug/Makefile   |   3 +
>  drivers/debug/kmemdump.c | 185 +++++++++++++++++++++++++++++++++++++++
>  include/linux/kmemdump.h |  52 +++++++++++
>  6 files changed, 260 insertions(+)
>  create mode 100644 drivers/debug/Kconfig
>  create mode 100644 drivers/debug/Makefile
>  create mode 100644 drivers/debug/kmemdump.c
>  create mode 100644 include/linux/kmemdump.h
> 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 7bdad836fc62..ef56588f559e 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -245,4 +245,6 @@ source "drivers/cdx/Kconfig"
>  
>  source "drivers/dpll/Kconfig"
>  
> +source "drivers/debug/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 45d1c3e630f7..cf544a405007 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -195,3 +195,5 @@ obj-$(CONFIG_CDX_BUS)		+= cdx/
>  obj-$(CONFIG_DPLL)		+= dpll/
>  
>  obj-$(CONFIG_S390)		+= s390/
> +
> +obj-y				+= debug/
> diff --git a/drivers/debug/Kconfig b/drivers/debug/Kconfig
> new file mode 100644
> index 000000000000..22348608d187
> --- /dev/null
> +++ b/drivers/debug/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0
> +menu "Generic Driver Debug Options"
> +
> +config DRIVER_KMEMDUMP
> +	bool "Allow drivers to register memory for dumping"

You use kmemdump in non-driver code as well.

> +	help
> +	  Kmemdump mechanism allows any driver to mark a specific memory area

I think it would be better to express this as "register specific memory
areas with kmemdump for dumping" - you're not really marking any
memory...

> +	  for later dumping purpose, depending on the functionality
> +	  of the attached backend. The backend would interface any hardware
> +	  mechanism that will allow dumping to complete regardless of the
> +	  state of the kernel (running, frozen, crashed, or any particular
> +	  state).
> +
> +	  Note that modules using this feature must be rebuilt if option
> +	  changes.

While true, hopefully no human should be needed to act upon this fact?

> +endmenu
> diff --git a/drivers/debug/Makefile b/drivers/debug/Makefile
> new file mode 100644
> index 000000000000..cc14dea250e3
> --- /dev/null
> +++ b/drivers/debug/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_DRIVER_KMEMDUMP) += kmemdump.o
> diff --git a/drivers/debug/kmemdump.c b/drivers/debug/kmemdump.c
> new file mode 100644
> index 000000000000..a685c0863e25
> --- /dev/null
> +++ b/drivers/debug/kmemdump.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/kmemdump.h>
> +#include <linux/idr.h>
> +
> +#define MAX_ZONES 512

Why is this 512?

Seems this depend on the backend, in which case 512 is unlikely to be
the choosen limit.

> +
> +static struct kmemdump_backend *backend;
> +static DEFINE_IDR(kmemdump_idr);
> +static DEFINE_MUTEX(kmemdump_lock);
> +static LIST_HEAD(kmemdump_list);
> +
> +/**
> + * kmemdump_register() - Register region into kmemdump.
> + * @handle: string of maximum 8 chars that identifies this region
> + * @zone: pointer to the zone of memory
> + * @size: region size
> + *
> + * Return: On success, it returns an allocated unique id that can be used
> + *	at a later point to identify the region. On failure, it returns
> + *	negative error value.

You can say this more succinctly, something like:

Return: "unique id for the zone, or negative errno on failure"

> + */
> +int kmemdump_register(char *handle, void *zone, size_t size)
> +{
> +	struct kmemdump_zone *z = kzalloc(sizeof(*z), GFP_KERNEL);
> +	int id;
> +
> +	if (!z)
> +		return -ENOMEM;
> +
> +	mutex_lock(&kmemdump_lock);
> +
> +	id = idr_alloc_cyclic(&kmemdump_idr, z, 0, MAX_ZONES, GFP_KERNEL);
> +	if (id < 0) {
> +		mutex_unlock(&kmemdump_lock);
> +		return id;

A goto out_unlock; seems reasonable here and below.

And you're leaking 'z'

> +	}
> +
> +	if (!backend)
> +		pr_debug("kmemdump backend not available yet, waiting...\n");

"waiting" tells me that we're waiting here, but you're "deferring".

> +
> +	z->zone = zone;
> +	z->size = size;
> +	z->id = id;
> +
> +	if (handle)
> +		strscpy(z->handle, handle, 8);

Isn't the 8 optional, given that z->handle is a statically sized array?

> +
> +	if (backend) {
> +		int ret;
> +
> +		ret = backend->register_region(id, handle, zone, size);
> +		if (ret) {
> +			mutex_unlock(&kmemdump_lock);
> +			return ret;
> +		}
> +		z->registered = true;
> +	}
> +
> +	mutex_unlock(&kmemdump_lock);
> +	return id;
> +}
> +EXPORT_SYMBOL_GPL(kmemdump_register);
> +
> +/**
> + * kmemdump_unregister() - Unregister region from kmemdump.
> + * @id: unique id that was returned when this region was successfully
> + *	registered initially.
> + *
> + * Return: None
> + */
> +void kmemdump_unregister(int id)
> +{
> +	struct kmemdump_zone *z;
> +
> +	mutex_lock(&kmemdump_lock);
> +
> +	z = idr_find(&kmemdump_idr, id);
> +	if (!z)
> +		return;

You forgot to unlock &kmemdump_lock.

> +	if (z->registered && backend)
> +		backend->unregister_region(z->id);
> +
> +	idr_remove(&kmemdump_idr, id);
> +	kfree(z);
> +
> +	mutex_unlock(&kmemdump_lock);
> +}
> +EXPORT_SYMBOL_GPL(kmemdump_unregister);
> +
> +static int kmemdump_register_fn(int id, void *p, void *data)
> +{
> +	struct kmemdump_zone *z = p;
> +	int ret;
> +
> +	if (z->registered)
> +		return 0;
> +
> +	ret = backend->register_region(z->id, z->handle, z->zone, z->size);
> +	if (ret)
> +		return ret;
> +	z->registered = true;
> +
> +	return 0;
> +}
> +
> +/**
> + * kmemdump_register_backend() - Register a backend into kmemdump.
> + * Only one backend is supported at a time.
> + * @be: Pointer to a driver allocated backend. This backend must have
> + *	two callbacks for registering and deregistering a zone from the
> + *	backend.
> + *
> + * Return: On success, it returns 0, negative error value otherwise.
> + */
> +int kmemdump_register_backend(struct kmemdump_backend *be)
> +{
> +	mutex_lock(&kmemdump_lock);
> +
> +	if (backend)
> +		return -EALREADY;

unlock

> +
> +	if (!be || !be->register_region || !be->unregister_region)
> +		return -EINVAL;

unlock


Although neither one of these cases actually need to be handled under
the lock.

> +
> +	backend = be;
> +	pr_info("kmemdump backend %s registered successfully.\n",

pr_debug() is probably enough.

> +		backend->name);
> +
> +	/* Try to call the backend for all previously requested zones */
> +	idr_for_each(&kmemdump_idr, kmemdump_register_fn, NULL);
> +
> +	mutex_unlock(&kmemdump_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(kmemdump_register_backend);
> +
> +static int kmemdump_unregister_fn(int id, void *p, void *data)
> +{
> +	int ret;
> +	struct kmemdump_zone *z = p;
> +
> +	if (!z->registered)
> +		return 0;
> +
> +	ret = backend->unregister_region(z->id);
> +	if (ret)
> +		return ret;
> +	z->registered = false;
> +
> +	return 0;
> +}
> +
> +/**
> + * kmemdump_register_backend() - Unregister the backend from kmemdump.
> + * Only one backend is supported at a time.
> + * Before deregistering, this will call the backend to unregister all the
> + * previously registered zones.

These three lines seems more suitable below the argument definitions.

> + * @be: Pointer to a driver allocated backend. This backend must match
> + *	the initially registered backend.
> + *
> + * Return: None
> + */
> +void kmemdump_unregister_backend(struct kmemdump_backend *be)
> +{
> +	mutex_lock(&kmemdump_lock);
> +
> +	if (backend != be) {
> +		mutex_unlock(&kmemdump_lock);
> +		return;
> +	}
> +
> +	/* Try to call the backend for all previously requested zones */
> +	idr_for_each(&kmemdump_idr, kmemdump_unregister_fn, NULL);
> +
> +	backend = NULL;
> +	pr_info("kmemdump backend %s removed successfully.\n", be->name);

pr_debug()

> +
> +	mutex_unlock(&kmemdump_lock);
> +}
> +EXPORT_SYMBOL_GPL(kmemdump_unregister_backend);
> diff --git a/include/linux/kmemdump.h b/include/linux/kmemdump.h
> new file mode 100644
> index 000000000000..b55b15c295ac
> --- /dev/null
> +++ b/include/linux/kmemdump.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _KMEMDUMP_H
> +#define _KMEMDUMP_H
> +
> +#define KMEMDUMP_ZONE_MAX_HANDLE 8
> +/**
> + * struct kmemdump_zone - region mark zone information
> + * @id: unique id for this zone
> + * @zone: pointer to the memory area for this zone
> + * @size: size of the memory area of this zone
> + * @registered: bool indicating whether this zone is registered into the
> + *	backend or not.
> + * @handle: a string representing this region
> + */
> +struct kmemdump_zone {

It seems this is the internal-only representation of the zones and isn't
part of the API (in either direction). Better move it into the
implementation.

> +	int id;
> +	void *zone;
> +	size_t size;
> +	bool registered;
> +	char handle[KMEMDUMP_ZONE_MAX_HANDLE];
> +};
> +
> +#define KMEMDUMP_BACKEND_MAX_NAME 128
> +/**
> + * struct kmemdump_backend - region mark backend information
> + * @name: the name of the backend
> + * @register_region: callback to register region in the backend
> + * @unregister_region: callback to unregister region in the backend
> + */
> +struct kmemdump_backend {
> +	char name[KMEMDUMP_BACKEND_MAX_NAME];
> +	int (*register_region)(unsigned int id, char *, void *, size_t);
> +	int (*unregister_region)(unsigned int id);
> +};
> +
> +#ifdef CONFIG_DRIVER_KMEMDUMP
> +int kmemdump_register(char *handle, void *zone, size_t size);
> +void kmemdump_unregister(int id);
> +#else
> +static inline int kmemdump_register(char *handle, void *area, size_t size)
> +{
> +	return 0;
> +}
> +
> +static inline void kmemdump_unregister(int id)
> +{
> +}
> +#endif
> +
> +int kmemdump_register_backend(struct kmemdump_backend *backend);
> +void kmemdump_unregister_backend(struct kmemdump_backend *backend);

These two functions are defined in kmemdump.c which is only built if
CONFIG_DRIVER_KMEMDUMP=y, so shouldn't they be defined under the guard
as well?

Regards,
Bjorn

> +#endif
> -- 
> 2.43.0
> 

Re: [RFC][PATCH 03/14] kmemdump: introduce qcom-md backend driver

[Bjorn Andersson]

On Tue, Apr 22, 2025 at 02:31:45PM +0300, Eugen Hristev wrote:
> Qualcomm Minidump is a backend driver for kmemdump.
> Regions are being registered into the shared memory on Qualcomm platforms
> and into the table of contents.
> Further, the firmware can read the table of contents and dump the memory
> accordingly.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
> Co-developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/debug/Kconfig   |  13 ++
>  drivers/debug/Makefile  |   1 +
>  drivers/debug/qcom_md.c | 467 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 481 insertions(+)
>  create mode 100644 drivers/debug/qcom_md.c
> 
> diff --git a/drivers/debug/Kconfig b/drivers/debug/Kconfig
> index 22348608d187..72a906487e02 100644
> --- a/drivers/debug/Kconfig
> +++ b/drivers/debug/Kconfig
> @@ -13,4 +13,17 @@ config DRIVER_KMEMDUMP
>  
>  	  Note that modules using this feature must be rebuilt if option
>  	  changes.
> +
> +config QCOM_MD_KMEMDUMP_BACKEND

s/md/minidump/ throughout the patch.

> +	tristate "Qualcomm Minidump kmemdump backend driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on DRIVER_KMEMDUMP
> +	help
> +	  Say y here to enable the Qualcomm Minidump kmemdump backend
> +	  driver.
> +	  With this backend, the registered regions are being linked
> +	  into the minidump table of contents. Further on, the firmware
> +	  will be able to read the table of contents and extract the
> +	  memory regions on case-by-case basis.
> +
>  endmenu
> diff --git a/drivers/debug/Makefile b/drivers/debug/Makefile
> index cc14dea250e3..d8a9db29cd15 100644
> --- a/drivers/debug/Makefile
> +++ b/drivers/debug/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  obj-$(CONFIG_DRIVER_KMEMDUMP) += kmemdump.o
> +obj-$(CONFIG_QCOM_MD_KMEMDUMP_BACKEND) += qcom_md.o
> diff --git a/drivers/debug/qcom_md.c b/drivers/debug/qcom_md.c
> new file mode 100644
> index 000000000000..1aff28e18230
> --- /dev/null
> +++ b/drivers/debug/qcom_md.c
> @@ -0,0 +1,467 @@
> +// SPDX-License-Identifier: GPL-2.0-only

I think you should inherit some copyrights from
drivers/remoteproc/qcom_common.c here...

> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/socinfo.h>

What do you need socinfo.h for? What about of*.h?

> +#include <linux/kmemdump.h>
> +
> +/*
> + * In some of the Old Qualcomm devices, boot firmware statically allocates 300
> + * as total number of supported region (including all co-processors) in
> + * minidump table out of which linux was using 201. In future, this limitation
> + * from boot firmware might get removed by allocating the region dynamically.
> + * So, keep it compatible with older devices, we can keep the current limit for
> + * Linux to 201.
> + */
> +#define MAX_NUM_ENTRIES	  201

MAX_NUM_REGIONS

> +
> +#define MAX_NUM_OF_SS           10

Drop the "OF" and expand "subsystem".

> +#define MAX_REGION_NAME_LENGTH  16
> +#define SBL_MINIDUMP_SMEM_ID	602
> +#define MINIDUMP_REGION_VALID	   ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
> +#define MINIDUMP_SS_ENCR_DONE	   ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
> +#define MINIDUMP_SS_ENABLED	   ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
> +
> +#define MINIDUMP_SS_ENCR_NOTREQ	   (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
> +
> +#define MINIDUMP_APSS_DESC	   0

MINIDUMP_SUBSYSTEM_APSS

> +
> +/**
> + * struct minidump - Minidump driver data information
> + * @apss_data: APSS driver data
> + * @md_lock: Lock to protect access to APSS minidump table
> + */
> +struct minidump {

The ordering of structures and functions in this file is haphazard.
You're mixing structures that relates to memory layout with driver
context and related functions are intermixed with unrelated functions.

Effectively the flow through this driver is:
 register
 find_region
 unregister
 register,
 valid_region,
 register
 unregister, 
 init,
 register,
 unregister,
 probe,

Or I guess "flow" is the wrong word here...

> +	struct device		*dev;
> +	struct minidump_ss_data	*apss_data;
> +	struct mutex		md_lock;
> +};
> +
> +/**
> + * struct minidump_region - Minidump region
> + * @name		: Name of the region to be dumped
> + * @seq_num:		: Use to differentiate regions with same name.
> + * @valid		: This entry to be dumped (if set to 1)
> + * @address		: Physical address of region to be dumped
> + * @size		: Size of the region
> + */
> +struct minidump_region {
> +	char	name[MAX_REGION_NAME_LENGTH];
> +	__le32	seq_num;
> +	__le32	valid;
> +	__le64	address;
> +	__le64	size;
> +};
> +
> +/**
> + * struct minidump_subsystem - Subsystem's SMEM Table of content
> + * @status : Subsystem toc init status
> + * @enabled : if set to 1, this region would be copied during coredump
> + * @encryption_status: Encryption status for this subsystem
> + * @encryption_required : Decides to encrypt the subsystem regions or not
> + * @region_count : Number of regions added in this subsystem toc
> + * @regions_baseptr : regions base pointer of the subsystem
> + */
> +struct minidump_subsystem {
> +	__le32	status;
> +	__le32	enabled;
> +	__le32	encryption_status;
> +	__le32	encryption_required;
> +	__le32	region_count;
> +	__le64	regions_baseptr;
> +};
> +
> +/**
> + * struct minidump_global_toc - Global Table of Content
> + * @status : Global Minidump init status
> + * @md_revision : Minidump revision
> + * @enabled : Minidump enable status
> + * @subsystems : Array of subsystems toc
> + */
> +struct minidump_global_toc {
> +	__le32				status;
> +	__le32				md_revision;

Why is this member prefixed with md_ when the others aren't?

> +	__le32				enabled;
> +	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
> +};
> +
> +/**
> + * struct minidump_ss_data - Minidump subsystem private data
> + * @md_ss_toc: Application Subsystem TOC pointer
> + * @md_regions: Application Subsystem region base pointer
> + */
> +struct minidump_ss_data {
> +	struct minidump_subsystem *md_ss_toc;
> +	struct minidump_region	  *md_regions;

Please clean these member names up.

> +};
> +
> +#define MINIDUMP_MAX_NAME_LENGTH	12

Why is this 12 here, 16 above, and 8 in kmemdump.c?

> +/**
> + * struct qcom_minidump_region - Minidump region information
> + *
> + * @name:	Minidump region name
> + * @virt_addr:  Virtual address of the entry.
> + * @phys_addr:	Physical address of the entry to dump.
> + * @size:	Number of bytes to dump from @address location,
> + *		and it should be 4 byte aligned.
> + */
> +struct qcom_minidump_region {
> +	char		name[MINIDUMP_MAX_NAME_LENGTH];
> +	void		*virt_addr;
> +	phys_addr_t	phys_addr;
> +	size_t		size;
> +	unsigned int	id;
> +};
> +
> +static LIST_HEAD(apss_md_rlist);

Took me a while to understand that this is the "application subsystem
minidump regions list". Perhaps this could be named
"minidump_regions_list" or something like that? 

> +
> +/**
> + * struct md_region_list - Minidump region list struct
> + *
> + * @md_region:	associated minidump region
> + * @list:  list head entry
> + */
> +struct md_region_list {
> +	struct qcom_minidump_region md_region;
> +	struct list_head list;
> +};
> +
> +static struct minidump *md;
> +
> +/**
> + * qcom_md_add_region() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: None
> + */
> +static void qcom_md_add_region(const struct qcom_minidump_region *region)
> +{
> +	struct minidump_subsystem *mdss_toc = md->apss_data->md_ss_toc;
> +	struct minidump_region *mdr;
> +	unsigned int region_cnt;
> +
> +	region_cnt = le32_to_cpu(mdss_toc->region_count);
> +	mdr = &md->apss_data->md_regions[region_cnt];
> +	strscpy(mdr->name, region->name, sizeof(mdr->name));
> +	mdr->address = cpu_to_le64(region->phys_addr);
> +	mdr->size = cpu_to_le64(region->size);
> +	mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
> +	region_cnt++;
> +	mdss_toc->region_count = cpu_to_le32(region_cnt);
> +}
> +
> +/**
> + * qcom_md_get_region_index() - Lookup minidump region by name
> + * @mdss_data: minidump subsystem data
> + * @region: minidump region.
> + *
> + * Return: On success, it returns the region index, on failure, returns
> + *	negative error value
> + */
> +static int qcom_md_get_region_index(struct minidump_ss_data *mdss_data,
> +				    const struct qcom_minidump_region *region)
> +{
> +	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
> +	struct minidump_region *mdr;
> +	unsigned int i;
> +	unsigned int count;
> +
> +	count = le32_to_cpu(mdss_toc->region_count);
> +	for (i = 0; i < count; i++) {
> +		mdr = &mdss_data->md_regions[i];
> +		if (!strcmp(mdr->name, region->name))
> +			return i;
> +	}
> +
> +	return -ENOENT;
> +}
> +
> +/**
> + * qcom_md_region_unregister() - Unregister region from APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_md_region_unregister(const struct qcom_minidump_region *region)

Could you please make this API pass the backend pointer, so that backend
drivers doesn't all need to rely on a global pointer to their context.

> +{
> +	struct minidump_ss_data *mdss_data = md->apss_data;
> +	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
> +	struct minidump_region *mdr;
> +	unsigned int region_cnt;
> +	unsigned int idx;
> +	int ret;
> +
> +	ret = qcom_md_get_region_index(mdss_data, region);
> +	if (ret < 0) {
> +		dev_err(md->dev, "%s region is not present\n", region->name);
> +		return ret;
> +	}
> +
> +	idx = ret;
> +	mdr = &mdss_data->md_regions[0];
> +	region_cnt = le32_to_cpu(mdss_toc->region_count);
> +	/*
> +	 * Left shift all the regions exist after this removed region
> +	 * index by 1 to fill the gap and zero out the last region
> +	 * present at the end.
> +	 */
> +	memmove(&mdr[idx], &mdr[idx + 1], (region_cnt - idx - 1) * sizeof(*mdr));
> +	memset(&mdr[region_cnt - 1], 0, sizeof(*mdr));
> +	region_cnt--;
> +	mdss_toc->region_count = cpu_to_le32(region_cnt);
> +
> +	return 0;
> +}
> +
> +/**
> + * qcom_md_region_register() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_md_region_register(const struct qcom_minidump_region *region)
> +{
> +	struct minidump_ss_data *mdss_data = md->apss_data;
> +	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
> +	unsigned int num_region;
> +	int ret;
> +
> +	ret = qcom_md_get_region_index(mdss_data, region);
> +	if (ret >= 0) {
> +		dev_info(md->dev, "%s region is already registered\n", region->name);

struct minidump_region->seq_num tells us that you can have multiple
entries with the same name; so why are you prohibiting this here?

If I understand the code two stack frames up correctly, you have a
cyclic id already - can't you use that as the index?

> +		return -EEXIST;
> +	}
> +
> +	/* Check if there is a room for a new entry */
> +	num_region = le32_to_cpu(mdss_toc->region_count);
> +	if (num_region >= MAX_NUM_ENTRIES) {
> +		dev_err(md->dev, "maximum region limit %u reached\n", num_region);
> +		return -ENOSPC;
> +	}
> +
> +	qcom_md_add_region(region);

Wouldn't it be better to inline qcom_md_add_region() here, to avoid
checking region_count in one place and then "blindly" appending items to
the array in another place.

> +
> +	return 0;
> +}
> +
> +/**
> + * qcom_minidump_valid_region() - Checks if region is valid
> + * @region: minidump region.
> + *
> + * Return: true if region is valid, false otherwise.
> + */
> +static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region)
> +{
> +	return region &&
> +		strnlen(region->name, MINIDUMP_MAX_NAME_LENGTH) < MINIDUMP_MAX_NAME_LENGTH &&
> +			region->virt_addr &&

This indentation makes it look like region->virt_addr is an argument to
the function call on the line above.

> +			region->size &&
> +			IS_ALIGNED(region->size, 4);
> +}
> +
> +/**
> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_minidump_region_register(const struct qcom_minidump_region *region)
> +{
> +	int ret;
> +
> +	if (!qcom_minidump_valid_region(region))

How could that be, you created "region" from scratch in the calling
stack frame?

> +		return -EINVAL;
> +
> +	mutex_lock(&md->md_lock);
> +	ret = qcom_md_region_register(region);
> +
> +	mutex_unlock(&md->md_lock);
> +	return ret;
> +}
> +
> +/**
> + * qcom_minidump_region_unregister() - Unregister region from APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
> +{
> +	int ret;
> +
> +	if (!qcom_minidump_valid_region(region))
> +		return -EINVAL;
> +
> +	mutex_lock(&md->md_lock);
> +	ret = qcom_md_region_unregister(region);
> +
> +	mutex_unlock(&md->md_lock);
> +	return ret;
> +}
> +
> +/**
> + * qcom_apss_md_table_init() - Initialize the minidump table
> + * @mdss_toc: minidump subsystem table of contents
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_apss_md_table_init(struct minidump_subsystem *mdss_toc)
> +{
> +	struct minidump_ss_data *mdss_data;
> +
> +	mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);

Why is minidump_ss_data a separate object from struct minidump?

> +	if (!mdss_data)
> +		return -ENOMEM;
> +
> +	mdss_data->md_ss_toc = mdss_toc;
> +	mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
> +					     sizeof(*mdss_data->md_regions),
> +					     GFP_KERNEL);
> +	if (!mdss_data->md_regions)
> +		return -ENOMEM;
> +
> +	mdss_toc = mdss_data->md_ss_toc;
> +	mdss_toc->regions_baseptr = cpu_to_le64(virt_to_phys(mdss_data->md_regions));
> +	mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
> +	mdss_toc->status = cpu_to_le32(1);
> +	mdss_toc->region_count = cpu_to_le32(0);
> +
> +	/* Tell bootloader not to encrypt the regions of this subsystem */
> +	mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
> +	mdss_toc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
> +
> +	md->apss_data = mdss_data;
> +
> +	return 0;
> +}
> +
> +/**
> + * register_md_region() - Register a new minidump region
> + * @id: unique id to identify the region
> + * @name: name of the region
> + * @vaddr: virtual memory address of the region start
> + * @size: size of the region
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int register_md_region(unsigned int id, char *name, void *vaddr,
> +			      size_t size)
> +{
> +	struct qcom_minidump_region *md_region;
> +	int ret;
> +
> +	struct md_region_list *mdr_list =
> +		kzalloc(sizeof(*mdr_list), GFP_KERNEL);

Weird line wrapping. Use up to 100 characters if you need to... Here
you'll hit 81 though...

> +	if (!mdr_list)
> +		return -ENOMEM;
> +	md_region = &mdr_list->md_region;
> +
> +	scnprintf(md_region->name, sizeof(md_region->name), "K%d%.8s", id, name);
> +	md_region->virt_addr = vaddr;
> +	md_region->phys_addr = virt_to_phys(vaddr);
> +	md_region->size = ALIGN(size, 4);
> +	md_region->id = id;
> +
> +	ret = qcom_minidump_region_register(md_region);
> +	if (ret < 0) {
> +		pr_err("failed to register region in minidump %d %s: err: %d\n",
> +		       id, name, ret);

You already printed when you get back here with an error.

And you're leaking the mdr_list allocation.

> +		return ret;
> +	}
> +
> +	list_add(&mdr_list->list, &apss_md_rlist);
> +	return 0;
> +}
> +
> +/**
> + * unregister_md_region() - Unregister a previously registered minidump region
> + * @id: unique id to identify the region
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int unregister_md_region(unsigned int id)
> +{
> +	int ret = -ENOENT;
> +	struct md_region_list *mdr_list;
> +	struct md_region_list *tmp;
> +
> +	list_for_each_entry_safe(mdr_list, tmp, &apss_md_rlist, list) {
> +		struct qcom_minidump_region *region;
> +
> +		region = &mdr_list->md_region;
> +		if (region->id == id) {
> +			ret = qcom_minidump_region_unregister(region);

What does it mean that "ret" is non-zero here? Does it make sense to
return a value from this function? Who's going to do what with that
information?

> +			list_del(&mdr_list->list);
> +			return ret;
> +		}
> +	}
> +
> +	pr_err("failed to unregister region from minidump %d\n", ret);

You can be more specific here...
> +
> +	return ret;

That would be -ENOENT...

> +}
> +
> +static struct kmemdump_backend qcom_md_backend = {
> +	.name = "qcom_md",
> +	.register_region = register_md_region,
> +	.unregister_region = unregister_md_region,
> +};
> +
> +static int qcom_md_probe(struct platform_device *pdev)
> +{
> +	struct minidump_global_toc *mdgtoc;
> +	size_t size;
> +	int ret;
> +
> +	md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);

Don't devres allocate resources and store the result in a global
pointer. Also, you forgot to check if it failed.

> +
> +	md->dev = &pdev->dev;
> +
> +	mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
> +	if (IS_ERR(mdgtoc)) {
> +		ret = PTR_ERR(mdgtoc);
> +		dev_err(md->dev, "Couldn't find minidump smem item %d\n", ret);

return?

> +	}
> +
> +	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
> +		dev_err(md->dev, "minidump table is not initialized %d\n", ret);
> +		return -ENAVAIL;
> +	}
> +
> +	mutex_init(&md->md_lock);
> +
> +	ret = qcom_apss_md_table_init(&mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
> +	if (ret) {
> +		dev_err(md->dev, "apss minidump initialization failed %d\n", ret);

The only way qcom_apss_md_table_init() can fails is malloc(), in which
case you already got a more specific error message. So you can drop this
one.

> +		return ret;

When you return here "md" is a dangling pointer.

Regards,
Bjorn

> +	}
> +
> +	return kmemdump_register_backend(&qcom_md_backend);
> +}
> +
> +static void qcom_md_remove(struct platform_device *pdev)
> +{
> +	kmemdump_unregister_backend(&qcom_md_backend);
> +}
> +
> +static struct platform_driver qcom_md_driver = {
> +	.probe = qcom_md_probe,
> +	.remove = qcom_md_remove,
> +	.driver  = {
> +		.name = "qcom-md",
> +	},
> +};
> +
> +module_platform_driver(qcom_md_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@linaro.org>");
> +MODULE_AUTHOR("Mukesh Ojha <quic_mojha@quicinc.com>");
> +MODULE_DESCRIPTION("Qualcomm kmemdump minidump backend driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.0
> 

Re: [RFC][PATCH 00/14] introduce kmemdump

[Eugen Hristev]

On 5/9/25 18:19, Eugen Hristev wrote:
> Hello Bjorn,
> 
> On 5/7/25 19:54, Bjorn Andersson wrote:
>> On Tue, Apr 22, 2025 at 02:31:42PM +0300, Eugen Hristev wrote:
>>> kmemdump is a mechanism which allows the kernel to mark specific memory
>>> areas for dumping or specific backend usage.
>>> Once regions are marked, kmemdump keeps an internal list with the regions
>>> and registers them in the backend.
>>> Further, depending on the backend driver, these regions can be dumped using
>>> firmware or different hardware block.
>>> Regions being marked beforehand, when the system is up and running, there
>>> is no need nor dependency on a panic handler, or a working kernel that can
>>> dump the debug information.
>>> The kmemdump approach works when pstore, kdump, or another mechanism do not.
>>> Pstore relies on persistent storage, a dedicated RAM area or flash, which
>>> has the disadvantage of having the memory reserved all the time, or another
>>> specific non volatile memory. Some devices cannot keep the RAM contents on
>>> reboot so ramoops does not work. Some devices do not allow kexec to run
>>> another kernel to debug the crashed one.
>>> For such devices, that have another mechanism to help debugging, like
>>> firmware, kmemdump is a viable solution.
>>>
>>> kmemdump can create a core image, similar with /proc/vmcore, with only
>>> the registered regions included. This can be loaded into crash tool/gdb and
>>> analyzed.
>>> To have this working, specific information from the kernel is registered,
>>> and this is done at kmemdump init time, no need for the kmemdump user to
>>> do anything.
>>>
>>> The implementation is based on the initial Pstore/directly mapped zones
>>> published as an RFC here:
>>> https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/
>>>
>>> The back-end implementation for qcom_smem is based on the minidump
>>> patch series and driver written by Mukesh Ojha, thanks:
>>> https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/
>>>
>>> I appreciate the feedback on this series, I know it is a longshot, and there
>>> is a lot to improve, but I hope I am on the right track.
>>>
>>> Thanks,
>>> Eugen
>>>
>>> PS. Here is how crash tool reports the dump:
>>>
>>>      KERNEL: /home/eugen/linux-minidump/vmlinux  [TAINTED]
>>>     DUMPFILE: /home/eugen/eee
>>
>> Can you please describe the steps taken to get acquire/generate this
>> file and how to invoke crash?
>>
> 
> Thank you for looking into this.
> 
> Next week, on 16th of May, on Friday, there will be a talk related to
> this patch series at Linaro Connect in Lisbon. In that talk I will also
> show a demo in which all the process of acquiring the core dump and
> crash will be covered.
> I will be traveling the following days, if I get the time I will submit
> the steps as a reply to this email, if not, then for sure I will submit
> them after the talk in Lisbon.

Hello again,

These are steps to try out the kmemdump patches.

Once you build the kernel using the patches (you will have to change the
config to enable it, and the backend : CONFIG_DRIVER_KMEMDUMP ,
CONFIG_QCOM_MD_KMEMDUMP_BACKEND and CONFIG_DRIVER_KMEMDUMP_COREIMAGE )
Kernel firmware must be set to mode 'mini' by kernel module parameter
like this : qcom_scm.download_mode=mini

After you boot the kernel, make sure the qcom_md module is loaded and
kernel displays this message :
"kmemdump backend %s registered successfully."

After this moment the crash could occur anytime.

Once the crash happens, the firmware will kick in and you will see on
the console the message saying Sahara init, etc, that the firmware is
waiting on download mode. (this is subject to firmware supporting this
mode, I am using sa8775p-ride board)

Once the board is in download mode, you can use the qdl tool (I
personally use edl , have not tried qdl yet), to get all the regions as
separate files.
The tool from the host computer will list the regions in the order they
were downloaded.

Once you have all the files simply use `cat` to put them all together,
in the order they were dowloaded.
e.g. md_KELF.BIN , then md_Kvmcorein.BIN, etc.

Once you have the resulted file, use `crash` tool to load it, like this:
`crash --no_modules --minimal /path/to/vmlinux /path/to/core/image`

Crash has to be compiled with target=ARM64

To use crash without '--minimal' option, some minor changes are required
to crash tool, which I will submit to crash mailing list once I get more
things sorted. Meanwhile I could provide this patch if needed for
testing. (Also, there is a missing nr_swapfile variable from the kernel
which needs to be kmemdumped, and not added to this series in this version)

Please let me know how this works for you, if you experience
difficulties, I can help or expand steps .
Thanks for trying this out !


> 
> Eugen
> 
>> Regards,
>> Bjorn
>>
>>>         CPUS: 8 [OFFLINE: 7]
>>>         DATE: Thu Jan  1 02:00:00 EET 1970
>>>       UPTIME: 00:00:28
>>>     NODENAME: qemuarm64
>>>      RELEASE: 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty
>>>      VERSION: #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>>>      MACHINE: aarch64  (unknown Mhz)
>>>       MEMORY: 0
>>>        PANIC: ""
>>>
>>> crash> log
>>> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd4b2]
>>> [    0.000000] Linux version 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty (eugen@eugen-station) (aarch64-none-linux-gnu-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614, GNU ld (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 2.42.0.20240614) #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>>> [    0.000000] KASLR enabled
>>> [...]
>>>
>>> Eugen Hristev (14):
>>>   Documentation: add kmemdump
>>>   kmemdump: introduce kmemdump
>>>   kmemdump: introduce qcom-md backend driver
>>>   soc: qcom: smem: add minidump device
>>>   Documentation: kmemdump: add section for coreimage ELF
>>>   kmemdump: add coreimage ELF layer
>>>   printk: add kmsg_kmemdump_register
>>>   kmemdump: coreimage: add kmsg registration
>>>   genirq: add irq_kmemdump_register
>>>   kmemdump: coreimage: add irq registration
>>>   panic: add panic_kmemdump_register
>>>   kmemdump: coreimage: add panic registration
>>>   sched: add sched_kmemdump_register
>>>   kmemdump: coreimage: add sched registration
>>>
>>>  Documentation/debug/index.rst      |  17 ++
>>>  Documentation/debug/kmemdump.rst   |  83 +++++
>>>  drivers/Kconfig                    |   2 +
>>>  drivers/Makefile                   |   2 +
>>>  drivers/debug/Kconfig              |  39 +++
>>>  drivers/debug/Makefile             |   5 +
>>>  drivers/debug/kmemdump.c           | 197 ++++++++++++
>>>  drivers/debug/kmemdump_coreimage.c | 293 ++++++++++++++++++
>>>  drivers/debug/qcom_md.c            | 467 +++++++++++++++++++++++++++++
>>>  drivers/soc/qcom/smem.c            |  10 +
>>>  include/linux/irqnr.h              |   1 +
>>>  include/linux/kmemdump.h           |  77 +++++
>>>  include/linux/kmsg_dump.h          |   6 +
>>>  include/linux/panic.h              |   1 +
>>>  include/linux/sched.h              |   1 +
>>>  kernel/irq/irqdesc.c               |   7 +
>>>  kernel/panic.c                     |   8 +
>>>  kernel/printk/printk.c             |  13 +
>>>  kernel/sched/core.c                |   7 +
>>>  19 files changed, 1236 insertions(+)
>>>  create mode 100644 Documentation/debug/index.rst
>>>  create mode 100644 Documentation/debug/kmemdump.rst
>>>  create mode 100644 drivers/debug/Kconfig
>>>  create mode 100644 drivers/debug/Makefile
>>>  create mode 100644 drivers/debug/kmemdump.c
>>>  create mode 100644 drivers/debug/kmemdump_coreimage.c
>>>  create mode 100644 drivers/debug/qcom_md.c
>>>  create mode 100644 include/linux/kmemdump.h
>>>
>>> -- 
>>> 2.43.0
>>>
> 

Re: [RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Eugen Hristev]

On 5/7/25 13:27, Eugen Hristev wrote:
> 
> 
> On 5/7/25 13:18, Thomas Gleixner wrote:
>>
>> $Subject: ... See
>>
>> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes
>>
>> On Tue, Apr 22 2025 at 14:31, Eugen Hristev wrote:
>>> Add function to register irq info into kmemdump.
>>
>> What is irq info? Please explain explicitly which information is exposed
>> and why.
>>
>>>  
>>> +void irq_kmemdump_register(void)
>>> +{
>>> +	kmemdump_register("irq", (void *)&nr_irqs, sizeof(nr_irqs));
>>> +}
>>> +EXPORT_SYMBOL_GPL(irq_kmemdump_register);
>>
>> Are you going to slap a gazillion of those register a single variable
>> functions all over the place?
>>
>> That's a really horrible idea.
>>
>> The obvious way to deal with that is to annotate the variable:
>>
>> static unsigned int nr_irqs = NR_IRQS;
>> KMEMDUMP_VAR(nr_irqs);
>>
>> Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
>> kmemdump specific section and then kmemdump can just walk that section
>> and dump stuff. No magic register functions and no extra storage
>> management for static/global variables.
>>
>> No?
> 
> Thank you very much for your review ! I will try it out.

I have tried this way and it's much cleaner ! thanks for the suggestion.

The thing that I am trying to figure out now is how to do something
similar for a dynamically allocated memory, e.g.
void *p = kmalloc(...);
and then I can annotate `p` itself, it's address and size, but what I
would also want to so dump the whole memory region pointed out by p. and
that area address and size cannot be figured out at compile time hence I
can't instantiate a struct inside the dedicated section for it.
Any suggestion on how to make that better ? Or just keep the function
call to register the area into kmemdump ?

Thanks again,
Eugen
> 
> Eugen
>>
>> Thanks,
>>
>>         tglx
> 

Re: [RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Thomas Gleixner]

On Fri, Jun 13 2025 at 17:33, Eugen Hristev wrote:
> On 5/7/25 13:27, Eugen Hristev wrote:
>>> Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
>>> kmemdump specific section and then kmemdump can just walk that section
>>> and dump stuff. No magic register functions and no extra storage
>>> management for static/global variables.
>>>
>>> No?
>> 
>> Thank you very much for your review ! I will try it out.
>
> I have tried this way and it's much cleaner ! thanks for the
> suggestion.

Welcome.

> The thing that I am trying to figure out now is how to do something
> similar for a dynamically allocated memory, e.g.
> void *p = kmalloc(...);
> and then I can annotate `p` itself, it's address and size, but what I
> would also want to so dump the whole memory region pointed out by p. and
> that area address and size cannot be figured out at compile time hence I
> can't instantiate a struct inside the dedicated section for it.
> Any suggestion on how to make that better ? Or just keep the function
> call to register the area into kmemdump ?

Right. For dynamically allocated memory there is obviously no compile
time magic possible.

But I think you can simplify the registration for dynamically allocated
memory significantly.

struct kmemdump_entry {
	void			*ptr;
        size_t			size;
        enum kmemdump_uids	uid;
};

You use that layout for the compile time table and the runtime
registrations.

I intentionally used an UID as that avoids string allocation and all of
the related nonsense. Mapping UID to a string is a post processing
problem and really does not need to be done in the kernel. The 8
character strings are horribly limited and a simple 4 byte unique id is
achieving the same and saving space.

Just stick the IDs into include/linux/kmemdump_ids.h and expose the
content for the post processing machinery.

So you want KMEMDUMP_VAR() for the compile time created table to either
automatically create that ID derived from the variable name or you add
an extra argument with the ID.

kmemdump_init()
        // Use a simple fixed size array to manage this
        // as it avoids all the memory allocation nonsense
        // This stuff is neither performance critical nor does allocating
        // a few hundred entries create a memory consumption problem
        // It consumes probably way less memory than the whole IDR/XARRAY allocation
        // string duplication logic consumes text and data space.
	kmemdump_entries = kcalloc(NR_ENTRIES, sizeof(*kmemdump_entries), GFP_KERNEL);

kmemdump_register(void *ptr, size_t size, enum kmemdump_uids uid)
{
        guard(entry_mutex);

	entry = kmemdump_find_empty_slot();
        if (!entry)
        	return;

        entry->ptr = ptr;
        entry->size = size;
        entry->uid = uid;

        // Make this unconditional by providing a dummy backend
        // implementation. If the backend changes re-register all
        // entries with the new backend and be done with it.
        backend->register(entry);
}

kmemdump_unregister(void *ptr)
{
        guard(entry_mutex);
        entry = find_entry(ptr);
        if (entry) {
                backend->unregister(entry);
        	memset(entry, 0, sizeof(*entry);
        }
}

You get the idea.

Coming back to the registration at the call site itself.

       struct foo = kmalloc(....);

       if (!foo)
       		return;

       kmemdump_register(foo, sizeof(*foo), KMEMDUMP_ID_FOO);

That's a code duplication shitshow. You can wrap that into:

       struct foo *foo = kmemdump_alloc(foo, KMEMDUMP_ID_FOO, kmalloc, ...);

#define kmemdump_alloc(var, id, fn, ...)				\
	({								\
        	void *__p = fn(##__VA_ARGS__);				\
									\
                if (__p)						\
                	kmemdump_register(__p, sizeof(*var), id);	\
		__p;
        })

or something daft like that. And provide the matching magic for the free
side.

Thoughts?

Thanks,

        tglx

Re: [RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Eugen Hristev]

On 6/14/25 00:10, Thomas Gleixner wrote:
> On Fri, Jun 13 2025 at 17:33, Eugen Hristev wrote:
>> On 5/7/25 13:27, Eugen Hristev wrote:
>>>> Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
>>>> kmemdump specific section and then kmemdump can just walk that section
>>>> and dump stuff. No magic register functions and no extra storage
>>>> management for static/global variables.
>>>>
>>>> No?
>>>
>>> Thank you very much for your review ! I will try it out.
>>
>> I have tried this way and it's much cleaner ! thanks for the
>> suggestion.
> 
> Welcome.
> 
>> The thing that I am trying to figure out now is how to do something
>> similar for a dynamically allocated memory, e.g.
>> void *p = kmalloc(...);
>> and then I can annotate `p` itself, it's address and size, but what I
>> would also want to so dump the whole memory region pointed out by p. and
>> that area address and size cannot be figured out at compile time hence I
>> can't instantiate a struct inside the dedicated section for it.
>> Any suggestion on how to make that better ? Or just keep the function
>> call to register the area into kmemdump ?
> 
> Right. For dynamically allocated memory there is obviously no compile
> time magic possible.
> 
> But I think you can simplify the registration for dynamically allocated
> memory significantly.
> 
> struct kmemdump_entry {
> 	void			*ptr;
>         size_t			size;
>         enum kmemdump_uids	uid;
> };
> 
> You use that layout for the compile time table and the runtime
> registrations.
> 
> I intentionally used an UID as that avoids string allocation and all of
> the related nonsense. Mapping UID to a string is a post processing
> problem and really does not need to be done in the kernel. The 8
> character strings are horribly limited and a simple 4 byte unique id is
> achieving the same and saving space.
> 
> Just stick the IDs into include/linux/kmemdump_ids.h and expose the
> content for the post processing machinery.
> 
> So you want KMEMDUMP_VAR() for the compile time created table to either
> automatically create that ID derived from the variable name or you add
> an extra argument with the ID.

First of all, thank you very much for taking the time to think about this !

In KMEMDUMP_VAR, I can use __UNIQUE_ID to derive something unique from
the variable name for the table entry.

The only problem with having something like

#define KMEMDUMP_VAR(sym) \
	 static struct entry __UNIQUE_ID(kmemdump_entry_##sym) ...

is when calling it with e.g. `init_mm.pgd` which will make the `.`
inside the name and that can't happen.
So I have to figure a way to remove unwanted chars or pass a name to the
macro.

I cannot do something like
static  void * ptr = &init_mm.pgd;
and then
KMEMDUMP_VAR(ptr)
because ptr's dereferencing can't happen at compile time to add it's
value into the table entry.

> 
> kmemdump_init()
>         // Use a simple fixed size array to manage this
>         // as it avoids all the memory allocation nonsense
>         // This stuff is neither performance critical nor does allocating
>         // a few hundred entries create a memory consumption problem
>         // It consumes probably way less memory than the whole IDR/XARRAY allocation
>         // string duplication logic consumes text and data space.
> 	kmemdump_entries = kcalloc(NR_ENTRIES, sizeof(*kmemdump_entries), GFP_KERNEL);
> 
> kmemdump_register(void *ptr, size_t size, enum kmemdump_uids uid)
> {
>         guard(entry_mutex);
> 
> 	entry = kmemdump_find_empty_slot();
>         if (!entry)
>         	return;
> 
>         entry->ptr = ptr;
>         entry->size = size;
>         entry->uid = uid;
> 
>         // Make this unconditional by providing a dummy backend
>         // implementation. If the backend changes re-register all
>         // entries with the new backend and be done with it.
>         backend->register(entry);
> }
> 
> kmemdump_unregister(void *ptr)
> {
>         guard(entry_mutex);
>         entry = find_entry(ptr);
>         if (entry) {
>                 backend->unregister(entry);
>         	memset(entry, 0, sizeof(*entry);
>         }
> }
> 
> You get the idea.
> 
> Coming back to the registration at the call site itself.
> 
>        struct foo = kmalloc(....);
> 
>        if (!foo)
>        		return;
> 
>        kmemdump_register(foo, sizeof(*foo), KMEMDUMP_ID_FOO);
> 
> That's a code duplication shitshow. You can wrap that into:
> 
>        struct foo *foo = kmemdump_alloc(foo, KMEMDUMP_ID_FOO, kmalloc, ...);
> 
> #define kmemdump_alloc(var, id, fn, ...)				\
> 	({								\
>         	void *__p = fn(##__VA_ARGS__);				\
> 									\
>                 if (__p)						\
>                 	kmemdump_register(__p, sizeof(*var), id);	\
> 		__p;
>         })
> 

I was thinking into a new variant of kmalloc, like e.g. kdmalloc() which
would be a wrapper over kmalloc and also register the region into
kmemdump like you are suggesting.
It would be like a `dumpable` kmalloc'ed memory.
And it could take an optional ID , if missing, it could generate one.

However this would mean yet another k*malloc friend, and it would
default to usual kmalloc if CONFIG_KMEMDUMP=n .
I am unsure whether this would be welcome by the community

Let me know what you think.

Thanks again !
Eugen

> or something daft like that. And provide the matching magic for the free
> side.
> 
> Thoughts?
> 
> Thanks,
> 
>         tglx

Re: [RFC][PATCH 09/14] genirq: add irq_kmemdump_register

[Thomas Gleixner]

On Mon, Jun 16 2025 at 13:12, Eugen Hristev wrote:
> On 6/14/25 00:10, Thomas Gleixner wrote:
> #define KMEMDUMP_VAR(sym) \
> 	 static struct entry __UNIQUE_ID(kmemdump_entry_##sym) ...
>
> is when calling it with e.g. `init_mm.pgd` which will make the `.`
> inside the name and that can't happen.
> So I have to figure a way to remove unwanted chars or pass a name to the
> macro.

You can do:

KMEMDUMP_VAR_MEMBER(init_mm, pgd);

and concatenate with a '.' for the symbol
and a '_' for the ID.

or simply

KMEMDUMP_VAR_ID(init_mm.pgg, INIT_MM_PGD);

>> #define kmemdump_alloc(var, id, fn, ...)				\
>> 	({								\
>>         	void *__p = fn(##__VA_ARGS__);				\
>> 									\
>>                 if (__p)						\
>>                 	kmemdump_register(__p, sizeof(*var), id);	\
>> 		__p;
>>         })
>> 
>
> I was thinking into a new variant of kmalloc, like e.g. kdmalloc() which
> would be a wrapper over kmalloc and also register the region into
> kmemdump like you are suggesting.
> It would be like a `dumpable` kmalloc'ed memory.
> And it could take an optional ID , if missing, it could generate one.
>
> However this would mean yet another k*malloc friend, and it would
> default to usual kmalloc if CONFIG_KMEMDUMP=n .
> I am unsure whether this would be welcome by the community

That's definitely more welcome than copying boilerplate code all over
the place. And if you do it the way I suggested, then you only have
_one_ macro for alloc and _one_ for free because you hand in the
allocation function with all of its arguments instead of creating an
ever increasing zoo of dedicated variants. But the MM people might have
different opinions.

Thanks,

        tglx