Re: [PATCH 12/13] vmx: Add a call-back to apply TSC scaling ratio to hardware

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/09/15 11:02, Haozhong Zhang wrote:
> On Tue, Sep 29, 2015 at 10:33:14AM +0100, Andrew Cooper wrote:
>> On 29/09/15 02:07, Haozhong Zhang wrote:
>>> On Mon, Sep 28, 2015 at 12:02:08PM -0400, Boris Ostrovsky wrote:
>>>> On 09/28/2015 03:13 AM, Haozhong Zhang wrote:
>>>>> This patch adds a new call-back setup_tsc_scaling in struct
>>>>> hvm_function_table to apply the TSC scaling ratio to hardware. For VMX,
>>>>> it writes the TSC scaling ratio to VMCS field TSC_MULTIPLIER.
>>>>>
>>>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>>>> ---
>>>>>  xen/arch/x86/hvm/hvm.c        | 1 +
>>>>>  xen/arch/x86/hvm/svm/svm.c    | 5 +++++
>>>>>  xen/arch/x86/hvm/vmx/vmx.c    | 8 ++++++++
>>>>>  xen/include/asm-x86/hvm/hvm.h | 3 +++
>>>>>  4 files changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>>> index 3522d20..2d8a148 100644
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -376,6 +376,7 @@ void hvm_setup_tsc_scaling(struct vcpu *v)
>>>>>      }
>>>>>      v->arch.tsc_scaling_ratio = ratio;
>>>>> +    hvm_funcs.setup_tsc_scaling(v);
>>>>>  }
>>>>>  void hvm_set_guest_tsc_fixed(struct vcpu *v, u64 guest_tsc, u64 at_tsc)
>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>>> index 73bc863..d890c1f 100644
>>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>>> @@ -2236,6 +2236,10 @@ static void svm_invlpg_intercept(unsigned long vaddr)
>>>>>      svm_asid_g_invlpg(curr, vaddr);
>>>>>  }
>>>>> +static void svm_setup_tsc_scaling(struct vcpu *v)
>>>>> +{
>>>>> +}
>>>>> +
>>>> Should this be wrmsrl(MSR_AMD64_TSC_RATIO, v->arch.tsc_scaling_ratio) ?
>>>>
>>>> -boris
>>>>
>>> MSR_AMD64_TSC_RATIO is set in svm_ctxt_switch_to() before entering guest.
>>>
>>> For VMX, the ratio is set to a VMCS field TSC_MULTIPLIER and it's not
>>> necessary to set it every time entering guest. Therefore, I introduce
>>> the call-back setup_tsc_scaling() to do this. For SVM, as the ratio is
>>> set every time entering guest, I leave the SVM version of setup_tsc_scaling()
>>> empty.
>> VT-x has a per-VMCS scale, while SVM has a per-core MSR to adjust the
>> scale.  These do require different modification algorithms.
>>
> Yes, this is what I mean.
>
>> However, if there is any chance that any part of the system can update
>> the ratio while an SVM VCPU is in context (which appears to be the
>> case), then MSR_AMD64_TSC_RATIO needs updating synchronously, or it will
>> be deferred until the next full context switch which could be an
>> arbitrary time into the future.  This appears to be a latent bug in the
>> SVM side.
>>
> In my patch, tsc ratio is set only when
>  1. a domain is created (by arch_domain_create()),
>  2. a vcpu's state is reset (by hvm_vcpu_reset_state()),
>  3. a vcpu's context is restored (by hvm_load_cpu_ctxt()), or
>  4. through the hypercall XEN_DOMCTL_settscinfo.
>
> (Correct me if I'm wrong below)
>
> For the first 3 cases, vcpu is definitely not in context, so it's safe
> to set tsc ratio without any latent bug. For the last case,
> arch_do_domctl() pauses the domain before updating tsc ratio, so it's
> also safe.

That logic appears to be correct, which would suggest that there isn't
actually a latent bug.

In such a case, we would typically make the hvm_funcs pointer optional,
and omit an empty stub on the SVM side.

~Andrew