Re: [PATCH v3 08/13] x86/hvm: Setup TSC scaling ratio

From: Haozhong Zhang <haozhong.zhang@intel.com>
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
	Jan Beulich <jbeulich@suse.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xen.org,
	Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH v3 08/13] x86/hvm: Setup TSC scaling ratio
Date: Tue, 5 Jan 2016 09:20:12 +0800	[thread overview]
Message-ID: <20160105012012.GD3619@hz-desktop.sh.intel.com> (raw)
In-Reply-To: <568ABC81.80006@oracle.com>

On 01/04/16 13:40, Boris Ostrovsky wrote:
> On 12/30/2015 10:03 PM, Haozhong Zhang wrote:
> >This patch adds a field tsc_scaling_ratio in struct hvm_vcpu to
> >record the TSC scaling ratio, and sets it up when tsc_set_info() is
> >called for a vcpu or when a vcpu is restored or reset.
> >
> >Before applying the TSC scaling ratio to CPU, we check its validity in
> >tsc_set_info(). If an invalid ratio is given, we will leave the default
> >value in tsc_scaling_ratio (i.e. ratio = 1) and setup guest TSC as if no
> >TSC scaling is used:
> >* For TSC_MODE_FAULT,
> >   - if a user-specified TSC frequency is given, we will set the guest
> >     TSC frequency to it; otherwise, we set it to the host TSC frequency.
> >   - if guest TSC frequency does not equal to host TSC frequency, we will
> >     emulate guest TSC (i.e. d->arch.vtsc is set to 1). In both cases,
> >     guest TSC runs in the guest TSC frequency.
> >* For TSC_MODE_PVRDTSCP,
> >   - we set the guest TSC frequency to the host TSC frequency.
> >   - guest rdtsc is executed natively in the host TSC frequency as
> >     before.
> >   - if rdtscp is not available to guest, it will be emulated; otherwise,
> >     it will be executed natively. In both cases, guest rdtscp gets TSC
> >     in the host TSC frequency as before.
> >
> >Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >---
> >Changes in v3:
> >  (addressing Boris Ostrovsky's comments)
> >  * Remove redundant checks.
> >  * Update comments in tsc_set_info(): during initial boot -> during domain creation.
> >  (addressing Kevin Tian's comments)
> >  * Add a validation step to check TSC scaling ratio before applying it
> >    to the hardware. If the validation fails, we will fallback to Xen's
> >    original way to handle guest TSC. hvm_setup_tsc_scaling() is always
> >    called when the validation succeeds, so it shoud never fail now.
> >
> >  xen/arch/x86/hvm/hvm.c            | 41 +++++++++++++++++++++++++++++++++++++++
> >  xen/arch/x86/hvm/svm/svm.c        |  6 ++++--
> >  xen/arch/x86/time.c               | 19 +++++++++++++++---
> >  xen/include/asm-x86/hvm/hvm.h     |  6 ++++++
> >  xen/include/asm-x86/hvm/svm/svm.h |  3 ---
> >  xen/include/asm-x86/hvm/vcpu.h    |  2 ++
> >  6 files changed, 69 insertions(+), 8 deletions(-)
> >
> >diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> >index 3648a44..3bf99b1 100644
> >--- a/xen/arch/x86/hvm/hvm.c
> >+++ b/xen/arch/x86/hvm/hvm.c
> >@@ -65,6 +65,7 @@
> >  #include <asm/mtrr.h>
> >  #include <asm/apic.h>
> >  #include <asm/vm_event.h>
> >+#include <asm/math64.h>
> >  #include <public/sched.h>
> >  #include <public/hvm/ioreq.h>
> >  #include <public/version.h>
> >@@ -301,6 +302,42 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat)
> >      return 1;
> >  }
> >+bool_t hvm_validate_tsc_scaling_ratio(uint32_t gtsc_khz)
> >+{
> >+    u64 ratio;
> >+
> >+    if ( !hvm_funcs.tsc_scaling_supported )
> >+        return FALSE;
> >+
> >+    ratio = mul_u64_u32_div(1ULL << hvm_funcs.tsc_scaling_ratio_frac_bits,
> >+                            gtsc_khz, cpu_khz);
> >+
> >+    return (!ratio || ratio > hvm_funcs.max_tsc_scaling_ratio) ? FALSE : TRUE;
> >+}
> >+
> >+void hvm_setup_tsc_scaling(struct vcpu *v)
> >+{
> >+    u64 ratio;
> >+
> >+    if ( !hvm_funcs.tsc_scaling_supported )
> >+        return;
> >+
> >+    /*
> >+     * 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,
> >+                            v->domain->arch.tsc_khz, cpu_khz);
> >+
> >+    if ( ratio == 0 || ratio > hvm_funcs.max_tsc_scaling_ratio )
> >+        return;
> >+
> >+    v->arch.hvm_vcpu.tsc_scaling_ratio = ratio;
> >+
> >+    if ( hvm_funcs.setup_tsc_scaling )
> >+        hvm_funcs.setup_tsc_scaling(v);
> >+}
> 
> Would be nice to factor out common code. E.g. replace
> hvm_validate_tsc_scaling_ratio() with something like get_ratio() which
> returns zero if gtsc_khz is bogus?
> 
> -boris

Yes, I'll change in the next version.

Haozhong