Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1713875 > unrolled thread

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

Started byYu Zhang <yu.c.zhang@linux.intel.com>
First post2017-08-17 14:00 +0200
Last post2017-08-17 16:40 +0200
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Yu Zhang <yu.c.zhang@linux.intel.com> - 2017-08-17 14:00 +0200
    Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Paolo Bonzini <pbonzini@redhat.com> - 2017-08-17 14:30 +0200
      Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Yu Zhang <yu.c.zhang@linux.intel.com> - 2017-08-17 14:50 +0200
        Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Yu Zhang <yu.c.zhang@linux.intel.com> - 2017-08-17 15:00 +0200
          Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Paolo Bonzini <pbonzini@redhat.com> - 2017-08-17 15:20 +0200
        Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Paolo Bonzini <pbonzini@redhat.com> - 2017-08-17 15:20 +0200
          Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Yu Zhang <yu.c.zhang@linux.intel.com> - 2017-08-17 15:50 +0200
            Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). Paolo Bonzini <pbonzini@redhat.com> - 2017-08-17 16:40 +0200

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

FromYu Zhang <yu.c.zhang@linux.intel.com>
Date2017-08-17 14:00 +0200
Subject[PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid().
Message-ID<ufrCV-4m1-5@gated-at.bofh.it>
Return false in kvm_cpuid() when it fails to find the cpuid
entry. Also, this routine(and its caller) is optimized with
a new argument - check_limit, so that the check_cpuid_limit()
fall back can be avoided.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
 arch/x86/include/asm/kvm_emulate.h |  4 ++--
 arch/x86/kvm/cpuid.c               | 18 ++++++++++++++----
 arch/x86/kvm/cpuid.h               |  9 ++++++++-
 arch/x86/kvm/emulate.c             | 17 ++++++++++-------
 arch/x86/kvm/svm.c                 |  2 +-
 arch/x86/kvm/trace.h               | 11 +++++++----
 arch/x86/kvm/x86.c                 |  6 +++---
 7 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index fde36f1..0e51a07 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -219,8 +219,8 @@ struct x86_emulate_ops {
 			 struct x86_instruction_info *info,
 			 enum x86_intercept_stage stage);
 
-	void (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
-			  u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+	bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt, u32 *eax, u32 *ebx,
+			  u32 *ecx, u32 *edx, int check_limit);
 	void (*set_nmi_mask)(struct x86_emulate_ctxt *ctxt, bool masked);
 
 	unsigned (*get_hflags)(struct x86_emulate_ctxt *ctxt);
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 59ca2ee..989ba4e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -853,15 +853,22 @@ static struct kvm_cpuid_entry2* check_cpuid_limit(struct kvm_vcpu *vcpu,
 	return kvm_find_cpuid_entry(vcpu, maxlevel->eax, index);
 }
 
-void kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
+bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
+	       u32 *ecx, u32 *edx, int check_limit)
 {
 	u32 function = *eax, index = *ecx;
 	struct kvm_cpuid_entry2 *best;
+	bool entry_found = true;
 
 	best = kvm_find_cpuid_entry(vcpu, function, index);
 
-	if (!best)
+	if (!best) {
+		entry_found = false;
+		if (!check_limit)
+			goto out;
+
 		best = check_cpuid_limit(vcpu, function, index);
+	}
 
 	if (best) {
 		*eax = best->eax;
@@ -870,7 +877,10 @@ void kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
 		*edx = best->edx;
 	} else
 		*eax = *ebx = *ecx = *edx = 0;
-	trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx);
+
+out:
+	trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, entry_found);
+	return entry_found;
 }
 EXPORT_SYMBOL_GPL(kvm_cpuid);
 
@@ -883,7 +893,7 @@ int kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
 
 	eax = kvm_register_read(vcpu, VCPU_REGS_RAX);
 	ecx = kvm_register_read(vcpu, VCPU_REGS_RCX);
