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

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 7/7/2020 12:41 AM, 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;
> +            }

Hi Paolo and Maxim,

Sorry to find out this long-time-ago mail although the code has been 
refactor nowadays. Recently, when testing WAITPKG exposure, I found 
WAITPKG is no longer controlled by "-overcommit cpu-pm", i.e. if 
!enable_cpu_pm && KVM cap reports WAITPKG (KVM set the cap in 
vmx_set_cpu_caps), this feature can still be seen in guest. It is 
obviously due to QEMU doesn't clear CPUID_7_0_ECX_WAITPKG explicitly if 
!enable_cpu_pm.

So, is this the expected behavior for this patch?

Thanks
Chenyi

>           }
>           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;
> +}
> 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);
> 
> --
> 2.26.2
> 
> 
>