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

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

[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 unnamed 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Caio Carrara <ccarrara@redhat.com>
---
v2:
 * Fixed typos. [eblake]
 * Removed unwanted manual test case. [ccarrara, ehabkost]
 * Not passing 'accel=kvm' on test's VM. [ehabkost]
 * Removed unneeded g_strdup() call. [ehabkost]
 * Formatted comment according to QEMU's coding style. [ehabkost]

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg579404.html
---
 target/i386/cpu.c                   | 11 ++++++++-
 tests/acceptance/cpu_definitions.py | 35 +++++++++++++++++++++++++++++
 2 files changed, 45 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..014b91e608 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3615,19 +3615,28 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
 
     x86_cpu_filter_features(xc);
 
+    /* Auxiliary dictionary to avoid duplicate entries in the list. */
+    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 = 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..4edad86799
--- /dev/null
+++ b/tests/acceptance/cpu_definitions.py
@@ -0,0 +1,35 @@
+# 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")
+        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")
-- 
2.19.1

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

[Eric Blake]

On 12/11/18 10:28 AM, 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 unnamed features as well as duplicated
> names.
> 
> For example, the qmp "query-cpu-definitions" below shows one empty
> string and repeated "mpx" entries:
> 

> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Careful. While I spotted typos in v1,...

> Reviewed-by: Caio Carrara <ccarrara@redhat.com>
> ---
> v2:
>   * Fixed typos. [eblake]

...and you indeed addressed them, me pointing out typos does not imply 
that I reviewed the patch for correctness.  In fact, I specifically did 
NOT give my R-by: tag to v1, because I'm not (yet?) familiar enough with 
the tests/acceptance/ framework to state that I have fully reviewed the 
patch for correctness; instead, I'm comfortable relying on the reviews 
of others (and I am again intentionally not giving R-by to v2).

Also, when posting a v2, you should include the R-by actually given to 
v1 only if the patch is roughly the same as the original.  Fixing minor 
issues that a reviewer pointed out, or doing obvious rebasing to changes 
applied earlier in the series or on upstream git, but where the 
algorithm of the patch itself did not change, is okay for keeping R-b 
(so if I _had_ given R-b, and your spelling changes were the only 
difference, then keeping my R-b would make sense); but where the patch 
is fundamentally different, such as:

>  * Removed unwanted manual test case. [ccarrara, ehabkost]
>  * Not passing 'accel=kvm' on test's VM. [ehabkost]

then omitting ALL R-by tags, in order to ensure that reviewers check 
that the new patch is still correct, is a wiser course of action.  Yes, 
this is more of a rule of thumb, and there are cases where keeping or 
dropping R-b is more of an art form than an exact science; but hopefully 
this helps you understand how the tag can be useful for iterative reviews.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Wainer dos Santos Moschetta]

On 12/11/2018 03:15 PM, Eric Blake wrote:
> On 12/11/18 10:28 AM, 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 unnamed features as well as duplicated
>> names.
>>
>> For example, the qmp "query-cpu-definitions" below shows one empty
>> string and repeated "mpx" entries:
>>
>
>> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Careful. While I spotted typos in v1,...
>
>> Reviewed-by: Caio Carrara <ccarrara@redhat.com>
>> ---
>> v2:
>>   * Fixed typos. [eblake]
>
> ...and you indeed addressed them, me pointing out typos does not imply 
> that I reviewed the patch for correctness.  In fact, I specifically 
> did NOT give my R-by: tag to v1, because I'm not (yet?) familiar 
> enough with the tests/acceptance/ framework to state that I have fully 
> reviewed the patch for correctness; instead, I'm comfortable relying 
> on the reviews of others (and I am again intentionally not giving R-by 
> to v2).
>
> Also, when posting a v2, you should include the R-by actually given to 
> v1 only if the patch is roughly the same as the original.  Fixing 
> minor issues that a reviewer pointed out, or doing obvious rebasing to 
> changes applied earlier in the series or on upstream git, but where 
> the algorithm of the patch itself did not change, is okay for keeping 
> R-b (so if I _had_ given R-b, and your spelling changes were the only 
> difference, then keeping my R-b would make sense); but where the patch 
> is fundamentally different, such as:
>
>>  * Removed unwanted manual test case. [ccarrara, ehabkost]
>>  * Not passing 'accel=kvm' on test's VM. [ehabkost]
>
> then omitting ALL R-by tags, in order to ensure that reviewers check 
> that the new patch is still correct, is a wiser course of action.  
> Yes, this is more of a rule of thumb, and there are cases where 
> keeping or dropping R-b is more of an art form than an exact science; 
> but hopefully this helps you understand how the tag can be useful for 
> iterative reviews.
>

