[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for 4.5] x86/viridian: Freeze time reference counter when domain is paused



> -----Original Message-----
> From: Andrew Cooper
> Sent: 13 October 2014 11:14
> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [PATCH for 4.5] x86/viridian: Freeze time reference counter
> when domain is paused
> 
> On 10/10/14 10:07, Paul Durrant wrote:
> > In XenServer system test it has become apparent that versions of Windows
> > that make use of the time reference counter enlightenment cannot cope
> with
> > large jumps forward in the value read from the MSR. Specifically,
> > suspending a very large domain took approx. 45 minutes to complete and
> > when the domain was resumed it was discovered that the WMI (Windows
> > Management Instrumentation) service had hung.
> 
> It is possibly worth noting in the commit message that the OSes idea of
> the suspend time happens at the SHEDOP_suspend time, while the VMs real
> suspend time is after all the memory is complete, and the toolstack
> calls DOMCTL_gethvmcontext.
> 
> For a large VM (or slow network/storage), this is a delta of 45 minutes.
> 

Ok.

> >
> > This patch adds code to freeze the value of the time reference counter
> > on domain pause and 'thaw' it on domain unpause, but only if the domain is
> > not shutting down. The absolute value of the counter is then saved in the
> > viridian domain context record. This prevents the guest OS from
> experiencing
> > large jumps in the value of the MSR and has been shown to reliably fix
> > the problem with WMI.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Keir Fraser <keir@xxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/viridian.c            |   62 +++++++++++++++++++++++++--
> -----
> >  xen/common/domain.c                    |   41 +++++++++++++++------
> >  xen/include/asm-x86/hvm/hvm.h          |    9 ++++-
> >  xen/include/asm-x86/hvm/viridian.h     |   27 ++++++++++++++
> >  xen/include/public/arch-x86/hvm/save.h |    1 +
> >  5 files changed, 116 insertions(+), 24 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> > index 6726168..7d55363 100644
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -289,6 +289,39 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> >      return 1;
> >  }
> >
> > +static int64_t raw_trc_val(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;
> > +}
> > +
> > +void viridian_time_ref_count_freeze(struct domain *d)
> > +{
> > +    struct viridian_time_ref_count *trc;
> > +
> > +    trc = &d->arch.hvm_domain.viridian.time_ref_count;
> > +
> > +    if ( test_and_clear_bit(_TRC_running, &trc->flags) )
> > +        trc->val = raw_trc_val(d) + trc->off;
> > +}
> > +
> > +void viridian_time_ref_count_thaw(struct domain *d)
> > +{
> > +    struct viridian_time_ref_count *trc;
> > +
> > +    trc = &d->arch.hvm_domain.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);
> > +}
> > +
> >  int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> >  {
> >      struct vcpu *v = current;
> > @@ -348,18 +381,19 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t
> *val)
> >
> >      case VIRIDIAN_MSR_TIME_REF_COUNT:
> >      {
> > -        uint64_t tsc;
> > -        struct time_scale tsc_to_ns;
> > +        struct viridian_time_ref_count *trc;
> > +
> > +        trc = &d->arch.hvm_domain.viridian.time_ref_count;
> >
> >          if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> >              return 0;
> >
> > -        perfc_incr(mshv_rdmsr_time_ref_count);
> > -        tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
> > +        if ( !test_and_set_bit(_TRC_accessed, &trc->flags) )
> > +            printk(XENLOG_G_INFO "d%d: VIRIDIAN MSR_TIME_REF_COUNT:
> accessed\n",
> > +                   d->domain_id);
> >
> > -        /* convert tsc to count of 100ns periods */
> > -        set_time_scale(&tsc_to_ns, d->arch.tsc_khz * 1000ul);
> > -        *val = scale_delta(tsc, &tsc_to_ns) / 100ul;
> > +        perfc_incr(mshv_rdmsr_time_ref_count);
> > +        *val = raw_trc_val(d) + trc->off;
> >          break;
> >      }
> >
> > @@ -450,9 +484,10 @@ static int viridian_save_domain_ctxt(struct domain
> *d, hvm_domain_context_t *h)
> >      if ( !is_viridian_domain(d) )
> >          return 0;
> >
> > -    ctxt.hypercall_gpa = d->arch.hvm_domain.viridian.hypercall_gpa.raw;
> > -    ctxt.guest_os_id   = d->arch.hvm_domain.viridian.guest_os_id.raw;
> > -
> > +    ctxt.time_ref_count = d-
> >arch.hvm_domain.viridian.time_ref_count.val;
> > +    ctxt.hypercall_gpa  = d->arch.hvm_domain.viridian.hypercall_gpa.raw;
> > +    ctxt.guest_os_id    = d->arch.hvm_domain.viridian.guest_os_id.raw;
> > +
> >      return (hvm_save_entry(VIRIDIAN_DOMAIN, 0, h, &ctxt) != 0);
> >  }
> >
> > @@ -460,11 +495,12 @@ static int viridian_load_domain_ctxt(struct
> domain *d, hvm_domain_context_t *h)
> >  {
> >      struct hvm_viridian_domain_context ctxt;
> >
> > -    if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> > +    if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> >          return -EINVAL;
> >
> > -    d->arch.hvm_domain.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
> > -    d->arch.hvm_domain.viridian.guest_os_id.raw   = ctxt.guest_os_id;
> > +    d->arch.hvm_domain.viridian.time_ref_count.val =
> ctxt.time_ref_count;
> > +    d->arch.hvm_domain.viridian.hypercall_gpa.raw  = ctxt.hypercall_gpa;
> > +    d->arch.hvm_domain.viridian.guest_os_id.raw    = ctxt.guest_os_id;
> >
> >      return 0;
> >  }
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 190998c..af5dc82 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -43,6 +43,10 @@
> >  #include <xen/trace.h>
> >  #include <xen/tmem.h>
> >
> > +#ifdef CONFIG_X86
> > +#include <asm/hvm/viridian.h>
> > +#endif
> > +
> >  /* Linux config option: propageted to domain0 */
> >  /* xen_processor_pmbits: xen control Cx, Px, ... */
> >  unsigned int xen_processor_pmbits = XEN_PROCESSOR_PM_PX;
> > @@ -706,6 +710,11 @@ void domain_shutdown(struct domain *d, u8
> reason)
> >          v->paused_for_shutdown = 1;
> >      }
> >
> > +#ifdef CONFIG_X86
> > +    if ( has_viridian_time_ref_count(d) )
> > +        viridian_time_ref_count_freeze(d);
> > +#endif
> > +
> >      __domain_finalise_shutdown(d);
> >
> >      spin_unlock(&d->shutdown_lock);
> > @@ -925,32 +934,44 @@ int vcpu_unpause_by_systemcontroller(struct
> vcpu *v)
> >      return 0;
> >  }
> >
> > -void domain_pause(struct domain *d)
> > +void do_domain_pause(struct domain *d, bool_t nosync)
> >  {
> >      struct vcpu *v;
> >
> > -    ASSERT(d != current->domain);
> > -
> >      atomic_inc(&d->pause_count);
> >
> >      for_each_vcpu( d, v )
> > -        vcpu_sleep_sync(v);
> > +        if ( nosync )
> > +            vcpu_sleep_nosync(v);
> > +        else
> > +            vcpu_sleep_sync(v);
> > +
> > +#ifdef CONFIG_X86
> > +    if ( has_viridian_time_ref_count(d) )
> > +        viridian_time_ref_count_freeze(d);
> > +#endif
> >  }
> >
> > -void domain_pause_nosync(struct domain *d)
> > +void domain_pause(struct domain *d)
> >  {
> > -    struct vcpu *v;
> > -
> > -    atomic_inc(&d->pause_count);
> > +    ASSERT(d != current->domain);
> > +    do_domain_pause(d, 0);
> > +}
> >
> > -    for_each_vcpu( d, v )
> > -        vcpu_sleep_nosync(v);
> > +void domain_pause_nosync(struct domain *d)
> > +{
> > +    do_domain_pause(d, 1);
> >  }
> 
> All this fiddling with a boolean nosync parameter might be neater done
> in the same style as I did with __domain_pause_by_systemcontroller(), by
> passing a function pointer.
> 

