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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 28.09.15 at 09:13, <haozhong.zhang@intel.com> 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?

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

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

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

min()

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

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

> @@ -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? And what's the
reason for the dependency on !vtsc (please also see the comment
ahead of tsc_set_info())?

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

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

Jan