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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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