[RFC,02/14] kmemdump: introduce kmemdump

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

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
>