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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Thu, Oct 22, 2015 at 03:19:19PM -0400, Boris Ostrovsky wrote:
> 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@xxxxxxxxx>
> >>>---
> >>>  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
>

I'll move the scaling logic out of [svm|vmx]_set_tsc_offset() in the
next version.

> >
> >>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@xxxxxxxxxxxxx
> >>http://lists.xen.org/xen-devel
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel