Re: [PATCH v2 1/2] cmd: Add command to display or save Linux PStore dumps

2020-03-17 Thread Heinrich Schuchardt

On 2/26/20 10:42 AM, Frédéric Danis wrote:

This patch adds a new pstore command allowing to display or save ramoops
logs (oops, panic, console, ftrace and user) generated by a previous
kernel crash.
PStore parameters can be set in U-Boot configuration file, or at run-time
using "pstore set" command. Records size should be the same as the ones
used by kernel, and should be a power of 2.
This command allows:
- to display uncompressed logs
- to save compressed or uncompressed logs, compressed logs are saved as a
   compressed stream, it may need some work to be able to decompress it,
   e.g. adding a fake header:
   "printf "\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x00" |
   cat - dmesg-ramoops-0.enc.z | gzip -dc"
- ECC part is not used to check memory corruption
- only 1st FTrace log is displayed or saved

Signed-off-by: Frédéric Danis 
---
  cmd/Kconfig|  63 ++
  cmd/Makefile   |   1 +
  cmd/pstore.c   | 505 +
  doc/index.rst  |   7 +
  doc/pstore.rst |  68 +++
  5 files changed, 644 insertions(+)
  create mode 100644 cmd/pstore.c
  create mode 100644 doc/pstore.rst

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 6403bc45a5..7e78343c3d 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1736,6 +1736,69 @@ config CMD_QFW
  feature is to allow easy loading of files passed to qemu-system
  via -kernel / -initrd

+config CMD_PSTORE
+   bool "pstore"
+   help
+ This provides access to Linux PStore. The main feature is to allow to
+ display or save PStore records.


Linux PStore can have many backends. I would suggest to mention
'rampoops' explicitely and point to the documentation that you have added.


+


Please, use 'if CMD_PSTORE' to indent all options depending on CMD_PSTORE'.


+config CMD_PSTORE_ADDR
+   hex "Mem Address"


Please, use whole words.

hex "Memory address"


+   depends on CMD_PSTORE
+   default "0x0"
+   help
+ Base addr used for PStore ramoops memory, should be identical to
+ ramoops.mem_address parameter used by kernel
+
+config CMD_PSTORE_SIZE


Linux calls the parameter mem_size. So shouldn't we call our parameter
CMD_PSTORE_MEM_SIZE.


+   hex "Mem size"


hex "Memory size"


+   depends on CMD_PSTORE
+   default "0x0"


A value of zero will lead to an error in Linux "The memory size and the
record/console size must be non-zero".

Please, provide a reasonable default.


+   help
+ Size of PStore ramoops memory, should be identical to ramoops.mem_size
+ parameter used by kernel


Please, describe the constraints on ramoops.mem_size. Does it have to be
larger than the some of the following parameters?


+
+config CMD_PSTORE_RECORD_SIZE
+   hex "Dump record size"
+   depends on CMD_PSTORE
+   default "0x1000"
+   help
+ Size of each dump done on oops/panic, should be identical to
+ ramoops.record_size parameter used by kernel
+
+config CMD_PSTORE_CONSOLE_SIZE
+   hex "Kernel console log size"
+   depends on CMD_PSTORE
+   default "0x1000"
+   help
+ Size of kernel console log, should be identical to
+ ramoops.console_size parameter used by kernel


Please, mention that this value must be non-zero.


+
+config CMD_PSTORE_FTRACE_SIZE
+   hex "FTrace log size"
+   depends on CMD_PSTORE
+   default "0x1000"
+   help
+ Size of ftrace log, should be identical to ramoops.ftrace_size
+ parameter used by kernel
+
+config CMD_PSTORE_PMSG_SIZE
+   hex "User space message log size"
+   depends on CMD_PSTORE
+   default "0x1000"
+   help
+ Size of user space message log, should be identical to
+ ramoops.pmsg_size parameter used by kernel
+
+config CMD_PSTORE_ECC_SIZE
+   int "ECC size"
+   depends on CMD_PSTORE
+   default "0"
+   help
+   if non-zero, the option enables ECC support and specifies ECC buffer
+   size in bytes (1 is a special value, means 16 bytes ECC), should be
+   identical to ramoops.ramoops_ecc parameter used by kernel
+
  source "cmd/mvebu/Kconfig"

  config CMD_TERMINAL
diff --git a/cmd/Makefile b/cmd/Makefile
index f1dd513a4b..06d7ad7375 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_CMD_PCI) += pci.o
  endif
  obj-$(CONFIG_CMD_PINMUX) += pinmux.o
  obj-$(CONFIG_CMD_PMC) += pmc.o
+obj-$(CONFIG_CMD_PSTORE) += pstore.o
  obj-$(CONFIG_CMD_PXE) += pxe.o pxe_utils.o
  obj-$(CONFIG_CMD_WOL) += wol.o
  obj-$(CONFIG_CMD_QFW) += qfw.o
diff --git a/cmd/pstore.c b/cmd/pstore.c
new file mode 100644
index 00..a14b8522ce
--- /dev/null
+++ b/cmd/pstore.c
@@ -0,0 +1,505 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright © 2019 Collabora Ltd
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct persistent_ram_buffer {
+   u32sig;
+   u32start;
+   u32size;
+   u8 data[0];
+};
+
+#defi

Re: [PATCH v2 1/2] cmd: Add command to display or save Linux PStore dumps

2020-03-18 Thread Frédéric Danis

Hi Heinrich,

On 17/03/2020 20:57, Heinrich Schuchardt wrote:

On 2/26/20 10:42 AM, Frédéric Danis wrote:


+Ramoops is an oops/panic logger that writes its logs to RAM before 
the system
+crashes. It works by logging oopses and panics in a circular buffer. 
Ramoops
+needs a system with persistent RAM so that the content of that area 
can survive

+after a restart.
+
+Ramoops uses a predefined memory area to store the dump.
+
+Ramoops parameters can be passed as kernel parameters or through 
Device Tree,


Please, add the node in image_setup_libfdt() as described in Linux's
Documentation/device-tree/bindings/reserved-memory/admin-guide/ramoops.rst. 



I'm not sure to understand what you expect here.
Can you please give me more info about it?



+i.e.::
+ ramoops.mem_address=0x3000 ramoops.mem_size=0x10 
ramoops.record_size=0x2000 ramoops.console_size=0x2000 
memmap=0x10$0x3000


Using the command line seems to be rather error prone.


Best regards,

Frédéric Danis