-	kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx);
+	kvm_cpuid(vcpu, &eax, &ebx, &ecx, &edx, CHECK_LIMIT);
 	kvm_register_write(vcpu, VCPU_REGS_RAX, eax);
 	kvm_register_write(vcpu, VCPU_REGS_RBX, ebx);
 	kvm_register_write(vcpu, VCPU_REGS_RCX, ecx);
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,
+};
+
+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);
 	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)
-- 
2.5.0

[toc] | [next] | [standalone]

#1713949

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-08-17 14:30 +0200
Message-ID<ufs5Y-4Nj-35@gated-at.bofh.it>
In reply to#1713875
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)
>

[toc] | [prev] | [next] | [standalone]

#1713974

FromYu Zhang <yu.c.zhang@linux.intel.com>
Date2017-08-17 14:50 +0200
Message-ID<ufspj-4V5-9@gated-at.bofh.it>
In reply to#1713949

On 8/17/2017 8:29 PM, Paolo Bonzini wrote:
> 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.

Thanks, Paolo.
So we just use true/false in emulate.c & svm.c, is this OK?
BTW could you please

>> +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!

Then I guess check_fxsr() should also use NO_CHECK_LIMIT('false' for a 
bool argument),
because it's also for eax=1?

And what about svm_vcpu_reset()?

I am not sure if leaf 1 is always available. And if the answer is yes, I 
do not think any of these
3 places(em_movbe/check_fxsr/svm_vcpu_reset) will need to fall back to 
check_cpuid_limit(),
nor do we need to check the return value of get_cpuid(). Do you agree? :-)

Yu

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

[toc] | [prev] | [next] | [standalone]

#1713994

FromYu Zhang <yu.c.zhang@linux.intel.com>
Date2017-08-17 15:00 +0200
Message-ID<ufsz1-4YZ-41@gated-at.bofh.it>
In reply to#1713974

On 8/17/2017 8:23 PM, Yu Zhang wrote:
>
>
> On 8/17/2017 8:29 PM, Paolo Bonzini wrote:
>> 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.
>
> Thanks, Paolo.
> So we just use true/false in emulate.c & svm.c, is this OK?
> BTW could you please
Sorry for the unfinished line. I was wondering, why can't emulate.c 
include cpuid.h?

Yu

[toc] | [prev] | [next] | [standalone]

#1714024

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-08-17 15:20 +0200
Message-ID<ufsSn-5nC-35@gated-at.bofh.it>
In reply to#1713994
On 17/08/2017 14:33, Yu Zhang wrote:
>>>>
>>>> +enum {
>>>> +    NO_CHECK_LIMIT = 0,
>>>> +    CHECK_LIMIT = 1,
>>>> +};
>>> emulate.c should not include cpuid.h.  The argument can be simply a
>>> bool, though.
>>
>> Thanks, Paolo.
>> So we just use true/false in emulate.c & svm.c, is this OK?

I would use true/false everywhere.

>> BTW could you please
> Sorry for the unfinished line. I was wondering, why can't emulate.c
> include cpuid.h?

The emulator should be separate from the rest of KVM, in principle it
could be used by userspace too.  So its interface should be as limited
as possible.

Paolo

[toc] | [prev] | [next] | [standalone]

#1714016

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-08-17 15:20 +0200
Message-ID<ufsSm-5nC-9@gated-at.bofh.it>
In reply to#1713974
On 17/08/2017 14:23, Yu Zhang wrote:
> 
> 
> On 8/17/2017 8:29 PM, Paolo Bonzini wrote:
>> 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.
> 
> Thanks, Paolo.
> So we just use true/false in emulate.c & svm.c, is this OK?
> BTW could you please
> 
>>> +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!
> 
> Then I guess check_fxsr() should also use NO_CHECK_LIMIT('false' for a
> bool argument), because it's also for eax=1?

Good point.

> And what about svm_vcpu_reset()?

