Re: [Xen-devel] [PATCH 06/13] x86/hvm: Scale host TSC when setting/getting guest TSC

[haozhong.zhang@xxxxxxxxx]

On Tue, Oct 27, 2015 at 09:10:19AM -0400, Boris Ostrovsky wrote:
> On 10/27/2015 04:44 AM, Haozhong Zhang wrote:
> >On Thu, Oct 22, 2015 at 08:17:29AM -0600, Jan Beulich wrote:
> >>>>>On 28.09.15 at 09:13, <haozhong.zhang@xxxxxxxxx> wrote:
> >>>The existing hvm_set_guest_tsc_fixed() and hvm_get_guest_tsc_fixed()
> >>>calculate the guest TSC by adding the TSC offset to the host TSC. When
> >>>the TSC scaling is enabled, the host TSC should be scaled first. This
> >>>patch adds the scaling logic to those two functions.
> >>Just like mentioned for the first twp patches - I'd first of all like to
> >>understand why the lack of scaling this wasn't an issue for SVM so
> >>far. What you reads plausible, but assuming that SVM TSC scaling
> >>code was tested, I'm hesitant to apply changes to it without
> >>understanding the details (or at least without SVM maintainers'
> >>consent).
> >>
> >Hi SVM maintainers,
> >
> >Could you help to review this patch 6 as well as patch 2? They intend
> >to fix bugs in SVM TSC ratio code (or code that affects SVM TSC ratio
> >code).
> >
> >The detailed explanations of patch 2 and patch 6 can be found at
> >http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg01490.html
> >and
> >http://lists.xenproject.org/archives/html/xen-devel/2015-10/msg02843.html
> >respectively.
> 
> I agree with patch 2 (so you can add my Reviewed-by).
>
Thank you!

> 
>  but I am not so sure about patch 6 (and 11, together with existing SVM
> handlers).
> 
> 
> I don't have latest Intel's manual handy but for SVM the guest TSC value is
> calculated as scaled host TSC plus *unscaled* VMCB's TSC offset. Is Intel's
> implementation similar?
>
Yes, scaled host TSC + unscaled TSC offset.

> Both svm_set_tsc_offset and vmx_set_tsc_offset (as proposed in patch 11)
> write VMCB/VMCS with guest (i.s. scaled) offset, and that doesn't seem
> right.
>
I think it's right. It does not scale the offset. It just doesn't
trust the offset from the argument, and calculate the offset by
subtracting the scaled host TSC from the current guest TSC.


> If I am right then I think (1) SVM is broken now and (2) patches 6 and 11
> don't fix this brokenness and instead propagate it to VMX.
> 
> (I should have thought about this when I last replied to you asking to move
> scaling out of vmx_set_tsc_offset(). But I re-read this code now and it
> doesn't make sense to me anymore).
So moving scaling out of [vmx|svm]_set_tsc_offset() is valid.

> 
> -boris
> 
> 
> >
> >Thanks,
> >Haozhong
> >
> >>>--- a/xen/arch/x86/hvm/hvm.c
> >>>+++ b/xen/arch/x86/hvm/hvm.c
> >>>@@ -388,13 +388,12 @@ void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 
> >>>guest_tsc, u64 at_tsc)
> >>>          tsc = hvm_get_guest_time_fixed(v, at_tsc);
> >>>          tsc = gtime_to_gtsc(v->domain, tsc);
> >>>      }
> >>>-    else if ( at_tsc )
> >>>-    {
> >>>-        tsc = at_tsc;
> >>>-    }
> >>>      else
> >>>      {
> >>>-        tsc = rdtsc();
> >>>+        tsc = at_tsc ? at_tsc : rdtsc();
> >>In cases like this please prefer the gcc extension allowing the middle
> >>operand of the ?: to be omitted.
> >>
> >>Jan
> >>
> 
> 
> _______________________________________________
> 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