Re: [RFC PATCH 2/3] x86/cpufeatures: Combine word 11 and 12 into new scattered features word 11

[Fenghua Yu <fenghua.yu@intel.com>]

On Fri, Jun 14, 2019 at 07:14:24AM -0700, Sean Christopherson wrote:
> On Fri, Jun 14, 2019 at 03:41:23PM +0200, Borislav Petkov wrote:
> > + Radim and Paolo. See upthread for context.
> > 
> > On Fri, Jun 14, 2019 at 06:17:02AM -0700, Fenghua Yu wrote:
> > > > Alternatively - and what I think is the better solution - would be to
> > > > remove those BUILD_BUG_ONs in x86_feature_cpuid and filter out the
> > > > Linux-defined leafs dynamically. This way the array won't have holes in
> > > > it.
> > > 
> > > Maybe adding a dummy slot in cpuid_leafs in patch 0002 to avoid the
> > > compilation errors?
> > 
> > Maybe you didn't read what you're replying to: "This way the array
> > won't have holes in it". Ontop of yours:
> > 
> > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> > index d78a61408243..03d6f3f7b27c 100644
> > --- a/arch/x86/kvm/cpuid.h
> > +++ b/arch/x86/kvm/cpuid.h
> > @@ -47,6 +47,7 @@ static const struct cpuid_reg reverse_cpuid[] = {
> >  	[CPUID_8000_0001_ECX] = {0x80000001, 0, CPUID_ECX},
> >  	[CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
> >  	[CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
> > +	[CPUID_7_1_EAX]       = {         7, 1, CPUID_EAX},
> >  	[CPUID_8000_0008_EBX] = {0x80000008, 0, CPUID_EBX},
> >  	[CPUID_6_EAX]         = {         6, 0, CPUID_EAX},
> >  	[CPUID_8000_000A_EDX] = {0x8000000a, 0, CPUID_EDX},
> > @@ -59,8 +60,9 @@ static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
> >  {
> >  	unsigned x86_leaf = x86_feature / 32;
> >  
> > -	BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
> > -	BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
> > +	if (x86_leaf == CPUID_LNX_1 ||
> > +	    x86_leaf == CPUID_LNX_4)
> > +		return NULL;
> >  
> >  	return reverse_cpuid[x86_leaf];
> >  }
> > 
> > That's what I mean with filter out dynamically.
> 
> This is wrong.  KVM isn't complaining about shuffling the order of feature
> words, it's complaining that code is trying to do a reverse CPUID lookup
> to a feature that isn't in the reverse_cpuid table.   Filtering out
> checks dynamically is just hiding bugs.
> 
> >In function ‘x86_feature_cpuid’,
> >     inlined from ‘guest_cpuid_get_register’ at arch/x86/kvm/cpuid.h:71:33,
> >     inlined from ‘guest_cpuid_has’ at arch/x86/kvm/cpuid.h:100:8,
> >     inlined from ‘kvm_get_msr_common’ at arch/x86/kvm/x86.c:2804:8:
> 
> This corresponds to "guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)",
> i.e. KVM is trying to query X86_FEATURE_ARCH_CAPABILITIES and is yelling
> that there is no reverse_cpuid entry for CPUID_7_EDX.
> 
> The problem is that 'enum cpuid_leafs' no longer matches up with the
> word numbers defined in cpufeatures.h, e.g. CPUID_7_EDX == 17 or so, but
> the entries in cpufeatures.h defined CPUID_7_EDX flags using word 18.
> 
> This patch also needs to modify NCAPINTS.

Changing NCAPINTS is heavy lifting to only solve this biset issue. After
applying patch 0003, the word 12 hole generated in patch 0002 is filled
in and no issue reported. If changing NCAPINTS in patch 0002, it needs to
be changed back again after applying 0003.

How about add this patch in patch 0002? I posted this patch in this thread
just now and post it here again:

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 526619906305..403f70c2e431 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -23,6 +23,7 @@ enum cpuid_leafs
 	CPUID_7_0_EBX,
 	CPUID_D_1_EAX,
 	CPUID_LNX_4,
+	CPUID_DUMMY,
 	CPUID_8000_0008_EBX,
 	CPUID_6_EAX,
 	CPUID_8000_000A_EDX,

Adding this small patch into patch 0002 will solve the build errors without
changing the build checks.

Thanks.

-Fenghua