| 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
| ||
|
Date: Thu, 17 Aug 2017 21:20:42 +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 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)
>>>>
>
Powered by blists - more mailing lists