From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC 04/31] xen/x86: Mask out unknown features from Xen's capabilities Date: Tue, 22 Dec 2015 17:01:15 +0000 Message-ID: <567981DB.50503@citrix.com> References: <1450301073-28191-1-git-send-email-andrew.cooper3@citrix.com> <1450301073-28191-5-git-send-email-andrew.cooper3@citrix.com> <56798B6A02000078000C25F9@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: <56798B6A02000078000C25F9@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: Xen-devel List-Id: xen-devel@lists.xenproject.org On 22/12/15 16:42, Jan Beulich wrote: >>>> On 16.12.15 at 22:24, wrote: >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -324,6 +324,7 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c) >> * we do "generic changes." >> */ >> for (i = 0; i < XEN_NR_FEATURESET_ENTRIES; ++i) { >> + c->x86_capability[i] &= known_features[i]; >> c->x86_capability[i] ^= inverted_features[i]; >> } > Assert that no unknown features slipped into the inverted ones? For v2, I will have a small program which makes these assertions, and interrupts the build if they fail. Most of the required assertions are on automatically generated values, or automatically derived values. > But see also below. > >> --- a/xen/arch/x86/cpuid/cpuid-private.h >> +++ b/xen/arch/x86/cpuid/cpuid-private.h >> @@ -10,6 +10,32 @@ >> >> #endif >> >> +/* Mask of bits which are shared between 1d and e1d. */ >> +#define SHARED_1d \ >> + (cpufeat_mask(X86_FEATURE_FPU) | \ >> + cpufeat_mask(X86_FEATURE_VME) | \ >> + cpufeat_mask(X86_FEATURE_DE) | \ >> + cpufeat_mask(X86_FEATURE_PSE) | \ >> + cpufeat_mask(X86_FEATURE_TSC) | \ >> + cpufeat_mask(X86_FEATURE_MSR) | \ >> + cpufeat_mask(X86_FEATURE_PAE) | \ >> + cpufeat_mask(X86_FEATURE_MCE) | \ >> + cpufeat_mask(X86_FEATURE_CX8) | \ >> + cpufeat_mask(X86_FEATURE_APIC) | \ >> + cpufeat_mask(X86_FEATURE_MTRR) | \ >> + cpufeat_mask(X86_FEATURE_PGE) | \ >> + cpufeat_mask(X86_FEATURE_MCA) | \ >> + cpufeat_mask(X86_FEATURE_CMOV) | \ >> + cpufeat_mask(X86_FEATURE_PAT) | \ >> + cpufeat_mask(X86_FEATURE_PSE36) | \ >> + cpufeat_mask(X86_FEATURE_MMX) | \ >> + cpufeat_mask(X86_FEATURE_FXSR)) > These are shared for AMD, but zero in the extended leaf for Intel. Indeed. They make handling of feature dependences rather tricky. > >> --- a/xen/arch/x86/cpuid/cpuid.c >> +++ b/xen/arch/x86/cpuid/cpuid.c >> @@ -1,5 +1,110 @@ >> #include "cpuid-private.h" >> >> +const uint32_t known_features[XEN_NR_FEATURESET_ENTRIES] = >> +{ >> + [cpufeat_word(X86_FEATURE_FPU)] = (SHARED_1d | >> + cpufeat_mask(X86_FEATURE_SEP) | > I can see how you try to remove fixed relationship, but using > FPU in the array index when no FPU appear in the list is at > least confusing. > > Looking at the rest, adding a new feature will become quite a bit > more involved. I think we need some better abstraction here, e.g. > a mechanism similar to the one I used in public/errno.h or the one > Linux uses to populate its syscall tables or /proc/cpuinfo's feature > list to allow multiple inclusion of a single list of definitions where all > properties of each individual feature are maintained on one line. > > The tables in this file would then be derived from this (perhaps > via further steps of machine processing). Actually, patch 16 "x86: Automatically generate known_features" moves this into the python script which flattens dependencies. That patch also has a TODO to reorder the series, which would drop most of this patch. ~Andrew