On Thu, Sep 09, 2021 at 10:41:48PM +0800, 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 exposing on Intel SPR platform because SPR has > less supported bitmap CPUID(0x14,1):EBX[15:0] than ICX. > > This patch enables passing through host's PT capabilities for the case > "-cpu host/max" while ensure named CPU model still has the fixed > PT feature set to not break the live migration. > > It introduces @has_specific_intel_pt_feature_set field for name CPU > model. If a named CPU model has this field as false, it will use fixed > PT feature set of ICX. Besides same to previous behavior, if fixed PT > feature set of ICX cannot be satisfied/supported by host, it disables PT > instead of adjusting the feature set based on host's capabilities. > > 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...@intel.com> > --- > target/i386/cpu.c | 106 ++++++++++++++++++++-------------------------- > target/i386/cpu.h | 1 + > 2 files changed, 47 insertions(+), 60 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 58e98210f219..00c4ad23110d 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -543,34 +543,24 @@ static CPUCacheInfo legacy_l3_cache = { > #define L2_ITLB_4K_ASSOC 4 > #define L2_ITLB_4K_ENTRIES 512 > > -/* CPUID Leaf 0x14 constants: */ > -#define INTEL_PT_MAX_SUBLEAF 0x1 > -/* > - * bit[00]: IA32_RTIT_CTL.CR3 filter can be set to 1 and IA32_RTIT_CR3_MATCH > - * MSR can be accessed; > - * bit[01]: Support Configurable PSB and Cycle-Accurate Mode; > - * bit[02]: Support IP Filtering, TraceStop filtering, and preservation > - * of Intel PT MSRs across warm reset; > - * bit[03]: Support MTC timing packet and suppression of COFI-based packets; > - */ > -#define INTEL_PT_MINIMAL_EBX 0xf > -/* > - * bit[00]: Tracing can be enabled with IA32_RTIT_CTL.ToPA = 1 and > - * IA32_RTIT_OUTPUT_BASE and IA32_RTIT_OUTPUT_MASK_PTRS MSRs can be > - * accessed; > - * bit[01]: ToPA tables can hold any number of output entries, up to the > - * maximum allowed by the MaskOrTableOffset field of > - * IA32_RTIT_OUTPUT_MASK_PTRS; > - * bit[02]: Support Single-Range Output scheme; > - */ > -#define INTEL_PT_MINIMAL_ECX 0x7 > -/* generated packets which contain IP payloads have LIP values */ > -#define INTEL_PT_IP_LIP (1 << 31) > -#define INTEL_PT_ADDR_RANGES_NUM 0x2 /* Number of configurable address > ranges */ > -#define INTEL_PT_ADDR_RANGES_NUM_MASK 0x7 > -#define INTEL_PT_MTC_BITMAP (0x0249 << 16) /* Support ART(0,3,6,9) */ > -#define INTEL_PT_CYCLE_BITMAP 0x1fff /* Support 0,2^(0~11) */ > -#define INTEL_PT_PSB_BITMAP (0x003f << 16) /* Support > 2K,4K,8K,16K,32K,64K */ > +#define INTEL_PT_MAX_SUBLEAF 0x1 > +#define INTEL_PT_DEFAULT_ADDR_RANGES_NUM 0x2 > +#define INTEL_PT_ADDR_RANGES_NUM_MASK 0x7 > +/* Support ART(0,3,6,9) */ > +#define INTEL_PT_DEFAULT_MTC_BITMAP 0x0249 > +/* Support 0,2^(0~11) */ > +#define INTEL_PT_DEFAULT_CYCLE_BITMAP 0x1fff > +/* Support 2K,4K,8K,16K,32K,64K */ > +#define INTEL_PT_DEFAULT_PSB_BITMAP 0x003f > + > +#define INTEL_PT_DEFAULT_0_EBX (CPUID_14_0_EBX_CR3_FILTER | \ > + CPUID_14_0_EBX_PSB | CPUID_14_0_EBX_IP_FILTER | > CPUID_14_0_EBX_MTC) > +#define INTEL_PT_DEFAULT_0_ECX (CPUID_14_0_ECX_TOPA | \ > + CPUID_14_0_ECX_MULTI_ENTRIES | CPUID_14_0_ECX_SINGLE_RANGE) > +#define INTEL_PT_DEFAULT_1_EAX (INTEL_PT_DEFAULT_MTC_BITMAP << 16 | \ > + INTEL_PT_DEFAULT_ADDR_RANGES_NUM) > +#define INTEL_PT_DEFAULT_1_EBX (INTEL_PT_DEFAULT_PSB_BITMAP << 16 | \ > + INTEL_PT_DEFAULT_CYCLE_BITMAP)
I like these new macros because they make the code at x86_cpu_filter_features() clearer. Can we do this in a separate patch, to be applied before "Introduce FeatureWordInfo for Intel PT CPUID leaf 0xD"? > > void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, > uint32_t vendor2, uint32_t vendor3) > @@ -1517,6 +1507,7 @@ typedef struct X86CPUDefinition { > int family; > int model; > int stepping; > + bool has_specific_intel_pt_feature_set; > FeatureWordArray features; > const char *model_id; > const CPUCaches *const cache_info; > @@ -5061,6 +5052,14 @@ static void x86_cpu_load_model(X86CPU *cpu, > X86CPUModel *model) > env->features[w] = def->features[w]; > } > > + if (!def->has_specific_intel_pt_feature_set) { > + 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; > + } There's nothing special about intel-pt, and other features might benefit from having default values set if omitted from the CPU model definition. I don't know yet what's the best solution here, but some possibilities are: A) The simpler and more obvious solution: just set features[FEAT_14_*] explicitly to INTEL_PT_DEFAULT_* on all CPU models in builtin_x86_defs. B) Treat (env->features[FEAT_14_0_EBX] == 0 && env->features[FEAT_14_0_ECX] == 0 && env->features[FEAT_14_1_EAX] == 0 && env->features[FEAT_14_1_EBX] == 0) as a special case that indicates that INTEL_PT_DEFAULT_* should be used instead of 0. C) Rework X86CPUDefinition completely and replace FeatureWordArray features; with something like: struct { FeatureWord w; uint32_t value; } *features; or: struct { const char *property, *value; } *features; so we can have a concept of omitted/default flags in builtin_x86_defs. (A) and (C) are generic but require a lot more work, so we could keep your solution or just implement (B). In either case, I suggest you add a comment explaining why the boolean flag or special case exists (I believe the answer is: "because all CPU models have the same default value for INTEL_PT_* and we don't want to manually copy/paste it to all entries in builtin_x86_defs"). > + > /* legacy-cache defaults to 'off' if CPU model provides cache info */ > cpu->legacy_cache = !def->cache_info; > > @@ -5465,14 +5464,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, > uint32_t count, > > if (count == 0) { > *eax = INTEL_PT_MAX_SUBLEAF; > - *ebx = INTEL_PT_MINIMAL_EBX; > - *ecx = INTEL_PT_MINIMAL_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_MTC_BITMAP | INTEL_PT_ADDR_RANGES_NUM; > - *ebx = INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP; > + *eax = env->features[FEAT_14_1_EAX]; > + *ebx = env->features[FEAT_14_1_EBX]; > } > break; > } > @@ -6081,6 +6077,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() > @@ -6089,8 +6086,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); This doesn't look right. The value of host_feat set here is useless outside this for loop, and there's no reason to change the scope of this variable. > uint64_t requested_features = env->features[w]; > uint64_t unavailable_features; > if (w == FEAT_14_1_EAX) { > @@ -6107,32 +6103,22 @@ 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 (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) { > + host_feat = x86_cpu_get_supported_feature_word(FEAT_14_0_ECX, false); This doesn't look right. You seem to be using the same variable (host_feat) for two completely different purposes. > > - if (!eax_0 || > - ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) || > - ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) || > - ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) || > - ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) < > - INTEL_PT_ADDR_RANGES_NUM) || > - ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) != > - (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) || > - ((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->use_default_intel_pt && > + ((env->features[FEAT_14_0_EBX] != INTEL_PT_DEFAULT_0_EBX) || > + ((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); Why is use_default_intel_pt necessary? Why can't this function simply validate whatever is inside env->features[FEAT_14_*]? > } > + > + if ((env->features[FEAT_14_0_ECX] ^ host_feat) & CPUID_14_0_ECX_LIP) > { What if CPUID_7_0_EBX_INTEL_PT is not set? What should be the value of host_feat? > + 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 f5478a169f9a..e6236c371c4f 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1639,6 +1639,7 @@ typedef struct CPUX86State { > 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; > -- > 2.27.0 > -- Eduardo