1. Patchew
  2. QEMU
  3. View series

[PATCH] KVM: x86: believe what KVM says about WAITPKG

Paolo Bonzini posted 1 patch 3 years, 9 months ago
Test FreeBSD passed
Test docker-quick@centos7 passed
Test checkpatch failed
Test docker-mingw@fedora passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200630151150.536580-1-pbonzini@redhat.com
Maintainers: Richard Henderson <rth@twiddle.net>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>
target/i386/cpu.c | 5 ++++-
target/i386/kvm.c | 6 ------
2 files changed, 4 insertions(+), 7 deletions(-)
[PATCH] KVM: x86: believe what KVM says about WAITPKG
Posted by Paolo Bonzini 3 years, 9 months ago 
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, of course making it conditional on host
availability which is not being done for the MONITOR bit.  It also makes
MONITOR conditional, though not strictly necessary, so that the code
looks the same for both and generally more natural.

Cc: qemu-stable@nongnu.org
Reported-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/cpu.c | 5 ++++-
 target/i386/kvm.c | 6 ------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c44cc510e1..0de8dc569a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6535,7 +6535,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         if (enable_cpu_pm) {
             host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
                        &cpu->mwait.ecx, &cpu->mwait.edx);
-            env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
+            env->features[FEAT_1_ECX] |=
+                x86_cpu_get_supported_feature_word(FEAT_1_ECX, cpu->migratable) & CPUID_EXT_MONITOR;
+            env->features[FEAT_7_0_ECX] |=
+                x86_cpu_get_supported_feature_word(FEAT_7_0_ECX, cpu->migratable) & 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..c9ef27080d 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.
-- 
2.26.2
Re: [PATCH] KVM: x86: believe what KVM says about WAITPKG
Posted by no-reply@patchew.org 3 years, 9 months ago 
Patchew URL: https://patchew.org/QEMU/20200630151150.536580-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH] KVM: x86: believe what KVM says about WAITPKG
Type: series
Message-id: 20200630151150.536580-1-pbonzini@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200624121841.17971-1-paul@xen.org -> patchew/20200624121841.17971-1-paul@xen.org
 * [new tag]         patchew/20200630152531.20657-1-aleksandar.qemu.devel@gmail.com -> patchew/20200630152531.20657-1-aleksandar.qemu.devel@gmail.com
 - [tag update]      patchew/cover.1593273671.git.elena.ufimtseva@oracle.com -> patchew/cover.1593273671.git.elena.ufimtseva@oracle.com
Switched to a new branch 'test'
70f0d7b KVM: x86: believe what KVM says about WAITPKG

=== OUTPUT BEGIN ===
ERROR: line over 90 characters
#32: FILE: target/i386/cpu.c:6541:
+                x86_cpu_get_supported_feature_word(FEAT_1_ECX, cpu->migratable) & CPUID_EXT_MONITOR;

ERROR: line over 90 characters
#34: FILE: target/i386/cpu.c:6543:
+                x86_cpu_get_supported_feature_word(FEAT_7_0_ECX, cpu->migratable) & CPUID_7_0_ECX_WAITPKG;

total: 2 errors, 0 warnings, 23 lines checked

Commit 70f0d7b338b2 (KVM: x86: believe what KVM says about WAITPKG) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200630151150.536580-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [PATCH] KVM: x86: believe what KVM says about WAITPKG
Posted by Maxim Levitsky 3 years, 9 months ago 
On Tue, 2020-06-30 at 11:11 -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, of course making it conditional on host
> availability which is not being done for the MONITOR bit.  It also makes
> MONITOR conditional, though not strictly necessary, so that the code
> looks the same for both and generally more natural.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/cpu.c | 5 ++++-
>  target/i386/kvm.c | 6 ------
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c44cc510e1..0de8dc569a 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6535,7 +6535,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          if (enable_cpu_pm) {
>              host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
>                         &cpu->mwait.ecx, &cpu->mwait.edx);
> -            env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
> +            env->features[FEAT_1_ECX] |=
> +                x86_cpu_get_supported_feature_word(FEAT_1_ECX, cpu->migratable) & CPUID_EXT_MONITOR;
> +            env->features[FEAT_7_0_ECX] |=
> +                x86_cpu_get_supported_feature_word(FEAT_7_0_ECX, cpu->migratable) & CPUID_7_0_ECX_WAITPKG;
> 
After some digging I now understand that these are defaults for 'host/max', which can later be overriden
by the user, so this is correct assuming that we indeed can trust 'x86_cpu_get_supported_feature_word'.


>          }
>          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..c9ef27080d 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;
> -        }

I think we need to keep some form of this hack, since the kernel doesn't report CPUID_7_0_ECX_WAITPKG via
KVM_GET_SUPPORTED_CPUID, so for this to work, we need to fix the kernel to report it. 
But to support older kernels that don't report this bit, we might still need this.
What do you think?

Note that kvm_arch_get_supported_cpuid also has a override for what KVM reports about CPUID_EXT_MONITOR
via KVM_GET_SUPPORTED_CPUID when cpu_pm=on and also does this without checking any conditions,
and it works because MWAIT is very old feature, and I guess it was the inspiration for the above override
that we are trying to remove.

Kernel sadly masks both MWAIT and WAITPKG in KVM_GET_SUPPORTED_CPUID currently
(it is in kvm_set_cpu_caps)


>      } else if (function == 7 && index == 0 && reg == R_EDX) {
>          /*
>           * Linux v4.17-v4.20 incorrectly return ARCH_CAPABILITIES on SVM hosts.

Best regards,
	Maxim Levitsky
Re: [PATCH] KVM: x86: believe what KVM says about WAITPKG
Posted by Paolo Bonzini 3 years, 9 months ago 
On 30/06/20 18:07, Maxim Levitsky wrote:
> I think we need to keep some form of this hack, since the kernel doesn't report CPUID_7_0_ECX_WAITPKG via
> KVM_GET_SUPPORTED_CPUID, so for this to work, we need to fix the kernel to report it. 
> But to support older kernels that don't report this bit, we might still need this.
> What do you think?
> 
> Note that kvm_arch_get_supported_cpuid also has a override for what KVM reports about CPUID_EXT_MONITOR
> via KVM_GET_SUPPORTED_CPUID when cpu_pm=on and also does this without checking any conditions,
> and it works because MWAIT is very old feature, and I guess it was the inspiration for the above override
> that we are trying to remove.
> 
> Kernel sadly masks both MWAIT and WAITPKG in KVM_GET_SUPPORTED_CPUID currently
> (it is in kvm_set_cpu_caps)

Right.  We need to use host_cpuid instead of kvm_arch_get_supported_cpuid.

Paolo

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.