[Crash-utility] Re: Fix irq_stack_size on ARM64

Hi Lianbo, I have checked my vmlinux again, and found that not contained the symbol "vmcoreinfo_data" and "vmcoreinfo_size", but it has "kasan_enable_current". So I think check "kasan_enable_current" when have symol "vmcoreinfo_data" won't work as excepted. /* Having the vmcoreinfo data */ Maybe the previous patch is more suitable for this situation ? + if (kernel_symbol_exists("kasan_enable_current")) { I found that my vmlinux not set CONFIG_KEXEC and other related kernel dump configurations. Maybe this is the reason that not contained "vmcoreinfo_data" symbol. By the way, my kernel was running as a guest OS, and the dump file was generated by the hypervisor. Thanks yeping lijiang <lijiang(a)redhat.com&gt; 于2024年7月31日周三 17:36写道： > On Wed, Jul 31, 2024 at 3:34 PM yp z <wonderzyp(a)gmail.com&gt; wrote: > >> Hi Lianbo, >> >> > Hi, sending a patch as attachment is bad, you can refer to the kernel >> doc: >> >> Thank to Dave for pointing that out. I apologize for not realizing this >> before, and resend it as following. >> >> > I think you are right, the else branch can work well regardless of >> whether vmcoreinfo is present or not. So I changed the execution order of >> the code: >> > Step 1. Try to get THREAD_SHIFT from tbnz instruction. >> > Step 2. If cannot get THREAD_SHIFT by step 1, change the thread_shift >> when KASAN is enabled and with vmcoreinfo. >> > Please help review this new patch. >> > > Hmm, No. I would recommend doing it as below: > > ... > /* Having the vmcoreinfo data and enabling KASAN */ > if (kernel_symbol_exists("vmcoreinfo_data") && > kernel_symbol_exists("vmcoreinfo_size") && > kernel_symbol_exists("kasan_enable_current")) { > min_thread_shift += 1; > thread_shift = (min_thread_shift < machdep->pageshift) ? > machdep->pageshift : min_thread_shift; > } else { > sprintf(buf1, "x/32i vectors"); > ... > } > ... > > if (!thread_shift) > return 0; > > return ((1UL) << thread_shift); > } > > > > When having the vmcoreinfo data and disabling KASAN, crash tool can work > well without your patches, It should be like this: > > ... > /* Having the vmcoreinfo data */ > } else { > sprintf(buf1, "x/32i vectors"); > ... > } > ... > > if (!thread_shift) > return 0; > > return ((1UL) << thread_shift); > } > > Just an idea. Can you help double check? Yeping. > > Thanks > Lianbo > > >> >> --- >> arm64.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 47 insertions(+), 2 deletions(-) >> >> diff --git a/arm64.c b/arm64.c >> index 78e6609..49799e6 100644 >> --- a/arm64.c >> +++ b/arm64.c >> @@ -94,6 +94,7 @@ static int arm64_is_uvaddr(ulong, struct task_context >> *); >> static void arm64_calc_KERNELPACMASK(void); >> static void arm64_recalc_KERNELPACMASK(void); >> static int arm64_get_vmcoreinfo(unsigned long *vaddr, const char *label, >> int base); >> +static ulong arm64_set_irq_stack_size(void); >> >> struct kernel_range { >> unsigned long modules_vaddr, modules_end; >> @@ -2234,8 +2235,10 @@ arm64_irq_stack_init(void) >> if (MEMBER_EXISTS("thread_union", "stack")) { >> if ((sz = MEMBER_SIZE("thread_union", "stack")) > >> 0) >> ms->irq_stack_size = sz; >> - } else >> - ms->irq_stack_size = ARM64_IRQ_STACK_SIZE; >> + } else { >> + ulong res = arm64_set_irq_stack_size(); >> + ms->irq_stack_size = (res > 0) ? res : >> ARM64_IRQ_STACK_SIZE; >> + } >> >> machdep->flags |= IRQ_STACKS; >> >> @@ -4950,6 +4953,48 @@ static void arm64_recalc_KERNELPACMASK(void){ >> } >> } >> >> +static ulong arm64_set_irq_stack_size(void) >> +{ >> + int min_thread_shift = 14; >> + ulong thread_shift = 0; >> + char buf1[BUFSIZE]; >> + char *pos1, *pos2; >> + int errflag = 0; >> + >> + sprintf(buf1, "x/32i vectors"); >> + open_tmpfile(); >> + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) { >> + goto out; >> + } >> + rewind(pc->tmpfile); >> + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { >> + if ((pos1 = strstr(buf1, "tbnz"))) { >> + if ((pos2 = strchr(pos1, '#'))) { >> + pos2 += 1; >> + for(pos1=pos2; *pos2!='\0' && *pos2!=','; >> pos2++); >> + *pos2 = '\0'; >> + thread_shift = stol(pos1, >> RETURN_ON_ERROR|QUIET, &errflag); >> + if (errflag) { >> + thread_shift = 0; >> + } >> + break; >> + } >> + } >> + } >> +out: >> + close_tmpfile(); >> + >> + if ((!thread_shift) && >> kernel_symbol_exists("kasan_enable_current")) { >> + min_thread_shift += 1; >> + thread_shift = (min_thread_shift < machdep->pageshift) ? >> machdep->pageshift : min_thread_shift; >> + } >> + >> + if (!thread_shift) >> + return 0; >> + else >> + return ((1UL) << thread_shift); >> +} >> + >> #endif /* ARM64 */ >> >> >> Thanks >> yeping. >> >> Dave Young <dyoung(a)redhat.com&gt; 于2024年7月31日周三 14:58写道： >> >>> > Please help review this new patch. >>> >>> Hi, sending a patch as attachment is bad, you can refer to the kernel >>> doc: >>> https://www.kernel.org/doc/html/v4.14/process/email-clients.html >>> >>> "Patches for the Linux kernel are submitted via email, preferably as >>> inline text in the body of the email. Some maintainers accept >>> attachments, but then the attachments should have content-type >>> text/plain. However, attachments are generally frowned upon because it >>> makes quoting portions of the patch more difficult in the patch review >>> process." >>> >>> Crash is different from kernel but the email process is similar, I >>> think you can resend it with a proper format instead of maintainers >>> manually copying them in the email body for review. >>> >>> Thanks >>> Dave >>> >>>