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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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