Yeah - probably would be neater. I was in two minds about that.

> >
> >  void domain_unpause(struct domain *d)
> >  {
> >      struct vcpu *v;
> >
> > +#ifdef CONFIG_X86
> > +    if ( has_viridian_time_ref_count(d) )
> > +        viridian_time_ref_count_thaw(d);
> > +#endif
> > +
> >      if ( atomic_dec_and_test(&d->pause_count) )
> >          for_each_vcpu( d, v )
> >              vcpu_wake(v);
> > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> > index 0d94c48..2dda908 100644
> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -340,12 +340,19 @@ static inline unsigned long
> hvm_get_shadow_gs_base(struct vcpu *v)
> >      return hvm_funcs.get_shadow_gs_base(v);
> >  }
> >
> > +
> > +#define has_hvm_params(d) \
> > +    ((d)->arch.hvm_domain.params != NULL)
> > +
> >  #define viridian_feature_mask(d) \
> > -    ((d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
> > +    (has_hvm_params(d) && (d)-
> >arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
> 
> The result of this expression is a boolean, not the viridian feature mask.
> 

Dammit.

> Furthermore, I don't think that has_hvm_params() is useful.  Any HVM
> domain will have params, and without an HVM check, looking into
> d->arch.hvm_domain is invalid.
> 

Not true, which is why I made the change. domain_pause() is called on an HVM 
domain before the array is allocated.

  Paul

> ~Andrew
> 
> >
> >  #define is_viridian_domain(d) \
> >      (is_hvm_domain(d) && (viridian_feature_mask(d) &
> HVMPV_base_freq))
> >
> > +#define has_viridian_time_ref_count(d) \
> > +    (is_viridian_domain(d) && (viridian_feature_mask(d) &
> HVMPV_time_ref_count))
> > +
> >  void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
> >                                 uint32_t *eax, uint32_t *ebx,
> >                                 uint32_t *ecx, uint32_t *edx);
> > diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-
> x86/hvm/viridian.h
> > index 496da33..4cab2e8 100644
> > --- a/xen/include/asm-x86/hvm/viridian.h
> > +++ b/xen/include/asm-x86/hvm/viridian.h
> > @@ -48,10 +48,24 @@ union viridian_hypercall_gpa
> >      } fields;
> >  };
> >
> > +struct viridian_time_ref_count
> > +{
> > +    unsigned long flags;
> > +
> > +#define _TRC_accessed 0
> > +#define TRC_accessed (1 << _TRC_accessed)
> > +#define _TRC_running 1
> > +#define TRC_running (1 << _TRC_running)
> > +
> > +    uint64_t val;
> > +    int64_t off;
> > +};
> > +
> >  struct viridian_domain
> >  {
> >      union viridian_guest_os_id guest_os_id;
> >      union viridian_hypercall_gpa hypercall_gpa;
> > +    struct viridian_time_ref_count time_ref_count;
> >  };
> >
> >  int
> > @@ -75,4 +89,17 @@ rdmsr_viridian_regs(
> >  int
> >  viridian_hypercall(struct cpu_user_regs *regs);
> >
> > +void viridian_time_ref_count_freeze(struct domain *d);
> > +void viridian_time_ref_count_thaw(struct domain *d);
> > +
> >  #endif /* __ASM_X86_HVM_VIRIDIAN_H__ */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/include/public/arch-x86/hvm/save.h
> b/xen/include/public/arch-x86/hvm/save.h
> > index 16d85a3..88aab7e 100644
> > --- a/xen/include/public/arch-x86/hvm/save.h
> > +++ b/xen/include/public/arch-x86/hvm/save.h
> > @@ -568,6 +568,7 @@ struct hvm_hw_cpu_xsave {
> >  struct hvm_viridian_domain_context {
> >      uint64_t hypercall_gpa;
> >      uint64_t guest_os_id;
> > +    uint64_t time_ref_count;
> >  };
> >
> >  DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct
> hvm_viridian_domain_context);
> 


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.