| Message ID | 1456193104-12761-3-git-send-email-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.02.16 at 03:05, <haozhong.zhang@intel.com> wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -298,6 +298,29 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat) > return 1; > } > > +/* > + * Get the ratio to scale host TSC frequency to gtsc_khz. zero will be > + * returned if TSC scaling is unavailable or ratio cannot be handled > + * by host CPU. Otherwise, a non-zero ratio will be returned. > + */ > +u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz) > +{ > + u64 ratio = gtsc_khz; > + u64 dummy = 0; "dummy" suggests it is unused, which it isn't. "tmp" or "hi" might be a little better, but since the meanings of the variables (also "ratio") differ for their roles an inputs and outputs, splitting inputs and outputs below would seem even better. In which case "dummy" become would an appropriate name again. > + if ( !hvm_tsc_scaling_supported ) > + return 0; > + > + /* ratio = (gtsc_khz << hvm_funcs.tsc_scaling.ratio_frac_bits) / cpu_khz */ > + asm ( > + "shldq %2,%1,%0; salq %2,%1; divq %3" > + : "+&d" (dummy), "+&a" (ratio) > + : "c" (hvm_funcs.tsc_scaling.ratio_frac_bits), > + "rm" ((u64) cpu_khz) ); And this DIVQ can't possibly #DE, e.g. when gtsc_khz is much larger than cpu_khz? I'd also prefer if the instruction got put on the same line as the "asm (". Considering that we're dealing with unsigned quantities here I'd further prefer if SHLQ was used instead of SALQ. And finally I'd suggest using named rather than numbered asm() arguments. > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -272,6 +272,14 @@ u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc); > #define hvm_tsc_scaling_supported \ > (!!hvm_funcs.tsc_scaling.ratio_frac_bits) > > +#define hvm_default_tsc_scaling_ratio \ > + (1ULL << hvm_funcs.tsc_scaling.ratio_frac_bits) > + > +#define hvm_vcpu_tsc_scaling_ratio(v) \ > + ((v)->domain->arch.hvm_domain.tsc_scaling_ratio) Since this is now a per-domain property I think it is misleading (and potentially hindering) for the called to pass in a struct vcpu * here. Please make this a struct domain *. Jan
On 02/24/16 08:01, Jan Beulich wrote: > >>> On 23.02.16 at 03:05, <haozhong.zhang@intel.com> wrote: > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -298,6 +298,29 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat) > > return 1; > > } > > > > +/* > > + * Get the ratio to scale host TSC frequency to gtsc_khz. zero will be > > + * returned if TSC scaling is unavailable or ratio cannot be handled > > + * by host CPU. Otherwise, a non-zero ratio will be returned. > > + */ > > +u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz) > > +{ > > + u64 ratio = gtsc_khz; > > + u64 dummy = 0; > > "dummy" suggests it is unused, which it isn't. "tmp" or "hi" might be > a little better, but since the meanings of the variables (also "ratio") > differ for their roles an inputs and outputs, splitting inputs and > outputs below would seem even better. In which case "dummy" > become would an appropriate name again. > Yes, I'll split input and outputs for dummy and ratio. > > + if ( !hvm_tsc_scaling_supported ) > > + return 0; > > + > > + /* ratio = (gtsc_khz << hvm_funcs.tsc_scaling.ratio_frac_bits) / cpu_khz */ > > + asm ( > > + "shldq %2,%1,%0; salq %2,%1; divq %3" > > + : "+&d" (dummy), "+&a" (ratio) > > + : "c" (hvm_funcs.tsc_scaling.ratio_frac_bits), > > + "rm" ((u64) cpu_khz) ); > > And this DIVQ can't possibly #DE, e.g. when gtsc_khz is much larger > than cpu_khz? > Oops, it could. Following check should be added before asm(): /* the quotient is too large to fit in the integral part of TSC scaling ratio */ if ( gtsc_khz / cpu_khz > (hvm_funcs.tsc_scaling.max_ratio >> hvm_funcs.tsc_scaling.ratio_frac_bits ) return 0; > I'd also prefer if the instruction got put on the same line as the > "asm (". Considering that we're dealing with unsigned quantities > here I'd further prefer if SHLQ was used instead of SALQ. And > finally I'd suggest using named rather than numbered asm() > arguments. > I'll make all these three changes. > > --- a/xen/include/asm-x86/hvm/hvm.h > > +++ b/xen/include/asm-x86/hvm/hvm.h > > @@ -272,6 +272,14 @@ u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc); > > #define hvm_tsc_scaling_supported \ > > (!!hvm_funcs.tsc_scaling.ratio_frac_bits) > > > > +#define hvm_default_tsc_scaling_ratio \ > > + (1ULL << hvm_funcs.tsc_scaling.ratio_frac_bits) > > + > > +#define hvm_vcpu_tsc_scaling_ratio(v) \ > > + ((v)->domain->arch.hvm_domain.tsc_scaling_ratio) > > Since this is now a per-domain property I think it is misleading (and > potentially hindering) for the called to pass in a struct vcpu * here. > Please make this a struct domain *. > I'll change to struct domain *, and also remove _vcpu in the name. Thanks, Haozhong
>>> On 24.02.16 at 16:42, <haozhong.zhang@intel.com> wrote: > On 02/24/16 08:01, Jan Beulich wrote: >> >>> On 23.02.16 at 03:05, <haozhong.zhang@intel.com> wrote: >> > + /* ratio = (gtsc_khz << hvm_funcs.tsc_scaling.ratio_frac_bits) / cpu_khz */ >> > + asm ( >> > + "shldq %2,%1,%0; salq %2,%1; divq %3" >> > + : "+&d" (dummy), "+&a" (ratio) >> > + : "c" (hvm_funcs.tsc_scaling.ratio_frac_bits), >> > + "rm" ((u64) cpu_khz) ); >> >> And this DIVQ can't possibly #DE, e.g. when gtsc_khz is much larger >> than cpu_khz? >> > > Oops, it could. Following check should be added before asm(): > /* the quotient is too large to fit in the integral part of TSC > scaling ratio */ > if ( gtsc_khz / cpu_khz > > (hvm_funcs.tsc_scaling.max_ratio >> > hvm_funcs.tsc_scaling.ratio_frac_bits ) > return 0; Well, wouldn't that need to be >= then, since the division truncates? Jan
On 02/24/16 08:51, Jan Beulich wrote: > >>> On 24.02.16 at 16:42, <haozhong.zhang@intel.com> wrote: > > On 02/24/16 08:01, Jan Beulich wrote: > >> >>> On 23.02.16 at 03:05, <haozhong.zhang@intel.com> wrote: > >> > + /* ratio = (gtsc_khz << hvm_funcs.tsc_scaling.ratio_frac_bits) / cpu_khz */ > >> > + asm ( > >> > + "shldq %2,%1,%0; salq %2,%1; divq %3" > >> > + : "+&d" (dummy), "+&a" (ratio) > >> > + : "c" (hvm_funcs.tsc_scaling.ratio_frac_bits), > >> > + "rm" ((u64) cpu_khz) ); > >> > >> And this DIVQ can't possibly #DE, e.g. when gtsc_khz is much larger > >> than cpu_khz? > >> > > > > Oops, it could. Following check should be added before asm(): > > /* the quotient is too large to fit in the integral part of TSC > > scaling ratio */ > > if ( gtsc_khz / cpu_khz > > > (hvm_funcs.tsc_scaling.max_ratio >> > > hvm_funcs.tsc_scaling.ratio_frac_bits ) > > return 0; > > Well, wouldn't that need to be >= then, since the division truncates? > No. The division truncation on (gtsc_khz/cpu_khz) gets the integral part that is the part I want to check whether can fit in the integral part of TSC scaling ratio. Haozhong
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index a72a6e7..b239bdb 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -298,6 +298,29 @@ int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat) return 1; } +/* + * Get the ratio to scale host TSC frequency to gtsc_khz. zero will be + * returned if TSC scaling is unavailable or ratio cannot be handled + * by host CPU. Otherwise, a non-zero ratio will be returned. + */ +u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz) +{ + u64 ratio = gtsc_khz; + u64 dummy = 0; + + if ( !hvm_tsc_scaling_supported ) + return 0; + + /* ratio = (gtsc_khz << hvm_funcs.tsc_scaling.ratio_frac_bits) / cpu_khz */ + asm ( + "shldq %2,%1,%0; salq %2,%1; divq %3" + : "+&d" (dummy), "+&a" (ratio) + : "c" (hvm_funcs.tsc_scaling.ratio_frac_bits), + "rm" ((u64) cpu_khz) ); + + return ratio > hvm_funcs.tsc_scaling.max_ratio ? 0 : ratio; +} + void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc) { uint64_t tsc; @@ -1643,6 +1666,9 @@ int hvm_domain_initialise(struct domain *d) register_portio_handler(d, 0xe9, 1, hvm_print_line); register_portio_handler(d, 0xcf8, 4, hvm_access_cf8); + if ( hvm_tsc_scaling_supported ) + d->arch.hvm_domain.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio; + rc = hvm_funcs.domain_initialise(d); if ( rc != 0 ) goto fail2; diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index b22d4a1..1136937 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -823,7 +823,7 @@ static uint64_t svm_scale_tsc(const struct vcpu *v, uint64_t tsc) { ASSERT(cpu_has_tsc_ratio && !v->domain->arch.vtsc); - return scale_tsc(tsc, vcpu_tsc_ratio(v)); + return scale_tsc(tsc, hvm_vcpu_tsc_scaling_ratio(v)); } static uint64_t svm_get_tsc_offset(uint64_t host_tsc, uint64_t guest_tsc, @@ -1000,7 +1000,7 @@ static inline void svm_tsc_ratio_save(struct vcpu *v) static inline void svm_tsc_ratio_load(struct vcpu *v) { if ( cpu_has_tsc_ratio && !v->domain->arch.vtsc ) - wrmsrl(MSR_AMD64_TSC_RATIO, vcpu_tsc_ratio(v)); + wrmsrl(MSR_AMD64_TSC_RATIO, hvm_vcpu_tsc_scaling_ratio(v)); } static void svm_ctxt_switch_from(struct vcpu *v) diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c index 2248dfa..fda9692 100644 --- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -1865,7 +1865,8 @@ void tsc_set_info(struct domain *d, */ if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() && (has_hvm_container_domain(d) ? - d->arch.tsc_khz == cpu_khz || hvm_tsc_scaling_supported : + (d->arch.tsc_khz == cpu_khz || + hvm_get_tsc_scaling_ratio(d->arch.tsc_khz)) : incarnation == 0) ) { case TSC_MODE_NEVER_EMULATE: @@ -1879,7 +1880,8 @@ void tsc_set_info(struct domain *d, d->arch.vtsc = !boot_cpu_has(X86_FEATURE_RDTSCP) || !host_tsc_is_safe(); enable_tsc_scaling = has_hvm_container_domain(d) && - hvm_tsc_scaling_supported && !d->arch.vtsc; + !d->arch.vtsc && + hvm_get_tsc_scaling_ratio(gtsc_khz ?: cpu_khz); d->arch.tsc_khz = (enable_tsc_scaling && gtsc_khz) ? gtsc_khz : cpu_khz; set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 ); d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns); @@ -1897,6 +1899,10 @@ void tsc_set_info(struct domain *d, d->arch.incarnation = incarnation + 1; if ( has_hvm_container_domain(d) ) { + if ( hvm_tsc_scaling_supported && !d->arch.vtsc ) + d->arch.hvm_domain.tsc_scaling_ratio = + hvm_get_tsc_scaling_ratio(d->arch.tsc_khz); + hvm_set_rdtsc_exiting(d, d->arch.vtsc); if ( d->vcpu && d->vcpu[0] && incarnation == 0 ) { diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h index 4b54c5d..678f10a 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -143,6 +143,8 @@ struct hvm_domain { */ uint64_t sync_tsc; + uint64_t tsc_scaling_ratio; + unsigned long *io_bitmap; /* List of permanently write-mapped pages. */ diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 7e87b10..6264d9e 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -272,6 +272,14 @@ u64 hvm_get_guest_tsc_fixed(struct vcpu *v, u64 at_tsc); #define hvm_tsc_scaling_supported \ (!!hvm_funcs.tsc_scaling.ratio_frac_bits) +#define hvm_default_tsc_scaling_ratio \ + (1ULL << hvm_funcs.tsc_scaling.ratio_frac_bits) + +#define hvm_vcpu_tsc_scaling_ratio(v) \ + ((v)->domain->arch.hvm_domain.tsc_scaling_ratio) + +u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz); + int hvm_set_mode(struct vcpu *v, int mode); void hvm_init_guest_time(struct domain *d); void hvm_set_guest_time(struct vcpu *v, u64 guest_time); diff --git a/xen/include/asm-x86/hvm/svm/svm.h b/xen/include/asm-x86/hvm/svm/svm.h index d60ec23..c954b7e 100644 --- a/xen/include/asm-x86/hvm/svm/svm.h +++ b/xen/include/asm-x86/hvm/svm/svm.h @@ -97,9 +97,6 @@ extern u32 svm_feature_flags; /* TSC rate */ #define DEFAULT_TSC_RATIO 0x0000000100000000ULL #define TSC_RATIO_RSVD_BITS 0xffffff0000000000ULL -#define TSC_RATIO(g_khz, h_khz) ( (((u64)(g_khz)<<32)/(u64)(h_khz)) & \ - ~TSC_RATIO_RSVD_BITS ) -#define vcpu_tsc_ratio(v) TSC_RATIO((v)->domain->arch.tsc_khz, cpu_khz) extern void svm_host_osvw_reset(void); extern void svm_host_osvw_init(void);
This patch adds a field tsc_scaling_ratio in struct hvm_domain to record the per-domain TSC scaling ratio, and sets it in tsc_set_info(). Before setting the per-domain TSC scaling ratio, 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> --- Changes in v5: * Drop "Reviewed-by" from Boris Ostrovsky because of following changes. * Rewrite 64-bit integer arithmetic in hvm_get_tsc_scaling_ratio() by inlined assembly. * Make the previously per-vcpu tsc_scaling_ratio to struct hvm_domain and setup it per-domain. --- xen/arch/x86/hvm/hvm.c | 26 ++++++++++++++++++++++++++ xen/arch/x86/hvm/svm/svm.c | 4 ++-- xen/arch/x86/time.c | 10 ++++++++-- xen/include/asm-x86/hvm/domain.h | 2 ++ xen/include/asm-x86/hvm/hvm.h | 8 ++++++++ xen/include/asm-x86/hvm/svm/svm.h | 3 --- 6 files changed, 46 insertions(+), 7 deletions(-)