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

Thu, 17 Aug 2017 05:46:09 -0700 


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)




























					Reply via email to