diff mbox series

[1/1] x86: add CPU flags supported inside libvirt

Message ID 20190718134537.22356-1-den@openvz.org (mailing list archive)
State New, archived
Headers show
Series [1/1] x86: add CPU flags supported inside libvirt | expand

Commit Message

Denis V. Lunev July 18, 2019, 1:45 p.m. UTC 
There are the following flags available in libvirt inside cpu_map.xm
    <feature name='cvt16'>
      <cpuid function='0x80000001' ecx='0x00040000'/>
    </feature>
    <feature name='cmt'> <!-- cqm -->
      <cpuid eax_in='0x07' ecx_in='0x00' ebx='0x00001000'/>
    </feature>
We have faced the problem that QEMU does not start once these flags are
present in the domain.xml.

This patch just adds proper names into the map.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <rth@twiddle.net>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
CC: Peter Krempa <pkrempa@redhat.com>
CC: Daniel P. Berrangé <berrange@redhat.com>
---
 target/i386/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini July 18, 2019, 1:52 p.m. UTC | #1 
On 18/07/19 15:45, Denis V. Lunev wrote:
> There are the following flags available in libvirt inside cpu_map.xm
>     <feature name='cvt16'>
>       <cpuid function='0x80000001' ecx='0x00040000'/>
>     </feature>
>     <feature name='cmt'> <!-- cqm -->
>       <cpuid eax_in='0x07' ecx_in='0x00' ebx='0x00001000'/>
>     </feature>
> We have faced the problem that QEMU does not start once these flags are
> present in the domain.xml.
> 
> This patch just adds proper names into the map.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> CC: Peter Krempa <pkrempa@redhat.com>
> CC: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  target/i386/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 805ce95247..88ba4dad47 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -870,7 +870,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              "lahf-lm", "cmp-legacy", "svm", "extapic",
>              "cr8legacy", "abm", "sse4a", "misalignsse",
>              "3dnowprefetch", "osvw", "ibs", "xop",
> -            "skinit", "wdt", NULL, "lwp",
> +            "skinit", "wdt", "cvt16", "lwp",
>              "fma4", "tce", NULL, "nodeid-msr",
>              NULL, "tbm", "topoext", "perfctr-core",
>              "perfctr-nb", NULL, NULL, NULL,
> @@ -1044,7 +1044,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              "fsgsbase", "tsc-adjust", NULL, "bmi1",
>              "hle", "avx2", NULL, "smep",
>              "bmi2", "erms", "invpcid", "rtm",
> -            NULL, NULL, "mpx", NULL,
> +            "cmt", NULL, "mpx", NULL,
>              "avx512f", "avx512dq", "rdseed", "adx",
>              "smap", "avx512ifma", "pcommit", "clflushopt",
>              "clwb", "intel-pt", "avx512pf", "avx512er",
> 

Oops, nice catch!  I've queued the patch for 4.1.

Paolo
Denis V. Lunev July 19, 2019, 1 p.m. UTC | #2 
On 7/18/19 4:52 PM, Paolo Bonzini wrote:
> On 18/07/19 15:45, Denis V. Lunev wrote:
>> There are the following flags available in libvirt inside cpu_map.xm
>>     <feature name='cvt16'>
>>       <cpuid function='0x80000001' ecx='0x00040000'/>
>>     </feature>
>>     <feature name='cmt'> <!-- cqm -->
>>       <cpuid eax_in='0x07' ecx_in='0x00' ebx='0x00001000'/>
>>     </feature>
>> We have faced the problem that QEMU does not start once these flags are
>> present in the domain.xml.
>>
>> This patch just adds proper names into the map.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Richard Henderson <rth@twiddle.net>
>> CC: Eduardo Habkost <ehabkost@redhat.com>
>> CC: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> CC: Peter Krempa <pkrempa@redhat.com>
>> CC: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>  target/i386/cpu.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 805ce95247..88ba4dad47 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -870,7 +870,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>              "lahf-lm", "cmp-legacy", "svm", "extapic",
>>              "cr8legacy", "abm", "sse4a", "misalignsse",
>>              "3dnowprefetch", "osvw", "ibs", "xop",
>> -            "skinit", "wdt", NULL, "lwp",
>> +            "skinit", "wdt", "cvt16", "lwp",
>>              "fma4", "tce", NULL, "nodeid-msr",
>>              NULL, "tbm", "topoext", "perfctr-core",
>>              "perfctr-nb", NULL, NULL, NULL,
>> @@ -1044,7 +1044,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>>              "fsgsbase", "tsc-adjust", NULL, "bmi1",
>>              "hle", "avx2", NULL, "smep",
>>              "bmi2", "erms", "invpcid", "rtm",
>> -            NULL, NULL, "mpx", NULL,
>> +            "cmt", NULL, "mpx", NULL,
>>              "avx512f", "avx512dq", "rdseed", "adx",
>>              "smap", "avx512ifma", "pcommit", "clflushopt",
>>              "clwb", "intel-pt", "avx512pf", "avx512er",
>>
> Oops, nice catch!  I've queued the patch for 4.1.
>
> Paolo
I have written small script to find differences between
CPU features in QEMU and libvirt.

#!/bin/bash
LIST=`awk "/name/{split(\\\$2, arr, \"'\"); print arr[2]}" x86_features.xml`
for feat in $LIST; do
    var=`grep \"$feat\" target/i386/cpu.c`
    if [ -z "$var" ]; then
        echo $feat
    fi
done

There are the following list of features present in libvirt
and missed in QEMU:

osxsave - removed in qemu in f1a23522b03
ospke - removed in qemu in 9ccb9784b57
pconfig - removed in qemu in 712f807e196.
mbm_total
mbm_local

Two last features are described as follows:
  <!-- cpuid level 0x0000000f:1 (edx) -->
  <feature name='mbm_total'>
    <cpuid eax_in='0x0f' ecx_in='0x01' edx='0x00000002'/>
  </feature>
  <feature name='mbm_local'>
    <cpuid eax_in='0x0f' ecx_in='0x01' edx='0x00000004'/>
  </feature>
This leaf is not supported in QEMU at all.

According to Intel 64 and IA-32 Architecture Developer manual
vol. 2a these bits are used for
EDX Bit 00: Supports L3 occupancy monitoring if 1.
Bit 01: Supports L3 Total Bandwidth monitoring if 1.
Bit 02: Supports L3 Local Bandwidth monitoring if 1.
Bits 31 - 03: Reserved.

Thus technically these 5 bits are able to produce problems
for QEMU if they will be found in domain.xml

Den
Eduardo Habkost July 19, 2019, 8:53 p.m. UTC | #3 
On Thu, Jul 18, 2019 at 04:45:37PM +0300, Denis V. Lunev wrote:
> There are the following flags available in libvirt inside cpu_map.xm
>     <feature name='cvt16'>
>       <cpuid function='0x80000001' ecx='0x00040000'/>

This is bit 18...

>     </feature>
>     <feature name='cmt'> <!-- cqm -->
>       <cpuid eax_in='0x07' ecx_in='0x00' ebx='0x00001000'/>
>     </feature>
> We have faced the problem that QEMU does not start once these flags are
> present in the domain.xml.
> 
> This patch just adds proper names into the map.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Richard Henderson <rth@twiddle.net>
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> CC: Peter Krempa <pkrempa@redhat.com>
> CC: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  target/i386/cpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 805ce95247..88ba4dad47 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -870,7 +870,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              "lahf-lm", "cmp-legacy", "svm", "extapic",
>              "cr8legacy", "abm", "sse4a", "misalignsse",
>              "3dnowprefetch", "osvw", "ibs", "xop",
> -            "skinit", "wdt", NULL, "lwp",
> +            "skinit", "wdt", "cvt16", "lwp",

...this is bit 14.

Anyway, the cvt16 bit was removed on purpose, and was never
supported.  See:
http://mid.mail-archive.com/508091FB.1030705@amd.com

>              "fma4", "tce", NULL, "nodeid-msr",
>              NULL, "tbm", "topoext", "perfctr-core",
>              "perfctr-nb", NULL, NULL, NULL,
> @@ -1044,7 +1044,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              "fsgsbase", "tsc-adjust", NULL, "bmi1",
>              "hle", "avx2", NULL, "smep",
>              "bmi2", "erms", "invpcid", "rtm",
> -            NULL, NULL, "mpx", NULL,
> +            "cmt", NULL, "mpx", NULL,

This is one is named "cqm" on Linux (X86_FEATURE_CQM).  I prefer
to keep consistency with the name already in use by Linux than
the one in libvirt that was never used.

You can still add a "cmt" alias property if you think it would be
useful.

Also, I see no code implementing migration of MSR_IA32_QM_EVTSEL,
MSR_IA32_QM_CTR, or other RDT-M state.  If the feature is not
safe for migration yet, you need to explicitly add the feature to
.unmigratable_flags.


>              "avx512f", "avx512dq", "rdseed", "adx",
>              "smap", "avx512ifma", "pcommit", "clflushopt",
>              "clwb", "intel-pt", "avx512pf", "avx512er",
> -- 
> 2.17.1
>
Paolo Bonzini July 19, 2019, 9:44 p.m. UTC | #4 
On 19/07/19 22:53, Eduardo Habkost wrote:
> This is one is named "cqm" on Linux (X86_FEATURE_CQM).  I prefer
> to keep consistency with the name already in use by Linux than
> the one in libvirt that was never used.
> 
> You can still add a "cmt" alias property if you think it would be
> useful.

