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 <waine...@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 <waine...@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
