Re: [Xen-devel] [PATCH 05/13] x86/hvm: Replace architecture TSC scaling by a common function

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Thu, Oct 22, 2015 at 07:52:08AM -0600, Jan Beulich wrote:
> >>> On 28.09.15 at 09:13, <haozhong.zhang@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -297,6 +297,59 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
> >      return 1;
> >  }
> >  
> > +/*
> > + * Multiply tsc by a fixed point number represented by ratio.
> > + *
> > + * The most significant 64-N bits (mult) of ratio represent the
> > + * integral part of the fixed point number; the remaining N bits
> > + * (frac) represent the fractional part, ie. ratio represents a fixed
> > + * point number (mult + frac * 2^(-N)).
> > + *
> > + * N equals to hvm_funcs.tsc_scaling_ratio_frac_bits.
> > + */
> > +static u64 __scale_tsc(u64 tsc, u64 ratio)
> 
> No double underscores please without good reason.
>

will rename into scale_tsc().

> > +{
> > +    u64 mult, frac, mask, _tsc;
> 
> _tsc is not a valid name for a local variable.
> 
> > +    int width, nr;
> 
> Both unsigned afaict.
> 
> > +    BUG_ON(hvm_funcs.tsc_scaling_ratio_frac_bits >= 64);
> > +
> > +    mult  = ratio >> hvm_funcs.tsc_scaling_ratio_frac_bits;
> > +    mask  = (1ULL << hvm_funcs.tsc_scaling_ratio_frac_bits) - 1;
> > +    frac  = ratio & mask;
> > +
> > +    width = 64 - hvm_funcs.tsc_scaling_ratio_frac_bits;
> > +    mask  = (1ULL << width) - 1;
> > +    nr    = hvm_funcs.tsc_scaling_ratio_frac_bits;
> > +
> > +    _tsc  = tsc;
> > +    _tsc *= mult;
> > +    _tsc += (tsc >> hvm_funcs.tsc_scaling_ratio_frac_bits) * frac;
> > +
> > +    while ( nr >= width )
> > +    {
> > +        _tsc += (((tsc >> (nr - width)) & mask) * frac) >> (64 - nr);
> > +        nr   -= width;
> > +    }
> 
> Please add a comment explaining what this loop is intended to do.
>

I'm rewriting this function without using this tricky.

> > +u64 hvm_scale_tsc(struct vcpu *v, u64 tsc)
> 
> const struct vcpu *v
> 
> > +{
> > +    u64 _tsc = tsc;
> 
> Here I don't even see the need for this misnamed variable.
>

will remove this variable

> Jan
> 

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