* [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine
@ 2018-12-07 22:14 Wainer dos Santos Moschetta
2018-12-07 22:26 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Wainer dos Santos Moschetta @ 2018-12-07 22:14 UTC (permalink / raw)
To: pbonzini, rth, ehabkost, qemu-devel; +Cc: ccarrara, crosa
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 <wainersm@redhat.com>
---
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
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f81d35e1f9..2502a3adda 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -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));
}
diff --git a/tests/acceptance/cpu_definitions.py b/tests/acceptance/cpu_definitions.py
new file mode 100644
index 0000000000..65cea0427e
--- /dev/null
+++ b/tests/acceptance/cpu_definitions.py
@@ -0,0 +1,61 @@
+# CPU definitions tests.
+#
+# Copyright (c) 2018 Red Hat, Inc.
+#
+# Author:
+# Wainer dos Santos Moschetta <wainersm@redhat.com>
+#
+# 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)
--
2.19.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine
2018-12-07 22:14 [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine Wainer dos Santos Moschetta
@ 2018-12-07 22:26 ` Eric Blake
2018-12-10 1:06 ` Caio Carrara
2018-12-10 16:46 ` Eduardo Habkost
2 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2018-12-07 22:26 UTC (permalink / raw)
To: Wainer dos Santos Moschetta, pbonzini, rth, ehabkost, qemu-devel
Cc: crosa, ccarrara
On 12/7/18 4:14 PM, Wainer dos Santos Moschetta wrote:
> 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/
> names.
>
> For example, the qmp "query-cpu-definitions" below shows one empty
> string and repeated "mpx" entries:
> +++ b/target/i386/cpu.c
> @@ -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/
> + repeated name. */
> + QDict *unique_feats_dict = qdict_new();
> +
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine
2018-12-07 22:14 [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine Wainer dos Santos Moschetta
2018-12-07 22:26 ` Eric Blake
@ 2018-12-10 1:06 ` Caio Carrara
2018-12-10 16:46 ` Eduardo Habkost
2 siblings, 0 replies; 6+ messages in thread
From: Caio Carrara @ 2018-12-10 1:06 UTC (permalink / raw)
To: Wainer dos Santos Moschetta; +Cc: pbonzini, rth, ehabkost, qemu-devel, crosa
On Fri, Dec 07, 2018 at 05:14:17PM -0500, Wainer dos Santos Moschetta wrote:
> 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 <wainersm@redhat.com>
> ---
> 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.
> ---
> 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
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index f81d35e1f9..2502a3adda 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -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));
> }
>
> diff --git a/tests/acceptance/cpu_definitions.py b/tests/acceptance/cpu_definitions.py
> new file mode 100644
> index 0000000000..65cea0427e
> --- /dev/null
> +++ b/tests/acceptance/cpu_definitions.py
> @@ -0,0 +1,61 @@
> +# CPU definitions tests.
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +# Wainer dos Santos Moschetta <wainersm@redhat.com>
> +#
> +# 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)
> --
> 2.19.1
>
--
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara@redhat.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine
2018-12-07 22:14 [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine Wainer dos Santos Moschetta
2018-12-07 22:26 ` Eric Blake
2018-12-10 1:06 ` Caio Carrara
@ 2018-12-10 16:46 ` Eduardo Habkost
2018-12-11 16:40 ` Wainer dos Santos Moschetta
2 siblings, 1 reply; 6+ messages in thread
From: Eduardo Habkost @ 2018-12-10 16:46 UTC (permalink / raw)
To: Wainer dos Santos Moschetta; +Cc: pbonzini, rth, qemu-devel, ccarrara, crosa
On Fri, Dec 07, 2018 at 05:14:17PM -0500, Wainer dos Santos Moschetta wrote:
> 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 <wainersm@redhat.com>
> ---
> 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
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index f81d35e1f9..2502a3adda 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -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 */
> +
> 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.
> + 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));
> }
>
> diff --git a/tests/acceptance/cpu_definitions.py b/tests/acceptance/cpu_definitions.py
> new file mode 100644
> index 0000000000..65cea0427e
> --- /dev/null
> +++ b/tests/acceptance/cpu_definitions.py
> @@ -0,0 +1,61 @@
> +# CPU definitions tests.
> +#
> +# Copyright (c) 2018 Red Hat, Inc.
> +#
> +# Author:
> +# Wainer dos Santos Moschetta <wainersm@redhat.com>
> +#
> +# 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")
Do you really need accel=kvm to reproduce the original bug?
> + 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"]
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.
> +
> + 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)
> --
> 2.19.1
>
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine
2018-12-10 16:46 ` Eduardo Habkost
@ 2018-12-11 16:40 ` Wainer dos Santos Moschetta
2018-12-11 16:55 ` Eduardo Habkost
0 siblings, 1 reply; 6+ messages in thread
From: Wainer dos Santos Moschetta @ 2018-12-11 16:40 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: pbonzini, rth, qemu-devel, ccarrara, crosa
On 12/10/2018 02:46 PM, Eduardo Habkost wrote:
> On Fri, Dec 07, 2018 at 05:14:17PM -0500, Wainer dos Santos Moschetta wrote:
>> 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 <wainersm@redhat.com>
>> ---
>> 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
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index f81d35e1f9..2502a3adda 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -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)
scripts/checkpatch.pl did not catch it. Should it?
Thanks!
- Wainer
>
> In this case, we can probably make the comment fit in a single
> line:
>
> /* Auxiliary dict to avoid duplicate entries in the list */
>
>> +
>> 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.
>
>> + 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));
>> }
>>
>> diff --git a/tests/acceptance/cpu_definitions.py b/tests/acceptance/cpu_definitions.py
>> new file mode 100644
>> index 0000000000..65cea0427e
>> --- /dev/null
>> +++ b/tests/acceptance/cpu_definitions.py
>> @@ -0,0 +1,61 @@
>> +# CPU definitions tests.
>> +#
>> +# Copyright (c) 2018 Red Hat, Inc.
>> +#
>> +# Author:
>> +# Wainer dos Santos Moschetta <wainersm@redhat.com>
>> +#
>> +# 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")
> Do you really need accel=kvm to reproduce the original bug?
>
>> + 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"]
> 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.
>
>> +
>> + 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)
>> --
>> 2.19.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine
2018-12-11 16:40 ` Wainer dos Santos Moschetta
@ 2018-12-11 16:55 ` Eduardo Habkost
0 siblings, 0 replies; 6+ messages in thread
From: Eduardo Habkost @ 2018-12-11 16:55 UTC (permalink / raw)
To: Wainer dos Santos Moschetta; +Cc: pbonzini, rth, qemu-devel, ccarrara, crosa
On Tue, Dec 11, 2018 at 02:40:33PM -0200, Wainer dos Santos Moschetta wrote:
[...]
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index f81d35e1f9..2502a3adda 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -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)
>
> scripts/checkpatch.pl did not catch it. Should it?
It should, we just need a volunteer who understands Perl to fix
that. :)
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-11 16:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-07 22:14 [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine Wainer dos Santos Moschetta
2018-12-07 22:26 ` Eric Blake
2018-12-10 1:06 ` Caio Carrara
2018-12-10 16:46 ` Eduardo Habkost
2018-12-11 16:40 ` Wainer dos Santos Moschetta
2018-12-11 16:55 ` Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).