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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant
> Sent: 04 August 2014 15:47
> To: Paul Durrant; 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: 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.
> 

Actually, I'm no longer convinced about that second part. It looks like only 
TSC is preserved across save/restore.

  Paul

>   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