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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 10/22/2015 10:17 AM, 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).
I don't see that this series will create any regressions in SVM . Most 
of the changes move SVM-specific code into HVM I didn't see any obvious 
problems there. I do have concern about patch 5 since I am sure I fully 
understand whether the new algorithm (in __scale_tsc()) is equivalent to 
current SVM code. I think you also had questions about that.
Having said this, the fact that this patch (and patch 9) fix bugs leads 
me to believe this feature may not have been thoroughly tested.
I don't have a pair of appropriate AMD systems to test this series with 
migration (which is where this can be verified). Aravind, can you find 
something and see how this works?
-boris


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