RE: [PATCH 1/2] i386/cpu: Add the Intel PT capabilities checking before extend the CPUID level
> -Original Message-
> From: Eduardo Habkost
> Sent: Wednesday, December 2, 2020 5:12 AM
> To: Kang, Luwei
> 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.
0x000e 0x00: eax=0x ebx=0x ecx=0x edx=0x
0x000f 0x00: eax=0x ebx=0x ecx=0x edx=0x
0x0010 0x00: eax=0x ebx=0x ecx=0x edx=0x
0x0011 0x00: eax=0x ebx=0x ecx=0x edx=0x
0x0012 0x00: eax=0x ebx=0x ecx=0x edx=0x
0x0013 0x00: eax=0x ebx=0x ecx=0x edx=0x
0x0014 0x00: eax=0x ebx=0x ecx=0x edx=0x
(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
> > ---
> > 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, >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)
> &&
>
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?
>
> Signed-off-by: Luwei Kang
> ---
> 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, >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, >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)
[PATCH 1/2] i386/cpu: Add the Intel PT capabilities checking before extend the CPUID level
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.
Signed-off-by: Luwei Kang
---
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, >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, >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
