Hi, Sorry for the long delay in reviewing this. Now that 5.2 is about to be released, we can try to merge this.
Comments below: On Wed, Oct 14, 2020 at 04:04:42PM +0800, Luwei Kang wrote: > The current implementation will extend the CPUID level to 0x14 if > Intel PT is enabled in the guest(in x86_cpu_expand_features()) and > the Intel PT will be disabled if it can't pass the capabilities > checking later(in x86_cpu_filter_features()). In this case, the > level of CPUID will be still 0x14 and the CPUID values from leaf > 0xe to 0x14 are all zero. > > This patch moves the capabilities checking before setting the > level of the CPUID. Why is this patch necessary and what problem does it fix? Is it a nice to have feature, or a bug fix? If you still want to change how the x86_cpu_adjust_level() code behaves, it should apply to all features filtered by x86_cpu_filter_features(), not just intel-pt, shouldn't it? > > Signed-off-by: Luwei Kang <luwei.k...@intel.com> > --- > target/i386/cpu.c | 63 ++++++++++++++++++++++++----------------------- > 1 file changed, 32 insertions(+), 31 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 9eafbe3690..24644abfd4 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6401,12 +6401,40 @@ static void x86_cpu_expand_features(X86CPU *cpu, > Error **errp) > > /* Intel Processor Trace requires CPUID[0x14] */ > if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT)) { > - if (cpu->intel_pt_auto_level) { > - x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > - } else if (cpu->env.cpuid_min_level < 0x14) { > + uint32_t eax_0, ebx_0, ecx_0, eax_1, ebx_1; > + > + eax_0 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 0, R_EAX); > + ebx_0 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 0, R_EBX); > + ecx_0 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 0, R_ECX); > + eax_1 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EAX); > + ebx_1 = kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EBX); > + > + 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 & INTEL_PT_IP_LIP)) { > + if (cpu->intel_pt_auto_level) { > + x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, > 0x14); > + } else if (cpu->env.cpuid_min_level < 0x14) { > + mark_unavailable_features(cpu, FEAT_7_0_EBX, > + CPUID_7_0_EBX_INTEL_PT, > + "Intel PT need CPUID leaf 0x14, please set by \"-cpu > ...,+intel-pt,min-level=0x14\""); > + } > + } else { > + /* > + * 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. > + */ > mark_unavailable_features(cpu, FEAT_7_0_EBX, > CPUID_7_0_EBX_INTEL_PT, > - "Intel PT need CPUID leaf 0x14, please set by \"-cpu > ...,+intel-pt,min-level=0x14\""); > + "host Intel PT features doesn't satisfy the guest > request."); > } > } > > @@ -6466,33 +6494,6 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool > verbose) > uint64_t unavailable_features = requested_features & ~host_feat; > 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_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 & INTEL_PT_IP_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. > - */ > - mark_unavailable_features(cpu, FEAT_7_0_EBX, > CPUID_7_0_EBX_INTEL_PT, prefix); > - } > - } > } > > static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > -- > 2.18.4 > -- Eduardo