On Wed, Jul 31, 2024 at 3:34 PM yp z <[email protected]> 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 <[email protected]> 于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 -- [email protected]
To unsubscribe send an email to [email protected]
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
