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:55:26AM -0400, Boris Ostrovsky wrote:
> On 10/27/2015 09:10 AM, 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).
> >
> >
> > 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?
> >
> >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 seem to have lost Haozhong on my previous email)
> 
> No, it is right (or, rather, since TSC offset is a constant it can be used
> as scaled or unscaled, depending on how you want to implement it).
>
Agree, it's right.

> So if you adjust patch 11 (and corresponding SVM code) to take scaling out
> from there this patch would be correct.
>
Yes, I'm going to do so in the next version.

> Of course I still can't test this since the two machines that I have
> available are running at fairly close frequencies.
>
Sorry for not mentioning this patchset can be tested on a single
machine as well. In patch 13, I introduce an option 'vtsc_khz' to xl
configuration. For a HVM domain, If 'vtsc_khz=xxx' is present, xl will
set vcpu's TSC frequency to xxx KHz.

Thus, we can
1) create a HVM domain with 'vtsc_khz=xxx' where xxx KHz
   is different from the host;
2) use 'xl save domid savedvm' to save the domain to a file;
3) use 'xl restore savedvm' to restore the domain.

Because 'xl save/restore' asks the hypervisor to do the same work as
that for 'xl migrate', we can use above approach to test on a single
machine.

Haozhong

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