Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752307AbdHQMpt (ORCPT ); Thu, 17 Aug 2017 08:45:49 -0400 Received: from mga11.intel.com ([192.55.52.93]:10295 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750957AbdHQMpr (ORCPT ); Thu, 17 Aug 2017 08:45:47 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,387,1498546800"; d="scan'208";a="1004785465" Subject: Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). To: Paolo Bonzini , kvm@vger.kernel.org References: <1502999558-2517-1-git-send-email-yu.c.zhang@linux.intel.com> <1502999558-2517-2-git-send-email-yu.c.zhang@linux.intel.com> <9deed5e8-52e9-8634-611f-553840a43f1a@redhat.com> Cc: linux-kernel@vger.kernel.org, rkrcmar@redhat.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, xiaoguangrong@tencent.com, joro@8bytes.org From: Yu Zhang Message-ID: <1493b2f0-0e6e-2dac-5ae3-1aed3595faed@linux.intel.com> Date: Thu, 17 Aug 2017 20:23:22 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <9deed5e8-52e9-8634-611f-553840a43f1a@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6798 Lines: 193 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) >> >