Re: [PATCH v4 04/10] x86/hvm: Collect information of TSC scaling ratio

From: "Jan Beulich" <JBeulich@suse.com>
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>
Subject: Re: [PATCH v4 04/10] x86/hvm: Collect information of TSC scaling ratio
Date: Fri, 05 Feb 2016 04:41:39 -0700	[thread overview]
Message-ID: <56B4988302000078000CF005@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1453067939-9121-5-git-send-email-haozhong.zhang@intel.com>

>>> On 17.01.16 at 22:58, <haozhong.zhang@intel.com> 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 <haozhong.zhang@intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> 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