Re: [PATCH RFC 01/31] xen/public: Export featureset information in the public API

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

On 22/12/2015 16:59, Jan Beulich wrote:
>>>> On 22.12.15 at 17:42, <andrew.cooper3@citrix.com> wrote:
>> On 22/12/15 16:28, Jan Beulich wrote:
>>>>>> On 16.12.15 at 22:24, <andrew.cooper3@citrix.com> wrote:
>>>> +/* VIA/Cyrix/Centaur-defined CPU features, CPUID level 0xC0000001, FSMAX+2 */
>>>> +#define X86_FEATURE_XSTORE	((FSMAX+1)*32+ 2) /* on-CPU RNG present (xstore insn) */
>>>> +#define X86_FEATURE_XSTORE_EN	((FSMAX+1)*32+ 3) /* on-CPU RNG enabled */
>>>> +#define X86_FEATURE_XCRYPT	((FSMAX+1)*32+ 6) /* on-CPU crypto (xcrypt insn) */
>>>> +#define X86_FEATURE_XCRYPT_EN	((FSMAX+1)*32+ 7) /* on-CPU crypto enabled */
>>>> +#define X86_FEATURE_ACE2	((FSMAX+1)*32+ 8) /* Advanced Cryptography Engine v2 */
>>>> +#define X86_FEATURE_ACE2_EN	((FSMAX+1)*32+ 9) /* ACE v2 enabled */
>>>> +#define X86_FEATURE_PHE		((FSMAX+1)*32+10) /* PadLock Hash Engine */
>>>> +#define X86_FEATURE_PHE_EN	((FSMAX+1)*32+11) /* PHE enabled */
>>>> +#define X86_FEATURE_PMM		((FSMAX+1)*32+12) /* PadLock Montgomery Multiplier */
>>>> +#define X86_FEATURE_PMM_EN	((FSMAX+1)*32+13) /* PMM enabled */
>>> None of these get consumed anywhere - if you already touch this
>>> code, just drop all of them.
>> X86_FEATURE_XSTORE gets used in xen/arch/x86/cpu/centaur.c to stash word
>> 0xC0000001, but nothing actually reads the stashed values.
>>
>> I could do a precursor patch which drops the stashing of this
>> information, which will result in NCAPINTS getting shorter by the end of
>> the series.
> Yes, that's what I meant to imply.
>
>>>> +/*
>>>> + * CPUID leaf shorthand:
>>>> + * - optional 'e', Extended (0x8xxxxxxx)
>>>> + * - leaf, uppercase hex
>>>> + * - register, lowercase
>>>> + * - optional subleaf, uppercase hex
>>>> + */
>>>> +#define XEN_FEATURESET_1d     0 /* 0x00000001.edx      */
>>>> +#define XEN_FEATURESET_1c     1 /* 0x00000001.ecx      */
>>>> +#define XEN_FEATURESET_e1d    2 /* 0x80000001.edx      */
>>>> +#define XEN_FEATURESET_e1c    3 /* 0x80000001.ecx      */
>>>> +#define XEN_FEATURESET_Da1    4 /* 0x0000000d:1.eax    */
>>>> +#define XEN_FEATURESET_7b0    5 /* 0x00000007:0.ebx    */
>>> This ends up being pretty cryptic.
>> Perhaps at a first glance, but there are so many uses that a shorthand
>> really is needed.  See especially the MSR masking patches towards the
>> end of the series.
> I understand that something short is needed. But as the main
> identifier in the ABI?

It really comes down to whom is intended to do what with a featureset.

These values are derivable as functions of the feature values
themselves, but that requires an assumption that the featureset bitmap
mirrors hardware precisely.  Providing this (names not withstanding) as
a part of the ABI is intended to provide that assurance.

I fully intend higher levels of the toolstack to deal with a featureset
as an arbitrary bitmap, but lower levels like libxc do need a greater
level of understanding of its layout.

>
>>>> +#define XEN_NR_FEATURESET_ENTRIES (XEN_FEATURESET_7b0 + 1)
>>> This shouldn't be exposed, as it will necessarily change sooner or later.
>> Hmm yes - I think I can alter where this lives, although libxc does need
>> to be able to get this value.
> Perhaps keep it where it is, but put it inside #ifdef __XEN__?

I reckon it would be better in cpuid-private.h, strictly making it
accessible only to to the components sharing that library.

~Andrew