[Crash-utility] [PATCH] ARM SMP
Mika Westerberg
ext-mika.1.westerberg at nokia.com
Thu Sep 30 13:12:18 UTC 2010
Hi Per,
On Thu, Sep 30, 2010 at 11:19:28AM +0200, ext Per Fransson wrote:
>
> This patch is an attempt to get the ball rolling on SMP support for ARM.
I noticed that this patch is line-wrapped so it doesn't apply cleanly (or is it
our brilliant exchange server which mangled that).
Few questions, see below.
> diff --git a/arm.c b/arm.c
> index 06b2f1c..bf346df 100644
> --- a/arm.c
> +++ b/arm.c
> @@ -73,7 +73,7 @@ struct arm_cpu_context_save {
> /*
> * Holds registers during the crash.
> */
> -static struct arm_pt_regs panic_task_regs;
> +static struct arm_pt_regs *panic_task_regs;
>
> #define PGDIR_SIZE() (4 * PAGESIZE())
> #define PGDIR_OFFSET(X) (((ulong)(X)) & (PGDIR_SIZE() - 1))
> @@ -484,71 +484,103 @@ arm_get_crash_notes(void)
> Elf32_Nhdr *note;
> ulong ptr, offset;
> char *buf, *p;
> + ulong *notes_ptrs;
> + ulong per_cpu_offsets_addr;
> + ulong *per_cpu_offsets;
> + ulong i;
>
> if (!symbol_exists("crash_notes"))
> return FALSE;
>
> crash_notes = symbol_value("crash_notes");
>
> - if (kt->cpus > 1)
> - error(WARNING, "only one CPU is currently supported\n");
> + notes_ptrs = GETBUF(kt->cpus*sizeof(notes_ptrs[0]));
>
> /*
> * Read crash_notes for the first CPU. crash_notes are in standard ELF
> * note format.
> */
> - if (!readmem(crash_notes, KVADDR, &ptr, sizeof(ptr), "crash_notes",
> + if (!readmem(crash_notes, KVADDR, ¬es_ptrs[kt->cpus-1],
> sizeof(notes_ptrs[kt->cpus-1]), "crash_notes",
> RETURN_ON_ERROR)) {
> - error(WARNING, "cannot read crash_notes\n");
> + error(WARNING, "cannot read crash_notes\n");
> + return FALSE;
You leak notes_ptrs here.
Just cosmetic thing but it seems that you are using 4 space indent (or is our
mailserver mangling that also). It would be better to be consistent with the style
used in that file (that is, tabs) :)
> + }
> +
> +
> + if (symbol_exists("__per_cpu_offset")) {
> +
> + /* Get the __per_cpu_offset array */
> + per_cpu_offsets_addr = symbol_value("__per_cpu_offset");
> +
> + per_cpu_offsets = GETBUF(kt->cpus*sizeof(*per_cpu_offsets));
> +
> + if (!readmem(per_cpu_offsets_addr, KVADDR, per_cpu_offsets,
> kt->cpus*sizeof(*per_cpu_offsets), "per_cpu_offsets",
> + RETURN_ON_ERROR)) {
> + error(WARNING, "cannot read per_cpu_offsets\n");
> return FALSE;
You leak per_cpu_offsets here.
> + }
> +
> + /* Add __per_cpu_offset for each cpu to form the pointer to the notes */
> + for (i = 0; i<kt->cpus; i++) {
> + notes_ptrs[i] = notes_ptrs[kt->cpus-1] + per_cpu_offsets[i];
> + }
> + FREEBUF(per_cpu_offsets);
> }
>
> buf = GETBUF(SIZE(note_buf));
> + panic_task_regs = GETBUF(kt->cpus*sizeof(*panic_task_regs));
> +
> + for (i=0;i<kt->cpus;i++) {
>
> - if (!readmem(ptr, KVADDR, buf, SIZE(note_buf), "note_buf_t",
> - RETURN_ON_ERROR)) {
> + if (!readmem(notes_ptrs[i], KVADDR, buf, SIZE(note_buf), "note_buf_t",
> + RETURN_ON_ERROR)) {
> error(WARNING, "failed to read note_buf_t\n");
> goto fail;
> - }
> + }
>
> - /*
> - * Do some sanity checks for this note before reading registers from it.
> - */
> - note = (Elf32_Nhdr *)buf;
> - p = buf + sizeof(Elf32_Nhdr);
> + /*
> + * Do some sanity checks for this note before reading registers from it.
> + */
> + note = (Elf32_Nhdr *)buf;
> + p = buf + sizeof(Elf32_Nhdr);
>
> - if (note->n_type != NT_PRSTATUS) {
> + if (note->n_type != NT_PRSTATUS) {
> error(WARNING, "invalid note (n_type != NT_PRSTATUS)\n");
> goto fail;
> - }
> - if (p[0] != 'C' || p[1] != 'O' || p[2] != 'R' || p[3] != 'E') {
> + }
> + if (p[0] != 'C' || p[1] != 'O' || p[2] != 'R' || p[3] != 'E') {
> error(WARNING, "invalid note (name != \"CORE\"\n");
> goto fail;
> - }
> + }
>
> - /*
> - * Find correct location of note data. This contains elf_prstatus
> - * structure which has registers etc. for the crashed task.
> - */
> - offset = sizeof(Elf32_Nhdr);
> - offset = roundup(offset + note->n_namesz, 4);
> - p = buf + offset; /* start of elf_prstatus */
> + /*
> + * Find correct location of note data. This contains elf_prstatus
> + * structure which has registers etc. for the crashed task.
> + */
> + offset = sizeof(Elf32_Nhdr);
> + offset = roundup(offset + note->n_namesz, 4);
> + p = buf + offset; /* start of elf_prstatus */
>
> - BCOPY(p + OFFSET(elf_prstatus_pr_reg), &panic_task_regs,
> - sizeof(panic_task_regs));
> + BCOPY(p + OFFSET(elf_prstatus_pr_reg), &panic_task_regs[i],
> + sizeof(panic_task_regs[i]));
> +
> + }
>
> /*
> * And finally we have pid and registers for the crashed task. This is
> * used later on when dumping backtrace.
> */
> ms->crash_task_pid = *(ulong *)(p + OFFSET(elf_prstatus_pr_pid));
> - ms->crash_task_regs = &panic_task_regs;
> + ms->crash_task_regs = panic_task_regs;
>
> FREEBUF(buf);
> + FREEBUF(notes_ptrs);
> return TRUE;
>
> fail:
> FREEBUF(buf);
> + FREEBUF(notes_ptrs);
> + FREEBUF(panic_task_regs);
> return FALSE;
> }
>
> @@ -996,20 +1028,20 @@ arm_get_dumpfile_stack_frame(struct bt_info
> *bt, ulong *nip, ulong *ksp)
> if (!ms->crash_task_regs)
> return FALSE;
>
> - if (tt->panic_task != bt->task || bt->tc->pid != ms->crash_task_pid)
> - return FALSE;
> -
Was there a reason to remove the check above? Is it so that when we have SMP
machine, there is still only one panic'ing task?
Anyway, if this this check is not needed anymore, I guess you can remove the
whole variable from machspec structure.
Best regards,
MW
> + if (!is_task_active(bt->task))
> + return FALSE;
> +
> /*
> * We got registers for panic task from crash_notes. Just return them.
> */
> - *nip = ms->crash_task_regs->ARM_pc;
> - *ksp = ms->crash_task_regs->ARM_sp;
> + *nip = ms->crash_task_regs[bt->tc->processor].ARM_pc;
> + *ksp = ms->crash_task_regs[bt->tc->processor].ARM_sp;
>
> /*
> * Also store pointer to all registers in case unwinding code needs
> * to access LR.
> */
> - bt->machdep = ms->crash_task_regs;
> + bt->machdep = &(ms->crash_task_regs[bt->tc->processor]);
>
> return TRUE;
> }
> diff --git a/defs.h b/defs.h
> index d431d6e..8a2291a 100755
> --- a/defs.h
> +++ b/defs.h
> @@ -85,7 +85,7 @@
> #define NR_CPUS (64)
> #endif
> #ifdef ARM
> -#define NR_CPUS (1)
> +#define NR_CPUS (4)
> #endif
>
> #define BUFSIZE (1500)
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
More information about the Crash-utility
mailing list