> -----Original Message----- > From: Eduardo Habkost <ehabk...@redhat.com> > Sent: Wednesday, December 2, 2020 5:12 AM > To: Kang, Luwei <luwei.k...@intel.com> > Cc: pbonz...@redhat.com; r...@twiddle.net; qemu-devel@nongnu.org > Subject: Re: [PATCH 1/2] i386/cpu: Add the Intel PT capabilities checking > before > extend the CPUID level > > 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?
Hi Eduardo, Let me clarify the issue. The cpuid level is 0xd if create a VM(cpu model qemu64) w/o intel pt feature on Snowridge HW. CMD: qemu-system-x86_64 -cpu qemu64 ... (Intel PT is disabled by default) The cpuid level will be extended to 0x14 if create a VM(cpu model qemu64) w/ intel pt feature on Snowridage. CMD: qemu-system-x86_64 -cpu qemu64,+intel-pt ... But the current software implementation will mask off intel pt if the host has LIP, and Snowridge support it. So cpuid level has been extended to 0x14(x86_cpu_expand_features) and Intel PT is disabled later(x86_cpu_filter_features), and the guest CPUID will include some zero items like this. 0x0000000e 0x00: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000 0x0000000f 0x00: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000 0x00000010 0x00: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000 0x00000011 0x00: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000 0x00000012 0x00: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000 0x00000013 0x00: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000 0x00000014 0x00: eax=0x00000000 ebx=0x00000000 ecx=0x00000000 edx=0x00000000 (Intel PT feature) x86_cpu_realizefn() |-> x86_cpu_expand_features() | IF has Intel PT feature | Then extended the cpuid level to 0x14; |-> x86_cpu_filter_features() IF has Intel PT feature & has LIP THEN mask off the guest Intel PT feature, *but* the cpuid level still 0x14. Expect result and how to: Don't impact the cpuid level if Intel PT can't be supported in the guest. So this patch moves the intel PT capabilities check before extending the cpuid level. Thanks, Luwei Kang > > > > > 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