Re: [PATCH RFC 1/2] KVM: x86: generalize guest_cpuid_has_ helpers

From: "Radim Krčmář" <rkrcmar@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH RFC 1/2] KVM: x86: generalize guest_cpuid_has_ helpers
Date: Fri, 4 Aug 2017 22:44:59 +0200	[thread overview]
Message-ID: <20170804204458.GA2119@potion> (raw)
In-Reply-To: <68bcf05b-1798-f4dd-d09b-41248a63e49e@redhat.com>

2017-08-04 17:20+0200, David Hildenbrand:
> On 02.08.2017 22:41, Radim Krčmář wrote:
> > This patch turns guest_cpuid_has_XYZ(cpuid) into guest_cpuid_has(cpuid,
> > X86_FEATURE_XYZ), which gets rid of many very similar helpers.
> > 
> > When seeing a X86_FEATURE_*, we can know which cpuid it belongs to, but
> > this information isn't in common code, so we recreate it for KVM.
> > 
> > Add some BUILD_BUG_ONs to make sure that it runs nicely.
> > 
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> > +static inline int *guest_cpuid_get_register(struct kvm_vcpu *vcpu, unsigned x86_feature)
> >  {
> >  	struct kvm_cpuid_entry2 *best;
> 
> somehow I don't like the name best. entry?

Sure.  This function always returns the entry we wanted, so best is
unfourtunate ...

> > +	struct cpuid_reg cpuid = x86_feature_cpuid(x86_feature);
> 
> you could make this const.

Ok.

> > -/*
> > - * NRIPS is provided through cpuidfn 0x8000000a.edx bit 3
> > - */
> > -#define BIT_NRIPS	3
> > -
> > -static inline bool guest_cpuid_has_nrips(struct kvm_vcpu *vcpu)
> > -{
> > -	struct kvm_cpuid_entry2 *best;
> > -
> > -	best = kvm_find_cpuid_entry(vcpu, 0x8000000a, 0);
> > -
> > -	/*
> > -	 * NRIPS is a scattered cpuid feature, so we can't use
> > -	 * X86_FEATURE_NRIPS here (X86_FEATURE_NRIPS would be bit
> > -	 * position 8, not 3).
> > -	 */
> 
> Is it okay to ignore that comment and use X86_FEATURE_NRIPS in the
> calling code?

X86_FEATURE_NRIPS is not scattered anymore, but I'll mention it in the
commit message. (Scattered feature would BUILD_BUG_ON.)
> 
> > -	return best && (best->edx & bit(BIT_NRIPS));
> > -}
> > -#undef BIT_NRIPS
> > -
> >  static inline int guest_cpuid_family(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_cpuid_entry2 *best;
> 
> 
> > -		if (index >= 0 && guest_cpuid_has_rdtscp(&vmx->vcpu))
> > +		if (index >= 0 && guest_cpuid_has(&vmx->vcpu, X86_FEATURE_MPX))
> 
> X86_FEATURE_RDTSCP ? (or is there an implication I don't know?)

Ugh, copy-paste error ... thanks.