Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid(). | From | Yu Zhang <> | Date | Thu, 17 Aug 2017 21:20:42 +0800 |
| |
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) >>>> >
| |