Hi Eric,

Yes, it helped a lot, thanks. And I apologize for my mistake, I'm gonna 
send a v3 fixing it.

Another doubt that I have: is it advisable to CC everyone that reviewed 
(with or without R-by) the previous version of my patch?

Thanks!

- Wainer

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

[Eric Blake]

On 12/11/18 1:47 PM, Wainer dos Santos Moschetta wrote:

> 
> Yes, it helped a lot, thanks. And I apologize for my mistake, I'm gonna 
> send a v3 fixing it.

You may want to wait a day or so for any other comments on v2, to 
minimize the resend churn. A maintainer can fix up tags, particularly 
when they are aware it is from a newer contributor still learning how 
things work.

> 
> Another doubt that I have: is it advisable to CC everyone that reviewed 
> (with or without R-by) the previous version of my patch?

CC'ing previous reviewers is generally a reasonable idea, since they are 
then more likely to double-check that the things they pointed out in the 
first version are indeed fixed in the respin (and since without the cc, 
it's a lot easier to miss that a respin is even available on-list for 
followup review, thanks to the list traffic volume).  You might not get 
a reply from everyone cc'd, but that's not fatal to acceptance of the patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Eduardo Habkost]

On Tue, Dec 11, 2018 at 11:28:46AM -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 unnamed 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",
>                 "",

I just noticed one thing: we probably want to find out the cause
of this empty entry, instead of ignoring it in the code.  Named
CPU models must only refer to named CPU features.

I think this is caused by CPUID_7_0_ECX_OSPKE, I will
investigate.  But this doesn't make your patch incorrect.


>                 "avx512vnni",
>                 "spec-ctrl",
>                 "ssbd",
>                 "3dnowprefetch",
>                 "xsavec",
>                 "xgetbv1",
>                 "mpx",
>                 "mpx",
>                 "avx512f",
>                 "avx512f",
>                 "avx512f",
>                 "pku"
>             ],
> (...)
> 
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Caio Carrara <ccarrara@redhat.com>
> ---
> v2:
>  * Fixed typos. [eblake]
>  * Removed unwanted manual test case. [ccarrara, ehabkost]
>  * Not passing 'accel=kvm' on test's VM. [ehabkost]
>  * Removed unneeded g_strdup() call. [ehabkost]
>  * Formatted comment according to QEMU's coding style. [ehabkost]
> 
> v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg579404.html
> ---
>  target/i386/cpu.c                   | 11 ++++++++-
>  tests/acceptance/cpu_definitions.py | 35 +++++++++++++++++++++++++++++
>  2 files changed, 45 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..014b91e608 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3615,19 +3615,28 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>  
>      x86_cpu_filter_features(xc);
>  
> +    /* Auxiliary dictionary to avoid duplicate entries in the list. */
> +    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 = x86_cpu_feature_name(w, i);
> +                if (!fname || qdict_haskey(unique_feats_dict, fname)) {
> +                    continue;
> +                }
>                  strList *new = g_new0(strList, 1);

I like mixed declarations, but unfortunately they are not allowed
by CODING_STYLE:

	5. Declarations

	Mixed declarations (interleaving statements and declarations within
	blocks) are generally not allowed; declarations should be at the beginning
	of blocks.


The logic now looks good, though.

> -                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..4edad86799
> --- /dev/null
> +++ b/tests/acceptance/cpu_definitions.py
> @@ -0,0 +1,35 @@
> +# 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")

I thought the explicit -machine option was here only because of
the old accel=kvm option, and the whole line would be removed.
Why do you need to explicitly ask for a Q35 machine to test this?


> +        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")

"unnamed"

> +            self.assertEqual(len(unavailable_features),
> +                             len(set(unavailable_features)),
> +                             name + " has duplicate feature")
> -- 
> 2.19.1
> 

-- 
Eduardo