Re: [Xen-devel] [PATCH 04/13] x86/hvm: Setup TSC scaling ratio

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Thu, Oct 22, 2015 at 07:13:07AM -0600, Jan Beulich wrote:
> >>> On 28.09.15 at 09:13, <haozhong.zhang@xxxxxxxxx> wrote:
> > This patch adds a field tsc_scaling_ratio in struct arch_vcpu to
> 
> Why not in struct hvm_vcpu? Are you intending any use for PV guests?
>

No, I'll move tsc_scaling_ratio to struct hvm_cpu.

> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -297,6 +297,34 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
> >      return 1;
> >  }
> >  
> > +void hvm_setup_tsc_scaling(struct vcpu *v)
> > +{
> > +    u64 ratio, khz;
> > +   s8 shift;
> 
> Hard tab.

will change to spaces

> 
> > +    if ( !hvm_funcs.tsc_scaling_supported )
> > +        return;
> > +
> > +    khz = v->domain->arch.tsc_khz;
> 
> I don't see the need for this variable in the first place. But if you
> absolutely want to keep it, I don't see why it needs to be u64
> when the field you load from is uint32_t.
>

will remove

> > +    shift = (hvm_funcs.tsc_scaling_ratio_frac_bits <= 32) ?
> > +        hvm_funcs.tsc_scaling_ratio_frac_bits : 32;
> 
> min()
>

yes

> > +    ratio = khz << shift;
> > +    do_div(ratio, cpu_khz);
> > +    ratio <<= hvm_funcs.tsc_scaling_ratio_frac_bits - shift;
> > +
> > +    if ( ratio == 0 ||
> > +         ratio > hvm_funcs.max_tsc_scaling_ratio ||
> > +         ratio & hvm_funcs.tsc_scaling_ratio_rsvd )
> 
> Parentheses around the operands of the & please.
>

will add

> > +    {
> > +        printk(XENLOG_WARNING
> > +               "Invalid TSC scaling ratio - virtual tsc khz=%lu\n",
> > +               khz);
> 
> Who can issue a call to this function under which conditions? I.e. is
> a non-ratelimited printk() okay here? Plus, without identifying the
> subject vcpu I don't think the message is of much use beyond your
> initial debugging purposes.
>

hvm_load_cpu_ctxt(), hvm_vcpu_reset_state() and tsc_set_info() call
this function. Am I correct that those functions are not called in
high rate? But I agree that the warning is useless w/o identifying the
subject vcpu and will add it.


> > @@ -2023,6 +2051,9 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
> > hvm_domain_context_t *h)
> >      if ( hvm_funcs.load_cpu_ctxt(v, &ctxt) < 0 )
> >          return -EINVAL;
> >  
> > +    if ( !v->domain->arch.vtsc && hvm_funcs.tsc_scaling_supported )
> > +        hvm_setup_tsc_scaling(v);
> 
> What's the rationale for putting it in this function?

I cannot remind clearly why it's needed here. I remember it's used in
the case that a domain is restored from a file by 'xl restore'. I'll
reply to this issue later.

> And what's the
> reason for the dependency on !vtsc (please also see the comment
> ahead of tsc_set_info())?
>

If v->domain->arch.vtsc == 1, guest rdtsc/rdtscp is trapped (setup in
tsc_set_info()) and emulated by hypervisor and the hardware TSC
scaling is not used in this case.

> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1956,6 +1956,8 @@ void tsc_set_info(struct domain *d,
> >          {
> >      case TSC_MODE_NEVER_EMULATE:
> >              d->arch.vtsc = 0;
> > +            if ( tsc_mode == TSC_MODE_NEVER_EMULATE )
> > +                d->arch.tsc_khz = cpu_khz;
> >              break;
> >          }
> 
> Depending on the changes to the first two patches: If this change
> would remain like this, please move out the TSC_MODE_NEVER_EMULATE
> case to be a standalone one again, since the way you do it here
> looks pretty confusing/odd.
>

Thanks for pointing out it! This two lines change is incorrect and
should be removed.

> > @@ -1981,8 +1983,14 @@ void tsc_set_info(struct domain *d,
> >      if ( is_hvm_domain(d) )
> >      {
> >          hvm_set_rdtsc_exiting(d, d->arch.vtsc);
> > -        if ( d->vcpu && d->vcpu[0] && incarnation == 0 )
> > +        if ( d->vcpu && d->vcpu[0] )
> >          {
> > +            if ( !d->arch.vtsc && hvm_funcs.tsc_scaling_supported )
> > +                hvm_setup_tsc_scaling(d->vcpu[0]);
> 
> And what about the other vCPU-s? If you mean this to be along
> the lines of the code that follows here, you should put this after
> the comment explaining that.
>

TSC scaling for other vcpus are set in hvm_vcpu_reset_state(). But I'm
not sure it can be moved together with the followed code because of
the followed
     if ( incarnation )
         return;

incarnation != 0 after migration and the setup of TSC scaling is
however necessary.

> Jan
> 

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