[Crash-utility] Re: [PATCH v1 1/3] ppc64: Fix bt printing error stack trace

Hi Aditya & lianbo, On Tue, Sep 24, 2024 at 2:28 AM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: > Hi Tao, > > On 24/09/22 10:25PM, Aditya Gupta wrote: >> Hi Tao, >> >> >> On 18/09/24 05:12, Tao Liu wrote: >>> A error stack trace of bt cmd observed: >>> >>> crash> bt 1 >>> PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" >>> #0 [c0000000037735c0] _end at c0000000037154b0 (unreliable) >>> #1 [c000000003773770] __switch_to at c00000000001fa9c >>> #2 [c0000000037737d0] __schedule at c00000000112e4ec >>> #3 [c0000000037738b0] schedule at c00000000112ea80 >>> ... >>> >>> The #0 stack trace is incorrect, the function address shouldn't exceed _end. >>> The reason is for kernel>=v6.2, the offset of pt_regs to sp changed from >>> STACK_FRAME_OVERHEAD, i.e 112, to STACK_SWITCH_FRAME_REGS. For >>> CONFIG_PPC64_ELF_ABI_V1, it's 112, for ABI_V2, it's 48. So the nip will read a >>> wrong value from stack when ABI_V2 enabled. >>> >>> To determine if ABI_V2 enabled is tricky. This patch do it by check the >>> following: >>> >>> In arch/powerpc/include/asm/ppc_asm.h: >>> #ifdef CONFIG_PPC64_ELF_ABI_V2 >>> #define STK_GOT 24 >>> #else >>> #define STK_GOT 40 >>> >>> In arch/powerpc/kernel/tm.S: >>> _GLOBAL(tm_reclaim) >>> mfcr r5 >>> mflr r0 >>> stw r5, 8(r1) >>> std r0, 16(r1) >>> std r2, STK_GOT(r1) >>> ... >>> >>> So a disassemble on tm_reclaim, and extract the STK_GOT value from std >>> instruction is used as the approach. >>> >>> After the patch: >>> crash> bt 1 >>> PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" >>> #0 [c0000000037737d0] __schedule at c00000000112e4ec >>> #1 [c0000000037738b0] schedule at c00000000112ea80 >>> ... >>> >>> Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; >>> --- >>> defs.h | 1 + >>> ppc64.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- >>> 2 files changed, 58 insertions(+), 3 deletions(-) >>> >>> diff --git a/defs.h b/defs.h >>> index 2231cb6..d5cb8cc 100644 >>> --- a/defs.h >>> +++ b/defs.h >>> @@ -4643,6 +4643,7 @@ struct efi_memory_desc_t { >>> #define MSR_PR_LG 14 /* Problem State / Privilege Level */ >>> /* Used to find the user or kernel-mode frame*/ >>> +#define STACK_SWITCH_FRAME_REGS 48 >>> #define STACK_FRAME_OVERHEAD 112 >>> #define EXCP_FRAME_MARKER 0x7265677368657265 >>> diff --git a/ppc64.c b/ppc64.c >>> index e8930a1..6e5f155 100644 >>> --- a/ppc64.c >>> +++ b/ppc64.c >>> @@ -72,6 +72,7 @@ static ulong pud_page_vaddr_l4(ulong pud); >>> static ulong pmd_page_vaddr_l4(ulong pmd); >>> static int is_opal_context(ulong sp, ulong nip); >>> void opalmsg(void); >>> +static bool is_ppc64_elf_abi_v2(void); >>> static int is_opal_context(ulong sp, ulong nip) >>> { >>> @@ -2813,6 +2814,51 @@ ppc64_get_sp(ulong task) >>> return sp; >>> } >>> +static bool >>> +is_ppc64_elf_abi_v2(void) >>> +{ >> >> Thanks for fixing these issues on ppc64 ! >> >> >> It is good. I am also checking if there is a way to implement >> 'is_ppc64_elf_abi_v2' by getting ABI version from the ELF notes, as 'file' >> command somehow does know: >> >> $ file vmlinux-little-endian vmlinux-little-endian: ELF 64-bit LSB >> executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 >> (SYSV), statically linked, >> BuildID[sha1]=f6449970e6af6cddeed1e260393e2377ccc0f369, not stripped >> >> That might be more dependable (if we can do that). > How about this approach ? > > I feel this might be more dependable, as the file command also uses > this, as well as fadump (PowerPC firmware-assisted dump) in the kernel. > >> As a sidenote, there is still some issue either in patch #1 or #3 >> causing all secondary CPUs (where swapper task is running) to show all >> threads as '_end' > What do you think ? I think your approach will work, thanks for your code! Please see my comments inlined in the code: > Either way what you chose, I do realise the idea to utilise the > 'std r2, STK_GOT(r1)' in such a way, was interesting and smart. > > static bool > is_ppc64_elf_abi_v2(void) > { > const char *kernel_file = pc->namelist; > int fd, swap; > char eheader[BUFSIZE]; > Elf64_Ehdr *elf64; > > if ((fd = open(kernel_file, O_RDONLY)) < 0) { > error(INFO, "%s: %s\n", kernel_file, strerror(errno)); > return FALSE; > } > if (read(fd, eheader, BUFSIZE) != BUFSIZE) { > error(INFO, "%s: %s\n", kernel_file, strerror(errno)); > close(fd); > return FALSE; > } > close(fd); > > if (!STRNEQ(eheader, ELFMAG) || eheader[EI_VERSION] != EV_CURRENT) > return FALSE; > > elf64 = (Elf64_Ehdr *)&eheader[0]; > > swap = (((eheader[EI_DATA] == ELFDATA2LSB) && > (__BYTE_ORDER == __BIG_ENDIAN)) || > ((eheader[EI_DATA] == ELFDATA2MSB) && > (__BYTE_ORDER == __LITTLE_ENDIAN))); The above code seems duplicated with the one in is_kernel(), in fact, is_kernel() will be invoked at the very beginning to determine if one file is vmlinux, so I would suggest to add our abi_v2 check here, like: in ppc64.c: int ppc64_elf_abi = 0; static bool is_ppc64_elf_abi_v2(void) { if (ppc64_elf_abi == 1) ... else if (ppc64_elf_abi == 2) ... else ... } in symbols.c:is_kernel(): ... } else if ((elf64->e_ident[EI_CLASS] == ELFCLASS64) && ((swap16(elf64->e_type, swap) == ET_EXEC) || ... case EM_PPC64: if (machine_type_mismatch(file, "PPC64", NULL, 0)) goto bailout; + else + ppc64_elf_abi = swap32(elf64->e_flags, swap); break; So we can initial the abi version at crash start only once, and get rid of the code duplication.