Re: [Xen-devel] [PATCH v2] viridian: unify time sources

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

Ping?

> -----Original Message-----
> From: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Sent: 17 June 2019 09:24
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monne 
> <roger.pau@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> Cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel