Re: [PATCH 11/13] vmx: Use scaled host TSC to calculate TSC offset

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 10/22/2015 01:12 PM, Haozhong Zhang wrote:
> On Thu, Oct 22, 2015 at 11:55:00AM -0400, Boris Ostrovsky wrote:
>> On 09/28/2015 03:13 AM, Haozhong Zhang wrote:
>>> If VMX TSC scaling is enabled and no TSC emulation is used,
>>> vmx_set_tsc_offset() will calculate the TSC offset by substracting the
>>> scaled host TSC from the current guest TSC.
>>>
>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>> ---
>>>   xen/arch/x86/hvm/vmx/vmx.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 454440e..163974d 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1102,11 +1102,26 @@ static void vmx_handle_cd(struct vcpu *v, unsigned long value)
>>>   static void vmx_set_tsc_offset(struct vcpu *v, u64 offset, u64 at_tsc)
>>>   {
>>> +    uint64_t host_tsc, guest_tsc;
>>> +    struct domain *d = v->domain;
>>> +
>>> +    guest_tsc = hvm_get_guest_tsc_fixed(v, at_tsc);
>>> +
>>> +    if ( cpu_has_vmx_tsc_scaling && !d->arch.vtsc )
>>> +    {
>>> +        host_tsc = at_tsc ? at_tsc : rdtsc();
>>> +        offset = guest_tsc - hvm_scale_tsc(v, host_tsc);
>>> +    }
>>> +
>>>       vmx_vmcs_enter(v);
>>> +    if ( !nestedhvm_enabled(d) )
>>> +        goto out;
>>> +
>>>       if ( nestedhvm_vcpu_in_guestmode(v) )
>>>           offset += nvmx_get_tsc_offset(v);
>>> +out:
>>>       __vmwrite(TSC_OFFSET, offset);
>>>       vmx_vmcs_exit(v);
>>>   }
>>
>> This (and corresponding SVM code) looks somewhat suspect to me: if the
>> processor supports scaling we are ignoring caller-provided offset.
>>
> Yes, if TSC scaling is available, [svm|vmx]_set_tsc_offset() do not
> trust the offset from callers and calculate a scaled version by itself
> instead.
>
> The original svm_set_tsc_offset() did so and I keep its semantics
> here.
>
> Maybe moving the scaling logic out of [svm|vmx]_set_tsc_offset() could
> make the semantics more clear.


Yes: since scaling is now architectural, offsets are (or at least can 
be) calculated correctly at HVM level and so arch-specific handlers 
should not have to do this. In case of hvm_set_guest_tsc_fixed() we 
definitely doing it twice.

-boris

>
>> Besides, at least when called from hvm_set_guest_tsc_fixed() --- we've
>> already taken scaling into account, that's what patch 6 does, doesn't it?
>>
> Yes.
>
>> -boris
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel