[PATCH v1 Resend] target/i386: set the CPUID level to 0x14 on old machine-type

[PATCH v1 Resend] target/i386: set the CPUID level to 0x14 on old machine-type

[Luwei Kang]

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

Re: [PATCH v1 Resend] target/i386: set the CPUID level to 0x14 on old machine-type

[Eduardo Habkost]

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

RE: [PATCH v1 Resend] target/i386: set the CPUID level to 0x14 on old machine-type

[Kang, Luwei]

> > 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

Re: [PATCH v1 Resend] target/i386: set the CPUID level to 0x14 on old machine-type

[Eduardo Habkost]

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