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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
> bounces@xxxxxxxxxxxxx] On Behalf Of Paul Durrant
> Sent: 04 August 2014 15:12
> To: Jan Beulich
> Cc: Ian Jackson; Keir (Xen.org); Stefano Stabellini; Ian Campbell; xen-
> devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 3/3] x86/viridian: Add partition time
> reference counter MSR support
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: 04 August 2014 15:01
> > To: Paul Durrant
> > Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@xxxxxxxxxxxxx;
> > 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@xxxxxxxxxx> 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.
> 

I think that hvm_get_guest_time() should have the right semantics after all, in 
that it appears to be ns since a vcpu was onlined and is preserved across 
save/restore.

  Paul

>   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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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