Re: [PATCH v2 1/5] KVM: x86: Add return value to kvm_cpuid().

[Paolo Bonzini]

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)
>>>
>>
>