Re: [PATCH RFC 04/31] xen/x86: Mask out unknown features from Xen's capabilities

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/12/15 16:42, Jan Beulich wrote:
>>>> On 16.12.15 at 22:24, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -324,6 +324,7 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
>>  	 * we do "generic changes."
>>  	 */
>>  	for (i = 0; i < XEN_NR_FEATURESET_ENTRIES; ++i) {
>> +		c->x86_capability[i] &= known_features[i];
>>  		c->x86_capability[i] ^= inverted_features[i];
>>  	}
> Assert that no unknown features slipped into the inverted ones?

For v2, I will have a small program which makes these assertions, and
interrupts the build if they fail.

Most of the required assertions are on automatically generated values,
or automatically derived values.

> But see also below.
>
>> --- a/xen/arch/x86/cpuid/cpuid-private.h
>> +++ b/xen/arch/x86/cpuid/cpuid-private.h
>> @@ -10,6 +10,32 @@
>>  
>>  #endif
>>  
>> +/* Mask of bits which are shared between 1d and e1d. */
>> +#define SHARED_1d                               \
>> +    (cpufeat_mask(X86_FEATURE_FPU)   |          \
>> +     cpufeat_mask(X86_FEATURE_VME)   |          \
>> +     cpufeat_mask(X86_FEATURE_DE)    |          \
>> +     cpufeat_mask(X86_FEATURE_PSE)   |          \
>> +     cpufeat_mask(X86_FEATURE_TSC)   |          \
>> +     cpufeat_mask(X86_FEATURE_MSR)   |          \
>> +     cpufeat_mask(X86_FEATURE_PAE)   |          \
>> +     cpufeat_mask(X86_FEATURE_MCE)   |          \
>> +     cpufeat_mask(X86_FEATURE_CX8)   |          \
>> +     cpufeat_mask(X86_FEATURE_APIC)  |          \
>> +     cpufeat_mask(X86_FEATURE_MTRR)  |          \
>> +     cpufeat_mask(X86_FEATURE_PGE)   |          \
>> +     cpufeat_mask(X86_FEATURE_MCA)   |          \
>> +     cpufeat_mask(X86_FEATURE_CMOV)  |          \
>> +     cpufeat_mask(X86_FEATURE_PAT)   |          \
>> +     cpufeat_mask(X86_FEATURE_PSE36) |          \
>> +     cpufeat_mask(X86_FEATURE_MMX)   |          \
>> +     cpufeat_mask(X86_FEATURE_FXSR))
> These are shared for AMD, but zero in the extended leaf for Intel.

Indeed.  They make handling of feature dependences rather tricky.

>
>> --- a/xen/arch/x86/cpuid/cpuid.c
>> +++ b/xen/arch/x86/cpuid/cpuid.c
>> @@ -1,5 +1,110 @@
>>  #include "cpuid-private.h"
>>  
>> +const uint32_t known_features[XEN_NR_FEATURESET_ENTRIES] =
>> +{
>> +    [cpufeat_word(X86_FEATURE_FPU)] = (SHARED_1d                           |
>> +                                       cpufeat_mask(X86_FEATURE_SEP)       |
> I can see how you try to remove fixed relationship, but using
> FPU in the array index when no FPU appear in the list is at
> least confusing.
>
> Looking at the rest, adding a new feature will become quite a bit
> more involved. I think we need some better abstraction here, e.g.
> a mechanism similar to the one I used in public/errno.h or the one
> Linux uses to populate its syscall tables or /proc/cpuinfo's feature
> list to allow multiple inclusion of a single list of definitions where all
> properties of each individual feature are maintained on one line.
>
> The tables in this file would then be derived from this (perhaps
> via further steps of machine processing).

Actually, patch  16 "x86: Automatically generate known_features" moves
this into the python script which flattens dependencies.

That patch also has a TODO to reorder the series, which would drop most
of this patch.

~Andrew