Re: [Qemu-devel] [PATCH] target/i386: Fixes to the check missing features routine

[Wainer dos Santos Moschetta <wainersm@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 <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
>>