From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40983) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gWl5Y-00085n-5C for qemu-devel@nongnu.org; Tue, 11 Dec 2018 11:40:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gWl5W-0001zo-3D for qemu-devel@nongnu.org; Tue, 11 Dec 2018 11:40:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41422) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gWl5S-0001wU-65 for qemu-devel@nongnu.org; Tue, 11 Dec 2018 11:40:48 -0500 References: <20181207221417.5152-1-wainersm@redhat.com> <20181210164657.GB4669@habkost.net> From: Wainer dos Santos Moschetta Message-ID: <5cd0b649-c74f-877d-4b5d-a0943a04c7eb@redhat.com> Date: Tue, 11 Dec 2018 14:40:33 -0200 MIME-Version: 1.0 In-Reply-To: <20181210164657.GB4669@habkost.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: pbonzini@redhat.com, rth@twiddle.net, qemu-devel@nongnu.org, ccarrara@redhat.com, crosa@redhat.com 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 >> --- >> 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 >> +# >> +# 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 >>