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.
> 
> > +                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.
> 
> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -540,6 +540,16 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_core_parking_t);
> >  #define XEN_RESOURCE_OP_MSR_READ  0
> >  #define XEN_RESOURCE_OP_MSR_WRITE 1
> >  
> > +/*
> > + * Specially handled MSRs:
> > + * - MSR_IA32_TSC
> > + * READ: Returns the scaled system time(ns) instead of raw timestamp. In
> > + *       multiple entry case, if other MSR read is followed by a 
> > MSR_IA32_TSC
> > + *       read, then both reads are guaranteed to be performed atomically 
> > (with
> > + *       IRQ disabled). The return time indicates the point of reading 
> > that MSR.
> > + * WRITE: Not supported.
> > + */
> 
> So before adding this I'd really like to once again understand what
> the consumer of this is going to use this for: The scaled system time
> normally isn't very useful to user mode code, hence whether we
> return ticks or nanoseconds doesn't seem to make a big difference -
> unless user mode code is expected to only ever look at the delta of
> two such values. In which case I'd consider obfuscating the real
> value by some artificial (and perhaps randomized at boot time) bias.
> 

Since Andrew has answered this (in another reply), I don't have to say much
here. Just one thing to make sure: Is obfuscating the return value still your
expectation?

Chao

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