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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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