Re: [Crash-utility] Re: [RFC]Patch to accomodate cpu_pda changesin x86_64 kernels
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 *).
Thanks again for catching this,
Dave