[Crash-utility] Re: Fix irq_stack_size on ARM64

[lijiang <lijiang@xxxxxxxxxx>]

On Wed, Jul 31, 2024 at 3:34 PM yp z <wonderzyp@xxxxxxxxx> 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 */
if (kernel_symbol_exists("vmcoreinfo_data") &&
    kernel_symbol_exists("vmcoreinfo_size") {

        /* enabling KASAN? */
        if (kernel_symbol_exists("kasan_enable_current")) {
            min_thread_shift += 1;
            thread_shift = (min_thread_shift < machdep->pageshift) ? machdep->pageshift : min_thread_shift;
       } else
            thread_shift = 0; /* Still use the value of ARM64_IRQ_STACK_SIZE */
} 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@xxxxxxxxxx> 于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

--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki