Re: constant_tsc support for SVM guest

[Wei Huang <wei.huang2@amd.com>]

On 4/23/21 4:27 PM, Eduardo Habkost wrote:
> On Fri, Apr 23, 2021 at 12:32:00AM -0500, Wei Huang wrote:
>> There was a customer request for const_tsc support on AMD guests. Right now
>> this feature is turned off by default for QEMU x86 CPU types (in
>> CPUID_Fn80000007_EDX[8]). However we are seeing a discrepancy in guest VM
>> behavior between Intel and AMD.
>>
>> In Linux kernel, Intel x86 code enables X86_FEATURE_CONSTANT_TSC based on
>> vCPU's family & model. So it ignores CPUID_Fn80000007_EDX[8] and guest VMs
>> have const_tsc enabled. On AMD, however, the kernel checks
>> CPUID_Fn80000007_EDX[8]. So const_tsc is disabled on AMD by default.
> 
> Oh.  This seems to defeat the purpose of the invtsc migration
> blocker we have.
> 
> Do we know when this behavior was introduced in Linux?

This code has existed in the kernel for a long time:

   commit 2b16a2353814a513cdb5c5c739b76a19d7ea39ce
   Author: Andi Kleen <ak@linux.intel.com>
   Date:   Wed Jan 30 13:32:40 2008 +0100

      x86: move X86_FEATURE_CONSTANT_TSC into early cpu feature detection

There was another related commit which might explain the reasoning of 
turning on CONSTANT_TSC based on CPU family on Intel:

   commit 40fb17152c50a69dc304dd632131c2f41281ce44
   Author: Venki Pallipadi <venkatesh.pallipadi@intel.com>
   Date:   Mon Nov 17 16:11:37 2008 -0800

      x86: support always running TSC on Intel CPUs

According to the commit above, there are two kernel features: 
CONSTANT_TSC and NONSTOP_TSC:

   * CONSTANT_TSC: TSC runs at constant rate
   * NONSTOP_TSC: TSC not stop in deep C-states

If CPUID_Fn80000007_EDX[8] == 1, both CONSTANT_TSC and NONSTOP_TSC are 
turned on. This applies to all x86 vendors. For Intel CPU with certain 
CPU families (i.e. c->x86 == 0x6 && c->x86_model >= 0x0e), it will turn 
on CONSTANT_TSC (but no NONSTOP_TSC) with CPUID_Fn80000007_EDX[8]=0.

I believe the migration blocker was created for the CONSTANT_TSC case: 
if vCPU claims to have a constant TSC rate, we have to make sure 
src/dest are matched with each other (having the same CPU frequency or 
have tsc_ratio). NONSTOP_TSC doesn't matter in this scope.

> 
>>
>> I am thinking turning on invtsc for EPYC CPU types (see example below). Most
>> AMD server CPUs have supported invariant TSC for a long time. So this change
>> is compatible with the hardware behavior. The only problem is live migration
>> support, which will be blocked because of invtsc. However this problem
>> should be considered very minor because most server CPUs support TscRateMsr
>> (see CPUID_Fn8000000A_EDX[4]), allowing VMs to migrate among CPUs with
>> different TSC rates. This live migration restriction can be lifted as long
>> as the destination supports TscRateMsr or has the same frequency as the
>> source (QEMU/libvirt do it).
>>
>> [BTW I believe this migration limitation might be unnecessary because it is
>> apparently OK for Intel guests to ignore invtsc while claiming const_tsc.
>> Have anyone reported issues?]
> 
> CCing Marcelo, who originally added the migration blocker in QEMU.
> 
>>
>> Do I miss anything here? Any comments about the proposal?
>>
>> Thanks,
>> -Wei
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index ad99cad0e7..3c48266884 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -4077,6 +4076,21 @@ static X86CPUDefinition builtin_x86_defs[] = {
>>                       { /* end of list */ }
>>                   }
>>               },
>> +            {
>> +                .version = 4,
>> +                .alias = "EPYC-IBPB",
>> +                .props = (PropValue[]) {
>> +                    { "ibpb", "on" },
>> +                    { "perfctr-core", "on" },
>> +                    { "clzero", "on" },
>> +                    { "xsaveerptr", "on" },
>> +                    { "xsaves", "on" },
> 
> You don't need to copy the properties from the previous version.
> The properties of version N are applied on top of the properties
> of version (N-1).

Will fix

> 
>> +                    { "invtsc", "on" },
>> +                    { "model-id",
>> +                      "AMD EPYC Processor" },
>> +                    { /* end of list */ }
>> +                }
>> +            },
>>               { /* end of list */ }
>>           }
>>       },
>> @@ -4189,6 +4203,15 @@ static X86CPUDefinition builtin_x86_defs[] = {
>>                       { /* end of list */ }
>>                   }
>>               },
>> +            {
>> +                .version = 3,
>> +                .props = (PropValue[]) {
>> +                    { "ibrs", "on" },
>> +                    { "amd-ssbd", "on" },
>> +                    { "invtsc", "on" },
>> +                    { /* end of list */ }
>> +                }
>> +            },
>>               { /* end of list */ }
>>           }
>>       },
>> @@ -4246,6 +4269,17 @@ static X86CPUDefinition builtin_x86_defs[] = {
>>           .xlevel = 0x8000001E,
>>           .model_id = "AMD EPYC-Milan Processor",
>>           .cache_info = &epyc_milan_cache_info,
>> +        .versions = (X86CPUVersionDefinition[]) {
>> +            { .version = 1 },
>> +            {
>> +                .version = 2,
>> +                .props = (PropValue[]) {
>> +                    { "invtsc", "on" },
>> +                    { /* end of list */ }
>> +                }
>> +            },
>> +            { /* end of list */ }
>> +        }
>>
>