No, this one should be left as is, it's just writing a register and not
checking a feature.

> I am not sure if leaf 1 is always available. And if the answer is yes, I
> do not think any of these 3 places(em_movbe/check_fxsr/svm_vcpu_reset) will
> need to fall back to check_cpuid_limit(),
> nor do we need to check the return value of get_cpuid(). Do you agree? :-)

I think the answer is no, but you don't need to check the return value
because testing against 0 is okay (if best is NULL, get_cpuid returns 0
for eax/ebx/ecx/edx).

Paolo

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

[toc] | [prev] | [next] | [standalone]

#1714042

FromYu Zhang <yu.c.zhang@linux.intel.com>
Date2017-08-17 15:50 +0200
Message-ID<uftln-5z4-11@gated-at.bofh.it>
In reply to#1714016

On 8/17/2017 9:17 PM, Paolo Bonzini wrote:
> On 17/08/2017 14:23, Yu Zhang wrote:
>>
>> On 8/17/2017 8:29 PM, Paolo Bonzini wrote:
>>> 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.
>> Thanks, Paolo.
>> So we just use true/false in emulate.c & svm.c, is this OK?
>> BTW could you please
>>
>>>> +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!
>> Then I guess check_fxsr() should also use NO_CHECK_LIMIT('false' for a
>> bool argument), because it's also for eax=1?
> Good point.
>
>> And what about svm_vcpu_reset()?
> No, this one should be left as is, it's just writing a register and not
> checking a feature.

Got it. Thanks.

>
>> I am not sure if leaf 1 is always available. And if the answer is yes, I
>> do not think any of these 3 places(em_movbe/check_fxsr/svm_vcpu_reset) will
>> need to fall back to check_cpuid_limit(),
>> nor do we need to check the return value of get_cpuid(). Do you agree? :-)
> I think the answer is no, but you don't need to check the return value
> because testing against 0 is okay (if best is NULL, get_cpuid returns 0
> for eax/ebx/ecx/edx).

OK. And to return 0 for eax/ebx/ecx/edx if check_cpuid_limit() is also 
to be omitted,
I'd better refactor this patch and move the "out:" before the if 
statement. :-)

                 best = check_cpuid_limit(vcpu, function, index);
         }

+out:
         if (best) {
                 *eax = best->eax;
                 *ebx = best->ebx;
@@ -887,7 +888,6 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 
*ebx,
         } else
                 *eax = *ebx = *ecx = *edx = 0;

-out:
         trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, entry_found);
         return entry_found;
  }

And for all get_cpuid() callers which is testing the existence of a 
feature, we do not need to
check the return value, just checking the flag in the register should be 
fine, correct?

Yu

>
> Paolo
>
>> Yu
>>
>>> 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)
>>>>
>

[toc] | [prev] | [next] | [standalone]

#1714088

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-08-17 16:40 +0200
Message-ID<ufu7L-67i-13@gated-at.bofh.it>
In reply to#1714042
On 17/08/2017 15:20, Yu Zhang wrote:
>>
> 
> OK. And to return 0 for eax/ebx/ecx/edx if check_cpuid_limit() is also
> to be omitted,
> I'd better refactor this patch and move the "out:" before the if
> statement. :-)
> 
>                 best = check_cpuid_limit(vcpu, function, index);
>         }
> 
> +out:
>         if (best) {
>                 *eax = best->eax;
>                 *ebx = best->ebx;
> @@ -887,7 +888,6 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32
> *ebx,
>         } else
>                 *eax = *ebx = *ecx = *edx = 0;
> 
> -out:
>         trace_kvm_cpuid(function, *eax, *ebx, *ecx, *edx, entry_found);
>         return entry_found;
>  }
> 
> And for all get_cpuid() callers which is testing the existence of a
> feature, we do not need to
> check the return value, just checking the flag in the register should be
> fine, correct?

Yes, correct!

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web