Re: [Xen-devel] [PATCH v2 31/62] x86: xen pv clock time source

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 01/12/2018 11:28 AM, Wei Liu wrote:
> It is a variant of TSC clock source.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Use the mapped vcpu_info.
> ---
>  xen/arch/x86/time.c | 89 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index 3b654d7b7d..4cbd068d17 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -29,6 +29,7 @@
>  #include <asm/mpspec.h>
>  #include <asm/processor.h>
>  #include <asm/fixmap.h>
> +#include <asm/guest.h>
>  #include <asm/mc146818rtc.h>
>  #include <asm/div64.h>
>  #include <asm/acpi.h>
> @@ -525,6 +526,91 @@ static struct platform_timesource __initdata plt_tsc =
>      .init = init_tsc,
>  };
>  
> +#ifdef CONFIG_XEN_GUEST
> +/************************************************************
> + * PLATFORM TIMER 5: XEN PV CLOCK SOURCE
> + *
> + * Xen clock source is a variant of TSC source.
> + */
> +
> +static uint64_t xen_timer_cpu_frequency(void)
> +{
> +    struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
> +    uint64_t freq;
> +
> +    freq = 1000000000ULL << 32;
> +    do_div(freq, info->tsc_to_system_mul);
> +    if ( info->tsc_shift < 0 )
> +        freq <<= -info->tsc_shift;
> +    else
> +        freq >>= info->tsc_shift;
> +
> +    return freq;
> +}
> +
> +static int64_t __init init_xen_timer(struct platform_timesource *pts)
> +{
> +    if ( !xen_guest )
> +        return 0;
> +
> +    pts->frequency = xen_timer_cpu_frequency();
> +
> +    return pts->frequency;
> +}
> +
Guests that don't have TSC_RELIABLE set in verify_tsc_reliability() you end up
evaluating this condition to true:

if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
     !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
    time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;

And that's possibly not necessary to keep sync the TSC in sync with pvclock?

Meaning the default rendezvous function (time_calibration_std_rendezvous) might
be enough.

> +static always_inline uint64_t read_cycle(const struct vcpu_time_info *info,
> +                                         uint64_t tsc)
> +{
> +    uint64_t delta = tsc - info->tsc_timestamp;
> +    struct time_scale ts = {
> +        .shift    = info->tsc_shift,
> +        .mul_frac = info->tsc_to_system_mul,
> +    };
> +    uint64_t offset = scale_delta(delta, &ts);
> +
> +    return info->system_time + offset;
> +}
> +
> +static uint64_t read_xen_timer(void)
> +{
> +    struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
> +    uint32_t version;
> +    uint64_t ret;
> +    uint64_t last;
> +    static uint64_t last_value;
> +
> +    do {
> +        version = info->version & ~1;
> +        /* Make sure version is read before the data */
> +        smp_rmb();
> +
> +        ret = read_cycle(info, rdtsc_ordered());
> +        /* Ignore fancy flags for now */
> +

Heh :)

When the dust settles around the security issues, perhaps this patch could help
(I can formally submit it after more proper testing, but should give an
expectation on the changes).

-- >8 --
Subject: x86/time: mark pvclock as stable if supported

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index c90524de9c..3aaa72b6bf 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -65,8 +65,12 @@ struct platform_timesource {
     s64 (*init)(struct platform_timesource *);
     void (*resume)(struct platform_timesource *);
     int counter_bits;
+    unsigned int flags;
 };

+/* Clocksource counter monotonically increases across CPUs */
+#define CLOCKSOURCE_F_STABLE    (1 << 0)
+
 static DEFINE_PER_CPU(struct cpu_time, cpu_time);

 /* Calibrate all CPUs to platform timer every EPOCH. */
@@ -533,9 +537,8 @@ static struct platform_timesource __initdata plt_tsc =
  * Xen clock source is a variant of TSC source.
  */

-static uint64_t xen_timer_cpu_frequency(void)
+static uint64_t xen_timer_cpu_frequency(struct vcpu_time_info *info)
 {
-    struct vcpu_time_info *info = &this_cpu(vcpu_info)->time;
     uint64_t freq;

     freq = 1000000000ULL << 32;
@@ -550,10 +553,20 @@ static uint64_t xen_timer_cpu_frequency(void)

 static int64_t __init init_xen_timer(struct platform_timesource *pts)
 {
+    struct vcpu_time_info *info;
+
     if ( !xen_guest )
         return 0;

-    pts->frequency = xen_timer_cpu_frequency();
+    info = &this_cpu(vcpu_info)->time;
+
+    pts->frequency = xen_timer_cpu_frequency(info);
+
+    /* Xen pvclock monotonically increases across CPUs */
+    if ( info->flags & XEN_PVCLOCK_TSC_STABLE_BIT ) {
+           pts->flags |= CLOCKSOURCE_F_STABLE;
+           printk("PV CLOCK marked as stable.\n");
+    }

     return pts->frequency;
 }
@@ -585,12 +598,14 @@ static uint64_t read_xen_timer(void)
         smp_rmb();

         ret = read_cycle(info, rdtsc_ordered());
-        /* Ignore fancy flags for now */

         /* Make sure version is reread after the data */
         smp_rmb();
     } while ( unlikely(version != info->version) );

+    if ( info->flags & XEN_PVCLOCK_TSC_STABLE_BIT )
+        return ret;
+
     /* Maintain a monotonic global value */
     do {
         last = read_atomic(&last_value);
@@ -632,6 +647,8 @@ static u64 plt_stamp64;          /* 64-bit platform counter
stamp           */
 static u64 plt_stamp;            /* hardware-width platform counter stamp   */
 static struct timer plt_overflow_timer;

+static bool clocksource_is_stable(void);
+
 static s_time_t __read_platform_stime(u64 platform_time)
 {
     u64 diff = platform_time - platform_timer_stamp;
@@ -1077,7 +1094,7 @@ static void __update_vcpu_system_time(struct vcpu *v, int
force)
      * pvclock read to check whether they can resort solely on this tuple
      * or if it further requires monotonicity checks with other vcpus.
      */
-    if ( clocksource_is_tsc() )
+    if ( clocksource_is_stable() )
         _u.flags |= XEN_PVCLOCK_TSC_STABLE_BIT;

     if ( is_hvm_domain(d) )
@@ -1564,7 +1581,7 @@ static void time_calibration(void *unused)
         .semaphore = ATOMIC_INIT(0)
     };

-    if ( clocksource_is_tsc() )
+    if ( clocksource_is_stable() )
     {
         local_irq_disable();
         r.master_stime = read_platform_stime(&r.master_tsc_stamp);
@@ -1740,6 +1757,9 @@ static int __init verify_tsc_reliability(void)
              */
             on_selected_cpus(&cpu_online_map, reset_percpu_time, NULL, 1);

+            /* TSC clocksource monotonically increases across CPUs. */
+            plt_tsc.flags |= CLOCKSOURCE_F_STABLE;
+
             /*
              * We won't do CPU Hotplug and TSC clocksource is being used which
              * means we have a reliable TSC, plus we don't sync with any other
@@ -1770,7 +1790,12 @@ static int __init verify_tsc_reliability(void)
      */
     if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
          !boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
-        time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
+    {
+       if ( clocksource_is_stable() )
+            time_calibration_rendezvous_fn = time_calibration_nop_rendezvous;
+        else if ( plt_src.read_counter != read_xen_timer )
+            time_calibration_rendezvous_fn = time_calibration_tsc_rendezvous;
+    }

     return 0;
 }
@@ -2057,6 +2082,23 @@ bool clocksource_is_tsc(void)
     return plt_src.read_counter == read_tsc;
 }

+static bool clocksource_is_stable(void)
+{
+    struct vcpu_time_info *info;
+
+    if ( !(plt_src.flags & CLOCKSOURCE_F_STABLE) )
+        return false;
+
+    if ( plt_src.read_counter == read_tsc )
+        return true;
+
+    info = &this_cpu(vcpu_info)->time;
+    if ( !(info->flags & XEN_PVCLOCK_TSC_STABLE_BIT) )
+        return false;
+
+    return true;
+}
+
 int host_tsc_is_safe(void)
 {
     return boot_cpu_has(X86_FEATURE_TSC_RELIABLE);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel