From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v4 04/10] x86/hvm: Collect information of TSC scaling ratio Date: Fri, 05 Feb 2016 04:41:39 -0700 Message-ID: <56B4988302000078000CF005@prv-mh.provo.novell.com> References: <1453067939-9121-1-git-send-email-haozhong.zhang@intel.com> <1453067939-9121-5-git-send-email-haozhong.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1453067939-9121-5-git-send-email-haozhong.zhang@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Haozhong Zhang Cc: Kevin Tian , Keir Fraser , Suravee Suthikulpanit , Andrew Cooper , xen-devel@lists.xen.org, Aravind Gopalakrishnan , Jun Nakajima , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org >>> On 17.01.16 at 22:58, wrote: > Both VMX TSC scaling and SVM TSC ratio use the 64-bit TSC scaling ratio, > but the number of fractional bits of the ratio is different between VMX > and SVM. This patch adds the architecture code to collect the number of > fractional bits and other related information into fields of struct > hvm_function_table so that they can be used in the common code. > > Signed-off-by: Haozhong Zhang > Reviewed-by: Kevin Tian > Reviewed-by: Boris Ostrovsky > --- > Changes in v4: > (addressing Jan Beulich's comments in v3 patch 12) > * Set TSC scaling parameters in hvm_funcs conditionally. > * Remove TSC scaling parameter tsc_scaling_supported in hvm_funcs which > can be derived from other parameters. > (code cleanup) > * Merge with v3 patch 11 "x86/hvm: Detect TSC scaling through hvm_funcs" > whose work can be done early in this patch. I really think this the scope of these changes should have invalidated all earlier tags. > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -1450,6 +1450,14 @@ const struct hvm_function_table * __init start_svm(void) > if ( !cpu_has_svm_nrips ) > clear_bit(SVM_FEATURE_DECODEASSISTS, &svm_feature_flags); > > + if ( cpu_has_tsc_ratio ) > + { > + svm_function_table.default_tsc_scaling_ratio = DEFAULT_TSC_RATIO; > + svm_function_table.max_tsc_scaling_ratio = ~TSC_RATIO_RSVD_BITS; > + svm_function_table.tsc_scaling_ratio_frac_bits = 32; > + svm_function_table.scale_tsc = svm_scale_tsc; > + } > + > #define P(p,s) if ( p ) { printk(" - %s\n", s); printed = 1; } > P(cpu_has_svm_npt, "Nested Page Tables (NPT)"); > P(cpu_has_svm_lbrv, "Last Branch Record (LBR) Virtualisation"); > @@ -2269,8 +2277,6 @@ static struct hvm_function_table __initdata svm_function_table = { > .nhvm_vmcx_hap_enabled = nsvm_vmcb_hap_enabled, > .nhvm_intr_blocked = nsvm_intr_blocked, > .nhvm_hap_walk_L1_p2m = nsvm_hap_walk_L1_p2m, > - > - .scale_tsc = svm_scale_tsc, > }; >>From at the first glance purely mechanical POV this change was unnecessary with ... > @@ -249,6 +261,8 @@ void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc); > u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc); > #define hvm_get_guest_tsc(v) hvm_get_guest_tsc_fixed(v, 0) > > +#define hvm_tsc_scaling_supported (!!hvm_funcs.default_tsc_scaling_ratio) ... this, but considering our general aim to avoid having NULL callback pointers wherever possible, I think this is more than just a mechanical concern: I'd prefer if at least the callback pointer always be statically initialized, and ideally also two of the other fields. Only one field should be dynamically initialized (unless - considering the VMX code to come - static initialization is impossible), and ideally one which, if zero, would not have any bad consequences if used by mistake (frac_bits maybe). And perhaps an ASSERT() should be placed inside svm_scale_tsc() making sure the dynamically initialized field actually is initialized. The conditional here would then check _all_ fields which either vendor's code leaves uninitialized (i.e. the VMX patch may then add to the above). Jan