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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/02/16 13:37, Jan Beulich wrote:
>>>> On 05.02.16 at 14:42, <andrew.cooper3@xxxxxxxxxx> 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.

known and inverted features are used from the AP bringup path, so can't
be init.

calculate_featureset(), and these _featuremask[] arrays can be init.

>
>> +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?

I don't see how cross-vendor is relevant here.  This is about reporting
the hosts modified featureset accurately to the toolstack.

Once the deep dependency logic logic is inserted in here, it is possible
that some of the common features are modified, at which point their
shared nature on AMD needs re-calculating.

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

Ok.

>
>> +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

ARRAY_SIZE(host_featureset) changes, although I guess this won't matter
if I standardise on FSCAPINTS.

>
> ?
>
>> +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_/ ?

Oops yes.

>
>>  static void __maybe_unused build_assertions(void)
> While affecting an earlier patch - __init?

Can do, but nothing from this actually gets emitted into a translation unit.

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

That does make it more awkward to iterate over, although I suppose I
don't do too much of that outside of cpuid.c

Also, without LTO, the function calls can't actually be omitted.  I am
tempted to leave it as-is and rely on code review to catch miuses.

~Andrew

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