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

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

On Mon, Jan 26, 2015 at 09:39:06AM +0000, Jan Beulich wrote:
> >>> On 26.01.15 at 03:41, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> > 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.
> 
> Oh, yes of course; sorry for the noise.
> 
> >> > +                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().
> 
> If you care about this then you also shouldn't rdtscll() when having
> received an error. I.e. all I really ask for here is consistency, not
> which specific behavior you select.
> 
Clear, thanks.
Chao

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