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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.16 at 15:57, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Of course. My comment was only about the additions done here.

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

I.e. you're not later going to use what you generate here to also
massage (or at least validate) what guests are going to see?

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

Right, since boot_cpu_data.x86_capability[] cannot, according to
earlier patches, have less than FSCAPINTS elements.

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

With the few(?) compiler versions you managed to test against, I
suppose...

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

Okay then.

Jan

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