[Crash-utility] Re: [PATCH v6 1/5] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg

Hi Lianbo, Just a ping for the series. Any pending reviews for upstreaming it ?

Thanks, Aditya Gupta On Mon, Jan 08, 2024 at 04:38:20PM +0800, Lianbo Jiang wrote: > Hi, Aditya > > Thank you for the great job. > > The v6 patch set looks good to me, and I did not see any new issues in my > tests. > > So for the v6: Ack. > > > Thanks > > Lianbo > > On 1/5/24 15:31, devel-request(a)lists.crash-utility.osci.io wrote: > >> Date: Fri, 5 Jan 2024 13:00:23 +0530 >> From: Aditya Gupta&lt;adityag(a)linux.ibm.com&gt; >> Subject: [Crash-utility] [PATCH v6 1/5] ppc64: correct gdb >> passthroughs by implementing machdep->get_cpu_reg >> To:<devel@lists.crash-utility.osci.io>,<lijiang@redhat.com>, >> Tao Liu&lt;ltao(a)redhat.com&gt; >> Cc: Mahesh J Salgaonkar&lt;mahesh(a)linux.ibm.com&gt;, "Naveen N. Rao" >> <naveen.n.rao(a)linux.vnet.ibm.com&gt; >> Message-ID:<20240105073027.378928-2-adityag@linux.ibm.com> >> Content-Type: text/plain; charset=UTF-8 >> >> Currently, gdb passthroughs of 'bt', 'frame', 'up', 'down', 'info >> locals' don't work. This is due to gdb not knowing the register values to >> unwind the stack frames >> >> Every gdb passthrough goes through `gdb_interface`. And then, gdb expects >> `crash_target::fetch_registers` to give it the register values, which is >> dependent on `machdep->get_cpu_reg` to read the register values for >> specific architecture. >> >> ┌────────────────────────┐ >> gdb passthrough (eg. "bt") │ │ >> crash ─────────────────────────▶│ │ >> │ gdb_interface │ >> │ │ >> │ │ >> │ ┌────────────────────┐ │ >> fetch_registers │ │ │ │ >> crash_target◀────────────────────────┼─┤ gdb │ │ >> ─────────────────────────┼▶│ │ │ >> Registers (SP,NIP, etc.)│ │ │ │ >> │ │ │ │ >> │ └────────────────────┘ │ >> └────────────────────────┘ >> >> Implement `machdep->get_cpu_reg` on PPC64, so that crash provides the >> register values to gdb to unwind stack frames properly >> >> With these changes, on powerpc, 'bt' command output in gdb mode, will look >> like this: >> >> gdb> bt >> #0 0xc0000000002a53e8 in crash_setup_regs (oldregs=<optimized out>, newregs=0xc00000000486f8d8) at ./arch/powerpc/include/asm/kexec.h:69 >> #1 __crash_kexec (regs=<optimized out>) at kernel/kexec_core.c:974 >> #2 0xc000000000168918 in panic (fmt=<optimized out>) at kernel/panic.c:358 >> #3 0xc000000000b735f8 in sysrq_handle_crash (key=<optimized out>) at drivers/tty/sysrq.c:155 >> #4 0xc000000000b742cc in __handle_sysrq (key=key@entry=99, check_mask=check_mask@entry=false) at drivers/tty/sysrq.c:602 >> #5 0xc000000000b7506c in write_sysrq_trigger (file=<optimized out>, buf=<optimized out>, count=2, ppos=<optimized out>) at drivers/tty/sysrq.c:1163 >> #6 0xc00000000069a7bc in pde_write (ppos=<optimized out>, count=<optimized out>, buf=<optimized out>, file=<optimized out>, pde=0xc000000009ed3a80) at fs/proc/inode.c:340 >> #7 proc_reg_write (file=<optimized out>, buf=<optimized out>, count=<optimized out>, ppos=<optimized out>) at fs/proc/inode.c:352 >> #8 0xc0000000005b3bbc in vfs_write (file=file@entry=0xc00000009dda7d00, buf=buf@entry=0xebcfc7c6040 <error: Cannot access memory at address 0xebcfc7c6040>, count=count@entry=2, pos=pos@entry=0xc00000000486fda0) at fs/read_write.c:582 >> >> instead of earlier output without this patch: >> >> gdb> bt >> #0 <unavailable> in ?? () >> Backtrace stopped: previous frame identical to this frame (corrupt stack?) >> >> Also, 'get_dumpfile_regs' has been introduced to get registers from >> multiple supported vmcore formats. Correspondingly a flag 'BT_NO_PRINT_REGS' >> has been introduced to tell helper functions to get registers, to not >> print registers with every call to backtrace in gdb. >> >> Note: This feature to support GDB unwinding doesn't support live debugging >> >> Signed-off-by: Aditya Gupta&lt;adityag(a)linux.ibm.com&gt; >> --- >> defs.h | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> kernel.c | 33 +++++++++++++++ >> ppc64.c | 105 +++++++++++++++++++++++++++++++++++++++++++++-- >> 3 files changed, 257 insertions(+), 3 deletions(-) >> >> diff --git a/defs.h b/defs.h >> index 5218a94fe4a4..615f3a37935a 100644 >> --- a/defs.h >> +++ b/defs.h >> @@ -6023,6 +6023,7 @@ int load_module_symbols_helper(char *); >> void unlink_module(struct load_module *); >> int check_specified_module_tree(char *, char *); >> int is_system_call(char *, ulong); >> +void get_dumpfile_regs(struct bt_info*, ulong*, ulong*); >> void generic_dump_irq(int); >> void generic_get_irq_affinity(int); >> void generic_show_interrupts(int, ulong *); >> @@ -6121,6 +6122,7 @@ ulong cpu_map_addr(const char *type); >> #define BT_REGS_NOT_FOUND (0x4000000000000ULL) >> #define BT_OVERFLOW_STACK (0x8000000000000ULL) >> #define BT_SKIP_IDLE (0x10000000000000ULL) >> +#define BT_NO_PRINT_REGS (0x20000000000000ULL) >> #define BT_SYMBOL_OFFSET (BT_SYMBOLIC_ARGS) >> #define BT_REF_HEXVAL (0x1) >> @@ -7854,4 +7856,124 @@ enum x86_64_regnum { >> LAST_REGNUM >> }; >> +/* >> + * Register numbers to make crash_target->fetch_registers() >> + * ---> machdep->get_cpu_reg() work properly. >> + * >> + * These register numbers and names are given according to output of >> + * `rs6000_register_name`, because that is what was being used by >> + * crash_target::fetch_registers in case of PPC64 >> + */ >> +enum ppc64_regnum { >> + PPC64_R0_REGNUM = 0, >> + PPC64_R1_REGNUM, >> + PPC64_R2_REGNUM, >> + PPC64_R3_REGNUM, >> + PPC64_R4_REGNUM, >> + PPC64_R5_REGNUM, >> + PPC64_R6_REGNUM, >> + PPC64_R7_REGNUM, >> + PPC64_R8_REGNUM, >> + PPC64_R9_REGNUM, >> + PPC64_R10_REGNUM, >> + PPC64_R11_REGNUM, >> + PPC64_R12_REGNUM, >> + PPC64_R13_REGNUM, >> + PPC64_R14_REGNUM, >> + PPC64_R15_REGNUM, >> + PPC64_R16_REGNUM, >> + PPC64_R17_REGNUM, >> + PPC64_R18_REGNUM, >> + PPC64_R19_REGNUM, >> + PPC64_R20_REGNUM, >> + PPC64_R21_REGNUM, >> + PPC64_R22_REGNUM, >> + PPC64_R23_REGNUM, >> + PPC64_R24_REGNUM, >> + PPC64_R25_REGNUM, >> + PPC64_R26_REGNUM, >> + PPC64_R27_REGNUM, >> + PPC64_R28_REGNUM, >> + PPC64_R29_REGNUM, >> + PPC64_R30_REGNUM, >> + PPC64_R31_REGNUM, >> + >> + PPC64_F0_REGNUM = 32, >> + PPC64_F1_REGNUM, >> + PPC64_F2_REGNUM, >> + PPC64_F3_REGNUM, >> + PPC64_F4_REGNUM, >> + PPC64_F5_REGNUM, >> + PPC64_F6_REGNUM, >> + PPC64_F7_REGNUM, >> + PPC64_F8_REGNUM, >> + PPC64_F9_REGNUM, >> + PPC64_F10_REGNUM, >> + PPC64_F11_REGNUM, >> + PPC64_F12_REGNUM, >> + PPC64_F13_REGNUM, >> + PPC64_F14_REGNUM, >> + PPC64_F15_REGNUM, >> + PPC64_F16_REGNUM, >> + PPC64_F17_REGNUM, >> + PPC64_F18_REGNUM, >> + PPC64_F19_REGNUM, >> + PPC64_F20_REGNUM, >> + PPC64_F21_REGNUM, >> + PPC64_F22_REGNUM, >> + PPC64_F23_REGNUM, >> + PPC64_F24_REGNUM, >> + PPC64_F25_REGNUM, >> + PPC64_F26_REGNUM, >> + PPC64_F27_REGNUM, >> + PPC64_F28_REGNUM, >> + PPC64_F29_REGNUM, >> + PPC64_F30_REGNUM, >> + PPC64_F31_REGNUM, >> + >> + PPC64_PC_REGNUM = 64, >> + PPC64_MSR_REGNUM = 65, >> + PPC64_CR_REGNUM = 66, >> + PPC64_LR_REGNUM = 67, >> + PPC64_CTR_REGNUM = 68, >> + PPC64_XER_REGNUM = 69, >> + PPC64_FPSCR_REGNUM = 70, >> + >> + PPC64_VR0_REGNUM = 106, >> + PPC64_VR1_REGNUM, >> + PPC64_VR2_REGNUM, >> + PPC64_VR3_REGNUM, >> + PPC64_VR4_REGNUM, >> + PPC64_VR5_REGNUM, >> + PPC64_VR6_REGNUM, >> + PPC64_VR7_REGNUM, >> + PPC64_VR8_REGNUM, >> + PPC64_VR9_REGNUM, >> + PPC64_VR10_REGNUM, >> + PPC64_VR11_REGNUM, >> + PPC64_VR12_REGNUM, >> + PPC64_VR13_REGNUM, >> + PPC64_VR14_REGNUM, >> + PPC64_VR15_REGNUM, >> + PPC64_VR16_REGNUM, >> + PPC64_VR17_REGNUM, >> + PPC64_VR18_REGNUM, >> + PPC64_VR19_REGNUM, >> + PPC64_VR20_REGNUM, >> + PPC64_VR21_REGNUM, >> + PPC64_VR22_REGNUM, >> + PPC64_VR23_REGNUM, >> + PPC64_VR24_REGNUM, >> + PPC64_VR25_REGNUM, >> + PPC64_VR26_REGNUM, >> + PPC64_VR27_REGNUM, >> + PPC64_VR28_REGNUM, >> + PPC64_VR29_REGNUM, >> + PPC64_VR30_REGNUM, >> + PPC64_VR31_REGNUM, >> + >> + PPC64_VSCR_REGNUM = 138, >> + PPC64_VRSAVE_REGNU = 139 >> +}; >> + >> #endif /* !GDB_COMMON */ >> diff --git a/kernel.c b/kernel.c >> index 6dcf414693e6..52b7ba09f390 100644 >> --- a/kernel.c >> +++ b/kernel.c >> @@ -3533,6 +3533,39 @@ get_lkcd_regs(struct bt_info *bt, ulong *eip, ulong *esp) >> machdep->get_stack_frame(bt, eip, esp); >> } >> +void >> +get_dumpfile_regs(struct bt_info *bt, ulong *eip, ulong *esp) >> +{ >> + bt->flags |= BT_NO_PRINT_REGS; >> + >> + if (NETDUMP_DUMPFILE()) >> + get_netdump_regs(bt, eip, esp); >> + else if (KDUMP_DUMPFILE()) >> + get_kdump_regs(bt, eip, esp); >> + else if (DISKDUMP_DUMPFILE()) >> + get_diskdump_regs(bt, eip, esp); >> + else if (KVMDUMP_DUMPFILE()) >> + get_kvmdump_regs(bt, eip, esp); >> + else if (LKCD_DUMPFILE()) >> + get_lkcd_regs(bt, eip, esp); >> + else if (XENDUMP_DUMPFILE()) >> + get_xendump_regs(bt, eip, esp); >> + else if (SADUMP_DUMPFILE()) >> + get_sadump_regs(bt, eip, esp); >> + else if (VMSS_DUMPFILE()) >> + get_vmware_vmss_regs(bt, eip, esp); >> + else if (REMOTE_PAUSED()) { >> + if (!is_task_active(bt->task) || !get_remote_regs(bt, eip, esp)) >> + machdep->get_stack_frame(bt, eip, esp); >> + } else >> + machdep->get_stack_frame(bt, eip, esp); >> + >> + bt->flags &= ~BT_NO_PRINT_REGS; >> + >> + bt->instptr = *eip; >> + bt->stkptr = *esp; >> +} >> + >> /* >> * Store the head of the kernel module list for future use. >> diff --git a/ppc64.c b/ppc64.c >> index e8930a139e0d..ea4821d86b9e 100644 >> --- a/ppc64.c >> +++ b/ppc64.c >> @@ -55,6 +55,8 @@ static void ppc64_set_bt_emergency_stack(enum emergency_stack_type type, >> static char * ppc64_check_eframe(struct ppc64_pt_regs *); >> static void ppc64_print_eframe(char *, struct ppc64_pt_regs *, >> struct bt_info *); >> +static int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size, >> + void *value); >> static void parse_cmdline_args(void); >> static int ppc64_paca_percpu_offset_init(int); >> static void ppc64_init_cpu_info(void); >> @@ -704,6 +706,8 @@ ppc64_init(int when) >> error(FATAL, "cannot malloc hwirqstack buffer space."); >> } >> + machdep->get_cpu_reg = ppc64_get_cpu_reg; >> + >> ppc64_init_paca_info(); >> if (!machdep->hz) { >> @@ -2501,6 +2505,99 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs *regs, >> ppc64_print_nip_lr(regs, 1); >> } >> +static int >> +ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size, >> + void *value) >> +{ >> + struct bt_info bt_info, bt_setup; >> + struct task_context *tc; >> + struct ppc64_pt_regs *pt_regs; >> + ulong ip, sp; >> + >> + if (LIVE()) { >> + /* doesn't support reading registers in live dump */ >> + return FALSE; >> + } >> + >> + /* Currently only handling registers available in ppc64_pt_regs: >> + * >> + * 0-31: r0-r31 >> + * 64: pc/nip >> + * 65: msr >> + * >> + * 67: lr >> + * 68: ctr >> + */ >> + switch (regno) { >> + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM: >> + >> + case PPC64_PC_REGNUM: >> + case PPC64_MSR_REGNUM: >> + case PPC64_LR_REGNUM: >> + case PPC64_CTR_REGNUM: >> + break; >> + >> + default: >> + // return false if we can't get that register >> + if (CRASHDEBUG(1)) >> + error(WARNING, "unsupported register, regno=%d\n", regno); >> + return FALSE; >> + } >> + >> + tc = CURRENT_CONTEXT(); >> + BZERO(&bt_setup, sizeof(struct bt_info)); >> + clone_bt_info(&bt_setup, &bt_info, tc); >> + fill_stackbuf(&bt_info); >> + >> + // reusing the get_dumpfile_regs function to get pt regs structure >> + get_dumpfile_regs(&bt_info, &sp, &ip); >> + pt_regs = (struct ppc64_pt_regs *)bt_info.machdep; >> + >> + if (!pt_regs) { >> + error(WARNING, "pt_regs not available for cpu %d\n", cpu); >> + return FALSE; >> + } >> + >> + switch (regno) { >> + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM: >> + if (size != sizeof(pt_regs->gpr[regno])) >> + return FALSE; // size mismatch >> + >> + memcpy(value, &pt_regs->gpr[regno], size); >> + break; >> + >> + case PPC64_PC_REGNUM: >> + if (size != sizeof(pt_regs->nip)) >> + return FALSE; // size mismatch >> + >> + memcpy(value, &pt_regs->nip, size); >> + break; >> + >> + case PPC64_MSR_REGNUM: >> + if (size != sizeof(pt_regs->msr)) >> + return FALSE; // size mismatch >> + >> + memcpy(value, &pt_regs->msr, size); >> + break; >> + >> + case PPC64_LR_REGNUM: >> + if (size != sizeof(pt_regs->link)) >> + return FALSE; // size mismatch >> + >> + memcpy(value, &pt_regs->link, size); >> + break; >> + >> + case PPC64_CTR_REGNUM: >> + if (size != sizeof(pt_regs->ctr)) >> + return FALSE; // size mismatch >> + >> + memcpy(value, &pt_regs->ctr, size); >> + break; >> + } >> + >> + return TRUE; >> +} >> + >> /* >> * For vmcore typically saved with KDump or FADump, get SP and IP values >> * from the saved ptregs. >> @@ -2613,9 +2710,11 @@ ppc64_get_dumpfile_stack_frame(struct bt_info *bt_in, ulong *nip, ulong *ksp) >> pt_regs = (struct ppc64_pt_regs *)bt->machdep; >> ur_nip = pt_regs->nip; >> ur_ksp = pt_regs->gpr[1]; >> - /* Print the collected regs for panic task. */ >> - ppc64_print_regs(pt_regs); >> - ppc64_print_nip_lr(pt_regs, 1); >> + if (!(bt->flags & BT_NO_PRINT_REGS)) { >> + /* Print the collected regs for panic task. */ >> + ppc64_print_regs(pt_regs); >> + ppc64_print_nip_lr(pt_regs, 1); >> + } >> } else if ((pc->flags & KDUMP) || >> ((pc->flags & DISKDUMP) && >> (*diskdump_flags & KDUMP_CMPRS_LOCAL))) { >> -- 2.43.0