Re: [PULL 31/53] KVM: x86: believe what KVM says about WAITPKG

[Maxim Levitsky]

On Mon, 2020-07-06 at 12:41 -0400, Paolo Bonzini wrote:
> Currently, QEMU is overriding KVM_GET_SUPPORTED_CPUID's answer for
> the WAITPKG bit depending on the "-overcommit cpu-pm" setting.  This is a
> bad idea because it does not even check if the host supports it, but it
> can be done in x86_cpu_realizefn just like we do for the MONITOR bit.
> 
> This patch moves it there, while making it conditional on host
> support for the related UMWAIT MSR.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c      |  3 +++
>  target/i386/kvm.c      | 11 +++++------
>  target/i386/kvm_i386.h |  1 +
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c44cc510e1..dc9ba06f1f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6536,6 +6536,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>              host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
>                         &cpu->mwait.ecx, &cpu->mwait.edx);
>              env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
> +            if (kvm_enabled() && kvm_has_waitpkg()) {
> +                env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
> +            }
>          }
>          if (kvm_enabled() && cpu->ucode_rev == 0) {
>              cpu->ucode_rev = kvm_arch_get_supported_msr_feature(kvm_state,
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 2b6b7443d2..b8455c89ed 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -411,12 +411,6 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, 
> uint32_t function,
>          if (host_tsx_blacklisted()) {
>              ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
>          }
> -    } else if (function == 7 && index == 0 && reg == R_ECX) {
> -        if (enable_cpu_pm) {
> -            ret |= CPUID_7_0_ECX_WAITPKG;
> -        } else {
> -            ret &= ~CPUID_7_0_ECX_WAITPKG;
> -        }
>      } else if (function == 7 && index == 0 && reg == R_EDX) {
>          /*
>           * Linux v4.17-v4.20 incorrectly return ARCH_CAPABILITIES on SVM 
> hosts.
> @@ -4730,3 +4724,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      abort();
>  }
> +
> +bool kvm_has_waitpkg(void)
> +{
> +    return has_msr_umwait;
Note that this depends on the fix in the kernel
to report this msr only when UMWAIT is supported.
I personally don't mind that.

If we want to support older kernels that don't,
then we have to run 'cpuid' ourselves and check the result.

Otherwise looks good to me.

Best regards,
        Maxim Levitsky

> +}
> diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h
> index 00bde7acaf..064b8798a2 100644
> --- a/target/i386/kvm_i386.h
> +++ b/target/i386/kvm_i386.h
> @@ -44,6 +44,7 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
>  
>  bool kvm_enable_x2apic(void);
>  bool kvm_has_x2apic_api(void);
> +bool kvm_has_waitpkg(void);
>  
>  bool kvm_hv_vpindex_settable(void);
>