Re: [Qemu-devel] [PATCH] i386: Disable OSPKE on CPU model definitions

[Robert Hoo]

On Tue, 2019-03-19 at 17:05 -0300, Eduardo Habkost wrote:
> Currently, the Cascadelake-Server, Icelake-Client, and
> Icelake-Server are always generating the following warning:
> 
>   qemu-system-x86_64: warning: \
>     host doesn't support requested feature: CPUID.07H:ECX [bit 4]
> 
> This happens because OSPKE was never returned by
> GET_SUPPORTED_CPUID or x86_cpu_get_supported_feature_word().
> OSPKE is a runtime flag automatically set by the KVM module or by
> TCG code, was always cleared by x86_cpu_filter_features(), and
> was not supposed to appear on the CPU model table.
> 
> Remove the OSPKE flag from the CPU model table entries, to avoid
> the bogus warning and avoid returning invalid feature data on
> query-cpu-* QMP commands.  As OSPKE was always cleared by
> x86_cpu_filter_features(), this won't have any guest-visible
> impact.
> 
> Include a test case that should detect the problem if we introduce
> a similar bug again.
> 
> Fixes: c7a88b52f62b ("i386: Add new model of Cascadelake-Server")
> Fixes: 8a11c62da914 ("i386: Add new CPU model Icelake-
> {Server,Client}")
> Cc: Tao Xu <address@hidden>
> Cc: Robert Hoo <address@hidden>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  target/i386/cpu.c               |  6 +++---
>  tests/acceptance/cpu_queries.py | 33
> +++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
>  create mode 100644 tests/acceptance/cpu_queries.py
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d90c01a059..38cfdec33b 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2533,7 +2533,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_7_0_EBX_AVX512BW | CPUID_7_0_EBX_AVX512CD |
>              CPUID_7_0_EBX_AVX512VL | CPUID_7_0_EBX_CLFLUSHOPT,
>          .features[FEAT_7_0_ECX] =
> -            CPUID_7_0_ECX_PKU | CPUID_7_0_ECX_OSPKE |
> +            CPUID_7_0_ECX_PKU |
>              CPUID_7_0_ECX_AVX512VNNI,
>          .features[FEAT_7_0_EDX] =
>              CPUID_7_0_EDX_SPEC_CTRL | CPUID_7_0_EDX_SPEC_CTRL_SSBD,
> @@ -2586,7 +2586,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_7_0_EBX_SMAP,
>          .features[FEAT_7_0_ECX] =
>              CPUID_7_0_ECX_VBMI | CPUID_7_0_ECX_UMIP |
> CPUID_7_0_ECX_PKU |
> -            CPUID_7_0_ECX_OSPKE | CPUID_7_0_ECX_VBMI2 |
> CPUID_7_0_ECX_GFNI |
> +            CPUID_7_0_ECX_VBMI2 | CPUID_7_0_ECX_GFNI |
>              CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
>              CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
>              CPUID_7_0_ECX_AVX512_VPOPCNTDQ,
> @@ -2644,7 +2644,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_7_0_EBX_AVX512VL | CPUID_7_0_EBX_CLFLUSHOPT,
>          .features[FEAT_7_0_ECX] =
>              CPUID_7_0_ECX_VBMI | CPUID_7_0_ECX_UMIP |
> CPUID_7_0_ECX_PKU |
> -            CPUID_7_0_ECX_OSPKE | CPUID_7_0_ECX_VBMI2 |
> CPUID_7_0_ECX_GFNI |
> +            CPUID_7_0_ECX_VBMI2 | CPUID_7_0_ECX_GFNI |
>              CPUID_7_0_ECX_VAES | CPUID_7_0_ECX_VPCLMULQDQ |
>              CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
>              CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57,
> diff --git a/tests/acceptance/cpu_queries.py
> b/tests/acceptance/cpu_queries.py
> new file mode 100644
> index 0000000000..e71edec39f
> --- /dev/null
> +++ b/tests/acceptance/cpu_queries.py
> @@ -0,0 +1,33 @@
> +# Sanity check of query-cpu-* results
> +#
> +# Copyright (c) 2019 Red Hat, Inc.
> +#
> +# Author:
> +#  Eduardo Habkost <address@hidden>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> +# later.  See the COPYING file in the top-level directory.
> +
> +import logging
> +
> +from avocado_qemu import Test
> +
> +class QueryCPUModelExpansion(Test):
> +    """
> +    Run query-cpu-model-expansion for each CPU model, and validate
> results
> +    """
> +
> +    def test(self):
> +        self.vm.set_machine('none')
> +        self.vm.add_args('-S')
> +        self.vm.launch()
> +
> +        cpus = self.vm.command('query-cpu-definitions')
> +        for c in cpus:
> +            print(repr(c))
> +            self.assertNotIn('', c['unavailable-features'],
> c['name'])
> +
> +        for c in cpus:
> +            model = {'name': c['name']}
> +            e = self.vm.command('query-cpu-model-expansion',
> model=model, type='full')
> +            self.assertEquals(e['model']['name'], c['name'])

Thanks for fix.

Reviewed-by: Robert Hoo <address@hidden>