From mboxrd@z Thu Jan 1 00:00:00 1970 From: Haozhong Zhang Subject: Re: [PATCH v5 2/6] x86/hvm: Setup TSC scaling ratio Date: Wed, 24 Feb 2016 23:42:47 +0800 Message-ID: <20160224154247.GB14964@hz-desktop.sh.intel.com> References: <1456193104-12761-1-git-send-email-haozhong.zhang@intel.com> <1456193104-12761-3-git-send-email-haozhong.zhang@intel.com> <56CDD3DB02000078000D5BED@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <56CDD3DB02000078000D5BED@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Kevin Tian , Keir Fraser , Jun Nakajima , Andrew Cooper , xen-devel@lists.xen.org, Aravind Gopalakrishnan , Suravee Suthikulpanit , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On 02/24/16 08:01, Jan Beulich wrote: > >>> On 23.02.16 at 03:05, 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