<div dir="ltr"><div dir="ltr">On Sat, Oct 14, 2023 at 9:39 PM <<a href="mailto:crash-utility-request@redhat.com">crash-utility-request@redhat.com</a>> wrote: </div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Date: Sat, 14 Oct 2023 19:09:30 +0530 From: Aditya Gupta <<a href="mailto:adityag@linux.ibm.com" target="_blank">adityag@linux.ibm.com</a>> To: <a href="mailto:crash-utility@redhat.com" target="_blank">crash-utility@redhat.com</a>, <<a href="mailto:lijiang@redhat.com" target="_blank">lijiang@redhat.com</a>> Cc: Hari Bathini <<a href="mailto:hbathini@linux.ibm.com" target="_blank">hbathini@linux.ibm.com</a>>, Mahesh J Salgaonkar <<a href="mailto:mahesh@linux.ibm.com" target="_blank">mahesh@linux.ibm.com</a>>, Sourabh Jain <<a href="mailto:sourabhjain@linux.ibm.com" target="_blank">sourabhjain@linux.ibm.com</a>>, <a href="mailto:d.hatayama@fujitsu.com" target="_blank">d.hatayama@fujitsu.com</a> Subject: [Crash-utility] [PATCH RESEND v2] diskdump: add hook for additional checks on prstatus notes validity Message-ID: <<a href="mailto:20231014133930.147343-1-adityag@linux.ibm.com" target="_blank">20231014133930.147343-1-adityag@linux.ibm.com</a>> Content-Type: text/plain; charset="US-ASCII"; x-default=true Upstream crash reports these warnings on PowerPC64: WARNING: cpu 0 invalid NT_PRSTATUS note (n_type != NT_PRSTATUS) ... Apart from these warnings, register values are also invalid. This warning was found in the commit: commit db8c030857b4 ("diskdump/netdump: fix segmentation fault caused by failure of stopping CPUs") With above commit, crash checks whether 'crash_notes' is initialised, before mapping PRSTATUS notes. But some architectures such as PowerPC64, in fadump case (firmware-assisted dump), don't populate 'crash_notes' since the registers are already stored in the cpu notes in the vmcore. Instead of checking 'crash_notes' for all architectures, introduce a machdep hook ('is_cpu_prstatus_valid'), for architectures to decide validity checks for PRSTATUS notes A default hook ('diskdump_is_cpu_prstatus_valid') has also been provided for all architectures other than PowerPC64, which checks if 'crash_notes' for a given cpu is valid, maintaining the current behaviour PowerPC64 doesn't utilise 'crash_notes' to get register values, so no additional checks are required Fixes: db8c030857b4 ("diskdump/netdump: fix segmentation fault caused by failure of stopping CPUs") Signed-off-by: Aditya Gupta <<a href="mailto:adityag@linux.ibm.com" target="_blank">adityag@linux.ibm.com</a>> --- Testing ======= NOTE: To test this on PowerPC64 with upstream kernel dump, AND on system with Radix MMU, following patch will also be needed to be applied: Link: <a href="https://listman.redhat.com/archives/crash-utility/2023-September/010961.html" rel="noreferrer" target="_blank">https://listman.redhat.com/archives/crash-utility/2023-September/010961.html</a> This is due to change in vmemmap address mapping for Radix MMU, since following patch in the kernel: 368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a different vmemmap handling function) More details about the change are in the linked patch. Basically what changed is, the address mapping for vmemmap address is now in kernel page table, in case of Radix MMU, instead of 'vmemmap_list' which is currently used in crash. Git Tree for Testing ==================== 1. With this patch (diskdump: add hook for additional ...) applied: <a href="https://github.com/adi-g15-ibm/crash/tree/bugzilla-203256-list-v2" rel="noreferrer" target="_blank">https://github.com/adi-g15-ibm/crash/tree/bugzilla-203256-list-v2</a> 2. With both this and the linked patch (ppc64: do page traversal ...) applied: <a href="https://github.com/adi-g15-ibm/crash/tree/bugzilla-203256-withupstreamradix" rel="noreferrer" target="_blank">https://github.com/adi-g15-ibm/crash/tree/bugzilla-203256-withupstreamradix</a> Changelog ========= V2 + added fix for netdump also, same as diskdump, as that was also modified by db8c030857b4 + fixed compile warnings </blockquote><div> </div><div>Thank you for the update, Aditya.</div><div> </div><div>For the v2: Ack.</div><div> </div><div>Thanks</div><div>Lianbo</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> --- --- defs.h | 2 ++ diskdump.c | 15 ++++++++++++--- netdump.c | 5 ++--- ppc64.c | 10 ++++++++++ 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/defs.h b/defs.h index 96a7a2a31471..23dee48759fb 100644 --- a/defs.h +++ b/defs.h @@ -1075,6 +1075,7 @@ struct machdep_table { void (*show_interrupts)(int, ulong *); int (*is_page_ptr)(ulong, physaddr_t *); int (*get_cpu_reg)(int, int, const char *, int, void *); + int (*is_cpu_prstatus_valid)(int cpu); }; /* @@ -7181,6 +7182,7 @@ int dumpfile_is_split(void); void show_split_dumpfiles(void); void x86_process_elf_notes(void *, unsigned long); void *diskdump_get_prstatus_percpu(int); +int diskdump_is_cpu_prstatus_valid(int cpu); int have_crash_notes(int cpu); void map_cpus_to_prstatus_kdump_cmprs(void); void diskdump_display_regs(int, FILE *); diff --git a/diskdump.c b/diskdump.c index 2c284ff3f97f..ad9a00b08ce1 100644 --- a/diskdump.c +++ b/diskdump.c @@ -142,13 +142,22 @@ int have_crash_notes(int cpu) return TRUE; } +int diskdump_is_cpu_prstatus_valid(int cpu) +{ + static int crash_notes_exists = -1; + + if (crash_notes_exists == -1) + crash_notes_exists = kernel_symbol_exists("crash_notes"); + + return (!crash_notes_exists || have_crash_notes(cpu)); +} + void map_cpus_to_prstatus_kdump_cmprs(void) { void **nt_ptr; int online, i, j, nrcpus; size_t size; - int crash_notes_exists; if (pc->flags2 & QEMU_MEM_DUMP_COMPRESSED) /* notes exist for all cpus */ goto resize_note_pointers; @@ -171,10 +180,9 @@ map_cpus_to_prstatus_kdump_cmprs(void) * Re-populate the array with the notes mapping to online cpus */ nrcpus = (kt->kernel_NR_CPUS ? kt->kernel_NR_CPUS : NR_CPUS); - crash_notes_exists = kernel_symbol_exists("crash_notes"); for (i = 0, j = 0; i < nrcpus; i++) { - if (in_cpu_map(ONLINE_MAP, i) && (!crash_notes_exists || have_crash_notes(i))) { + if (in_cpu_map(ONLINE_MAP, i) && machdep->is_cpu_prstatus_valid(i)) { dd->nt_prstatus_percpu[i] = nt_ptr[j++]; dd->num_prstatus_notes = MAX(dd->num_prstatus_notes, i+1); @@ -1076,6 +1084,7 @@ diskdump_init(char *unused, FILE *fptr) if (!DISKDUMP_VALID() && !KDUMP_CMPRS_VALID()) return FALSE; + machdep->is_cpu_prstatus_valid = diskdump_is_cpu_prstatus_valid; dd->ofp = fptr; return TRUE; } diff --git a/netdump.c b/netdump.c index 61ddeaa08831..390786364959 100644 --- a/netdump.c +++ b/netdump.c @@ -75,7 +75,6 @@ map_cpus_to_prstatus(void) void **nt_ptr; int online, i, j, nrcpus; size_t size; - int crash_notes_exists; if (pc->flags2 & QEMU_MEM_DUMP_ELF) /* notes exist for all cpus */ return; @@ -98,10 +97,9 @@ map_cpus_to_prstatus(void) * Re-populate the array with the notes mapping to online cpus */ nrcpus = (kt->kernel_NR_CPUS ? kt->kernel_NR_CPUS : NR_CPUS); - crash_notes_exists = kernel_symbol_exists("crash_notes"); for (i = 0, j = 0; i < nrcpus; i++) { - if (in_cpu_map(ONLINE_MAP, i) && (!crash_notes_exists || have_crash_notes(i))) { + if (in_cpu_map(ONLINE_MAP, i) && machdep->is_cpu_prstatus_valid(i)) { nd->nt_prstatus_percpu[i] = nt_ptr[j++]; nd->num_prstatus_notes = MAX(nd->num_prstatus_notes, i+1); @@ -735,6 +733,7 @@ netdump_init(char *unused, FILE *fptr) if (!VMCORE_VALID()) return FALSE; + machdep->is_cpu_prstatus_valid = diskdump_is_cpu_prstatus_valid; nd->ofp = fptr; check_dumpfile_size(pc->dumpfile); diff --git a/ppc64.c b/ppc64.c index fc34006f4863..5a8ef9e58173 100644 --- a/ppc64.c +++ b/ppc64.c @@ -298,6 +298,15 @@ struct machine_specific book3e_machine_specific = { .is_vmaddr = book3e_is_vmaddr, }; +/** + * No additional checks are required on PPC64, for checking if PRSTATUS notes + * is valid + */ +static int ppc64_is_cpu_prstatus_valid(int cpu) +{ + return TRUE; +} + #define SKIBOOT_BASE 0x30000000 /* @@ -400,6 +409,7 @@ ppc64_init(int when) machdep->value_to_symbol = generic_machdep_value_to_symbol; machdep->get_kvaddr_ranges = ppc64_get_kvaddr_ranges; machdep->init_kernel_pgd = NULL; + machdep->is_cpu_prstatus_valid = ppc64_is_cpu_prstatus_valid; if (symbol_exists("vmemmap_populate")) { if (symbol_exists("vmemmap")) { -- 2.41.0 </blockquote></div></div>