Re: [PATCH v2 04/14] x86/time.c: Use correct guest TSC frequency in tsc_get_info()

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 12/08/15 13:26, Boris Ostrovsky wrote:
> On 12/07/2015 08:08 PM, Haozhong Zhang wrote:
> >On 12/07/15 12:53, Boris Ostrovsky wrote:
> >>On 12/06/2015 03:58 PM, Haozhong Zhang wrote:
> >>>When the TSC mode of a HVM container is TSC_MODE_DEFAULT or
> >>>TSC_MODE_PVRDTSCP and no TSC emulation is used, the existing
> >>>tsc_get_info() uses the host TSC frequency (cpu_khz) as the guest TSC
> >>>frequency. However, tsc_set_info() may set the guest TSC frequency to a
> >>>value different than the host. In order to keep consistent to
> >>>tsc_set_info(), this patch makes tsc_get_info() use the value set by
> >>>tsc_set_info() as the guest TSC frequency.
> >>>
> >>>Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >>>---
> >>>  xen/arch/x86/time.c | 12 +++++++++---
> >>>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>>
> >>>diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> >>>index 1091e69..95df4f1 100644
> >>>--- a/xen/arch/x86/time.c
> >>>+++ b/xen/arch/x86/time.c
> >>>@@ -1749,6 +1749,9 @@ void tsc_get_info(struct domain *d, uint32_t *tsc_mode,
> >>>                    uint64_t *elapsed_nsec, uint32_t *gtsc_khz,
> >>>                    uint32_t *incarnation)
> >>>  {
> >>>+    bool_t enable_tsc_scaling = has_hvm_container_domain(d) &&
> >>>+                                cpu_has_tsc_ratio;
> >>&& !d->arch.vtsc ?
> >>
> >>(assuming my comment to the previous patch is correct).
> >>
> >enable_tsc_scaling in tsc_get_info() is always used in the condition
> >!d->arch.vtsc, so it's not necessary to include it again in
> >enable_tsc_scaling.
> >
> >>>+
> >>>      *incarnation = d->arch.incarnation;
> >>>      *tsc_mode = d->arch.tsc_mode;
> >>>@@ -1769,7 +1772,7 @@ void tsc_get_info(struct domain *d, uint32_t *tsc_mode,
> >>>          }
> >>>          tsc = rdtsc();
> >>>          *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns);
> >>>-        *gtsc_khz = cpu_khz;
> >>>+        *gtsc_khz = enable_tsc_scaling ? d->arch.tsc_khz : cpu_khz;
> >>>          break;
> >>>      case TSC_MODE_PVRDTSCP:
> >>>          if ( d->arch.vtsc )
> >>>@@ -1779,10 +1782,13 @@ void tsc_get_info(struct domain *d, uint32_t *tsc_mode,
> >>>          }
> >>>          else
> >>>          {
> >>>+            struct time_scale *scale = enable_tsc_scaling ?
> >>>+                                       &this_cpu(cpu_time).tsc_scale :
> >>>+                                       &d->arch.vtsc_to_ns;
> >>IIUIC tsc_scale is host property and so why would it be used if TSC scaling
> >>is available to guests?
> >>
> >scale is used below to convert a host TSC to nanosec. When TSC scaling is used,
> >d->arch.vtsc_to_ns may not base on the host TSC frequency, so I turn to the host
> >tsc_scale instead.
> 
> OK. Can we then use tsc_scale for both with and without TSC scaling? (on the
> set side too).
>
Yes, we can, because early_time_init() calculates tsc_scale based on
cpu_khz. I'll change it in the next version.

> (And as a side note, I think that the comment in include/asm-x86/domain.h
> describing vtsc_to_ns as being used for emulated cases is rather misleading.
> We use it for both emulated and native)
>
agree, I'll update those comments in the next version.

Haozhong

> -boris
> 
> 
> >
> >Haozhong
> >
> >>-boris
> >>
> >>
> >>>              tsc = rdtsc();
> >>>-            *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns) -
> >>>+            *elapsed_nsec = scale_delta(tsc, scale) -
> >>>                              d->arch.vtsc_offset;
> >>>-            *gtsc_khz = 0; /* ignored by tsc_set_info */
> >>>+            *gtsc_khz = enable_tsc_scaling ? d->arch.tsc_khz : 0;
> >>>          }
> >>>          break;
> >>>      }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel