| Message ID | 20250422113156.575971-1-eugen.hristev@linaro.org |
|---|---|
| Headers | show |
| Series | introduce kmemdump | expand |
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.
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
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
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
$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
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
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 >
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 >>
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;
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 >
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 >
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
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
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