| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1493b2f0-0e6e-2dac-5ae3-1aed3595faed@linux.intel.com>
Date: Thu, 17 Aug 2017 20:23:22 +0800
From: Yu Zhang <yu.c.zhang@...ux.intel.com>
To: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, rkrcmar@...hat.com,
tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
xiaoguangrong@...cent.com, joro@...tes.org
Subject: Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid().
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)
>>
>
Powered by blists - more mailing lists