Actually KVM does not mark it as supported:

        const u32 kvm_cpuid_7_0_ebx_x86_features =
                F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
                F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | f_mpx | F(RDSEED) |
                F(ADX) | F(SMAP) | F(AVX512IFMA) | F(AVX512F) | F(AVX512PF) |
                F(AVX512ER) | F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(AVX512DQ) |
                F(SHA_NI) | F(AVX512BW) | F(AVX512VL) | f_intel_pt;

Paolo
Eduardo Habkost July 19, 2019, 10:05 p.m. UTC | #5 
On Fri, Jul 19, 2019 at 11:44:57PM +0200, Paolo Bonzini wrote:
> On 19/07/19 22:53, Eduardo Habkost wrote:
> > This is one is named "cqm" on Linux (X86_FEATURE_CQM).  I prefer
> > to keep consistency with the name already in use by Linux than
> > the one in libvirt that was never used.
> > 
> > You can still add a "cmt" alias property if you think it would be
> > useful.
> 
> Actually KVM does not mark it as supported:
> 
>         const u32 kvm_cpuid_7_0_ebx_x86_features =
>                 F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
>                 F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | f_mpx | F(RDSEED) |
>                 F(ADX) | F(SMAP) | F(AVX512IFMA) | F(AVX512F) | F(AVX512PF) |
>                 F(AVX512ER) | F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(AVX512DQ) |
>                 F(SHA_NI) | F(AVX512BW) | F(AVX512VL) | f_intel_pt;

We can still add the feature name if we also set it on
.unmigratable_features, but I don't see why it would be useful.
Is anybody working to support exposing RDT-M to guests?
Paolo Bonzini July 19, 2019, 10:16 p.m. UTC | #6 
On 20/07/19 00:05, Eduardo Habkost wrote:
>> Actually KVM does not mark it as supported:
>>
>>         const u32 kvm_cpuid_7_0_ebx_x86_features =
>>                 F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
>>                 F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | f_mpx | F(RDSEED) |
>>                 F(ADX) | F(SMAP) | F(AVX512IFMA) | F(AVX512F) | F(AVX512PF) |
>>                 F(AVX512ER) | F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(AVX512DQ) |
>>                 F(SHA_NI) | F(AVX512BW) | F(AVX512VL) | f_intel_pt;
> We can still add the feature name if we also set it on
> .unmigratable_features, but I don't see why it would be useful.
> Is anybody working to support exposing RDT-M to guests?

I don't think so.

Paolo
Jiri Denemark Aug. 14, 2019, 11:36 a.m. UTC | #7 
On Thu, Jul 18, 2019 at 16:45:37 +0300, Denis V. Lunev wrote:
> There are the following flags available in libvirt inside cpu_map.xm
>     <feature name='cvt16'>
>       <cpuid function='0x80000001' ecx='0x00040000'/>
>     </feature>
>     <feature name='cmt'> <!-- cqm -->
>       <cpuid eax_in='0x07' ecx_in='0x00' ebx='0x00001000'/>
>     </feature>
> We have faced the problem that QEMU does not start once these flags are
> present in the domain.xml.

Libvirt should not add this to the XML by itself (when using host-model
CPU, for example) so the user must have asked for the feature
explicitly. Thus I don't see any problem with QEMU refusing to start
with such configuration. And the workaround is easy, just don't do it.

I'm not sure about cvt16, but IIRC cmt and mbm_* features were added as
a way to detect whether the host CPU supports perf monitoring counters.
I think tt was not the brightest idea, but there's no reason why QEMU
should support enabling these features. Unless it actually makes sense
for QEMU.

If there are any issues with libvirt passing these features to QEMU
without explicit request from the user, we should address them in
libvirt.

Jirka
diff mbox series

Patch

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 805ce95247..88ba4dad47 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -870,7 +870,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "lahf-lm", "cmp-legacy", "svm", "extapic",
             "cr8legacy", "abm", "sse4a", "misalignsse",
             "3dnowprefetch", "osvw", "ibs", "xop",
-            "skinit", "wdt", NULL, "lwp",
+            "skinit", "wdt", "cvt16", "lwp",
             "fma4", "tce", NULL, "nodeid-msr",
             NULL, "tbm", "topoext", "perfctr-core",
             "perfctr-nb", NULL, NULL, NULL,
@@ -1044,7 +1044,7 @@  static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             "fsgsbase", "tsc-adjust", NULL, "bmi1",
             "hle", "avx2", NULL, "smep",
             "bmi2", "erms", "invpcid", "rtm",
-            NULL, NULL, "mpx", NULL,
+            "cmt", NULL, "mpx", NULL,
             "avx512f", "avx512dq", "rdseed", "adx",
             "smap", "avx512ifma", "pcommit", "clflushopt",
             "clwb", "intel-pt", "avx512pf", "avx512er",