Re: [Xen-devel] [PATCH v3 04/13] x86/time.c: Scale host TSC in pvclock properly

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 01/08/16 02:20, Jan Beulich wrote:
> >>> On 31.12.15 at 04:03, <haozhong.zhang@xxxxxxxxx> wrote:
> > This patch makes the pvclock return the scaled host TSC and
> > corresponding scaling parameters to HVM domains if guest TSC is not
> > emulated and TSC scaling is enabled.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
> > ---
> > Changes in v3:
> >  (addressing Boris Ostrovsky's comments)
> >  * No changes in fact. tsc_set_info() does not set d->arch.vtsc to 0
> >    if host_tsc_is_safe() is not satisfied, so it is safe to use
> >    d->arch.vtsc_to_ns here.
> > 
> >  xen/arch/x86/time.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index d83f068..b31634a 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -815,10 +815,18 @@ static void __update_vcpu_system_time(struct vcpu *v, 
> > int force)
> >      }
> >      else
> >      {
> > -        tsc_stamp = t->local_tsc_stamp;
> > -
> > -        _u.tsc_to_system_mul = t->tsc_scale.mul_frac;
> > -        _u.tsc_shift         = (s8)t->tsc_scale.shift;
> > +        if ( has_hvm_container_domain(d) && cpu_has_tsc_ratio )
> 
> For symmetry with patches 1 and 2 I'd expect !d->arch.vtsc to be
> added here too.
>

even though it's already in the !d->arch.vtsc branch? But I'm fine to
add it for symmetric.

> > +        {
> > +            tsc_stamp            = hvm_funcs.scale_tsc(v, 
> > t->local_tsc_stamp);
> > +            _u.tsc_to_system_mul = d->arch.vtsc_to_ns.mul_frac;
> > +            _u.tsc_shift         = d->arch.vtsc_to_ns.shift;
> > +        }
> > +        else
> > +        {
> > +            tsc_stamp            = t->local_tsc_stamp;
> > +            _u.tsc_to_system_mul = t->tsc_scale.mul_frac;
> > +            _u.tsc_shift         = (s8)t->tsc_scale.shift;
> 
> The case has been pointless anyway, and you don't add a similar
> one in the if() branch - please delete it.
>

I don't understand why it's pointless. The inner if branch is to
ensure pvclock provides the scaled TSC information when TSC scaling is
used, and the inner else branch is to keep the existing behavior when
TSC scaling is not used. For the outer if branch, TSC scaling is never
used, so no change is needed.

Haozhong

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