Re: constant_tsc support for SVM guest

[Babu Moger]

Hi Wei,
I dont know the background of this feature. I will let some else to
comment on that.

The patch exposes the feature TscInvariant to the guest successfully.
Tested it on my AMD box. I have few comments on your patch below.


On 4/23/21 12:32 AM, 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.
> 
> 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?]
> 
> 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",

We dont need this alias. I think this is there only for legacy purposes.

> +                .props = (PropValue[]) {
> +                    { "ibpb", "on" },
> +                    { "perfctr-core", "on" },
> +                    { "clzero", "on" },
> +                    { "xsaveerptr", "on" },
> +                    { "xsaves", "on" },

You dont need to list any of these features again. All the old features
will be added implicitly.

> +                    { "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" },

Same as above. Dont need these previous features.

> +                    { "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 */ }
> +        }

Here is the updated patch..
=========================================================================

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ad99cad0e7..27287c1343 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4077,6 +4077,13 @@ static X86CPUDefinition builtin_x86_defs[] = {
                     { /* end of list */ }
                 }
             },
+            {
+              .version = 4,
+              .props = (PropValue[]) {
+                    { "invtsc", "on" },
+                    { /* end of list */ }
+                }
+            },
             { /* end of list */ }
         }
     },
@@ -4189,6 +4196,14 @@ static X86CPUDefinition builtin_x86_defs[] = {
                     { /* end of list */ }
                 }
             },
+            {
+              .version = 3,
+              .props = (PropValue[]) {
+                    { "invtsc", "on" },
+                    { /* end of list */ }
+                }
+            },
+
             { /* end of list */ }
         }
     },
@@ -4246,6 +4261,18 @@ 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 */ }
+        }
     },
 };



thanks