[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH 1/2] i386/cpu: Add the Intel PT capabilities checking before
|
From: |
Kang, Luwei |
|
Subject: |
RE: [PATCH 1/2] i386/cpu: Add the Intel PT capabilities checking before extend the CPUID level |
|
Date: |
Wed, 2 Dec 2020 08:40:14 +0000 |
> -----Original Message-----
> From: Eduardo Habkost <ehabkost@redhat.com>
> Sent: Wednesday, December 2, 2020 5:12 AM
> To: Kang, Luwei <luwei.kang@intel.com>
> Cc: pbonzini@redhat.com; rth@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.kang@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