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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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