Re: [PATCH v2 11/30] xen/x86: Calculate maximum host and guest featuresets

["Jan Beulich" <JBeulich@suse.com>]

>>> On 05.02.16 at 14:42, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -1,13 +1,165 @@
>  #include <xen/lib.h>
>  #include <asm/cpuid.h>
> +#include <asm/hvm/hvm.h>
> +#include <asm/hvm/vmx/vmcs.h>
> +#include <asm/processor.h>
> +
> +#define COMMON_1D INIT_COMMON_FEATURES
>  
>  const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>  const uint32_t inverted_features[] = INIT_INVERTED_FEATURES;
>  
> +static const uint32_t pv_featuremask[] = INIT_PV_FEATURES;
> +static const uint32_t hvm_shadow_featuremask[] = INIT_HVM_SHADOW_FEATURES;
> +static const uint32_t hvm_hap_featuremask[] = INIT_HVM_HAP_FEATURES;

Considering that calculate_featuresets() gets called from
__start_xen(), that function and some others as well as the
above could apparently all live in .init.* sections.

> +uint32_t __read_mostly raw_featureset[FSCAPINTS];
> +uint32_t __read_mostly host_featureset[FSCAPINTS];
> +uint32_t __read_mostly pv_featureset[FSCAPINTS];
> +uint32_t __read_mostly hvm_featureset[FSCAPINTS];
> +
> +static void sanitise_featureset(uint32_t *fs)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < FSCAPINTS; ++i )
> +    {
> +        /* Clamp to known mask. */
> +        fs[i] &= known_features[i];
> +    }
> +
> +    switch ( boot_cpu_data.x86_vendor )
> +    {
> +    case X86_VENDOR_INTEL:
> +        /* Intel clears the common bits in e1d. */
> +        fs[FEATURESET_e1d] &= ~COMMON_1D;
> +        break;
> +
> +    case X86_VENDOR_AMD:
> +        /* AMD duplicates the common bits between 1d and e1d. */
> +        fs[FEATURESET_e1d] = ((fs[FEATURESET_1d]  &  COMMON_1D) |
> +                              (fs[FEATURESET_e1d] & ~COMMON_1D));
> +        break;
> +    }

How is this meant to work with cross vendor migration?

> +static void calculate_raw_featureset(void)
> +{
> +    unsigned int i, max, tmp;
> +
> +    max = cpuid_eax(0);
> +
> +    if ( max >= 1 )
> +        cpuid(0x1, &tmp, &tmp,
> +              &raw_featureset[FEATURESET_1c],
> +              &raw_featureset[FEATURESET_1d]);
> +    if ( max >= 7 )
> +        cpuid_count(0x7, 0, &tmp,
> +                    &raw_featureset[FEATURESET_7b0],
> +                    &raw_featureset[FEATURESET_7c0],
> +                    &tmp);
> +    if ( max >= 0xd )
> +        cpuid_count(0xd, 1,
> +                    &raw_featureset[FEATURESET_Da1],
> +                    &tmp, &tmp, &tmp);
> +
> +    max = cpuid_eax(0x80000000);
> +    if ( max >= 0x80000001 )

I don't recall where it was that I recently stumbled across a similar
check, but this is dangerous: Instead of >= this should check that
the upper 16 bits equal 0x8000 and the lower ones are >= 1.

> +static void calculate_host_featureset(void)
> +{
> +    memcpy(host_featureset, boot_cpu_data.x86_capability,
> +           sizeof(host_featureset));
> +}

Why not simply

#define host_featureset boot_cpu_data.x86_capability

?

> +static void calculate_pv_featureset(void)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
> +        pv_featureset[i] = host_featureset[i] & pv_featuremask[i];

I think when two arrays are involved simply using FSCAPINTS
as the upper bound would be more appropriate (and shorter).

> +static void calculate_hvm_featureset(void)
> +{
> +    unsigned int i;
> +    const uint32_t *hvm_featuremask;
> +
> +    if ( !hvm_enabled )
> +        return;
> +
> +    hvm_featuremask = hvm_funcs.hap_supported ?
> +        hvm_hap_featuremask : hvm_shadow_featuremask;
> +
> +    for ( i = 0; i < ARRAY_SIZE(hvm_featureset); ++i )
> +        hvm_featureset[i] = host_featureset[i] & hvm_featuremask[i];
> +
> +    /* Unconditionally claim to be able to set the hypervisor bit. */
> +    __set_bit(X86_FEATURE_HYPERVISOR, hvm_featureset);
> +
> +    /*
> +     * On AMD, PV guests are entirely unable to use 'sysenter' as Xen runs in
> +     * long mode (and init_amd() has cleared it out of host capabilities), but
> +     * HVM guests are able if running in protected mode.
> +     */
> +    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
> +         test_bit(X86_FEATURE_SEP, raw_featureset) )
> +        __set_bit(X86_FEATURE_SEP, hvm_featureset);
> +
> +    /*
> +     * With VT-x, some features are only supported by Xen if dedicated
> +     * hardware support is also available.
> +     */
> +    if ( cpu_has_vmx )
> +    {
> +        if ( !(vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) ||
> +             !(vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS) )
> +            __clear_bit(X86_FEATURE_MPX, hvm_featureset);
> +
> +        if ( !cpu_has_vmx_xsaves )
> +            __clear_bit(X86_FEATURE_XSAVES, hvm_featureset);
> +
> +        if ( !cpu_has_vmx_pcommit )
> +            __clear_bit(X86_FEATURE_PCOMMIT, hvm_featureset);
> +    }
> +
> +    sanitise_featureset(pv_featureset);

s/pv_/hvm_/ ?

>  static void __maybe_unused build_assertions(void)

While affecting an earlier patch - __init?

> --- a/xen/include/asm-x86/cpuid.h
> +++ b/xen/include/asm-x86/cpuid.h
> @@ -5,12 +5,29 @@
>  
>  #define FSCAPINTS FEATURESET_NR_ENTRIES
>  
> +#define FEATURESET_1d     0 /* 0x00000001.edx      */
> +#define FEATURESET_1c     1 /* 0x00000001.ecx      */
> +#define FEATURESET_e1d    2 /* 0x80000001.edx      */
> +#define FEATURESET_e1c    3 /* 0x80000001.ecx      */
> +#define FEATURESET_Da1    4 /* 0x0000000d:1.eax    */
> +#define FEATURESET_7b0    5 /* 0x00000007:0.ebx    */
> +#define FEATURESET_7c0    6 /* 0x00000007:0.ecx    */
> +#define FEATURESET_e7d    7 /* 0x80000007.edx      */
> +#define FEATURESET_e8b    8 /* 0x80000008.ebx      */
> +
>  #ifndef __ASSEMBLY__
>  #include <xen/types.h>
>  
>  extern const uint32_t known_features[FSCAPINTS];
>  extern const uint32_t inverted_features[FSCAPINTS];
>  
> +extern uint32_t raw_featureset[FSCAPINTS];
> +extern uint32_t host_featureset[FSCAPINTS];
> +extern uint32_t pv_featureset[FSCAPINTS];
> +extern uint32_t hvm_featureset[FSCAPINTS];

I wonder whether it wouldn't be better to make these accessible
only via function calls, with the functions returning pointers to
const, to avoid seducing people into fiddling with these from
outside cpuid.c.

Jan