Re: [RFC PATCH 4/4] x86/TSC: Use RDTSCP

[Lendacky, Thomas]

On 12/11/2018 04:59 PM, Andy Lutomirski wrote:
> On Tue, Dec 11, 2018 at 2:23 PM Borislav Petkov <bp@xxxxxxxxx> wrote:
>>
>> From: Borislav Petkov <bp@xxxxxxx>
>>
>> Currently, the kernel uses
>>
>>   [LM]FENCE; RDTSC
>>
>> in the timekeeping code, to guarantee monotonicity of time where the
>> *FENCE is selected based on vendor.
>>
>> Replace that sequence with RDTSCP which is faster or on-par and gives
>> the same guarantees.
>>
>> A microbenchmark on Intel shows that the change is on-par.
>>
>> On AMD, the change is either on-par with the current LFENCE-prefixed
>> RDTSC and some are slightly better with RDTSCP.
>>
>> The comparison is done with the LFENCE-prefixed RDTSC (and not with the
>> MFENCE-prefixed one, as one would normally expect) because all modern
>> AMD families make LFENCE serializing and thus avoid the heavy MFENCE by
>> effectively enabling X86_FEATURE_LFENCE_RDTSC.
>>
>> Co-developed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
>> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
>> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>> Cc: John Stultz <john.stultz@xxxxxxxxxx>
>> Cc: x86@xxxxxxxxxx
>> Link: https://lkml.kernel.org/r/20181119184556.11479-1-bp@xxxxxxxxx
>> ---
>>  arch/x86/include/asm/msr.h | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
>> index 91e4cf189914..5cc3930cb465 100644
>> --- a/arch/x86/include/asm/msr.h
>> +++ b/arch/x86/include/asm/msr.h
>> @@ -217,6 +217,8 @@ static __always_inline unsigned long long rdtsc(void)
>>   */
>>  static __always_inline unsigned long long rdtsc_ordered(void)
>>  {
>> +       DECLARE_ARGS(val, low, high);
>> +
>>         /*
>>          * The RDTSC instruction is not ordered relative to memory
>>          * access.  The Intel SDM and the AMD APM are both vague on this
>> @@ -227,9 +229,19 @@ static __always_inline unsigned long long rdtsc_ordered(void)
>>          * ordering guarantees as reading from a global memory location
>>          * that some other imaginary CPU is updating continuously with a
>>          * time stamp.
>> +        *
>> +        * Thus, use the preferred barrier on the respective CPU, aiming for
>> +        * RDTSCP as the default.
>>          */
>> -       barrier_nospec();
>> -       return rdtsc();
>> +       asm volatile(ALTERNATIVE_3("rdtsc",
>> +                                  "mfence; rdtsc", X86_FEATURE_MFENCE_RDTSC,
>> +                                  "lfence; rdtsc", X86_FEATURE_LFENCE_RDTSC,
>> +                                  "rdtscp", X86_FEATURE_RDTSCP)
>> +                       : EAX_EDX_RET(val, low, high)
>> +                       /* RDTSCP clobbers ECX with MSR_TSC_AUX. */
>> +                       :: "ecx");
>> +
>> +       return EAX_EDX_VAL(val, low, high);
>>  }
> 
> This whole series seems reasonable, except that it caused me to look
> at barrier_nospec().  And I had a bit of a WTF moment, as in "WTF does
> RDTSC have to do with a speculation protection barrier".  Does it
> actually make sense?

It does seem overloaded in that sense, but the feature means that LFENCE
is serializing and so can be used in rdtsc_ordered. In the same sense,
barrier_nospec is looking for whether LFENCE is serializing and preferring
that over MFENCE since it is lighter weight.

In light of how they're being used now, they could probably stand to be
renamed in some way.

Thanks,
Tom

>