Re: [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 04 August 2014 15:01
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@lists.xen.org;
> Keir (Xen.org)
> Subject: Re: [PATCH v2 3/3] x86/viridian: Add partition time reference
> counter MSR support
> 
> >>> On 04.08.14 at 15:12, <paul.durrant@citrix.com> wrote:
> > +static s_time_t get_time_ref_count(struct domain *d)
> > +{
> > +    s_time_t now = get_s_time() / 100;
> > +    struct viridian_time_ref_count *trc;
> > +
> > +    trc = &d->arch.hvm_domain.viridian.time_ref_count;
> > +
> > +    spin_lock(&trc->lock);
> 
> How well do you think this will scale, taking into consideration our
> recent observations with certain Windows versions having all its
> CPUs race for time stamps over apparently extended periods of
> time? It would seem to me that a cmpxchg() based approach
> might be preferable here.

Yes, that may well be true.

> 
> > +
> > +    if ( trc->last == 0 )
> > +        gdprintk(XENLOG_G_INFO, "TIME_REFERENCE_COUNTER
> accessed\n");
> 
> As per the comments on patch 2, this needs to be either printk() or
> the _G_ in the middle converted to just _.
> 

Ok.

> > +
> > +    if ( (now - trc->last) > 0 )
> > +        trc->last = now;
> > +    else
> > +        now = ++trc->last;
> 
> And how well is that going to work (and match Windows'
> expectations) when the migration destination host's NOW() is
> significantly smaller than the source host's?
> 

I misunderstood what get_s_time() is returning so I think you're correct that this will not work will in that case. It's basically the same as the PV rdtsc code; but maybe that won't work either.

  Paul

> > @@ -344,6 +383,14 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> >          *val = v->arch.hvm_vcpu.viridian.apic_assist.raw;
> >          break;
> >
> > +    case VIRIDIAN_MSR_TIME_REF_COUNT:
> > +        if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> > +            return 0;
> > +
> > +        perfc_incr(mshv_rdmsr_time_ref_count);
> > +        *val = (uint64_t)get_time_ref_count(d);
> 
> Pointless cast.
> 
> Jan