Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest

[Liu, Jinsong]

Eduardo Habkost wrote:
> On Tue, Mar 20, 2012 at 12:53:57PM +0000, Liu, Jinsong wrote:
>> Rik van Riel wrote:
>>> On 03/09/2012 01:27 PM, Liu, Jinsong wrote:
>>> 
>>>> As for 'tsc deadline' feature exposing, my patch (as attached) just
>>>> obey qemu general cpuid exposing method, and also satisfied your
>>>> target I think.
>>> 
>>> One question.
>>> 
>>> Why is TSC_DEADLINE not exposed in the cpuid allowed feature
>>> bits in do_cpuid_ent() in arch/x86/kvm/x86.c ?
>>> 
>>>          /* cpuid 1.ecx */
>>>          const u32 kvm_supported_word4_x86_features =
>>>                  F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
>>>                  0 /* DS-CPL, VMX, SMX, EST */ |
>>>                  0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /*
>>>                  Reserved */ | F(FMA) | F(CX16) | 0 /* xTPR Update,
>>>                  PDCM */ | 0 /* Reserved, DCA */ | F(XMM4_1) |
>>>                  F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
>>>                  0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE
>>>                  */ | F(AVX) | F(F16C) | F(RDRAND);
>>> 
>>> Would it make sense to expose F(TSC_DEADLINE) above?
>>> 
>>> Or is there something truly special about tsc deadline
>>> that means it should be different from everything else?
>> 
>> Because the feature depends on KVM_CREATE_IRQCHIP, which we cannot
>> guarantee will be called, we expose it via a
>> KVM_CAP_TSC_DEADLINE_TIMER and not KVM_GET_SUPPORTED_CPUID.  
> 
> We have many other features that depend on proper support from
> userspace otherwise they wouldn't work, but are listed on
> GET_SUPPORTED_CPUID, don't we? Why is TSC-deadline special?
> 
> GET_SUPPORTED_CPUID just means "KVM supports it as long as userspace
> supports it too and enables it", it doesn't mean "CPUID bit that will
> be enabled by default"[1].
> 
>> Refer changeset 4d25a066b69fb749a39d0d4c610689dd765a0b0e.
> 
> That changeset was necessary because the kernel was doing this on
> update_cpu
> 
>       if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
>               best->function == 0x1) {
>               best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
> 
> And that was really wrong, because it enabled the bit unconditionally.
> But I don't understand why KVM_CAP_TSC_DEADLINE_TIMER was created if
> we already have KVM_GET_SUPPORTED_CPUID to tell userspace which bits
> are supported by KVM.
> 

Yes, exactly. That's why we need this patch.

> 
> [1] From Documentation/virtual/kvm/api.txt:
> 
> "KVM_GET_SUPPORTED_CPUID
> [...]
> This ioctl returns x86 cpuid features which are supported by both the
> hardware and kvm.  Userspace can use the information returned by this
> ioctl to construct cpuid information (for KVM_SET_CPUID2) that is
> consistent with hardware, kernel, and userspace capabilities, and with
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^
> user requirements (for example, the user may wish to constrain cpuid
> to emulate older hardware, or for feature consistency across a
> cluster)." 

The fixbug patch is implemented by Jan and Avi, I reply per my understanding.

I think for tsc deadline timer feature, KVM_CAP_TSC_DEADLINE_TIMER is slightly 
better than KVM_GET_SUPPORTED_CPUID. If use KVM_GET_SUPPORTED_CPUID, it means 
tsc deadline features bind to host cpuid, while it fact it could be pure 
software emulated by kvm (though currently it implemented as bound to 
hareware). For the sake of extension, it choose KVM_CAP_TSC_DEADLINE_TIMER.

Thanks,
Jinsong