Hi Conor,

On Fri, Oct 14, 2022 at 09:41:38PM +0800, Xianting Tian wrote:

Add arch_crash_save_vmcoreinfo(), which exports VM layout(MODULES, VMALLOC,
VMEMMAP and KERNEL_LINK_ADDR ranges), va bits and ram base for vmcore.

Default pagetable levels and PAGE_OFFSET aren't same for different kernel
version as below. For default pagetable levels, it sets sv57 on defaultly
in latest kernel and do fallback to try to set sv48 on boot time if sv57
is not supported in current hardware.

nit: This would read better as "it sets sv57 by default and falls back
to setting sv48 at boot time if sv57 is not supported by the hardware".

For ram base, the default value is 0x80200000 for qemu riscv64 env, 0x200000
for riscv64 SoC platform(eg, SoC platform of RISC-V XuanTie 910 CPU).

The second part of this sentence I'm not really sure that that is true,
I think you should reword it to something like "...for qemu riscv64 and,
for example, is 0x200000 on the XuanTie 910 CPU." and avoid applying
that number blanketly for other SoCs.

 * Linux Kernel 5.18 ~
 *      PGTABLE_LEVELS = 5
 *      PAGE_OFFSET = 0xff60000000000000
 * Linux Kernel 5.17 ~
 *      PGTABLE_LEVELS = 4
 *      PAGE_OFFSET = 0xffffaf8000000000
 * Linux Kernel 4.19 ~
 *      PGTABLE_LEVELS = 3
 *      PAGE_OFFSET = 0xffffffe000000000

Since these configurations change from time to time and version to version,
it is preferable to export them via vmcoreinfo than to change the crash's
code frequently, it can simplify the development of crash tool.

Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
 arch/riscv/kernel/Makefile     |  1 +
 arch/riscv/kernel/crash_core.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 arch/riscv/kernel/crash_core.c

diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index db6e4b1294ba..4cf303a779ab 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_KGDB)		+= kgdb.o
 obj-$(CONFIG_KEXEC_CORE)	+= kexec_relocate.o crash_save_regs.o machine_kexec.o
 obj-$(CONFIG_KEXEC_FILE)	+= elf_kexec.o machine_kexec_file.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump.o
+obj-$(CONFIG_CRASH_CORE)	+= crash_core.o
 
 obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o
 
diff --git a/arch/riscv/kernel/crash_core.c b/arch/riscv/kernel/crash_core.c
new file mode 100644
index 000000000000..8d7f5ff108da
--- /dev/null
+++ b/arch/riscv/kernel/crash_core.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/crash_core.h>
+#include <linux/pagemap.h>
+
+void arch_crash_save_vmcoreinfo(void)
+{
+	VMCOREINFO_NUMBER(VA_BITS);
+	VMCOREINFO_NUMBER(phys_ram_base);
+
+	vmcoreinfo_append_str("NUMBER(PAGE_OFFSET)=0x%lx\n", PAGE_OFFSET);
+	vmcoreinfo_append_str("NUMBER(VMALLOC_START)=0x%lx\n", VMALLOC_START);
+	vmcoreinfo_append_str("NUMBER(VMALLOC_END)=0x%lx\n", VMALLOC_END);
+	vmcoreinfo_append_str("NUMBER(VMEMMAP_START)=0x%lx\n", VMEMMAP_START);
+	vmcoreinfo_append_str("NUMBER(VMEMMAP_END)=0x%lx\n", VMEMMAP_END);
+#ifdef CONFIG_64BIT
+	vmcoreinfo_append_str("NUMBER(MODULES_VADDR)=0x%lx\n", MODULES_VADDR);
+	vmcoreinfo_append_str("NUMBER(MODULES_END)=0x%lx\n", MODULES_END);
+#endif
+
+	if (IS_ENABLED(CONFIG_64BIT)) {

You've already got a #ifdef CONFIG_64BIT above, is there a reason why
you'd use the IS_ENABLED here rather than merge this with the above
section? I'm a big fan of IS_ENABLED but I'm not sure what it adds here,
maybe you can show me the light :)

The IS_ENABLED() check prevents the line from getting executed, but
unlike an #ifdef it relies on it to be parsable.

I wrote this arch_crash_save_vmcoreinfo() func with reference to this:
static void __init print_vm_layout(void) // arch/riscv/mm/init.c
{
        pr_notice("Virtual kernel memory layout:\n");
        print_ml("fixmap", (unsigned long)FIXADDR_START,
                (unsigned long)FIXADDR_TOP);
        print_ml("pci io", (unsigned long)PCI_IO_START,
                (unsigned long)PCI_IO_END);
        print_ml("vmemmap", (unsigned long)VMEMMAP_START,
                (unsigned long)VMEMMAP_END);
        print_ml("vmalloc", (unsigned long)VMALLOC_START,
                (unsigned long)VMALLOC_END);
#ifdef CONFIG_64BIT
        print_ml("modules", (unsigned long)MODULES_VADDR,
                (unsigned long)MODULES_END);
#endif
        print_ml("lowmem", (unsigned long)PAGE_OFFSET,
                (unsigned long)high_memory);
        if (IS_ENABLED(CONFIG_64BIT)) {
#ifdef CONFIG_KASAN
                print_ml("kasan", KASAN_SHADOW_START, KASAN_SHADOW_END);
#endif

                print_ml("kernel", (unsigned long)KERNEL_LINK_ADDR,
                         (unsigned long)ADDRESS_SPACE_END);
        }
}

Thanks,
Conor.

+#ifdef CONFIG_KASAN
+		vmcoreinfo_append_str("NUMBER(KASAN_SHADOW_START)=0x%lx\n", KASAN_SHADOW_START);
+		vmcoreinfo_append_str("NUMBER(KASAN_SHADOW_END)=0x%lx\n", KASAN_SHADOW_END);
+#endif
+		vmcoreinfo_append_str("NUMBER(KERNEL_LINK_ADDR)=0x%lx\n", KERNEL_LINK_ADDR);
+		vmcoreinfo_append_str("NUMBER(ADDRESS_SPACE_END)=0x%lx\n", ADDRESS_SPACE_END);
+	}
+}
-- 
2.17.1