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

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

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>
> ---
>  arch/x86/kvm/cpuid.h | 202 +++++++++++++++++----------------------------------
>  arch/x86/kvm/mmu.c   |   7 +-
>  arch/x86/kvm/mtrr.c  |   2 +-
>  arch/x86/kvm/svm.c   |   2 +-
>  arch/x86/kvm/vmx.c   |  26 +++----
>  arch/x86/kvm/x86.c   |  38 +++++-----
>  6 files changed, 105 insertions(+), 172 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index da6728383052..3b17d915b608 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -3,6 +3,7 @@
>  
>  #include "x86.h"
>  #include <asm/cpu.h>
> +#include <asm/processor.h>
>  
>  int kvm_update_cpuid(struct kvm_vcpu *vcpu);
>  bool kvm_mpx_supported(void);
> @@ -29,95 +30,78 @@ static inline int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
>  	return vcpu->arch.maxphyaddr;
>  }
>  
> -static inline bool guest_cpuid_has_xsave(struct kvm_vcpu *vcpu)
> +struct cpuid_reg {
> +	u32 function;
> +	u32 index;
> +	int reg;
> +};
> +
> +static const struct cpuid_reg reverse_cpuid[] = {
> +	[CPUID_1_EDX]         = {         1, 0, CPUID_EDX},
> +	[CPUID_8000_0001_EDX] = {0x80000001, 0, CPUID_EDX},
> +	[CPUID_8086_0001_EDX] = {0x80860001, 0, CPUID_EDX},
> +	[CPUID_1_ECX]         = {         1, 0, CPUID_ECX},
> +	[CPUID_C000_0001_EDX] = {0xc0000001, 0, CPUID_EDX},
> +	[CPUID_8000_0001_ECX] = {0xc0000001, 0, CPUID_ECX},
> +	[CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
> +	[CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
> +	[CPUID_F_0_EDX]       = {       0xf, 0, CPUID_EDX},
> +	[CPUID_F_1_EDX]       = {       0xf, 1, CPUID_EDX},
> +	[CPUID_8000_0008_EBX] = {0x80000008, 0, CPUID_EBX},
> +	[CPUID_6_EAX]         = {         6, 0, CPUID_EAX},
> +	[CPUID_8000_000A_EDX] = {0x8000000a, 0, CPUID_EDX},
> +	[CPUID_7_ECX]         = {         7, 0, CPUID_ECX},
> +	[CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX},
> +};
> +
> +static inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
> +{
> +	unsigned x86_leaf = x86_feature / 32;
> +
> +	BUILD_BUG_ON(!__builtin_constant_p(x86_leaf));
> +	BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
> +	BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
> +
> +	return reverse_cpuid[x86_leaf];
> +}
> +
> +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?

> +	struct cpuid_reg cpuid = x86_feature_cpuid(x86_feature);

you could make this const.

>  
> -	if (!static_cpu_has(X86_FEATURE_XSAVE))
> +	best = kvm_find_cpuid_entry(vcpu, cpuid.function, cpuid.index);
> +	if (!best)
> +		return NULL;
> +
> +	switch (cpuid.reg) {
> +	case CPUID_EAX:
> +		return &best->eax;
> +	case CPUID_EBX:
> +		return &best->ebx;
> +	case CPUID_ECX:
> +		return &best->ecx;
> +	case CPUID_EDX:
> +		return &best->edx;
> +	default:
> +		BUILD_BUG();
> +		return NULL;
> +	}
> +}
> +

[...]

> -/*
> - * 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?

> -	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?)

>  			move_msr_up(vmx, index, save_nmsrs++);
>  		/*
>  		 * MSR_STAR is only needed on long mode guests, and only

> -		if (!guest_cpuid_has_rdtscp(vcpu) && !msr_info->host_initiated)
> +		if (!msr_info->host_initiated &&
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_MPX))

X86_FEATURE_RDTSCP ?

>  			return 1;
>  		/* Otherwise falls through */
>  	default:
> @@ -3307,7 +3303,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		break;
>  	case MSR_IA32_BNDCFGS:
>  		if (!kvm_mpx_supported() ||
> -		    (!msr_info->host_initiated && !guest_cpuid_has_mpx(vcpu)))
> +		    (!msr_info->host_initiated &&
> +		     !guest_cpuid_has(vcpu, X86_FEATURE_MPX)))
>  			return 1;
>  		if (is_noncanonical_address(data & PAGE_MASK) ||
>  		    (data & MSR_IA32_BNDCFGS_RSVD))
> @@ -3370,7 +3367,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			clear_atomic_switch_msr(vmx, MSR_IA32_XSS);
>  		break;
>  	case MSR_TSC_AUX:
> -		if (!guest_cpuid_has_rdtscp(vcpu) && !msr_info->host_initiated)
> +		if (!msr_info->host_initiated &&
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_MPX))

dito

>  			return 1;
>  		/* Check reserved bit, higher 32 bits should be zero */
>  		if ((data >> 32) != 0)
> @@ -9383,7 +9381,7 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>  	u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
>  
>  	if (vmx_rdtscp_supported()) {
> -		bool rdtscp_enabled = guest_cpuid_has_rdtscp(vcpu);
> +		bool rdtscp_enabled = guest_cpuid_has(vcpu, X86_FEATURE_MPX);

dito

>  		if (!rdtscp_enabled)

-- 

Thanks,

David