Re: [PATCH, i386]: Add support for CPUID 4 in driver-i386.c

[Andi Kleen <andi at firstfloor dot org>]

n Sat, Oct 11, 2008 at 07:56:23PM +0200, Uros Bizjak wrote:
> Hello!
> 
> This patch is loosely based on Andi's patch to use CPUID function 4 to 
> describe Intel caches. In addition to parsing CPUID function 4 values, 
> attached patch updates CPUID 2 table to the latest known CPUID 
> documentation, it fixes Xeon MP CPUID fn 2 problems and handles number 
> of times to execute CPUID fn 2 instruction to obtain complete cache 
> hierarchy description.

I suspect the CPUID 2 table update is not really needed, because
most CPUs now can describe themselves using CPUID 4.

The xeon_mp variable is quite misnamed because there are lots of quite
different SKUs calling themselves that. Also it wouldn't surprise
me if that CPU already had CPUID 4, so the hack is likely not needed.

But the big open question is what to do with level2 vs level3?
Perhaps pass both as arguments and let the optimization passes decide?

> +#define __cpuid_count(level, count, a, b, c, d)		\
> +  __asm__ ("xchgl\t%%ebx, %1\n\t"			\
> +	   "cpuid\n\t"					\
> +	   "xchgl\t%%ebx, %1\n\t"			\
> +	   : "=a" (a), "=r" (b), "=c" (c), "=d" (d)	\
> +	   : "0" (level), "2" (count))

I don't think a PIC cpuid_count is really needed.

> +  for (i = 24; i >= 0; i -= 8)
> +    switch ((reg >> i) & 0xff)
> +      {
> +      case 0x0a:

As a personal nit if you're going to redo this anyways then
a lookup table would be much shorter than code.

> +	case CACHE_END:
> +	  return;
> +	case CACHE_DATA:
> +	case CACHE_UNIFIED:
> +	  {
> +	    switch ((eax >> 5) & 0x07)
> +	      {
> +	      case 1:
> +		cache = level1;
> +		break;
> +	      case 2:
> +		cache = level2;
> +		break;
> +	      default:
> +		cache = NULL;

That seems like a weird value for a enum

-Andi
-- 
ak@linux.intel.com