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 *).