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
