From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH v5 2/6] x86/hvm: Setup TSC scaling ratio
Date: Wed, 24 Feb 2016 08:01:31 -0700
Message-ID: <56CDD3DB02000078000D5BED@prv-mh.provo.novell.com>
References: <1456193104-12761-1-git-send-email-haozhong.zhang@intel.com>
	<1456193104-12761-3-git-send-email-haozhong.zhang@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1456193104-12761-3-git-send-email-haozhong.zhang@intel.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Haozhong Zhang <haozhong.zhang@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, Andrew Cooper <andrew.cooper3@citrix.com>, xen-devel@lists.xen.org, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>, Jun Nakajima <jun.nakajima@intel.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>
List-Id: xen-devel@lists.xenproject.org

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

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

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.

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

Jan