From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v2 11/30] xen/x86: Calculate maximum host and guest featuresets Date: Mon, 15 Feb 2016 08:07:44 -0700 Message-ID: <56C1F7D002000078000D233A@prv-mh.provo.novell.com> References: <1454679743-18133-1-git-send-email-andrew.cooper3@citrix.com> <1454679743-18133-12-git-send-email-andrew.cooper3@citrix.com> <56C1E2C602000078000D2231@prv-mh.provo.novell.com> <56C1E758.20207@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56C1E758.20207@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Xen-devel List-Id: xen-devel@lists.xenproject.org >>> On 15.02.16 at 15:57, wrote: > On 15/02/16 13:37, Jan Beulich wrote: >>>>> On 05.02.16 at 14:42, wrote: >>> --- a/xen/arch/x86/cpuid.c >>> +++ b/xen/arch/x86/cpuid.c >>> @@ -1,13 +1,165 @@ >>> #include >>> #include >>> +#include >>> +#include >>> +#include >>> + >>> +#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 >>> >>> 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