[Crash-utility] Re: [RFC]Patch to accomodate cpu_pda changesin x86_64 kernels

[Dave Anderson]

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