On 12/8/2022 2:25 PM, Xiaoyao Li wrote:
> commit e37a5c7fa459 ("i386: Add Intel Processor Trace feature support")
> added the support of Intel PT by making CPUID[14] of PT as fixed feature
> set (from ICX) for any CPU model on any host. This truly breaks the PT
> exposure on Intel SPR platform because SPR has less supported bitmap of
> CPUID(0x14,1):EBX[15:0] than ICX.
>
> To fix the problem, enable pass through of host's PT capabilities for
> the cases "-cpu host/max" that it won't use default fixed PT feature set
> of ICX but expand automatically based on get_supported_cpuid reported by
> host. Meanwhile, it needs to ensure named CPU model still has the fixed
> PT feature set to not break the live migration case of
> "-cpu named_cpu_model,+intel-pt"
>
> Introduces env->use_default_intel_pt flag.
> - True means it's old CPU model that uses fixed PT feature set of ICX.
> - False means the named CPU model has its own PT feature set.
>
> Besides, to keep the same behavior for old CPU models that validate PT
> feature set against default fixed PT feature set of ICX in addition to
> validate from host's capabilities (via get_supported_cpuid) in
> x86_cpu_filter_features().
>
> In the future, new named CPU model, e.g., Sapphire Rapids, can define
> its own PT feature set by setting @has_specific_intel_pt_feature_set to
> true and defines it's own FEAT_14_0_EBX, FEAT_14_0_ECX, FEAT_14_1_EAX
> and FEAT_14_1_EBX.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> ---
> target/i386/cpu.c | 71 ++++++++++++++++++++++++++---------------------
> target/i386/cpu.h | 1 +
> 2 files changed, 40 insertions(+), 32 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e302cbbebfc5..24f3c7b06698 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5194,6 +5194,21 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
> env->features[w] = def->features[w];
> }
>
> + /*
> + * All (old) named CPU models have the same default values for INTEL_PT_*
> + *
> + * Assign the default value here since we don't want to manually copy/paste
> + * it to all entries in builtin_x86_defs.
> + */
> + if (!env->features[FEAT_14_0_EBX] && !env->features[FEAT_14_0_ECX] &&
> + !env->features[FEAT_14_1_EAX] && !env->features[FEAT_14_1_EBX]) {
> + env->use_default_intel_pt = true;
> + env->features[FEAT_14_0_EBX] = INTEL_PT_DEFAULT_0_EBX;
> + env->features[FEAT_14_0_ECX] = INTEL_PT_DEFAULT_0_ECX;
> + env->features[FEAT_14_1_EAX] = INTEL_PT_DEFAULT_1_EAX;
> + env->features[FEAT_14_1_EBX] = INTEL_PT_DEFAULT_1_EBX;
> + }
> +
> /* legacy-cache defaults to 'off' if CPU model provides cache info */
> cpu->legacy_cache = !def->cache_info;
>
> @@ -5716,14 +5731,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>
> if (count == 0) {
> *eax = INTEL_PT_MAX_SUBLEAF;
> - *ebx = INTEL_PT_DEFAULT_0_EBX;
> - *ecx = INTEL_PT_DEFAULT_0_ECX;
> - if (env->features[FEAT_14_0_ECX] & CPUID_14_0_ECX_LIP) {
> - *ecx |= CPUID_14_0_ECX_LIP;
> - }
> + *ebx = env->features[FEAT_14_0_EBX];
> + *ecx = env->features[FEAT_14_0_ECX];
> } else if (count == 1) {
> - *eax = INTEL_PT_DEFAULT_1_EAX;
> - *ebx = INTEL_PT_DEFAULT_1_EBX;
> + *eax = env->features[FEAT_14_1_EAX];
> + *ebx = env->features[FEAT_14_1_EBX];
> }
> break;
> }
> @@ -6425,6 +6437,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> CPUX86State *env = &cpu->env;
> FeatureWord w;
> const char *prefix = NULL;
> + uint64_t host_feat;
>
> if (verbose) {
> prefix = accel_uses_host_cpuid()
> @@ -6433,8 +6446,7 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> }
>
> for (w = 0; w < FEATURE_WORDS; w++) {
> - uint64_t host_feat =
> - x86_cpu_get_supported_feature_word(w, false);
> + host_feat = x86_cpu_get_supported_feature_word(w, false);
> uint64_t requested_features = env->features[w];
> uint64_t unavailable_features;
>
> @@ -6458,31 +6470,26 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
> mark_unavailable_features(cpu, w, unavailable_features, prefix);
> }
>
> - if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
> - kvm_enabled()) {
> - KVMState *s = CPU(cpu)->kvm_state;
> - uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
> - uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
> - uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
> - uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
> - uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
> -
> - if (!eax_0 ||
> - ((ebx_0 & INTEL_PT_DEFAULT_0_EBX) != INTEL_PT_DEFAULT_0_EBX) ||
> - ((ecx_0 & INTEL_PT_DEFAULT_0_ECX) != INTEL_PT_DEFAULT_0_ECX) ||
> - ((eax_1 & INTEL_PT_DEFAULT_MTC_BITMAP) != INTEL_PT_DEFAULT_MTC_BITMAP) ||
> - ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> - INTEL_PT_DEFAULT_ADDR_RANGES_NUM) ||
> - ((ebx_1 & INTEL_PT_DEFAULT_1_EBX) != INTEL_PT_DEFAULT_1_EBX) ||
> - ((ecx_0 & CPUID_14_0_ECX_LIP) !=
> - (env->features[FEAT_14_0_ECX] & CPUID_14_0_ECX_LIP))) {
> - /*
> - * Processor Trace capabilities aren't configurable, so if the
> - * host can't emulate the capabilities we report on
> - * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
> - */
> + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> + /*
> + * env->use_default_intel_pt is true means the CPU model doesn't have
> + * INTEL_PT_* specified. In this case, we need to check it has the
> + * value of default INTEL_PT to not break live migration
> + */
> + if (env->use_default_intel_pt &&
> + ((env->features[FEAT_14_0_EBX] != INTEL_PT_DEFAULT_0_EBX) ||
When will the env->use_default_intel_pt be true and env->features[FEAT_14_0_EBX]
!= INTEL_PT_DEFAULT_0_EBX? It seems they will always be equal if
env->use_default_intel_pt is true according to your code above.
> + ((env->features[FEAT_14_0_ECX] & ~CPUID_14_0_ECX_LIP) !=
> + INTEL_PT_DEFAULT_0_ECX) ||
> + (env->features[FEAT_14_1_EAX] != INTEL_PT_DEFAULT_1_EAX) ||
> + (env->features[FEAT_14_1_EBX] != INTEL_PT_DEFAULT_1_EBX))) {
> mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, prefix);
> }
> +
> + host_feat = x86_cpu_get_supported_feature_word(FEAT_14_0_ECX, false);
> + if ((env->features[FEAT_14_0_ECX] ^ host_feat) & CPUID_14_0_ECX_LIP) {
> + warn_report("Cannot configure different Intel PT IP payload format than hardware");
> + mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, NULL);
> + }
> }
> }
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 93fb5a87b40e..91a3971c1c29 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1784,6 +1784,7 @@ typedef struct CPUArchState {
> uint32_t cpuid_vendor2;
> uint32_t cpuid_vendor3;
> uint32_t cpuid_version;
> + bool use_default_intel_pt;
> FeatureWordArray features;
> /* Features that were explicitly enabled/disabled */
> FeatureWordArray user_features;
