target/i386/cpu.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
The CPUID level need to be set to 0x14 manually on old
machine-type if Intel PT is enabled in guest. e.g. in Qemu 3.1
-machine pc-i440fx-3.1 -cpu qemu64,+intel-pt
will be CPUID[0].EAX(level)=7 and CPUID[7].EBX[25](intel-pt)=1.
Some Intel PT capabilities are exposed by leaf 0x14 and the
missing capabilities will cause some MSRs access failed.
This patch add a warning message to inform the user to extend
the CPUID level.
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
target/i386/cpu.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a624163..f67c479 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5440,8 +5440,12 @@ 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) &&
- kvm_enabled() && cpu->intel_pt_auto_level) {
- x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14);
+ kvm_enabled()) {
+ if (cpu->intel_pt_auto_level)
+ x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14);
+ else
+ warn_report("Intel PT need CPUID leaf 0x14, please set "
+ "by \"-cpu ...,+intel-pt,level=0x14\"");
}
/* CPU topology with multi-dies support requires CPUID[0x1F] */
--
1.8.3.1
On Wed, Oct 30, 2019 at 02:28:02PM +0800, Luwei Kang wrote: > The CPUID level need to be set to 0x14 manually on old > machine-type if Intel PT is enabled in guest. e.g. in Qemu 3.1 > -machine pc-i440fx-3.1 -cpu qemu64,+intel-pt > will be CPUID[0].EAX(level)=7 and CPUID[7].EBX[25](intel-pt)=1. > > Some Intel PT capabilities are exposed by leaf 0x14 and the > missing capabilities will cause some MSRs access failed. > This patch add a warning message to inform the user to extend > the CPUID level. Note that a warning is not an acceptable fix for a QEMU crash. We still need to fix the QEMU crash reported at: https://lore.kernel.org/qemu-devel/20191024141536.GU6744@habkost.net/ > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Luwei Kang <luwei.kang@intel.com> The subject line says "v1", but this patch is different from the v1 you sent earlier. If you are sending a different patch, please indicate it is a new version. Please also indicate what changed between different patch versions, to help review. > --- > target/i386/cpu.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index a624163..f67c479 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -5440,8 +5440,12 @@ 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) && > - kvm_enabled() && cpu->intel_pt_auto_level) { Not directly related to the warning: do you know why we have a kvm_enabled() check here? It seems unnecessary. We want CPUID level to be correct for all accelerators. > - x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > + kvm_enabled()) { > + if (cpu->intel_pt_auto_level) > + x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > + else > + warn_report("Intel PT need CPUID leaf 0x14, please set " > + "by \"-cpu ...,+intel-pt,level=0x14\""); The warning shouldn't be triggered if level is already >= 0x14. It is probably a good idea to mention that this happens only on pc-*-3.1 and older, as updating the machine-type is a better solution to the problem than manually setting the "level" property. This will print the warning multiple times if there are multiple VCPUs. You can use warn_report_once() to avoid that. > } > > /* CPU topology with multi-dies support requires CPUID[0x1F] */ > -- > 1.8.3.1 > -- Eduardo
> > The CPUID level need to be set to 0x14 manually on old machine-type if > > Intel PT is enabled in guest. e.g. in Qemu 3.1 -machine pc-i440fx-3.1 > > -cpu qemu64,+intel-pt will be CPUID[0].EAX(level)=7 and > > CPUID[7].EBX[25](intel-pt)=1. > > > > Some Intel PT capabilities are exposed by leaf 0x14 and the missing > > capabilities will cause some MSRs access failed. > > This patch add a warning message to inform the user to extend the > > CPUID level. > > Note that a warning is not an acceptable fix for a QEMU crash. > We still need to fix the QEMU crash reported at: > https://lore.kernel.org/qemu-devel/20191024141536.GU6744@habkost.net/ > > > > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > > The subject line says "v1", but this patch is different from the > v1 you sent earlier. > > If you are sending a different patch, please indicate it is a new version. Please also > indicate what changed between different patch versions, to help review. Got it. I fix a code style problem in resending patch (remove the '\n'). ERROR: Error messages should not contain newlines #36: FILE: target/i386/cpu.c:5448: + "by \"-cpu ...,+intel-pt,level=0x14\"\n"); total: 1 errors, 0 warnings, 14 lines checked > > > --- > > target/i386/cpu.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c index > > a624163..f67c479 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -5440,8 +5440,12 @@ 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) && > > - kvm_enabled() && cpu->intel_pt_auto_level) { > > Not directly related to the warning: do you know why we have a > kvm_enabled() check here? It seems unnecessary. We want CPUID level to be correct > for all accelerators. Intel PT virtualization enabling in KVM guest need some hardware enhancement and EPT must be enabled in KVM. I think it can't work for e.g. tcg pure simulation accelerator. > > > - x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > > + kvm_enabled()) { > > + if (cpu->intel_pt_auto_level) > > + x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > > + else > > + warn_report("Intel PT need CPUID leaf 0x14, please set " > > + "by \"-cpu ...,+intel-pt,level=0x14\""); > > The warning shouldn't be triggered if level is already >= 0x14. > > It is probably a good idea to mention that this happens only on > pc-*-3.1 and older, as updating the machine-type is a better solution to the problem > than manually setting the "level" > property. > > This will print the warning multiple times if there are multiple VCPUs. You can use > warn_report_once() to avoid that. Got it. Will fix. As you mentioned in this email " a warning is not an acceptable fix for a QEMU crash." We can't change the configuration of the old machine type because it may break the ABI compatibility. May I add more check on Intel PT, if CPUID[7].EBX[25] (intel-pt) = 1 and level is <0x14, mask off this feature? Or do you have any other suggestions? Thanks, Luwei Kang > > > } > > > > /* CPU topology with multi-dies support requires CPUID[0x1F] > > */ > > -- > > 1.8.3.1 > > > > -- > Eduardo
On Wed, Nov 06, 2019 at 12:55:32AM +0000, Kang, Luwei wrote: > > > The CPUID level need to be set to 0x14 manually on old machine-type if > > > Intel PT is enabled in guest. e.g. in Qemu 3.1 -machine pc-i440fx-3.1 > > > -cpu qemu64,+intel-pt will be CPUID[0].EAX(level)=7 and > > > CPUID[7].EBX[25](intel-pt)=1. > > > > > > Some Intel PT capabilities are exposed by leaf 0x14 and the missing > > > capabilities will cause some MSRs access failed. > > > This patch add a warning message to inform the user to extend the > > > CPUID level. > > > > Note that a warning is not an acceptable fix for a QEMU crash. > > We still need to fix the QEMU crash reported at: > > https://lore.kernel.org/qemu-devel/20191024141536.GU6744@habkost.net/ > > > > > > > > > > Suggested-by: Eduardo Habkost <ehabkost@redhat.com> > > > Signed-off-by: Luwei Kang <luwei.kang@intel.com> > > > > The subject line says "v1", but this patch is different from the > > v1 you sent earlier. > > > > If you are sending a different patch, please indicate it is a new version. Please also > > indicate what changed between different patch versions, to help review. > > Got it. I fix a code style problem in resending patch (remove the '\n'). > > ERROR: Error messages should not contain newlines > #36: FILE: target/i386/cpu.c:5448: > + "by \"-cpu ...,+intel-pt,level=0x14\"\n"); > total: 1 errors, 0 warnings, 14 lines checked > > > > > > --- > > > target/i386/cpu.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c index > > > a624163..f67c479 100644 > > > --- a/target/i386/cpu.c > > > +++ b/target/i386/cpu.c > > > @@ -5440,8 +5440,12 @@ 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) && > > > - kvm_enabled() && cpu->intel_pt_auto_level) { > > > > Not directly related to the warning: do you know why we have a > > kvm_enabled() check here? It seems unnecessary. We want CPUID level to be correct > > for all accelerators. > > Intel PT virtualization enabling in KVM guest need some hardware enhancement and > EPT must be enabled in KVM. I think it can't work for e.g. tcg pure simulation accelerator. I don't get it. If what you are saying is true, checking for kvm_enabled() here is completely unnecessary. If it is not, checking for kvm_enabled() here is incorrect. > > > > > > - x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > > > + kvm_enabled()) { > > > + if (cpu->intel_pt_auto_level) > > > + x86_cpu_adjust_level(cpu, &cpu->env.cpuid_min_level, 0x14); > > > + else > > > + warn_report("Intel PT need CPUID leaf 0x14, please set " > > > + "by \"-cpu ...,+intel-pt,level=0x14\""); > > > > The warning shouldn't be triggered if level is already >= 0x14. > > > > It is probably a good idea to mention that this happens only on > > pc-*-3.1 and older, as updating the machine-type is a better solution to the problem > > than manually setting the "level" > > property. > > > > This will print the warning multiple times if there are multiple VCPUs. You can use > > warn_report_once() to avoid that. > > Got it. Will fix. > > As you mentioned in this email " a warning is not an acceptable fix for a QEMU crash." > We can't change the configuration of the old machine type because it may break the > ABI compatibility. May I add more check on Intel PT, if CPUID[7].EBX[25] (intel-pt) = 1 > and level is <0x14, mask off this feature? Or do you have any other suggestions? Masking off the feature if level is < 0x14 would possibly work if we are 100% sure that existing kernel+QEMU versions crashed when (intel-pt=on && level < 0x14) so there's no ABI compatibility with working configurations to worry about. But it would be even better to make kvm_put_msrs() less fragile. Unexpected CPUID data shouldn't make the function crash. -- Eduardo
© 2016 - 2024 Red Hat, Inc.