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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan

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