Re: [Xen-devel] [PATCH] x86: Adjust rdtsc inline assembly

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.02.15 at 13:25, <andrew.cooper3@xxxxxxxxxx> wrote:
> The single use of the old rdtsc() in emulate_privileged_op() is altered to use
> the new rdtsc() and the rdmsr_writeback path to set eax/edx appropriately.

I'm not entirely sure about this one - the current code surely is slightly
faster than the replacement. Question is how much this matters. I'd
suggest to be on the safe side and simply open-code the asm() there.
If you decide to follow that, there are a few more cosmetic things which
otherwise I would adjust while committing:

> @@ -1426,13 +1426,13 @@ static void __init tsc_check_writability(void)
>      if ( boot_cpu_has(X86_FEATURE_TSC_RELIABLE) )
>          return;
>  
> -    rdtscll(tsc);
> +    tsc = rdtsc();
>      if ( wrmsr_safe(MSR_IA32_TSC, 0) == 0 )
>      {
>          uint64_t tmp, tmp2;
> -        rdtscll(tmp2);
> +        tmp2 = rdtsc();

Blank line between declaration and statements. And perhaps the
assignment could become the variable's initializer.

> @@ -1973,8 +1973,7 @@ void tsc_set_info(struct domain *d,
>          else {
>              /* when using native TSC, offset is nsec relative to power-on
>               * of physical machine */
> -            uint64_t tsc = 0;
> -            rdtscll(tsc);
> +            uint64_t tsc = rdtsc();
>              d->arch.vtsc_offset = scale_delta(tsc,&d->arch.vtsc_to_ns) -

Blank line missing again.

> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -71,17 +71,14 @@ static inline int wrmsr_safe(unsigned int msr, uint64_t 
> val)
>      return _rc;
>  }
>  
> -#define rdtsc(low,high) \
> -     __asm__ __volatile__("rdtsc" : "=a" (low), "=d" (high))
> +static inline uint64_t rdtsc(void)
> +{
> +    uint32_t low, high;
>  
> -#define rdtscl(low) \
> -     __asm__ __volatile__("rdtsc" : "=a" (low) : : "edx")
> +    __asm__ __volatile__("rdtsc" : "=a" (low), "=d" (high));
>  
> -#define rdtscll(val) do { \
> -     unsigned int _eax, _edx; \
> -     asm volatile("rdtsc" : "=a" (_eax), "=d" (_edx)); \
> -     (val) = ((unsigned long)_eax) | (((unsigned long)_edx)<<32); \
> -} while(0)
> +    return (uint64_t)high << 32 | low;

Parentheses around the << please.

Jan


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