From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC 01/31] xen/public: Export featureset information in the public API Date: Wed, 23 Dec 2015 10:05:19 +0000 Message-ID: <567A71DF.2070109@citrix.com> References: <1450301073-28191-1-git-send-email-andrew.cooper3@citrix.com> <1450301073-28191-2-git-send-email-andrew.cooper3@citrix.com> <5679885102000078000C25AD@prv-mh.provo.novell.com> <56797D8E.3080408@citrix.com> <56798F7502000078000C2627@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56798F7502000078000C2627@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On 22/12/2015 16:59, Jan Beulich wrote: >>>> On 22.12.15 at 17:42, wrote: >> On 22/12/15 16:28, Jan Beulich wrote: >>>>>> On 16.12.15 at 22:24, 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