Re: [Xen-devel] [PATCH 06/13] x86/hvm: Scale host TSC when setting/getting guest TSC

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Thu, Oct 22, 2015 at 08:17:29AM -0600, Jan Beulich wrote:
> >>> On 28.09.15 at 09:13, <haozhong.zhang@xxxxxxxxx> wrote:
> > The existing hvm_set_guest_tsc_fixed() and hvm_get_guest_tsc_fixed()
> > calculate the guest TSC by adding the TSC offset to the host TSC. When
> > the TSC scaling is enabled, the host TSC should be scaled first. This
> > patch adds the scaling logic to those two functions.
> 
> Just like mentioned for the first twp patches - I'd first of all like to
> understand why the lack of scaling this wasn't an issue for SVM so
> far. What you reads plausible, but assuming that SVM TSC scaling
> code was tested, I'm hesitant to apply changes to it without
> understanding the details (or at least without SVM maintainers'
> consent).
>

The current SVM TSC ratio code does not seem correct w/o patch 6 (as
well as patch 2, but I only analyze patch 6 here). Following is the
explanation.

When SVM TSC ratio is used and the ratio is not 1,

1. The original hvm_get_guest_tsc_fixed(v, at_tsc) returns
     (at_tsc ? : rdtsc()) + v->arch.hvm_vcpu.cache_tsc_offset

   It's called in following control flows:

   * hvm_msr_write_intercept(MSR_IA32_TSC_DEADLINE, msr_content, ...)
       vlapic_tdt_msr_set(..., msr_content)
         guest_tsc = hvm_get_guest_tsc(...)
           hvm_get_guest_tsc_fixed(..., 0)

   * hvm_save_cpu_ctxt()
       svm_save_vmcb_ctxt(v, ...)
         svm_save_cpu_state(v, data)
           data->tsc = hvm_get_guest_tsc_fixed(v, 
v->domain->arch.hvm_domain.sync_tsc)

   * svm_set_tsc_offset(v, offset, at_tsc)
       guest_tsc = hvm_get_guest_tsc_fixed(v, at_tsc)

   In all above control flows, hvm_get_guest_tsc_fixed() is expected
   to return the guest TSC. And if its 2nd argument at_tsc is not
   zero, at_tsc is always a host TSC value. Thus,
   hvm_get_guest_tsc_fixed() should scale at_tsc or rdtsc() in order
   to get the correct guest TSC, but it doesn't.

2. In the original hvm_set_guest_tsc_fixed(v, guest_tsc, at_tsc),
     v->arch.hvm_vcpu.cache_tsc_offset = guest_tsc - (at_tsc ? : rdtsc())

   It's called in following control flows:

   * hvm_set_guest_tsc(v, t)
       hvm_set_guest_tsc_fixed(v, t, 0)

   * hvm_load_cpu_ctxt()
       svm_load_vmcb_ctxt(v, ctxt)
         svm_load_cpu_state(v, ctxt)
           hvm_set_guest_tsc_fixed(v, ctxt->tsc, 
v->domain->arch.hvm_domain.sync_tsc)

   In all above control flows, if the 3rd argument at_tsc of
   hvm_set_guest_tsc_fixed() is not zero, it's always a host TSC
   value. In order to get the correct TSC offset,
   hvm_set_guest_tsc_fixed() should scale at_tsc or rdtsc(), but it
   doesn't.

So this patch 6 is necessary to fix hvm_[g|s]et_guest_tsc_fixed() in
the TSC scaling circumstance.

Haozhong

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel