Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH v2] viridian: unify time sources
@ 2019-06-17  8:23 Paul Durrant
  2019-06-20  8:44 ` Paul Durrant
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Paul Durrant @ 2019-06-17  8:23 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monné

Currently, the time_ref_count enlightened time source maintains an offset
such that time is frozen when the domain paused, but the reference_tsc
enlightened time source does not. After migrate, the reference_tsc source
may become invalidated (e.g. because of host cpu frequency mismatch) which
will cause Windows to fall back to time_ref_count. Thus, the guest will
observe a jump in time equivalent to the offset.

This patch unifies the two enlightened time sources such that the same
offset applies to both of them. Also, it's not really necessary to have
two different functions to calculating a 10MHz counter value, time_now() and
raw_trc_val(), so this patch removes the latter implementation. The
unification also allows removal of the reference_tsc_valid flag.

Whilst in the area, this patch also takes the opportunity to constify a few
pointers which were missed in earlier patches.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

v2:
 - Expand comments above newly added TscOffset setting
---
 xen/arch/x86/hvm/viridian/time.c   | 105 +++++++++++++----------------
 xen/include/asm-x86/hvm/viridian.h |   1 -
 2 files changed, 45 insertions(+), 61 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/time.c b/xen/arch/x86/hvm/viridian/time.c
index 2a3c9697d7..28e3d88ca9 100644
--- a/xen/arch/x86/hvm/viridian/time.c
+++ b/xen/arch/x86/hvm/viridian/time.c
@@ -26,9 +26,10 @@ typedef struct _HV_REFERENCE_TSC_PAGE
     uint64_t Reserved2[509];
 } HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
 
-static void update_reference_tsc(struct domain *d, bool initialize)
+static void update_reference_tsc(const struct domain *d, bool initialize)
 {
     struct viridian_domain *vd = d->arch.hvm.viridian;
+    const struct viridian_time_ref_count *trc = &vd->time_ref_count;
     const struct viridian_page *rt = &vd->reference_tsc;
     HV_REFERENCE_TSC_PAGE *p = rt->ptr;
     uint32_t seq;
@@ -44,7 +45,9 @@ static void update_reference_tsc(struct domain *d, bool initialize)
      * with this, allowing vtsc to be turned off, but support for this is
      * not yet present in the hypervisor. Thus is it is possible that
      * migrating a Windows VM between hosts of differing TSC frequencies
-     * may result in large differences in guest performance.
+     * may result in large differences in guest performance. Any jump in
+     * TSC due to migration down-time can, however, be compensated for by
+     * setting the TscOffset value (see below).
      */
     if ( !host_tsc_is_safe() || d->arch.vtsc )
     {
@@ -62,8 +65,6 @@ static void update_reference_tsc(struct domain *d, bool initialize)
 
         printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: invalidated\n",
                d->domain_id);
-
-        vd->reference_tsc_valid = false;
         return;
     }
 
@@ -75,8 +76,11 @@ static void update_reference_tsc(struct domain *d, bool initialize)
      *
      * Windows uses a 100ns tick, so we need a scale which is cpu
      * ticks per 100ns shifted left by 64.
+     * The offset value is calculated on restore after migration and
+     * ensures that Windows will not see a large jump in ReferenceTime.
      */
     p->TscScale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
+    p->TscOffset = trc->off;
     smp_wmb();
 
     seq = p->TscSequence + 1;
@@ -84,46 +88,6 @@ static void update_reference_tsc(struct domain *d, bool initialize)
         seq = 1;
 
     p->TscSequence = seq;
-    vd->reference_tsc_valid = true;
-}
-
-static int64_t raw_trc_val(const struct domain *d)
-{
-    uint64_t tsc;
-    struct time_scale tsc_to_ns;
-
-    tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
-
-    /* convert tsc to count of 100ns periods */
-    set_time_scale(&tsc_to_ns, d->arch.tsc_khz * 1000ul);
-    return scale_delta(tsc, &tsc_to_ns) / 100ul;
-}
-
-static void time_ref_count_freeze(const struct domain *d)
-{
-    struct viridian_time_ref_count *trc =
-        &d->arch.hvm.viridian->time_ref_count;
-
-    if ( test_and_clear_bit(_TRC_running, &trc->flags) )
-        trc->val = raw_trc_val(d) + trc->off;
-}
-
-static void time_ref_count_thaw(const struct domain *d)
-{
-    struct viridian_time_ref_count *trc =
-        &d->arch.hvm.viridian->time_ref_count;
-
-    if ( !d->is_shutting_down &&
-         !test_and_set_bit(_TRC_running, &trc->flags) )
-        trc->off = (int64_t)trc->val - raw_trc_val(d);
-}
-
-static int64_t time_ref_count(const struct domain *d)
-{
-    struct viridian_time_ref_count *trc =
-        &d->arch.hvm.viridian->time_ref_count;
-
-    return raw_trc_val(d) + trc->off;
 }
 
 /*
@@ -136,7 +100,7 @@ static int64_t time_ref_count(const struct domain *d)
  * 128 bit number which is then shifted 64 times to the right to obtain
  * the high 64 bits."
  */
-static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
+static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, int64_t offset)
 {
     uint64_t result;
 
@@ -153,22 +117,46 @@ static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
     return result + offset;
 }
 
-static uint64_t time_now(struct domain *d)
+static uint64_t trc_val(const struct domain *d, int64_t offset)
 {
     uint64_t tsc, scale;
 
-    /*
-     * If the reference TSC page is not enabled, or has been invalidated
-     * fall back to the partition reference counter.
-     */
-    if ( !d->arch.hvm.viridian->reference_tsc_valid )
-        return time_ref_count(d);
-
-    /* Otherwise compute reference time in the same way the guest would */
     tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
     scale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
 
-    return scale_tsc(tsc, scale, 0);
+    return scale_tsc(tsc, scale, offset);
+}
+
+static void time_ref_count_freeze(const struct domain *d)
+{
+    struct viridian_time_ref_count *trc =
+        &d->arch.hvm.viridian->time_ref_count;
+
+    if ( test_and_clear_bit(_TRC_running, &trc->flags) )
+        trc->val = trc_val(d, trc->off);
+}
+
+static void time_ref_count_thaw(const struct domain *d)
+{
+    struct viridian_domain *vd = d->arch.hvm.viridian;
+    struct viridian_time_ref_count *trc = &vd->time_ref_count;
+
+    if ( d->is_shutting_down ||
+         test_and_set_bit(_TRC_running, &trc->flags) )
+        return;
+
+    trc->off = (int64_t)trc->val - trc_val(d, 0);
+
+    if ( vd->reference_tsc.msr.enabled )
+        update_reference_tsc(d, false);
+}
+
+static int64_t time_ref_count(const struct domain *d)
+{
+    const struct viridian_time_ref_count *trc =
+        &d->arch.hvm.viridian->time_ref_count;
+
+    return trc_val(d, trc->off);
 }
 
 static void stop_stimer(struct viridian_stimer *vs)
@@ -196,7 +184,7 @@ static void start_stimer(struct viridian_stimer *vs)
     const struct vcpu *v = vs->v;
     struct viridian_vcpu *vv = v->arch.hvm.viridian;
     unsigned int stimerx = vs - &vv->stimer[0];
-    int64_t now = time_now(v->domain);
+    int64_t now = time_ref_count(v->domain);
     int64_t expiration;
     s_time_t timeout;
 
@@ -285,7 +273,7 @@ static void poll_stimer(struct vcpu *v, unsigned int stimerx)
 
     if ( !viridian_synic_deliver_timer_msg(v, vs->config.sintx,
                                            stimerx, vs->expiration,
-                                           time_now(v->domain)) )
+                                           time_ref_count(v->domain)) )
         return;
 
     clear_bit(stimerx, &vv->stimer_pending);
@@ -641,10 +629,7 @@ void viridian_time_load_domain_ctxt(
     vd->reference_tsc.msr.raw = ctxt->reference_tsc;
 
     if ( vd->reference_tsc.msr.enabled )
-    {
         viridian_map_guest_page(d, &vd->reference_tsc);
-        update_reference_tsc(d, false);
-    }
 }
 
 /*
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 54e46cc4c4..010c8b58d4 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -116,7 +116,6 @@ struct viridian_domain
     union viridian_page_msr hypercall_gpa;
     struct viridian_time_ref_count time_ref_count;
     struct viridian_page reference_tsc;
-    bool reference_tsc_valid;
 };
 
 void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
-- 
2.20.1.2.gb21ebb671


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Xen-devel] [PATCH v2] viridian: unify time sources
  2019-06-17  8:23 [Xen-devel] [PATCH v2] viridian: unify time sources Paul Durrant
@ 2019-06-20  8:44 ` Paul Durrant
  2019-06-21 13:49 ` Alexandru Stefan ISAILA
  2019-06-21 15:08 ` Jan Beulich
  2 siblings, 0 replies; 7+ messages in thread
From: Paul Durrant @ 2019-06-20  8:44 UTC (permalink / raw)
  To: Jan Beulich, Andrew Cooper, Roger Pau Monne, Wei Liu
  Cc: xen-devel, Paul Durrant

Ping?

> -----Original Message-----
> From: Paul Durrant <paul.durrant@citrix.com>
> Sent: 17 June 2019 09:24
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [PATCH v2] viridian: unify time sources
> 
> Currently, the time_ref_count enlightened time source maintains an offset
> such that time is frozen when the domain paused, but the reference_tsc
> enlightened time source does not. After migrate, the reference_tsc source
> may become invalidated (e.g. because of host cpu frequency mismatch) which
> will cause Windows to fall back to time_ref_count. Thus, the guest will
> observe a jump in time equivalent to the offset.
> 
> This patch unifies the two enlightened time sources such that the same
> offset applies to both of them. Also, it's not really necessary to have
> two different functions to calculating a 10MHz counter value, time_now() and
> raw_trc_val(), so this patch removes the latter implementation. The
> unification also allows removal of the reference_tsc_valid flag.
> 
> Whilst in the area, this patch also takes the opportunity to constify a few
> pointers which were missed in earlier patches.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> 
> v2:
>  - Expand comments above newly added TscOffset setting
> ---
>  xen/arch/x86/hvm/viridian/time.c   | 105 +++++++++++++----------------
>  xen/include/asm-x86/hvm/viridian.h |   1 -
>  2 files changed, 45 insertions(+), 61 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/viridian/time.c b/xen/arch/x86/hvm/viridian/time.c
> index 2a3c9697d7..28e3d88ca9 100644
> --- a/xen/arch/x86/hvm/viridian/time.c
> +++ b/xen/arch/x86/hvm/viridian/time.c
> @@ -26,9 +26,10 @@ typedef struct _HV_REFERENCE_TSC_PAGE
>      uint64_t Reserved2[509];
>  } HV_REFERENCE_TSC_PAGE, *PHV_REFERENCE_TSC_PAGE;
> 
> -static void update_reference_tsc(struct domain *d, bool initialize)
> +static void update_reference_tsc(const struct domain *d, bool initialize)
>  {
>      struct viridian_domain *vd = d->arch.hvm.viridian;
> +    const struct viridian_time_ref_count *trc = &vd->time_ref_count;
>      const struct viridian_page *rt = &vd->reference_tsc;
>      HV_REFERENCE_TSC_PAGE *p = rt->ptr;
>      uint32_t seq;
> @@ -44,7 +45,9 @@ static void update_reference_tsc(struct domain *d, bool initialize)
>       * with this, allowing vtsc to be turned off, but support for this is
>       * not yet present in the hypervisor. Thus is it is possible that
>       * migrating a Windows VM between hosts of differing TSC frequencies
> -     * may result in large differences in guest performance.
> +     * may result in large differences in guest performance. Any jump in
> +     * TSC due to migration down-time can, however, be compensated for by
> +     * setting the TscOffset value (see below).
>       */
>      if ( !host_tsc_is_safe() || d->arch.vtsc )
>      {
> @@ -62,8 +65,6 @@ static void update_reference_tsc(struct domain *d, bool initialize)
> 
>          printk(XENLOG_G_INFO "d%d: VIRIDIAN REFERENCE_TSC: invalidated\n",
>                 d->domain_id);
> -
> -        vd->reference_tsc_valid = false;
>          return;
>      }
> 
> @@ -75,8 +76,11 @@ static void update_reference_tsc(struct domain *d, bool initialize)
>       *
>       * Windows uses a 100ns tick, so we need a scale which is cpu
>       * ticks per 100ns shifted left by 64.
> +     * The offset value is calculated on restore after migration and
> +     * ensures that Windows will not see a large jump in ReferenceTime.
>       */
>      p->TscScale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
> +    p->TscOffset = trc->off;
>      smp_wmb();
> 
>      seq = p->TscSequence + 1;
> @@ -84,46 +88,6 @@ static void update_reference_tsc(struct domain *d, bool initialize)
>          seq = 1;
> 
>      p->TscSequence = seq;
> -    vd->reference_tsc_valid = true;
> -}
> -
> -static int64_t raw_trc_val(const struct domain *d)
> -{
> -    uint64_t tsc;
> -    struct time_scale tsc_to_ns;
> -
> -    tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
> -
> -    /* convert tsc to count of 100ns periods */
> -    set_time_scale(&tsc_to_ns, d->arch.tsc_khz * 1000ul);
> -    return scale_delta(tsc, &tsc_to_ns) / 100ul;
> -}
> -
> -static void time_ref_count_freeze(const struct domain *d)
> -{
> -    struct viridian_time_ref_count *trc =
> -        &d->arch.hvm.viridian->time_ref_count;
> -
> -    if ( test_and_clear_bit(_TRC_running, &trc->flags) )
> -        trc->val = raw_trc_val(d) + trc->off;
> -}
> -
> -static void time_ref_count_thaw(const struct domain *d)
> -{
> -    struct viridian_time_ref_count *trc =
> -        &d->arch.hvm.viridian->time_ref_count;
> -
> -    if ( !d->is_shutting_down &&
> -         !test_and_set_bit(_TRC_running, &trc->flags) )
> -        trc->off = (int64_t)trc->val - raw_trc_val(d);
> -}
> -
> -static int64_t time_ref_count(const struct domain *d)
> -{
> -    struct viridian_time_ref_count *trc =
> -        &d->arch.hvm.viridian->time_ref_count;
> -
> -    return raw_trc_val(d) + trc->off;
>  }
> 
>  /*
> @@ -136,7 +100,7 @@ static int64_t time_ref_count(const struct domain *d)
>   * 128 bit number which is then shifted 64 times to the right to obtain
>   * the high 64 bits."
>   */
> -static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
> +static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, int64_t offset)
>  {
>      uint64_t result;
> 
> @@ -153,22 +117,46 @@ static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
>      return result + offset;
>  }
> 
> -static uint64_t time_now(struct domain *d)
> +static uint64_t trc_val(const struct domain *d, int64_t offset)
>  {
>      uint64_t tsc, scale;
> 
> -    /*
> -     * If the reference TSC page is not enabled, or has been invalidated
> -     * fall back to the partition reference counter.
> -     */
> -    if ( !d->arch.hvm.viridian->reference_tsc_valid )
> -        return time_ref_count(d);
> -
> -    /* Otherwise compute reference time in the same way the guest would */
>      tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
>      scale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
> 
> -    return scale_tsc(tsc, scale, 0);
> +    return scale_tsc(tsc, scale, offset);
> +}
> +
> +static void time_ref_count_freeze(const struct domain *d)
> +{
> +    struct viridian_time_ref_count *trc =
> +        &d->arch.hvm.viridian->time_ref_count;
> +
> +    if ( test_and_clear_bit(_TRC_running, &trc->flags) )
> +        trc->val = trc_val(d, trc->off);
> +}
> +
> +static void time_ref_count_thaw(const struct domain *d)
> +{
> +    struct viridian_domain *vd = d->arch.hvm.viridian;
> +    struct viridian_time_ref_count *trc = &vd->time_ref_count;
> +
> +    if ( d->is_shutting_down ||
> +         test_and_set_bit(_TRC_running, &trc->flags) )
> +        return;
> +
> +    trc->off = (int64_t)trc->val - trc_val(d, 0);
> +
> +    if ( vd->reference_tsc.msr.enabled )
> +        update_reference_tsc(d, false);
> +}
> +
> +static int64_t time_ref_count(const struct domain *d)
> +{
> +    const struct viridian_time_ref_count *trc =
> +        &d->arch.hvm.viridian->time_ref_count;
> +
> +    return trc_val(d, trc->off);
>  }
> 
>  static void stop_stimer(struct viridian_stimer *vs)
> @@ -196,7 +184,7 @@ static void start_stimer(struct viridian_stimer *vs)
>      const struct vcpu *v = vs->v;
>      struct viridian_vcpu *vv = v->arch.hvm.viridian;
>      unsigned int stimerx = vs - &vv->stimer[0];
> -    int64_t now = time_now(v->domain);
> +    int64_t now = time_ref_count(v->domain);
>      int64_t expiration;
>      s_time_t timeout;
> 
> @@ -285,7 +273,7 @@ static void poll_stimer(struct vcpu *v, unsigned int stimerx)
> 
>      if ( !viridian_synic_deliver_timer_msg(v, vs->config.sintx,
>                                             stimerx, vs->expiration,
> -                                           time_now(v->domain)) )
> +                                           time_ref_count(v->domain)) )
>          return;
> 
>      clear_bit(stimerx, &vv->stimer_pending);
> @@ -641,10 +629,7 @@ void viridian_time_load_domain_ctxt(
>      vd->reference_tsc.msr.raw = ctxt->reference_tsc;
> 
>      if ( vd->reference_tsc.msr.enabled )
> -    {
>          viridian_map_guest_page(d, &vd->reference_tsc);
> -        update_reference_tsc(d, false);
> -    }
>  }
> 
>  /*
> diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
> index 54e46cc4c4..010c8b58d4 100644
> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -116,7 +116,6 @@ struct viridian_domain
>      union viridian_page_msr hypercall_gpa;
>      struct viridian_time_ref_count time_ref_count;
>      struct viridian_page reference_tsc;
> -    bool reference_tsc_valid;
>  };
> 
>  void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
> --
> 2.20.1.2.gb21ebb671

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Xen-devel] [PATCH v2] viridian: unify time sources
  2019-06-17  8:23 [Xen-devel] [PATCH v2] viridian: unify time sources Paul Durrant
  2019-06-20  8:44 ` Paul Durrant
@ 2019-06-21 13:49 ` Alexandru Stefan ISAILA
  2019-06-21 13:58   ` Paul Durrant
  2019-06-21 15:08 ` Jan Beulich
  2 siblings, 1 reply; 7+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-06-21 13:49 UTC (permalink / raw)
  To: Paul Durrant, xen-devel
  Cc: Andrew Cooper, Jan Beulich, Wei Liu, Roger Pau Monné

>   /*
> @@ -136,7 +100,7 @@ static int64_t time_ref_count(const struct domain *d)
>    * 128 bit number which is then shifted 64 times to the right to obtain
>    * the high 64 bits."
>    */

Is there a good reason for using signed offset here? If so then maybe 
you should change the return type or check for bounds.

> -static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
> +static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, int64_t offset)
>   {
>       uint64_t result;
>   
> @@ -153,22 +117,46 @@ static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
>       return result + offset;
>   }
>   
> -static uint64_t time_now(struct domain *d)
> +static uint64_t trc_val(const struct domain *d, int64_t offset)
>   {
>       uint64_t tsc, scale;
>   
> -    /*
> -     * If the reference TSC page is not enabled, or has been invalidated
> -     * fall back to the partition reference counter.
> -     */
> -    if ( !d->arch.hvm.viridian->reference_tsc_valid )
> -        return time_ref_count(d);
> -
> -    /* Otherwise compute reference time in the same way the guest would */
>       tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
>       scale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
>   
> -    return scale_tsc(tsc, scale, 0);
> +    return scale_tsc(tsc, scale, offset);
> +}

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Xen-devel] [PATCH v2] viridian: unify time sources
  2019-06-21 13:49 ` Alexandru Stefan ISAILA
@ 2019-06-21 13:58   ` Paul Durrant
  2019-06-21 15:20     ` Jan Beulich
  0 siblings, 1 reply; 7+ messages in thread
From: Paul Durrant @ 2019-06-21 13:58 UTC (permalink / raw)
  To: Alexandru Stefan ISAILA, xen-devel
  Cc: Andrew Cooper, Jan Beulich, Wei Liu, Roger Pau Monne

> -----Original Message-----
> From: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
> Sent: 21 June 2019 14:49
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Jan Beulich <jbeulich@suse.com>;
> Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v2] viridian: unify time sources
> 
> >   /*
> > @@ -136,7 +100,7 @@ static int64_t time_ref_count(const struct domain *d)
> >    * 128 bit number which is then shifted 64 times to the right to obtain
> >    * the high 64 bits."
> >    */
> 
> Is there a good reason for using signed offset here? If so then maybe
> you should change the return type or check for bounds.

The offset is actually negative most of the time but the resulting reference time should be unsigned so the return type of time_ref_count() does need fixing.

  Paul

>
> > -static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
> > +static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, int64_t offset)
> >   {
> >       uint64_t result;
> >
> > @@ -153,22 +117,46 @@ static uint64_t scale_tsc(uint64_t tsc, uint64_t scale, uint64_t offset)
> >       return result + offset;
> >   }
> >
> > -static uint64_t time_now(struct domain *d)
> > +static uint64_t trc_val(const struct domain *d, int64_t offset)
> >   {
> >       uint64_t tsc, scale;
> >
> > -    /*
> > -     * If the reference TSC page is not enabled, or has been invalidated
> > -     * fall back to the partition reference counter.
> > -     */
> > -    if ( !d->arch.hvm.viridian->reference_tsc_valid )
> > -        return time_ref_count(d);
> > -
> > -    /* Otherwise compute reference time in the same way the guest would */
> >       tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
> >       scale = ((10000ul << 32) / d->arch.tsc_khz) << 32;
> >
> > -    return scale_tsc(tsc, scale, 0);
> > +    return scale_tsc(tsc, scale, offset);
> > +}
> 
> Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Xen-devel] [PATCH v2] viridian: unify time sources
  2019-06-17  8:23 [Xen-devel] [PATCH v2] viridian: unify time sources Paul Durrant
  2019-06-20  8:44 ` Paul Durrant
  2019-06-21 13:49 ` Alexandru Stefan ISAILA
@ 2019-06-21 15:08 ` Jan Beulich
  2 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2019-06-21 15:08 UTC (permalink / raw)
  To: Paul Durrant; +Cc: Andrew Cooper, xen-devel, WeiLiu, Roger Pau Monne

>>> On 17.06.19 at 10:23, <paul.durrant@citrix.com> wrote:
> Currently, the time_ref_count enlightened time source maintains an offset
> such that time is frozen when the domain paused, but the reference_tsc
> enlightened time source does not. After migrate, the reference_tsc source
> may become invalidated (e.g. because of host cpu frequency mismatch) which
> will cause Windows to fall back to time_ref_count. Thus, the guest will
> observe a jump in time equivalent to the offset.
> 
> This patch unifies the two enlightened time sources such that the same
> offset applies to both of them. Also, it's not really necessary to have
> two different functions to calculating a 10MHz counter value, time_now() and
> raw_trc_val(), so this patch removes the latter implementation. The
> unification also allows removal of the reference_tsc_valid flag.
> 
> Whilst in the area, this patch also takes the opportunity to constify a few
> pointers which were missed in earlier patches.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

This looks okay to me now, but I don't feel qualified to give an R-b.
While not strictly applicable for a file maintained by you,
Acked-by: Jan Beulich <jbeulich@suse.com>
will have to do here.

Jan



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Xen-devel] [PATCH v2] viridian: unify time sources
  2019-06-21 13:58   ` Paul Durrant
@ 2019-06-21 15:20     ` Jan Beulich
  2019-06-21 15:21       ` Paul Durrant
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2019-06-21 15:20 UTC (permalink / raw)
  To: Paul Durrant; +Cc: aisaila, Andrew Cooper, xen-devel, WeiLiu, Roger Pau Monne

>>> On 21.06.19 at 15:58, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
>> Sent: 21 June 2019 14:49
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org 
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Jan Beulich 
> <jbeulich@suse.com>;
>> Roger Pau Monne <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v2] viridian: unify time sources
>> 
>> >   /*
>> > @@ -136,7 +100,7 @@ static int64_t time_ref_count(const struct domain *d)
>> >    * 128 bit number which is then shifted 64 times to the right to obtain
>> >    * the high 64 bits."
>> >    */
>> 
>> Is there a good reason for using signed offset here? If so then maybe
>> you should change the return type or check for bounds.
> 
> The offset is actually negative most of the time but the resulting reference 
> time should be unsigned so the return type of time_ref_count() does need 
> fixing.

Is switching it from int64_t to uint64_t all that's needed? I could
do this while committing (which I was about to).

Jan



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Xen-devel] [PATCH v2] viridian: unify time sources
  2019-06-21 15:20     ` Jan Beulich
