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

Radim Krčmář
2017-08-04T20:45:10Z

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.