linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Wanpeng Li <kernellwp@gmail.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 1/3] KVM: X86: Provides userspace with a capability to not intercept MWAIT
Date: Thu, 8 Mar 2018 21:31:32 +0100	[thread overview]
Message-ID: <20180308203132.GJ12290@flask> (raw)
In-Reply-To: <1519897782-8124-1-git-send-email-wanpengli@tencent.com>

2018-03-01 17:49+0800, Wanpeng Li:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Allowing a guest to execute MWAIT without interception enables a guest
> to put a (physical) CPU into a power saving state, where it takes
> longer to return from than what may be desired by the host.
> 
> Don't give a guest that power over a host by default. (Especially,
> since nothing prevents a guest from using MWAIT even when it is not
> advertised via CPUID.)
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> @@ -1041,7 +1041,8 @@ On systems that do not support this ioctl, it always fails. On systems that
>  do support it, it only works for extensions that are supported for enablement.
>  
>  To check if a capability can be enabled, the KVM_CHECK_EXTENSION ioctl should
> -be used.
> +be used. Blindly passing the KVM_CHECK_EXTENSION result to KVM_ENABLE_CAP is
> +a valid thing to do when vCPUs are associated to dedicated physical CPUs.

This is not true even for x86 KVM_CAP_SPLIT_IRQCHIP and neither is is a
need to limit ourselves.  Just leave it be.

> +7.13 KVM_CAP_X86_DISABLE_EXITS
> +
> +Architectures: x86
> +Parameters: args[0] defines which exits are disabled
> +Returns: 0 on success, -EINVAL when args[0] contains invalid exits
> +
> +Valid exits in args[0] are
> +
> +#define KVM_X86_DISABLE_EXITS_MWAIT            (1 << 0)
> +
> +Enabling this capability on a VM provides userspace with a way to no
> +longer intercepts some instructions for improved latency in some
> +workloads. Not enable KVM_FEATURE_PV_UNHALT if you block HLT.

The last sentence belong to the patch that enables HLT.
KVM could in theory handle the case (although it makes no sense), so if
it doesn't currently work, please add a check to kvm_update_cpuid() that
forbids KVM_FEATURE_PV_UNHALT when halt exits are disabled.

Also, it would be nicer to write that as
"Do not enable KVM_FEATURE_PV_UNHALT if you disable HLT exits."

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -2780,9 +2780,15 @@ static int msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs __user *user_msrs,
>  	return r;
>  }
>  
> +static inline bool kvm_mwait_can_in_guest(void)

I think kvm_can_mwait_in_guest would be a better name.

> +{
> +	return boot_cpu_has(X86_FEATURE_MWAIT) &&
> +		!boot_cpu_has_bug(X86_BUG_MONITOR);
> +}
> +
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index b91215d..cd1215e 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -2,8 +2,6 @@
>  #ifndef ARCH_X86_KVM_X86_H
>  #define ARCH_X86_KVM_X86_H
>  
> -#include <asm/processor.h>
> -#include <asm/mwait.h>
>  #include <linux/kvm_host.h>
>  #include <asm/pvclock.h>
>  #include "kvm_cache_regs.h"
> @@ -264,10 +262,12 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec)
>  	    __rem;						\
>  	 })
>  
> -static inline bool kvm_mwait_in_guest(void)
> +#define KVM_X86_DISABLE_EXITS_MWAIT          (1 << 0)
> +#define KVM_X86_DISABLE_VALID_EXITS          (KVM_X86_DISABLE_EXITS_MWAIT)
> +
> +static inline bool kvm_mwait_in_guest(struct kvm *kvm)
>  {
> -	return boot_cpu_has(X86_FEATURE_MWAIT) &&
> -		!boot_cpu_has_bug(X86_BUG_MONITOR);
> +	return kvm->arch.mwait_in_guest;

With this nice variable, the wrapper actually makes the code harder to
read.  Please use kvm->arch.mwait_in_guest directly (and the same for
the other two future exits),

thanks.

  parent reply	other threads:[~2018-03-08 20:32 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-01  9:49 [PATCH 1/3] KVM: X86: Provides userspace with a capability to not intercept MWAIT Wanpeng Li
2018-03-01  9:49 ` [PATCH 2/3] KVM: X86: Provides userspace with a capability to not intercept HLT Wanpeng Li
2018-03-08 20:40   ` Radim Krčmář
2018-03-09  0:51     ` Wanpeng Li
2018-03-01  9:49 ` [PATCH 3/3] KVM: X86: Provides userspace with a capability to not intercept PAUSE Wanpeng Li
2018-03-08 20:52   ` Radim Krčmář
2018-03-08 20:31 ` Radim Krčmář [this message]
2018-03-08 20:55   ` [PATCH 1/3] KVM: X86: Provides userspace with a capability to not intercept MWAIT Radim Krčmář
2018-03-09  2:34   ` Wanpeng Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180308203132.GJ12290@flask \
    --to=rkrcmar@redhat.com \
    --cc=kernellwp@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).