From mboxrd@z Thu Jan 1 00:00:00 1970 From: Haozhong Zhang Subject: Re: [PATCH 04/13] x86/hvm: Setup TSC scaling ratio Date: Thu, 22 Oct 2015 23:55:45 +0800 Message-ID: <20151022155545.GB16418@hzzhang-OptiPlex-9020.sh.intel.com> References: <1443424438-13404-1-git-send-email-haozhong.zhang@intel.com> <1443424438-13404-5-git-send-email-haozhong.zhang@intel.com> <5628FD0302000078000AD8EE@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: <5628FD0302000078000AD8EE@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 , Wei Liu , Ian Campbell , Stefano Stabellini , Jun Nakajima , Andrew Cooper , Ian Jackson , xen-devel@lists.xen.org, Aravind Gopalakrishnan , Suravee Suthikulpanit , Keir Fraser , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On Thu, Oct 22, 2015 at 07:13:07AM -0600, Jan Beulich wrote: > >>> On 28.09.15 at 09:13, wrote: > > This patch adds a field tsc_scaling_ratio in struct arch_vcpu to > > Why not in struct hvm_vcpu? Are you intending any use for PV guests? > No, I'll move tsc_scaling_ratio to struct hvm_cpu. > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -297,6 +297,34 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat) > > return 1; > > } > > > > +void hvm_setup_tsc_scaling(struct vcpu *v) > > +{ > > + u64 ratio, khz; > > + s8 shift; > > Hard tab. will change to spaces > > > + if ( !hvm_funcs.tsc_scaling_supported ) > > + return; > > + > > + khz = v->domain->arch.tsc_khz; > > I don't see the need for this variable in the first place. But if you > absolutely want to keep it, I don't see why it needs to be u64 > when the field you load from is uint32_t. > will remove > > + shift = (hvm_funcs.tsc_scaling_ratio_frac_bits <= 32) ? > > + hvm_funcs.tsc_scaling_ratio_frac_bits : 32; > > min() > yes > > + ratio = khz << shift; > > + do_div(ratio, cpu_khz); > > + ratio <<= hvm_funcs.tsc_scaling_ratio_frac_bits - shift; > > + > > + if ( ratio == 0 || > > + ratio > hvm_funcs.max_tsc_scaling_ratio || > > + ratio & hvm_funcs.tsc_scaling_ratio_rsvd ) > > Parentheses around the operands of the & please. > will add > > + { > > + printk(XENLOG_WARNING > > + "Invalid TSC scaling ratio - virtual tsc khz=%lu\n", > > + khz); > > Who can issue a call to this function under which conditions? I.e. is > a non-ratelimited printk() okay here? Plus, without identifying the > subject vcpu I don't think the message is of much use beyond your > initial debugging purposes. > hvm_load_cpu_ctxt(), hvm_vcpu_reset_state() and tsc_set_info() call this function. Am I correct that those functions are not called in high rate? But I agree that the warning is useless w/o identifying the subject vcpu and will add it. > > @@ -2023,6 +2051,9 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) > > if ( hvm_funcs.load_cpu_ctxt(v, &ctxt) < 0 ) > > return -EINVAL; > > > > + if ( !v->domain->arch.vtsc && hvm_funcs.tsc_scaling_supported ) > > + hvm_setup_tsc_scaling(v); > > What's the rationale for putting it in this function? I cannot remind clearly why it's needed here. I remember it's used in the case that a domain is restored from a file by 'xl restore'. I'll reply to this issue later. > And what's the > reason for the dependency on !vtsc (please also see the comment > ahead of tsc_set_info())? > If v->domain->arch.vtsc == 1, guest rdtsc/rdtscp is trapped (setup in tsc_set_info()) and emulated by hypervisor and the hardware TSC scaling is not used in this case. > > --- a/xen/arch/x86/time.c > > +++ b/xen/arch/x86/time.c > > @@ -1956,6 +1956,8 @@ void tsc_set_info(struct domain *d, > > { > > case TSC_MODE_NEVER_EMULATE: > > d->arch.vtsc = 0; > > + if ( tsc_mode == TSC_MODE_NEVER_EMULATE ) > > + d->arch.tsc_khz = cpu_khz; > > break; > > } > > Depending on the changes to the first two patches: If this change > would remain like this, please move out the TSC_MODE_NEVER_EMULATE > case to be a standalone one again, since the way you do it here > looks pretty confusing/odd. > Thanks for pointing out it! This two lines change is incorrect and should be removed. > > @@ -1981,8 +1983,14 @@ void tsc_set_info(struct domain *d, > > if ( is_hvm_domain(d) ) > > { > > hvm_set_rdtsc_exiting(d, d->arch.vtsc); > > - if ( d->vcpu && d->vcpu[0] && incarnation == 0 ) > > + if ( d->vcpu && d->vcpu[0] ) > > { > > + if ( !d->arch.vtsc && hvm_funcs.tsc_scaling_supported ) > > + hvm_setup_tsc_scaling(d->vcpu[0]); > > And what about the other vCPU-s? If you mean this to be along > the lines of the code that follows here, you should put this after > the comment explaining that. > TSC scaling for other vcpus are set in hvm_vcpu_reset_state(). But I'm not sure it can be moved together with the followed code because of the followed if ( incarnation ) return; incarnation != 0 after migration and the setup of TSC scaling is however necessary. > Jan >