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
