Hi Dave, Following the changes in the way cpu_pda is accessed on x86_64 kernels, running crash to analyse kdump based crash dumps gave the following error: crash: cannot resolve "cpu_pda" The following patch fixes this. Kindly review.

Thanks Rachita Signed-off-by: Rachita Kothiyal <rachita(a)in.ibm.com&gt; --- defs.h | 10 ++++ x86_64.c | 139 ++++++++++++++++++++++++++++++++++++++++++++------------------- 2 files changed, 108 insertions(+), 41 deletions(-) diff -puN x86_64.c~crash-fix-cpu-pda x86_64.c --- crash-4.0-2.19/x86_64.c~crash-fix-cpu-pda 2006-01-31 17:21:35.430960480 +0530 +++ crash-4.0-2.19-rachita/x86_64.c 2006-01-31 17:23:44.893279216 +0530 @@ -367,7 +367,7 @@ x86_64_cpu_pda_init(void) { int i, cpus, nr_pda, cpunumber; char *cpu_pda_buf; - ulong level4_pgt, data_offset; + ulong level4_pgt, data_offset, *cpu_pda_addr; struct syment *sp, *nsp; ulong offset, istacksize; @@ -383,36 +383,68 @@ x86_64_cpu_pda_init(void) cpu_pda_buf = GETBUF(SIZE(x8664_pda)); - if (!(nr_pda = get_array_length("cpu_pda", NULL, 0))) - nr_pda = NR_CPUS; - - for (i = cpus = 0; i < nr_pda; i++) { - if (!CPU_PDA_READ(i, cpu_pda_buf)) - break; - if (VALID_MEMBER(x8664_pda_level4_pgt)) { - level4_pgt = ULONG(cpu_pda_buf + OFFSET(x8664_pda_level4_pgt)); - if (!VALID_LEVEL4_PGT_ADDR(level4_pgt)) + if (symbol_exists("_cpu_pda")) { + if (!(nr_pda = get_array_length("_cpu_pda", NULL, 0))) + nr_pda = NR_CPUS; + cpu_pda_addr = GETBUF(sizeof(unsigned long)); + for (i = cpus = 0; i < nr_pda; i++) { + *cpu_pda_addr = NULL; + if (!_CPU_PDA_READ(i, cpu_pda_buf)) break; - } - cpunumber = INT(cpu_pda_buf + OFFSET(x8664_pda_cpunumber)); - if (cpunumber != cpus) - break; - cpus++; - if (VALID_MEMBER(x8664_pda_data_offset)) { - data_offset = ULONG(cpu_pda_buf + - OFFSET(x8664_pda_data_offset)); - kt->__per_cpu_offset[i] = data_offset; - kt->flags |= PER_CPU_OFF; - } else - data_offset = 0; + if (VALID_MEMBER(x8664_pda_level4_pgt)) { + level4_pgt = ULONG(cpu_pda_buf + OFFSET(x8664_pda_level4_pgt)); + if (!VALID_LEVEL4_PGT_ADDR(level4_pgt)) + break; + } + cpunumber = INT(cpu_pda_buf + OFFSET(x8664_pda_cpunumber)); + if (cpunumber != cpus) + break; + cpus++; - machdep->machspec->stkinfo.ibase[i] = ULONG(cpu_pda_buf + - OFFSET(x8664_pda_irqstackptr)); + if (VALID_MEMBER(x8664_pda_data_offset)) { + data_offset = ULONG(cpu_pda_buf + + OFFSET(x8664_pda_data_offset)); + kt->__per_cpu_offset[i] = data_offset; + kt->flags |= PER_CPU_OFF; + } else + data_offset = 0; + machdep->machspec->stkinfo.ibase[i] = ULONG(cpu_pda_buf + + OFFSET(x8664_pda_irqstackptr)); + if (CRASHDEBUG(2)) + fprintf(fp, "CPU%d: level4_pgt: %lx data_offset: %lx\n", + i, level4_pgt, data_offset); + } + FREEBUF(cpu_pda_addr); + } else if (symbol_exists("cpu_pda")) { + if (!(nr_pda = get_array_length("cpu_pda", NULL, 0))) + nr_pda = NR_CPUS; + for (i = cpus = 0; i < nr_pda; i++) { + if (!CPU_PDA_READ(i, cpu_pda_buf)) + break; + if (VALID_MEMBER(x8664_pda_level4_pgt)) { + level4_pgt = ULONG(cpu_pda_buf + OFFSET(x8664_pda_level4_pgt)); + if (!VALID_LEVEL4_PGT_ADDR(level4_pgt)) + break; + } + cpunumber = INT(cpu_pda_buf + OFFSET(x8664_pda_cpunumber)); + if (cpunumber != cpus) + break; + cpus++; - if (CRASHDEBUG(2)) - fprintf(fp, "CPU%d: level4_pgt: %lx data_offset: %lx\n", - i, level4_pgt, data_offset); + if (VALID_MEMBER(x8664_pda_data_offset)) { + data_offset = ULONG(cpu_pda_buf + + OFFSET(x8664_pda_data_offset)); + kt->__per_cpu_offset[i] = data_offset; + kt->flags |= PER_CPU_OFF; + } else + data_offset = 0; + machdep->machspec->stkinfo.ibase[i] = ULONG(cpu_pda_buf + + OFFSET(x8664_pda_irqstackptr)); + if (CRASHDEBUG(2)) + fprintf(fp, "CPU%d: level4_pgt: %lx data_offset: %lx\n", + i, level4_pgt, data_offset); + } } @@ -2718,29 +2750,51 @@ x86_64_get_smp_cpus(void) { int i, cpus, nr_pda, cpunumber; char *cpu_pda_buf; - ulong level4_pgt; + ulong level4_pgt, *cpu_pda_addr; if (!VALID_STRUCT(x8664_pda)) return 1; cpu_pda_buf = GETBUF(SIZE(x8664_pda)); - if (!(nr_pda = get_array_length("cpu_pda", NULL, 0))) - nr_pda = NR_CPUS; + if (symbol_exists("_cpu_pda")){ + if (!(nr_pda = get_array_length("_cpu_pda", NULL, 0))) + nr_pda = NR_CPUS; + cpu_pda_addr = GETBUF(sizeof(unsigned long)); + for (i = cpus = 0; i < nr_pda; i++) { + *cpu_pda_addr = NULL; + if (!_CPU_PDA_READ(i, cpu_pda_buf)) + break; - for (i = cpus = 0; i < nr_pda; i++) { - if (!CPU_PDA_READ(i, cpu_pda_buf)) - break; + if (VALID_MEMBER(x8664_pda_level4_pgt)) { + level4_pgt = ULONG(cpu_pda_buf + OFFSET(x8664_pda_level4_pgt)); + if (!VALID_LEVEL4_PGT_ADDR(level4_pgt)) + break; + } + cpunumber = INT(cpu_pda_buf + OFFSET(x8664_pda_cpunumber)); + if (cpunumber != cpus) + break; + cpus++; + } + FREEBUF(cpu_pda_addr); + } + else if (symbol_exists("cpu_pda")){ + if (!(nr_pda = get_array_length("cpu_pda", NULL, 0))) + nr_pda = NR_CPUS; + for (i = cpus = 0; i < nr_pda; i++) { + if (!CPU_PDA_READ(i, cpu_pda_buf)) + break; - if (VALID_MEMBER(x8664_pda_level4_pgt)) { - level4_pgt = ULONG(cpu_pda_buf + OFFSET(x8664_pda_level4_pgt)); - if (!VALID_LEVEL4_PGT_ADDR(level4_pgt)) + if (VALID_MEMBER(x8664_pda_level4_pgt)) { + level4_pgt = ULONG(cpu_pda_buf + OFFSET(x8664_pda_level4_pgt)); + if (!VALID_LEVEL4_PGT_ADDR(level4_pgt)) + break; + } + cpunumber = INT(cpu_pda_buf + OFFSET(x8664_pda_cpunumber)); + if (cpunumber != cpus) break; + cpus++; } - cpunumber = INT(cpu_pda_buf + OFFSET(x8664_pda_cpunumber)); - if (cpunumber != cpus) - break; - cpus++; } FREEBUF(cpu_pda_buf); @@ -2828,7 +2882,10 @@ x86_64_display_cpu_data(void) boot_cpu = TRUE; cpus = 1; } - cpu_pda = symbol_value("cpu_pda"); + if (symbol_exists("_cpu_pda")) + cpu_pda = symbol_value("_cpu_pda"); + else if (symbol_exists("cpu_pda")) + cpu_pda = symbol_value("cpu_pda"); for (cpu = 0; cpu < cpus; cpu++) { if (boot_cpu) diff -puN defs.h~crash-fix-cpu-pda defs.h --- crash-4.0-2.19/defs.h~crash-fix-cpu-pda 2006-01-31 17:25:59.479818952 +0530 +++ crash-4.0-2.19-rachita/defs.h 2006-01-31 17:26:23.969096016 +0530 @@ -1840,6 +1840,16 @@ struct load_module { #define PAGEBASE(X) (((ulong)(X)) & (ulong)machdep->pagemask) +#define _CPU_PDA_READ(CPU, BUFFER) \ + ((STRNEQ("_cpu_pda", closest_symbol((symbol_value("_cpu_pda") + \ + ((CPU) * sizeof(unsigned long)))))) && \ + (readmem(symbol_value("_cpu_pda") + ((CPU) * sizeof(unsigned long)),\ + KVADDR, cpu_pda_addr, sizeof(unsigned long), "_cpu_pda addr", \ + FAULT_ON_ERROR)) && \ + (readmem(*cpu_pda_addr , \ + KVADDR, (BUFFER), SIZE(x8664_pda), "cpu_pda entry", \ + FAULT_ON_ERROR))) + #define CPU_PDA_READ(CPU, BUFFER) \ (STRNEQ("cpu_pda", closest_symbol((symbol_value("cpu_pda") + \ ((CPU) * SIZE(x8664_pda))))) && \ _

Badari Pulavarty wrote: > On Tue, 2006-01-31 at 21:38 +0530, Rachita Kothiyal wrote: > > On Tue, Jan 31, 2006 at 09:11:36AM -0500, Dave Anderson wrote: > > > > > > Thanks for handling this. I haven't looked at this, and correct > me > > > if I'm wrong, but the kernel (what version exactly?) now has > contains > > > a _cpu_pda[NR_CPUS] array containing pointers to the actual per- > cpu > > > x8664_pda structures. > > > > Hi Dave, > > > > This change was introduced 2.6.16-rc1 onwards. > > > > > > Unless I'm missing something, that simplifies things and should > work > > > just fine. But I can't test it here other than to verify that > it's > > > backwards compatible. Can you verify that? > > > > Yes, this is infact more simpler and cleaner. I have incorporated > the > > changes and regenerated the patch, which I am sending along. I > have > > also tested it on a 2.6.16-rc1 kernel, and it works fine. > > > > Thanks > > Rachita > > Hmm.. Lots of duplicated code. I hacked the same thing earlier to > make > it work. I incorporated my changes into yours. > > Does this look better ? > > (BTW, is there a way to combine CPU_PDA_READ() and _CPU_PDA_READ() > by > abstracting some of it out ?) > > Thanks, > Badari > > Rachita, Badari, I think I agree about the unnecessary duplication of code in x86_64_cpu_pda_init() and x86_64_get_smp_cpus(). And while abstracting some of it out to CPU_PDA_READ() may be desirable, keeping them separate may make future maintenance and/or understandability of the differences may make it worth keeping them separate. But I don't have a preference either way. However, upon further review, I don't see how this piece of the patch can work for x86_64_display_cpu_data(): @@ -2828,7 +2855,10 @@ x86_64_display_cpu_data(void) boot_cpu = TRUE; cpus = 1; } q - cpu_pda = symbol_value("cpu_pda"); + if (symbol_exists("_cpu_data")) + cpu_pda = symbol_value("_cpu_pda"); + else if (symbol_exists("cpu_data")) + cpu_pda = symbol_value("cpu_pda"); for (cpu = 0; cpu < cpus; cpu++) { if (boot_cpu) because here is how "cpu_data" is used later on in the function:

On Tue, Jan 31, 2006 at 01:18:55PM -0500, Dave Anderson wrote: > Rachita, Badari, > > I think I agree about the unnecessary duplication of code in > x86_64_cpu_pda_init() and x86_64_get_smp_cpus(). That's right Dave. It can be avoided. > > And while abstracting some of it out to CPU_PDA_READ() may be > desirable, keeping them separate may make future maintenance > and/or understandability of the differences may make it worth > keeping them separate. But I don't have a preference either way. > > However, upon further review, I don't see how this piece of the > patch can work for x86_64_display_cpu_data(): > > so not only will it display bogus data when sent to dump_struct(), > the increment at the bottom of the loop would be wrong for the > _cpu_data array. If you just run "mach -c", the x8664_pda display > is bogus, right? > > It would just need get_symbol_data() to be run on the contents of > each entry in the _cpu_data[] array, and that value should be passed > to dump_struct(); and the increment at the bottom would just be the > size of a pointer. Do you mean something like this: @@ -2828,7 +2885,11 @@ x86_64_display_cpu_data(void) boot_cpu = TRUE; cpus = 1; } - cpu_pda = symbol_value("cpu_pda"); + if (symbol_exists("_cpu_pda")) { + __cpu_pda = 1; + cpu_pda = symbol_value("_cpu_pda"); + } else if (symbol_exists("cpu_pda")) + cpu_pda = symbol_value("cpu_pda"); for (cpu = 0; cpu < cpus; cpu++) { if (boot_cpu) @@ -2838,10 +2899,18 @@ x86_64_display_cpu_data(void) dump_struct("cpuinfo_x86", cpu_data, 0); fprintf(fp, "\n"); - dump_struct("x8664_pda", cpu_pda, 0); - + + if ( __cpu_pda == 1 ) { + cpu_pda_addr = 0; + readmem(cpu_pda, KVADDR, &cpu_pda_addr, + sizeof(unsigned long), "_cpu_pda addr", FAULT_ON_ERROR); + dump_struct("x8664_pda", cpu_pda_addr, 0); + cpu_pda += sizeof(unsigned long); + } else { + dump_struct("x8664_pda", cpu_pda, 0); + cpu_pda += SIZE(x8664_pda); + } cpu_data += SIZE(cpuinfo_x86); - cpu_pda += SIZE(x8664_pda); } Thanks Rachita

On Wed, Feb 01, 2006 at 08:47:07AM -0500, Dave Anderson wrote: > > That should do it. > > Minor nits: for clarity's sake, I'd make the local variable have the > same name as the kernel symbol it's representing (i.e. _cpu_data > instead of __cpu_data), just check it as a boolean instead of it > being == 1, and increment cpu_pda by sizeof(void *). Hi Dave Incorporating the suggested changes and resending the patch. Kindly review. Thanks Rachita Signed-off-by: Rachita Kothiyal <rachita(a)in.ibm.com&gt; --- defs.h | 9 +++++++ x86_64.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++---------------- 2 files changed, 67 insertions(+), 19 deletions(-) diff -puN x86_64.c~crash-fix-cpu-pda x86_64.c --- crash-4.0-2.19/x86_64.c~crash-fix-cpu-pda 2006-02-01 20:08:14.000000000 +0530 +++ crash-4.0-2.19-rachita/x86_64.c 2006-02-01 20:59:59.033113672 +0530 @@ -365,9 +365,9 @@ x86_64_dump_machdep_table(ulong arg) static void x86_64_cpu_pda_init(void) { - int i, cpus, nr_pda, cpunumber; + int i, cpus, nr_pda, cpunumber, _cpu_pda; char *cpu_pda_buf; - ulong level4_pgt, data_offset; + ulong level4_pgt, data_offset, cpu_pda_addr; struct syment *sp, *nsp; ulong offset, istacksize; @@ -383,12 +383,26 @@ x86_64_cpu_pda_init(void) cpu_pda_buf = GETBUF(SIZE(x8664_pda)); - if (!(nr_pda = get_array_length("cpu_pda", NULL, 0))) - nr_pda = NR_CPUS; + if (symbol_exists("_cpu_pda")) { + if (!(nr_pda = get_array_length("_cpu_pda", NULL, 0))) + nr_pda = NR_CPUS; + _cpu_pda = TRUE; + } else { + if (!(nr_pda = get_array_length("cpu_pda", NULL, 0))) + nr_pda = NR_CPUS; + _cpu_pda = FALSE; + } for (i = cpus = 0; i < nr_pda; i++) { - if (!CPU_PDA_READ(i, cpu_pda_buf)) - break; + if (_cpu_pda) { + cpu_pda_addr = 0; + if (!_CPU_PDA_READ(i, cpu_pda_buf)) + break;

Rachita Kothiyal wrote: > On Tue, Jan 31, 2006 at 01:18:55PM -0500, Dave Anderson wrote: > > Rachita, Badari, > > > > I think I agree about the unnecessary duplication of code in > > x86_64_cpu_pda_init() and x86_64_get_smp_cpus(). > > That's right Dave. It can be avoided. > > > > > And while abstracting some of it out to CPU_PDA_READ() may be > > desirable, keeping them separate may make future maintenance > > and/or understandability of the differences may make it worth > > keeping them separate. But I don't have a preference either way. > > > > However, upon further review, I don't see how this piece of the > > patch can work for x86_64_display_cpu_data(): > > > > so not only will it display bogus data when sent to dump_struct(), > > the increment at the bottom of the loop would be wrong for the > > _cpu_data array. If you just run "mach -c", the x8664_pda display > > is bogus, right? > > > > It would just need get_symbol_data() to be run on the contents of > > each entry in the _cpu_data[] array, and that value should be passed > > to dump_struct(); and the increment at the bottom would just be the > > size of a pointer. > > Do you mean something like this: > > @@ -2828,7 +2885,11 @@ x86_64_display_cpu_data(void) > boot_cpu = TRUE; > cpus = 1; > } > - cpu_pda = symbol_value("cpu_pda"); > + if (symbol_exists("_cpu_pda")) { > + __cpu_pda = 1; > + cpu_pda = symbol_value("_cpu_pda"); > + } else if (symbol_exists("cpu_pda")) > + cpu_pda = symbol_value("cpu_pda"); > > for (cpu = 0; cpu < cpus; cpu++) { > if (boot_cpu) > @@ -2838,10 +2899,18 @@ x86_64_display_cpu_data(void) > > dump_struct("cpuinfo_x86", cpu_data, 0); > fprintf(fp, "\n"); > - dump_struct("x8664_pda", cpu_pda, 0); > - > + > + if ( __cpu_pda == 1 ) { > + cpu_pda_addr = 0; > + readmem(cpu_pda, KVADDR, &cpu_pda_addr, > + sizeof(unsigned long), "_cpu_pda addr", FAULT_ON_ERROR); > + dump_struct("x8664_pda", cpu_pda_addr, 0); > + cpu_pda += sizeof(unsigned long); > + } else { > + dump_struct("x8664_pda", cpu_pda, 0); > + cpu_pda += SIZE(x8664_pda); > + } > cpu_data += SIZE(cpuinfo_x86); > - cpu_pda += SIZE(x8664_pda); > } > > Thanks > Rachita That should do it. Minor nits: for clarity's sake, I'd make the local variable have the same name as the kernel symbol it's representing (i.e. _cpu_data instead of __cpu_data), just check it as a boolean instead of it being == 1, and increment cpu_pda by sizeof(void *).