target/i386: Fixes to the check missing features routine
PATCH 1/1 target/i386: Fixes to the check missing features routine
The x86_cpu_class_check_missing_features() returns a list of unavailable features compared to the host CPU. Currently it may return empty strings for unamed features as well as duplicated names.
For example, the qmp "query-cpu-definitions" below shows one empty string and repeated "mpx" entries:
(...)
{"execute": "query-cpu-definitions"}
(...)
{
"name": "Cascadelake-Server",
"typename": "Cascadelake-Server-x86_64-cpu",
"unavailable-features": [
"hle",
"rtm",
"mpx",
"avx512f",
"avx512dq",
"rdseed",
"adx",
"smap",
"clflushopt",
"clwb",
"intel-pt",
"avx512cd",
"avx512bw",
"avx512vl",
"pku",
"",
"avx512vnni",
"spec-ctrl",
"ssbd",
"3dnowprefetch",
"xsavec",
"xgetbv1",
"mpx",
"mpx",
"avx512f",
"avx512f",
"avx512f",
"pku"
],(...)
---
Note: the skipped testcase was used to test fix in my system so it has assumptions about the host CPU. It's impracticial to change it to allow running on any system though. Therefore, I am okay on either leave or remove it. Opinions?
--- target/i386/cpu.c | 12 +++++- tests/acceptance/cpu_definitions.py | 61 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 tests/acceptance/cpu_definitions.py
target/i386/cpu.c +11 -1
@@ -3615,19 +3615,29 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,x86_cpu_filter_features (xc); +/* Uses an auxiliar dictionary to ensure the list of features has not +repeated name. */ +QDict *unique_feats_dict =qdict_new (); +for (w =0 ; w <FEATURE_WORDS ; w++) {uint32_t filtered = xc->filtered_features[w];int i;for (i =0 ; i <32 ; i++) {if (filtered & (1UL << i)) { +const char *fname =g_strdup (x86_cpu_feature_name (w, i)); +if (!fname ||qdict_haskey (unique_feats_dict, fname)) { +continue ; + }strList *new =g_new0 (strList,1 ); - new->value =g_strdup (x86_cpu_feature_name (w, i)); + new->value =g_strdup (fname); *next = new; next = &new->next; +qdict_put_null (unique_feats_dict, new->value); } } } +g_free (unique_feats_dict);object_unref (OBJECT (xc)); }
tests/acceptance/cpu_definitions.py new file +61 -0
61 lines not on this page · raw message
Signature · 1 line
2.19.1
Quoted 3 lines · Wainer dos Santos Moschetta
The
x86_cpu_class_check_missing_features()returns a list of unavailable features compared to the host CPU. Currently it may return empty strings for unamed features as well as duplicated
s/unamed/unnamed/
target/i386/cpu.c static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, · line 3618
Quoted 11 lines
names.
For example, the qmp "query-cpu-definitions" below shows one empty string and repeated "mpx" entries:
+++
b/target/i386/cpu.ctarget/i386/cpu.c +1
-0@@ -3615,19 +3615,29 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,x86_cpu_filter_features (xc); +/* Uses an auxiliar dictionary to ensure the list of features has not
s/auxiliar/auxiliary/
Quoted 3 lines
target/i386/cpu.c +3
-0+repeated name. */ +QDict *unique_feats_dict =qdict_new (); +
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 44 lines · Wainer dos Santos Moschetta
The
x86_cpu_class_check_missing_features()returns a list of unavailable features compared to the host CPU. Currently it may return empty strings for unamed features as well as duplicated names.For example, the qmp "query-cpu-definitions" below shows one empty string and repeated "mpx" entries:
(...)
{"execute": "query-cpu-definitions"}
(...){ "name": "Cascadelake-Server", "typename": "Cascadelake-Server-x86_64-cpu", "unavailable-features": [ "hle", "rtm", "mpx", "avx512f", "avx512dq", "rdseed", "adx", "smap", "clflushopt", "clwb", "intel-pt", "avx512cd", "avx512bw", "avx512vl", "pku", "", "avx512vnni", "spec-ctrl", "ssbd", "3dnowprefetch", "xsavec", "xgetbv1", "mpx", "mpx", "avx512f", "avx512f", "avx512f", "pku" ],
Quoted 8 lines
(...)
Signed-off-by Wainer dos Santos Moschetta <[email protected]>---
Note: the skipped testcase was used to test fix in my system so it has assumptions about the host CPU. It's impracticial to change it to allow running on any system though. Therefore, I am okay on either leave or remove it. Opinions?
I disagree with this test. This is an always skipping test that tend to become easily a meaningless dead code. If your real tests that is not being skipped have proper coverage than it should be enough.
Quoted 111 lines
--- target/i386/cpu.c | 12 +++++- tests/acceptance/cpu_definitions.py | 61 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 tests/acceptance/cpu_definitions.pytarget/i386/cpu.c +11
-1@@ -3615,19 +3615,29 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,x86_cpu_filter_features (xc); +/* Uses an auxiliar dictionary to ensure the list of features has not +repeated name. */ +QDict *unique_feats_dict =qdict_new (); +for (w =0 ; w <FEATURE_WORDS ; w++) {uint32_t filtered = xc->filtered_features[w];int i;for (i =0 ; i <32 ; i++) {if (filtered & (1UL << i)) { +const char *fname =g_strdup (x86_cpu_feature_name (w, i)); +if (!fname ||qdict_haskey (unique_feats_dict, fname)) { +continue ; + }strList *new =g_new0 (strList,1 ); - new->value =g_strdup (x86_cpu_feature_name (w, i)); + new->value =g_strdup (fname); *next = new; next = &new->next; +qdict_put_null (unique_feats_dict, new->value); } } } +g_free (unique_feats_dict);object_unref (OBJECT (xc)); }tests/acceptance/cpu_definitions.py new file +61
-0@@ -0,0 +1,61 @@ +# CPU definitions tests. +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Wainer dos Santos Moschetta <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado import skip +from avocado_qemu import Test + + +class CPUDefinitions(Test): + """ + Tests for the CPU definitions. + + :avocado: enable + :avocado: tags=x86_64 + """ + def test_unavailable_features(self): + self.vm.add_args("-machine", "q35,accel=kvm") + self.vm.launch() + cpu_definitions = self.vm.command('query-cpu-definitions') + self.assertTrue(len(cpu_definitions) > 0) + for cpu_model in cpu_definitions: + name = cpu_model.get('name') + unavailable_features = cpu_model.get('unavailable-features') + + self.assertNotIn("", unavailable_features, + name + " has unamed feature") + self.assertEqual(len(unavailable_features), + len(set(unavailable_features)), + name + " has duplicate feature") + + @skip("Have assumptions about the host CPU") + def test_unavailable_features_manual(self): + """ + This test is meant for manual testing only because the list of expected + unavailable features depend on the actual host CPU knowledge. + """ + expected_cpu = 'Cascadelake-Server' + expected_unavailable_features = ["hle", "rtm", "mpx", "avx512f", + "avx512dq", "rdseed", "adx", "smap", + "clflushopt", "clwb", "intel-pt", + "avx512cd", "avx512bw", "avx512vl", + "pku", "avx512vnni", "spec-ctrl", + "ssbd", "3dnowprefetch", "xsavec", + "xgetbv1"] + + self.vm.add_args("-machine", "q35,accel=kvm") + self.vm.launch() + cpu_definitions = self.vm.command('query-cpu-definitions') + self.assertTrue(len(cpu_definitions) > 0) + + cpus = [cpu_model for cpu_model in cpu_definitions + if cpu_model['name'] == expected_cpu] + actual_unavailable_features = cpus[0]['unavailable-features'] + self.assertCountEqual(expected_unavailable_features, + actual_unavailable_features)Signature · 1 line
2.19.1
Signature · 3 lines
Caio Carrara Software Engineer, Virt Team - Red Hat [email protected]
Quoted 63 lines · Wainer dos Santos Moschetta
The
x86_cpu_class_check_missing_features()returns a list of unavailable features compared to the host CPU. Currently it may return empty strings for unamed features as well as duplicated names.For example, the qmp "query-cpu-definitions" below shows one empty string and repeated "mpx" entries:
(...)
{"execute": "query-cpu-definitions"}
(...){ "name": "Cascadelake-Server", "typename": "Cascadelake-Server-x86_64-cpu", "unavailable-features": [ "hle", "rtm", "mpx", "avx512f", "avx512dq", "rdseed", "adx", "smap", "clflushopt", "clwb", "intel-pt", "avx512cd", "avx512bw", "avx512vl", "pku", "", "avx512vnni", "spec-ctrl", "ssbd", "3dnowprefetch", "xsavec", "xgetbv1", "mpx", "mpx", "avx512f", "avx512f", "avx512f", "pku" ],(...)
Signed-off-by Wainer dos Santos Moschetta <[email protected]>---
Note: the skipped testcase was used to test fix in my system so it has assumptions about the host CPU. It's impracticial to change it to allow running on any system though. Therefore, I am okay on either leave or remove it. Opinions?--- target/i386/cpu.c | 12 +++++- tests/acceptance/cpu_definitions.py | 61 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 tests/acceptance/cpu_definitions.pytarget/i386/cpu.c +0
-0
target/i386/cpu.c static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, · line 3620
Quoted 6 lines
target/i386/cpu.c +3
-0@@ -3615,19 +3615,29 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,x86_cpu_filter_features (xc); +/* Uses an auxiliar dictionary to ensure the list of features has not +repeated name. */ +QDict *unique_feats_dict =qdict_new ();
Multiline comments are formatted this way:
/*
* like
* this
*/
(See CODING_STYLE for details)
In this case, we can probably make the comment fit in a single line:
/* Auxiliary dict to avoid duplicate entries in the list */
target/i386/cpu.c static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, · line 3627
Quoted 7 lines
target/i386/cpu.c +2
-0+for (w =0 ; w <FEATURE_WORDS ; w++) {uint32_t filtered = xc->filtered_features[w];int i;for (i =0 ; i <32 ; i++) {if (filtered & (1UL << i)) { +const char *fname =g_strdup (x86_cpu_feature_name (w, i));
I believe you didn't mean to call g_strdup() here, as you are now calling g_strdup(fname) below.
Quoted 36 lines
target/i386/cpu.c +6
-1+if (!fname ||qdict_haskey (unique_feats_dict, fname)) { +continue ; + }strList *new =g_new0 (strList,1 ); - new->value =g_strdup (x86_cpu_feature_name (w, i)); + new->value =g_strdup (fname); *next = new; next = &new->next; +qdict_put_null (unique_feats_dict, new->value); } } } +g_free (unique_feats_dict);object_unref (OBJECT (xc)); }tests/acceptance/cpu_definitions.py new file +13
-0@@ -0,0 +1,61 @@ +# CPU definitions tests. +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Wainer dos Santos Moschetta <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado import skip +from avocado_qemu import Test +
tests/acceptance/cpu_definitions.py line 23
Quoted 10 lines
tests/acceptance/cpu_definitions.py new file +10
-0+ +class CPUDefinitions(Test): + """ + Tests for the CPU definitions. + + :avocado: enable + :avocado: tags=x86_64 + """ + def test_unavailable_features(self): + self.vm.add_args("-machine", "q35,accel=kvm")
Do you really need accel=kvm to reproduce the original bug?
Quoted 17 lines
tests/acceptance/cpu_definitions.py new file +17
-0+ self.vm.launch() + cpu_definitions = self.vm.command('query-cpu-definitions') + self.assertTrue(len(cpu_definitions) > 0) + for cpu_model in cpu_definitions: + name = cpu_model.get('name') + unavailable_features = cpu_model.get('unavailable-features') + + self.assertNotIn("", unavailable_features, + name + " has unamed feature") + self.assertEqual(len(unavailable_features), + len(set(unavailable_features)), + name + " has duplicate feature") + + @skip("Have assumptions about the host CPU") + def test_unavailable_features_manual(self): + """ + This test is meant for manual testing only because the list of expected
tests/acceptance/cpu_definitions.py line 50
Quoted 10 lines
tests/acceptance/cpu_definitions.py new file +10
-0+ unavailable features depend on the actual host CPU knowledge. + """ + expected_cpu = 'Cascadelake-Server' + expected_unavailable_features = ["hle", "rtm", "mpx", "avx512f", + "avx512dq", "rdseed", "adx", "smap", + "clflushopt", "clwb", "intel-pt", + "avx512cd", "avx512bw", "avx512vl", + "pku", "avx512vnni", "spec-ctrl", + "ssbd", "3dnowprefetch", "xsavec", + "xgetbv1"]
It looks like this test will work only on one specific host CPU model. It seems very unlikely that anybody will ever try to run it manually. I suggest just deleting it.
Quoted 14 lines
tests/acceptance/cpu_definitions.py new file +11
-0+ + self.vm.add_args("-machine", "q35,accel=kvm") + self.vm.launch() + cpu_definitions = self.vm.command('query-cpu-definitions') + self.assertTrue(len(cpu_definitions) > 0) + + cpus = [cpu_model for cpu_model in cpu_definitions + if cpu_model['name'] == expected_cpu] + actual_unavailable_features = cpus[0]['unavailable-features'] + self.assertCountEqual(expected_unavailable_features, + actual_unavailable_features)Signature · 1 line
2.19.1
Signature · 1 line
Eduardo
Quoted 72 lines · Eduardo Habkost
Quoted 63 lines · Wainer dos Santos Moschetta
The
x86_cpu_class_check_missing_features()returns a list of unavailable features compared to the host CPU. Currently it may return empty strings for unamed features as well as duplicated names.For example, the qmp "query-cpu-definitions" below shows one empty string and repeated "mpx" entries:
(...)
{"execute": "query-cpu-definitions"}
(...){ "name": "Cascadelake-Server", "typename": "Cascadelake-Server-x86_64-cpu", "unavailable-features": [ "hle", "rtm", "mpx", "avx512f", "avx512dq", "rdseed", "adx", "smap", "clflushopt", "clwb", "intel-pt", "avx512cd", "avx512bw", "avx512vl", "pku", "", "avx512vnni", "spec-ctrl", "ssbd", "3dnowprefetch", "xsavec", "xgetbv1", "mpx", "mpx", "avx512f", "avx512f", "avx512f", "pku" ],(...)
Signed-off-by Wainer dos Santos Moschetta <[email protected]>---
Note: the skipped testcase was used to test fix in my system so it has assumptions about the host CPU. It's impracticial to change it to allow running on any system though. Therefore, I am okay on either leave or remove it. Opinions?--- target/i386/cpu.c | 12 +++++- tests/acceptance/cpu_definitions.py | 61 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-)create mode 100644
tests/acceptance/cpu_definitions.pytarget/i386/cpu.c +0
-0target/i386/cpu.c static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, · line 3620
Quoted 6 lines
target/i386/cpu.c +3
-0@@ -3615,19 +3615,29 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,x86_cpu_filter_features (xc); +/* Uses an auxiliar dictionary to ensure the list of features has not +repeated name. */ +QDict *unique_feats_dict =qdict_new ();Multiline comments are formatted this way:
Quoted 6 lines
/*
* like
* this
*/(See
CODING_STYLEfor details)
scripts/checkpatch.pl did not catch it. Should it?
Thanks!
- Wainer
Quoted 109 lines
In this case, we can probably make the comment fit in a single line:
/* Auxiliary dict to avoid duplicate entries in the list */target/i386/cpu.c static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, · line 3627
Quoted 7 lines
target/i386/cpu.c +2
-0+for (w =0 ; w <FEATURE_WORDS ; w++) {uint32_t filtered = xc->filtered_features[w];int i;for (i =0 ; i <32 ; i++) {if (filtered & (1UL << i)) { +const char *fname =g_strdup (x86_cpu_feature_name (w, i));I believe you didn't mean to call
g_strdup()here, as you are now callingg_strdup(fname)below.Quoted 36 lines
target/i386/cpu.c +6
-1+if (!fname ||qdict_haskey (unique_feats_dict, fname)) { +continue ; + }strList *new =g_new0 (strList,1 ); - new->value =g_strdup (x86_cpu_feature_name (w, i)); + new->value =g_strdup (fname); *next = new; next = &new->next; +qdict_put_null (unique_feats_dict, new->value); } } } +g_free (unique_feats_dict);object_unref (OBJECT (xc)); }tests/acceptance/cpu_definitions.py new file +13
-0@@ -0,0 +1,61 @@ +# CPU definitions tests. +# +# Copyright (c) 2018 Red Hat, Inc. +# +# Author: +# Wainer dos Santos Moschetta <[email protected]> +# +# This work is licensed under the terms of the GNU GPL, version 2 or +# later. See the COPYING file in the top-level directory. + +from avocado import skip +from avocado_qemu import Test +tests/acceptance/cpu_definitions.py line 23
Quoted 10 lines
tests/acceptance/cpu_definitions.py new file +10
-0+ +class CPUDefinitions(Test): + """ + Tests for the CPU definitions. + + :avocado: enable + :avocado: tags=x86_64 + """ + def test_unavailable_features(self): + self.vm.add_args("-machine", "q35,accel=kvm")Do you really need accel=kvm to reproduce the original bug?
Quoted 17 lines
tests/acceptance/cpu_definitions.py new file +17
-0+ self.vm.launch() + cpu_definitions = self.vm.command('query-cpu-definitions') + self.assertTrue(len(cpu_definitions) > 0) + for cpu_model in cpu_definitions: + name = cpu_model.get('name') + unavailable_features = cpu_model.get('unavailable-features') + + self.assertNotIn("", unavailable_features, + name + " has unamed feature") + self.assertEqual(len(unavailable_features), + len(set(unavailable_features)), + name + " has duplicate feature") + + @skip("Have assumptions about the host CPU") + def test_unavailable_features_manual(self): + """ + This test is meant for manual testing only because the list of expectedtests/acceptance/cpu_definitions.py line 50
Quoted 10 lines
tests/acceptance/cpu_definitions.py new file +10
-0+ unavailable features depend on the actual host CPU knowledge. + """ + expected_cpu = 'Cascadelake-Server' + expected_unavailable_features = ["hle", "rtm", "mpx", "avx512f", + "avx512dq", "rdseed", "adx", "smap", + "clflushopt", "clwb", "intel-pt", + "avx512cd", "avx512bw", "avx512vl", + "pku", "avx512vnni", "spec-ctrl", + "ssbd", "3dnowprefetch", "xsavec", + "xgetbv1"]It looks like this test will work only on one specific host CPU model. It seems very unlikely that anybody will ever try to run it manually. I suggest just deleting it.
Quoted 14 lines
tests/acceptance/cpu_definitions.py new file +11
-0+ + self.vm.add_args("-machine", "q35,accel=kvm") + self.vm.launch() + cpu_definitions = self.vm.command('query-cpu-definitions') + self.assertTrue(len(cpu_definitions) > 0) + + cpus = [cpu_model for cpu_model in cpu_definitions + if cpu_model['name'] == expected_cpu] + actual_unavailable_features = cpus[0]['unavailable-features'] + self.assertCountEqual(expected_unavailable_features, + actual_unavailable_features)Signature · 1 line
2.19.1
On Tue, Dec 11, 2018 at 02:40:33PM -0200, Wainer dos Santos Moschetta wrote:
[...]
Quoted 11 lines
Quoted 11 lines
target/i386/cpu.c static void x86_cpu_class_check_missing_features(X86CPUClass *xcc, · line 3618
Quoted 9 lines
target/i386/cpu.c +3
-0@@ -3615,19 +3615,29 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,x86_cpu_filter_features (xc); +/* Uses an auxiliar dictionary to ensure the list of features has not +repeated name. */ +QDict *unique_feats_dict =qdict_new ();Multiline comments are formatted this way:
Quoted 8 lines
Quoted 6 lines
/*
* like
* this
*/(See
CODING_STYLEfor details)
scripts/checkpatch.pldid not catch it. Should it?
It should, we just need a volunteer who understands Perl to fix that. :)
Signature · 1 line
Eduardo