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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.12.15 at 22:24, <andrew.cooper3@xxxxxxxxxx> 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?
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.

> --- 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).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel