Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid().

[Paolo Bonzini]

On 17/08/2017 21:52, Yu Zhang wrote:
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index ac15193..3e759cf 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -21,7 +21,14 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>  int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
>  			      struct kvm_cpuid2 *cpuid,
>  			      struct kvm_cpuid_entry2 __user *entries);
> -void kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
> +
> +enum {
> +	NO_CHECK_LIMIT = 0,
> +	CHECK_LIMIT = 1,
> +};

emulate.c should not include cpuid.h.  The argument can be simply a
bool, though.

> +bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
> +	       u32 *ecx, u32 *edx, int check_limit);
>  
>  int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
>  
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index fb00559..46daa37 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -28,6 +28,7 @@
>  
>  #include "x86.h"
>  #include "tss.h"
> +#include "cpuid.h"
>  
>  /*
>   * Operand types
> @@ -2333,8 +2334,10 @@ static int emulator_has_longmode(struct x86_emulate_ctxt *ctxt)
>  
>  	eax = 0x80000001;
>  	ecx = 0;
> -	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> -	return edx & bit(X86_FEATURE_LM);
> +	if (ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, NO_CHECK_LIMIT))
> +		return edx & bit(X86_FEATURE_LM);
> +	else
> +		return 0;
>  }
>  
>  #define GET_SMSTATE(type, smbase, offset)				  \
> @@ -2636,7 +2639,7 @@ static bool vendor_intel(struct x86_emulate_ctxt *ctxt)
>  	u32 eax, ebx, ecx, edx;
>  
>  	eax = ecx = 0;
> -	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> +	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, NO_CHECK_LIMIT);
>  	return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx
>  		&& ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx
>  		&& edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx;
> @@ -2656,7 +2659,7 @@ static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt)
>  
>  	eax = 0x00000000;
>  	ecx = 0x00000000;
> -	ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> +	ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, NO_CHECK_LIMIT);
>  	/*
>  	 * Intel ("GenuineIntel")
>  	 * remark: Intel CPUs only support "syscall" in 64bit
> @@ -3551,7 +3554,7 @@ static int em_movbe(struct x86_emulate_ctxt *ctxt)
>  	/*
>  	 * Check MOVBE is set in the guest-visible CPUID leaf.
>  	 */
> -	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> +	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, CHECK_LIMIT);

This should be NO_CHECK_LIMIT.

Otherwise okay!

Paolo

>  	if (!(ecx & FFL(MOVBE)))
>  		return emulate_ud(ctxt);
>  
> @@ -3865,7 +3868,7 @@ static int em_cpuid(struct x86_emulate_ctxt *ctxt)
>  
>  	eax = reg_read(ctxt, VCPU_REGS_RAX);
>  	ecx = reg_read(ctxt, VCPU_REGS_RCX);
> -	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> +	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, CHECK_LIMIT);
>  	*reg_write(ctxt, VCPU_REGS_RAX) = eax;
>  	*reg_write(ctxt, VCPU_REGS_RBX) = ebx;
>  	*reg_write(ctxt, VCPU_REGS_RCX) = ecx;
> @@ -3924,7 +3927,7 @@ static int check_fxsr(struct x86_emulate_ctxt *ctxt)
>  {
>  	u32 eax = 1, ebx, ecx = 0, edx;
>  
> -	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> +	ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx, CHECK_LIMIT);
>  	if (!(edx & FFL(FXSR)))
>  		return emulate_ud(ctxt);
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 1fa9ee5..9def4a8 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1580,7 +1580,7 @@ static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>  	}
>  	init_vmcb(svm);
>  
> -	kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy);
> +	kvm_cpuid(vcpu, &eax, &dummy, &dummy, &dummy, CHECK_LIMIT);
>  	kvm_register_write(vcpu, VCPU_REGS_RDX, eax);
>  
>  	if (kvm_vcpu_apicv_active(vcpu) && !init_event)
> diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
> index 0a6cc67..8a202c4 100644
> --- a/arch/x86/kvm/trace.h
> +++ b/arch/x86/kvm/trace.h
> @@ -151,8 +151,8 @@ TRACE_EVENT(kvm_fast_mmio,
>   */
>  TRACE_EVENT(kvm_cpuid,
>  	TP_PROTO(unsigned int function, unsigned long rax, unsigned long rbx,
> -		 unsigned long rcx, unsigned long rdx),
> -	TP_ARGS(function, rax, rbx, rcx, rdx),
> +		 unsigned long rcx, unsigned long rdx, bool found),
> +	TP_ARGS(function, rax, rbx, rcx, rdx, found),
>  
>  	TP_STRUCT__entry(
>  		__field(	unsigned int,	function	)
> @@ -160,6 +160,7 @@ TRACE_EVENT(kvm_cpuid,
>  		__field(	unsigned long,	rbx		)
>  		__field(	unsigned long,	rcx		)
>  		__field(	unsigned long,	rdx		)
> +		__field(	bool,		found		)
>  	),
>  
>  	TP_fast_assign(
> @@ -168,11 +169,13 @@ TRACE_EVENT(kvm_cpuid,
>  		__entry->rbx		= rbx;
>  		__entry->rcx		= rcx;
>  		__entry->rdx		= rdx;
> +		__entry->found		= found;
>  	),
>  
> -	TP_printk("func %x rax %lx rbx %lx rcx %lx rdx %lx",
> +	TP_printk("func %x rax %lx rbx %lx rcx %lx rdx %lx, cpuid entry %s",
>  		  __entry->function, __entry->rax,
> -		  __entry->rbx, __entry->rcx, __entry->rdx)
> +		  __entry->rbx, __entry->rcx, __entry->rdx,
> +		  __entry->found ? "found" : "not found")
>  );
>  
>  #define AREG(x) { APIC_##x, "APIC_" #x }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e40a779..ee99fc1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5213,10 +5213,10 @@ static int emulator_intercept(struct x86_emulate_ctxt *ctxt,
>  	return kvm_x86_ops->check_intercept(emul_to_vcpu(ctxt), info, stage);
>  }
>  
> -static void emulator_get_cpuid(struct x86_emulate_ctxt *ctxt,
> -			       u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
> +static bool emulator_get_cpuid(struct x86_emulate_ctxt *ctxt,
> +			u32 *eax, u32 *ebx, u32 *ecx, u32 *edx, int check_limit)
>  {
> -	kvm_cpuid(emul_to_vcpu(ctxt), eax, ebx, ecx, edx);
> +	return kvm_cpuid(emul_to_vcpu(ctxt), eax, ebx, ecx, edx, check_limit);
>  }
>  
>  static ulong emulator_read_gpr(struct x86_emulate_ctxt *ctxt, unsigned reg)
>