Re: [Xen-devel] [PATCH v6 1/5] x86: allow reading MSR_IA32_TSC with XENPF_resource_op

[Chao Peng <chao.p.peng@xxxxxxxxxxxxxxx>]

On Fri, Jan 23, 2015 at 02:28:04PM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 14:40, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> > @@ -133,10 +135,39 @@ static void resource_access(void *info)
> >          switch ( entry->u.cmd )
> >          {
> >          case XEN_RESOURCE_OP_MSR_READ:
> > -            ret = rdmsr_safe(entry->idx, entry->val);
> > +            if ( unlikely(entry->idx == MSR_IA32_TSC) ) {
> > +                /* Return scaled time instead of raw timestamp */
> > +                entry->val = get_s_time_fixed(tsc);
> 
> This is going to be bogus when happening on the first entry.
> Either disallow it, or rdtscll() here if tsc == 0.

No, get_s_time_fixed() will take care of this. It calls rdtscll() when
tsc == 0. This is the way how NOW() works.

> 
> > +                ret = 0;
> > +            }
> > +            else
> > +            {
> > +                unsigned long irqflags;
> > +                /*
> > +                 * If next entry is MSR_IA32_TSC read, then the actual 
> > rdtscll
> > +                 * is performed together with current entry, with IRQ 
> > disabled.
> > +                 */
> > +                bool_t read_tsc = (i < ra->nr_done - 1 &&
> > +                                   unlikely(entry[1].idx == MSR_IA32_TSC &&
> > +                                   entry[1].u.cmd == 
> > XEN_RESOURCE_OP_MSR_READ));
> 
> Just like you do the rdtscll() without regard to rc (which is fine),
> I don't think you need that last part of the condition.

How about if the next entry is MSR_IA32_TSC write?  I don’t want to
introduce unnecessary IRQ locking and a useless rdtscll().

Chao

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