@ 2019-06-21 15:21       ` Paul Durrant
  0 siblings, 0 replies; 7+ messages in thread
From: Paul Durrant @ 2019-06-21 15:21 UTC (permalink / raw)
  To: Jan Beulich; +Cc: aisaila, Andrew Cooper, xen-devel, WeiLiu, Roger Pau Monne

> -----Original Message-----
> From: Jan Beulich <JBeulich@suse.com>
> Sent: 21 June 2019 16:21
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: aisaila@bitdefender.com; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; xen-devel <xen-devel@lists.xenproject.org>; WeiLiu <wl@xen.org>
> Subject: RE: [Xen-devel] [PATCH v2] viridian: unify time sources
> 
> >>> On 21.06.19 at 15:58, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
> >> Sent: 21 June 2019 14:49
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org>; Jan Beulich
> > <jbeulich@suse.com>;
> >> Roger Pau Monne <roger.pau@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH v2] viridian: unify time sources
> >>
> >> >   /*
> >> > @@ -136,7 +100,7 @@ static int64_t time_ref_count(const struct domain *d)
> >> >    * 128 bit number which is then shifted 64 times to the right to obtain
> >> >    * the high 64 bits."
> >> >    */
> >>
> >> Is there a good reason for using signed offset here? If so then maybe
> >> you should change the return type or check for bounds.
> >
> > The offset is actually negative most of the time but the resulting reference
> > time should be unsigned so the return type of time_ref_count() does need
> > fixing.
> 
> Is switching it from int64_t to uint64_t all that's needed? I could
> do this while committing (which I was about to).

Yes, that's all that's needed and it would be nice to avoid sending a v3.

Thanks,

  Paul

> 
> Jan
> 


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17  8:23 [Xen-devel] [PATCH v2] viridian: unify time sources Paul Durrant
2019-06-20  8:44 ` Paul Durrant
2019-06-21 13:49 ` Alexandru Stefan ISAILA
2019-06-21 13:58   ` Paul Durrant
2019-06-21 15:20     ` Jan Beulich
2019-06-21 15:21       ` Paul Durrant
2019-06-21 15:08 ` Jan Beulich

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox