| Message ID | 1502999558-2517-2-git-send-email-yu.c.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) >> >
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) >
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
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) >>> >> >
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
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) >>>> >
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
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)
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(-)