Re: [PATCH v5 2/6] x86/hvm: Setup TSC scaling ratio

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 02/24/16 08:01, Jan Beulich wrote:
> >>> On 23.02.16 at 03:05, <haozhong.zhang@intel.com> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -298,6 +298,29 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
> >      return 1;
> >  }
> >  
> > +/*
> > + * Get the ratio to scale host TSC frequency to gtsc_khz. zero will be
> > + * returned if TSC scaling is unavailable or ratio cannot be handled
> > + * by host CPU. Otherwise, a non-zero ratio will be returned.
> > + */
> > +u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz)
> > +{
> > +    u64 ratio = gtsc_khz;
> > +    u64 dummy = 0;
> 
> "dummy" suggests it is unused, which it isn't. "tmp" or "hi" might be
> a little better, but since the meanings of the variables (also "ratio")
> differ for their roles an inputs and outputs, splitting inputs and
> outputs below would seem even better. In which case "dummy"
> become would an appropriate name again.
>

Yes, I'll split input and outputs for dummy and ratio.

> > +    if ( !hvm_tsc_scaling_supported )
> > +        return 0;
> > +
> > +    /* ratio = (gtsc_khz << hvm_funcs.tsc_scaling.ratio_frac_bits) / cpu_khz */
> > +    asm (
> > +        "shldq %2,%1,%0; salq %2,%1; divq %3"
> > +        : "+&d" (dummy), "+&a" (ratio)
> > +        : "c" (hvm_funcs.tsc_scaling.ratio_frac_bits),
> > +          "rm" ((u64) cpu_khz) );
> 
> And this DIVQ can't possibly #DE, e.g. when gtsc_khz is much larger
> than cpu_khz?
>

Oops, it could. Following check should be added before asm():
        /* the quotient is too large to fit in the integral part of TSC scaling ratio */
        if ( gtsc_khz / cpu_khz >
             (hvm_funcs.tsc_scaling.max_ratio >> hvm_funcs.tsc_scaling.ratio_frac_bits )
            return 0;

> I'd also prefer if the instruction got put on the same line as the
> "asm (". Considering that we're dealing with unsigned quantities
> here I'd further prefer if SHLQ was used instead of SALQ. And
> finally I'd suggest using named rather than numbered asm()
> arguments.
>

I'll make all these three changes.

> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -272,6 +272,14 @@ u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc);
> >  #define hvm_tsc_scaling_supported \
> >      (!!hvm_funcs.tsc_scaling.ratio_frac_bits)
> >  
> > +#define hvm_default_tsc_scaling_ratio \
> > +    (1ULL << hvm_funcs.tsc_scaling.ratio_frac_bits)
> > +
> > +#define hvm_vcpu_tsc_scaling_ratio(v) \
> > +    ((v)->domain->arch.hvm_domain.tsc_scaling_ratio)
> 
> Since this is now a per-domain property I think it is misleading (and
> potentially hindering) for the called to pass in a struct vcpu * here.
> Please make this a struct domain *.
>

I'll change to struct domain *, and also remove _vcpu in the name.

Thanks,
Haozhong