Re: arm64: Get CPU registers without crash_notes symbol

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Lianbo,

Could you help to review our patch?

Thanks
James Hsu

On Fri, 2021-08-20 at 01:26 +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:
> -----Original Message-----
> > Dear Kazu,
> > 
> > Sorry for late reply
> > Thanks for your suggestion and I have prepared a V2 patch, please
> > help
> > to review.
> 
> ok, I've modified your v2 patch to fix the following compilation
> warning
> and rewrite the commit log, and attached it.
> 
> arm64.c: In function ‘arm64_init’:
> arm64.c:3728:35: warning: ‘note’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
>      offset = roundup(offset + note->n_namesz, 4);
>                                    ^~
Kazu, thanks for your kind help.

> Lianbo, could you review the attached patch?
> 
> Thanks,
> Kazu
> 
> > 
> > BTW, I have switched to evolution mail system, if still look like
> > html
> > format, please let me know, thanks.
> > 
> > Signed-off-by: James Hsu <james.hsu@xxxxxxxxxxxx>
> > 
> > diff --git a/arm64.c b/arm64.c
> > index 8934961..03c8167 100644
> > --- a/arm64.c
> > +++ b/arm64.c
> > @@ -3667,8 +3667,42 @@ arm64_get_crash_notes(void)
> >  	ulong *notes_ptrs;
> >  	ulong i, found;
> > 
> > -	if (!symbol_exists("crash_notes"))
> > +	if (!symbol_exists("crash_notes")) {
> > +		if (DISKDUMP_DUMPFILE() || KDUMP_DUMPFILE()) {
> > +			if (!(ms->panic_task_regs = calloc((size_t)kt-
> > > cpus, sizeof(struct arm64_pt_regs))))
> > 
> > +				error(FATAL, "cannot calloc
> > panic_task_regs space\n");
> > +
> > +			for  (i = found = 0; i < kt->cpus; i++) {
> > +				if (DISKDUMP_DUMPFILE())
> > +					note =
> > diskdump_get_prstatus_percpu(i);
> > +				else if (KDUMP_DUMPFILE())
> > +					note =
> > netdump_get_prstatus_percpu(i);
> > +
> > +				if(!note) {
> > +					error(WARNING, "cpu %d: cannot
> > find NT_PRSTATUS note\n", i);
> > +					continue;
> > +				}
> > +
> > +				/*
> > +				 * Find correct location of note data.
> > This contains elf_prstatus
> > +				 * structure which has registers etc.
> > for the crashed task.
> > +				 */
> > +				offset = sizeof(Elf64_Nhdr);
> > +				offset = roundup(offset + note-
> > > n_namesz, 4);
> > 
> > +				p = (char *)note + offset; /* start of
> > elf_prstatus */
> > +
> > +				BCOPY(p + OFFSET(elf_prstatus_pr_reg),
> > &ms->panic_task_regs[i],
> > +				      sizeof(struct arm64_pt_regs));
> > +
> > +				found++;
> > +			}
> > +			if (!found) {
> > +				free(ms->panic_task_regs);
> > +				ms->panic_task_regs = NULL;
> > +			}
> > +		}
> >  		return;
> > +	}
> > 
> >  	crash_notes = symbol_value("crash_notes");
> > 
> > Thanks
> > James Hsu
> > 
> > On Wed, 2021-08-11 at 02:52 +0000, HAGIO KAZUHITO(萩尾 一仁) wrote:
> > > -----Original Message-----
> > > > 
> > > > Dear Crash maintainers,
> > > > 
> > > > We want to improve the crash_tool for the case that need
> > > > offline
> > > > cpu  register info even Kdump without
> > > > crash_notes.
> > > > 
> > > > For one thing,  we will prepare a Kdump with all CPU register
> > > > info
> > > > in ELF note.
> > > > For another thing, we need to modify crash_tool and make it
> > > > support
> > > > adding offline cpu register info even
> > > > Kdump without crash_notes.
> > > > 
> > > > We prepare a patch for the case with ARCH=arm64.
> > > > It is verified with our kdump without crash_note and can get
> > > > all
> > > > cpu register.
> > > > Please take your time to review our patch and look forward to
> > > > receiving your comments.
> > > > 
> > > > Patch detail as below
> > > 
> > > I've rewritten the commit log, is this ok?
> > > --
> > > arm64: Get CPU registers from ELF notes even without crash_notes
> > > symbol
> > > 
> > > Currently arm64 crash retrieves the CPU registers from
> > > crash_notes
> > > symbol
> > > or ELF notes only when the symbol exists, but there are dumpfiles
> > > which
> > > have the registers in ELF notes without the symbol.
> > > 
> > > With the patch, crash can retrieve the registers from ELF notes
> > > without
> > > the crash_notes symbol.
> > > --
> > > 
> > > And please add Signed-off-by tag.
> > > 
> > > > --- crash-7.3.0/arm64.c	2021-04-27 16:01:07.000000000 +0800
> > > > +++ crash-7.3.0.mod/arm64.c	2021-06-15 17:13:54.037273227
> > > > +0800
> > > > @@ -3667,8 +3667,41 @@ arm64_get_crash_notes(void)
> > > >  	ulong *notes_ptrs;
> > > >  	ulong i, found;
> > > > 
> > > > -	if (!symbol_exists("crash_notes"))
> > > > +	if (!symbol_exists("crash_notes")) {
> > > > +		if (DISKDUMP_DUMPFILE() || KDUMP_DUMPFILE()) {
> > > > +			if (!(ms->panic_task_regs =
> > > > calloc((size_t)kt-
> > > > > cpus, sizeof(struct arm64_pt_regs))))
> > > > 
> > > > +				error(FATAL, "cannot calloc
> > > > panic_task_regs space\n");
> > > > +
> > > > +			for  (i = found = 0; i < kt->cpus; i++)
> > > > {
> > > > +				if (DISKDUMP_DUMPFILE())
> > > > +					note =
> > > > diskdump_get_prstatus_percpu(i);
> > > > +				else if (KDUMP_DUMPFILE())
> > > > +					note =
> > > > netdump_get_prstatus_percpu(i);
> > > > +				else {
> > > > +					error(WARNING, "cpu %d:
> > > > cannot
> > > > find NT_PRSTATUS note\n", i);
> > > > +					continue;
> > > > +				}
> > > 
> > > This else block should be separated from the if block like this?
> > > 
> > > if (!note) {
> > > 	error(WARNING, "cpu %d: cannot find NT_PRSTATUS note\n", i);
> > > 	continue;
> > > }
> > > 
> > > > +
> > > > +				/*
> > > > +				 * Find correct location of
> > > > note data.
> > > > This contains elf_prstatus
> > > > +				 * structure which has
> > > > registers etc.
> > > > for the crashed task.
> > > > +				 */
> > > > +				offset = sizeof(Elf64_Nhdr);
> > > > +				offset = roundup(offset + note-
> > > > > n_namesz, 4);
> > > > 
> > > > +				p = (char *)note + offset; /*
> > > > start of
> > > > elf_prstatus */
> > > > +
> > > > +				BCOPY(p +
> > > > OFFSET(elf_prstatus_pr_reg),
> > > > &ms->panic_task_regs[i],
> > > > +				      sizeof(struct
> > > > arm64_pt_regs));
> > > > +
> > > > +				found++;
> > > > +			}
> > > > +		}
> > > > +		if (!found) {
> > > > +			free(ms->panic_task_regs);
> > > > +			ms->panic_task_regs = NULL;
> > > > +		}
> > > 
> > > This if block should be within the if (DISKDUMP_DUMPFILE()...
> > > block.
> > > 
> > > >  		return;
> > > > +	}
> > > > 
> > > >  	crash_notes = symbol_value("crash_notes");
> > > > 
> > > 
> > > (and this email still looks HTML one to me :-)
> > > 
> > > Thanks,
> > > Kazu
> > > 

Attachment: crash-arm64-support_v3.bin
Description: Binary data

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility

[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux