Re: [git-pull -tip] x86: cpu_debug patches

[Ingo Molnar]

* Jaswinder Singh Rajput <jaswinder@xxxxxxxxxx> wrote:

> On Wed, 2009-04-29 at 12:50 +0200, Ingo Molnar wrote:
> > * Jaswinder Singh Rajput <jaswinder@xxxxxxxxxx> wrote:
> > 
> > > On Tue, 2009-04-28 at 19:28 +0200, Ingo Molnar wrote:
> > > > * Jaswinder Singh Rajput <jaswinder@xxxxxxxxxx> wrote:
> > > > 
> > > > > @@ -850,10 +903,10 @@ static int cpu_init_cpu(void)
> > > > >  		cpui = &cpu_data(cpu);
> > > > >  		if (!cpu_has(cpui, X86_FEATURE_MSR))
> > > > >  			continue;
> > > > > -		per_cpu(cpu_model, cpu) = ((cpui->x86_vendor << 16) |
> > > > > -					   (cpui->x86 << 8) |
> > > > > -					   (cpui->x86_model));
> > > > > -		per_cpu(cpu_modelflag, cpu) = get_cpu_modelflag(cpu);
> > > > > +		per_cpu(cpu_modelflag, cpu) = get_cpu_flag(cpui);
> > > > > +		if (!per_cpu(cpu_modelflag, cpu))
> > > > > +			send_report(per_cpu(cpu_priv_count, cpu), cpui);
> > > > 
> > > > This means that if the CPU is not enumerated in the model table 
> > > > explicitly, we'll fall back to some really minimal output, right?
> > > > 
> > > 
> > > Yes.
> > 
> > That's a bug really: it means that for every new CPU type that comes 
> > around we need to update this code. I.e. precisely for those CPUs 
> > where we might need the most help from such a debug facility, we 
> > wont have much info to look at ... New CPUs generally support all 
> > the CPU features that are displayed here, in a compatible manner.
> > 
> > So that needs to be improved/changed to not be tied to such a static 
> > 'cpu model' enumeration but instead be CPU feature flags driven. See 
> > all the existing cpu_has_*() tests we have.
> > 
> 
> cpu_has_*() is based on boot_cpu. So you mean cpu_has tests. right ?

Correct.

> We can use cpu_has tests for unknown processors but 'cpu model' is 
> accurate and cover all range.

So is cpu_has. If it's not accurate then various other pieces of x86 
code might break as well.

> cpu_has does not cover following registers:
> 1. platform
> 2. poweron
> 3. control
> 4. bios
> 5. freq
> 6. cache
> 7. misc
> 8. base
> 9. ver
> 10. conf
> 
> So is this looks OK to you:
> 
> 1. first check for 'cpu model' if CPU not supported then goto 2
> 2. check for cpu_has tests 

No, please get rid of the 'cpu model' based feature tests altogether 
and extend the _existing_ feature flags with the above details, when 
needed.

That way we reuse the existing cpu_has checks and extend them. 
Instead of implementing a private facility in cpu_debug.c.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/