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

[Pavel Emelyanov]

On 12/12/2012 06:04 PM, Cyrill Gorcunov wrote:
> 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?

No. Since this function checks some _specific_ (_hardcoded_) feature, it should
be named respectively.

>>> +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?
> 
>>From changelog
> 
>      - adds ability to find some of rt-features the _cpu_ supports. In
>        particular if xsave insn is supported we need to know how big memory
>        slab is needed to keep complete fpu frame
> 
> this gives us the size of fpu frame. Moreover, since we need to call cpuid
> anyway, we do a fast early check for features we need from cpuid results.
> If we have bits needed on hw level, we still need to check if kernel was not booted
> with "nofxsr", which disables fxsave instruction and prevents us from continue
> working, if everything is fine -- we can do restore.

That's OK, but why not use _one_ way of getting this -- either cpuid or proc?
Why both?

> 	Cyrill
> .
>