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

[Badari Pulavarty]

On Wed, 2006-02-01 at 08:47 -0500, Dave Anderson wrote:
> 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 *).

Only reason why I used local variable name as __cpu_pda instead of
using the same name as kernel symbol to easily distinguish both.
When I get error like "Cannot resolve _cpu_pda", I do "grep" to
find out what it is and where its used. If we mix local names with
real kernel symbols, we need to understand more and confusing 
sometimes. 

Anyway, you are the maintainer - whatever you wish :)

Thanks,
Badari