[CRIU] [PATCH 06/22] cpu: Prepare scaffold for obtaining cpu features

[Cyrill Gorcunov]

On Wed, Dec 12, 2012 at 05:47:29PM +0400, Pavel Emelyanov wrote:
> > +#define X86_FEATURE_MTRR		(0*32+12) /* Memory Type Range Registers */
> > +#define X86_FEATURE_PGE			(0*32+13) /* Page Global Enable */
> > +#define X86_FEATURE_MCA			(0*32+14) /* Machine Check Architecture */
> > +#define X86_FEATURE_CMOV		(0*32+15) /* CMOV instructions */
> > +						  /* (plus FCMOVcc, FCOMI with FPU) */
> 
> Why do we need all this crap, while we use only 3 of them?

These are common bits and I don't know which else we might need in near
future. Keeping the whole cpuid infrastructure close to one implemented
in kernel is certainly a win.

> > +int cpu_parse_flags(CpuFeature *feature, char *line)
> > +{
> > +	char *tok;
> > +
> > +	for (tok = strtok(line, " \t\n"); tok;
> > +	     tok = strtok(NULL, " \t\n")) {
> > +		if (!strcmp(tok, x86_cap_flags[X86_FEATURE_FXSR]))
> > +			feature->fxsr = true;
> > +		else if (!strcmp(tok, x86_cap_flags[X86_FEATURE_XSAVE]))
> > +			feature->xsave = true;
> > +		else if (!strcmp(tok, x86_cap_flags[X86_FEATURE_FPU]))
> > +			feature->fpu = true;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Should be in proc_parse.c

Hmmm, well I did it on purpose to relax proc_parse from  specific code,
but sure I can update it.

> > +int cpu_fill_feature(CpuFeature *feature)
> > +{
> > +	/*
> > +	 * Simply fills the cache if nil passed.
> > +	 */
> > +
> > +	if (feature) {
> > +		cpu_feature__init(feature);
> > +		if (likely(use_cached_feature)) {
> > +			feature->fpu	= cached_feature.fpu;
> > +			feature->fxsr	= cached_feature.fxsr;
> > +			feature->xsave	= cached_feature.xsave;
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	if (parse_cpuinfo_features(&cached_feature))
> > +		return -1;
> > +
> > +	use_cached_feature = true;
> > +
> > +	if (!feature)
> > +		return 0;
> > +
> > +	return cpu_fill_feature(feature);
> 
> This is awful :( This kind of code is typically written like
> 
> if (!cache)
> 	cache = refill_cache
> 
> val = cache

Doesn't look that awful for me, but sure, will update.

> > +bool cpu_check_feature(CpuFeature *feature)
> > +{

> Name of function is bad.

Would cpu_verify_feature make you happy?

> > +int cpu_rt_init(void)
> > +{
> > +	u32 eax, ebx, ecx, edx;
> > +
> > +	/*
> > +	 * Make sure the cpu we're running on
> > +	 * can run FPU related stuff.
> > +	 */
> > +	cpuid(1, &eax, &ebx, &ecx, &edx);
> > +	((u32 *)rt_cpu_features)[0] = edx;
> > +	((u32 *)rt_cpu_features)[4] = ecx;
> > +
> 
> First we call cpuid...
> 
> > +	if (cpu_fill_feature(NULL))
> > +		return -1;
> > +
> > +	if (!cpu_check_feature(&cached_feature))
> > +		return -1;
> > +
> > +	return 0;
> > +}
> 
> ... then read /proc/cpuinfo. What for?