From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Zhang, Haozhong" Subject: Re: [PATCH v4 06/10] x86/hvm: Setup TSC scaling ratio Date: Tue, 16 Feb 2016 17:44:06 +0800 Message-ID: <20160216094406.GB6519@hz-desktop.sh.intel.com> References: <1453067939-9121-1-git-send-email-haozhong.zhang@intel.com> <1453067939-9121-7-git-send-email-haozhong.zhang@intel.com> <56B4B7AD02000078000CF10B@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: <56B4B7AD02000078000CF10B@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: "Tian, Kevin" , Keir Fraser , Suravee Suthikulpanit , Andrew Cooper , "xen-devel@lists.xen.org" , Aravind Gopalakrishnan , "Nakajima, Jun" , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On 02/05/16 21:54, Jan Beulich wrote: > >>> On 17.01.16 at 22:58, wrote: > > +u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz) > > +{ > > + u64 ratio; > > + > > + if ( !hvm_tsc_scaling_supported ) > > + return 0; > > + > > + /* > > + * The multiplication of the first two terms may overflow a 64-bit > > + * integer, so use mul_u64_u32_div() instead to keep precision. > > + */ > > + ratio = mul_u64_u32_div(1ULL << hvm_funcs.tsc_scaling_ratio_frac_bits, > > + gtsc_khz, cpu_khz); > > Is this the only use for this new math64 function? If so, I don't > see the point of adding that function, because (leaving limited > significant bits aside) the above simply is > > (gtsc_khz << hvm_funcs.tsc_scaling_ratio_frac_bits) / cpu_khz > > which can be had without any multiplication. Personally, if indeed > the only use I'd favor converting the above to inline assembly > here instead of adding that helper function (just like we have a > number of asm()-s in x86/time.c for similar reasons). > OK, I'll rewrite it as asm(). mul_u64_u32_div() will not be used any more and will be removed. I'll also inline another math64 function mul_u64_u64_shr() in its single caller hvm_scale_tsc(). Then the math64 patch will be dropped in the next version. > > +void hvm_setup_tsc_scaling(struct vcpu *v) > > +{ > > + v->arch.hvm_vcpu.tsc_scaling_ratio = > > + hvm_get_tsc_scaling_ratio(v->domain->arch.tsc_khz); > > +} > > So why again is this per-vCPU setup of per-vCPU state when it > only depends on a per-domain input? If this was per-domain, its > setup could be where it belongs - in arch_hvm_load(). > It's a per-domain state. I'll the state to x86's struct arch_domain where other TSC fields are (or struct hvm_domain, because this is only used for HVM?). Then it will be setup in tsc_set_info() after guest tsc frequency is determined. > > @@ -5504,6 +5536,9 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip) > > hvm_set_segment_register(v, x86_seg_gdtr, ®); > > hvm_set_segment_register(v, x86_seg_idtr, ®); > > > > + if ( hvm_tsc_scaling_supported && !d->arch.vtsc ) > > + hvm_setup_tsc_scaling(v); > > Could you remind me why this is needed? What state of the guest > would have changed making this necessary? Is this perhaps just > because it's per-vCPU instead of per-domain? > > Jan > Yes, just because I mistakenly made it per-vcpu. So it will be not necessary in this patch after tsc_scaling_ratio become per-domain. Haozhong