Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751005AbdCOXg7 (ORCPT ); Wed, 15 Mar 2017 19:36:59 -0400 Received: from mail-qk0-f194.google.com ([209.85.220.194]:36014 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750771AbdCOXfi (ORCPT ); Wed, 15 Mar 2017 19:35:38 -0400 Date: Wed, 15 Mar 2017 19:35:34 -0400 From: "Gabriel L. Somlo" To: "Michael S. Tsirkin" Cc: linux-kernel@vger.kernel.org, Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , Jonathan Corbet , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Joerg Roedel , kvm@vger.kernel.org, linux-doc@vger.kernel.org Subject: Re: [PATCH v5 untested] kvm: better MWAIT emulation for guests Message-ID: <20170315233534.GG2239@HEDWIG.INI.CMU.EDU> References: <1489612895-12799-1-git-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1489612895-12799-1-git-send-email-mst@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9184 Lines: 226 On Wed, Mar 15, 2017 at 11:22:18PM +0200, Michael S. Tsirkin wrote: > Guests running Mac OS 5, 6, and 7 (Leopard through Lion) have a problem: > unless explicitly provided with kernel command line argument > "idlehalt=0" they'd implicitly assume MONITOR and MWAIT availability, > without checking CPUID. > > We currently emulate that as a NOP but on VMX we can do better: let > guest stop the CPU until timer, IPI or memory change. CPU will be busy > but that isn't any worse than a NOP emulation. > > Note that mwait within guests is not the same as on real hardware > because halt causes an exit while mwait doesn't. For this reason it > might not be a good idea to use the regular MWAIT flag in CPUID to > signal this capability. Add a flag in the hypervisor leaf instead. > > Additionally, we add a capability for QEMU - e.g. if it knows there's an > isolated CPU dedicated for the VCPU it can set the standard MWAIT flag > to improve guest behaviour. Same behavior (on the mac pro 1,1 running F22 with custom-compiled kernel from kvm git master, plus this patch on top). The OS X 10.7 kernel hangs (or at least progresses extremely slowly) on boot, does not bring up guest graphical interface within the first 10 minutes that I waited for it. That, in contrast with the default nop-based emulation where the guest comes up within 30 seconds. I will run another round of tests on a newer Mac (4-year-old macbook air) and report back tomorrow. Going off on a tangent, why would encouraging otherwise well-behaved guests (like linux ones, for example) to use MWAIT be desirable to begin with ? Is it a matter of minimizing the overhead associated with exiting and re-entering L1 ? Because if so, AFAIR staying inside L1 and running guest-mode MWAIT in a tight loop will actually waste the host CPU without the opportunity to yield to some other L0 thread. Sorry if I fell into the middle of an ongoing conversation on this and missed most of the relevant context, in which case please feel free to ignore me... :) Thanks, --G > > Reported-by: "Gabriel L. Somlo" > Signed-off-by: Michael S. Tsirkin > --- > > This is for Gabriel's testing only. A bit rushed so untested. > > Documentation/virtual/kvm/api.txt | 9 +++++++++ > Documentation/virtual/kvm/cpuid.txt | 6 ++++++ > arch/x86/include/uapi/asm/kvm_para.h | 1 + > arch/x86/kvm/cpuid.c | 3 +++ > arch/x86/kvm/svm.c | 2 -- > arch/x86/kvm/vmx.c | 6 ++++-- > arch/x86/kvm/x86.c | 3 +++ > arch/x86/kvm/x86.h | 28 ++++++++++++++++++++++++++++ > include/uapi/linux/kvm.h | 1 + > 9 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > index 3c248f7..6ee2e43 100644 > --- a/Documentation/virtual/kvm/api.txt > +++ b/Documentation/virtual/kvm/api.txt > @@ -4147,3 +4147,12 @@ This capability, if KVM_CHECK_EXTENSION indicates that it is > available, means that that the kernel can support guests using the > hashed page table MMU defined in Power ISA V3.00 (as implemented in > the POWER9 processor), including in-memory segment tables. > + > +8.5 KVM_CAP_X86_GUEST_MWAIT > + > +Architectures: x86 > + > +This capability indicates that guest using memory monotoring instructions > +(MWAIT/MWAITX) to stop the virtual CPU will not cause a VM exit. As such time > +spent while virtual CPU is halted in this way will then be accounted for as > +guest running time on the host (as opposed to e.g. HLT). > diff --git a/Documentation/virtual/kvm/cpuid.txt b/Documentation/virtual/kvm/cpuid.txt > index 3c65feb..04c201c 100644 > --- a/Documentation/virtual/kvm/cpuid.txt > +++ b/Documentation/virtual/kvm/cpuid.txt > @@ -54,6 +54,12 @@ KVM_FEATURE_PV_UNHALT || 7 || guest checks this feature bit > || || before enabling paravirtualized > || || spinlock support. > ------------------------------------------------------------------------------ > +KVM_FEATURE_MWAIT || 8 || guest can use monitor/mwait > + || || to halt the VCPU without exits, > + || || time spent while halted in this > + || || way is accounted for on host as > + || || VCPU run time. > +------------------------------------------------------------------------------ > KVM_FEATURE_CLOCKSOURCE_STABLE_BIT || 24 || host will warn if no guest-side > || || per-cpu warps are expected in > || || kvmclock. > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > index cff0bb6..9cc77a7 100644 > --- a/arch/x86/include/uapi/asm/kvm_para.h > +++ b/arch/x86/include/uapi/asm/kvm_para.h > @@ -24,6 +24,7 @@ > #define KVM_FEATURE_STEAL_TIME 5 > #define KVM_FEATURE_PV_EOI 6 > #define KVM_FEATURE_PV_UNHALT 7 > +#define KVM_FEATURE_MWAIT 8 > > /* The last 8 bits are used to indicate how to interpret the flags field > * in pvclock structure. If no bits are set, all flags are ignored. > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index efde6cc..5638102 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -594,6 +594,9 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > if (sched_info_on()) > entry->eax |= (1 << KVM_FEATURE_STEAL_TIME); > > + if (kvm_mwait_in_guest()) > + entry->eax |= (1 << KVM_FEATURE_MWAIT); > + > entry->ebx = 0; > entry->ecx = 0; > entry->edx = 0; > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index d1efe2c..18e53bc 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -1198,8 +1198,6 @@ static void init_vmcb(struct vcpu_svm *svm) > set_intercept(svm, INTERCEPT_CLGI); > set_intercept(svm, INTERCEPT_SKINIT); > set_intercept(svm, INTERCEPT_WBINVD); > - set_intercept(svm, INTERCEPT_MONITOR); > - set_intercept(svm, INTERCEPT_MWAIT); > set_intercept(svm, INTERCEPT_XSETBV); > > control->iopm_base_pa = iopm_base; > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 98e82ee..ea0c96a 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -3547,11 +3547,13 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf) > CPU_BASED_USE_IO_BITMAPS | > CPU_BASED_MOV_DR_EXITING | > CPU_BASED_USE_TSC_OFFSETING | > - CPU_BASED_MWAIT_EXITING | > - CPU_BASED_MONITOR_EXITING | > CPU_BASED_INVLPG_EXITING | > CPU_BASED_RDPMC_EXITING; > > + if (!kvm_mwait_in_guest()) > + min |= CPU_BASED_MWAIT_EXITING | > + CPU_BASED_MONITOR_EXITING; > + > opt = CPU_BASED_TPR_SHADOW | > CPU_BASED_USE_MSR_BITMAPS | > CPU_BASED_ACTIVATE_SECONDARY_CONTROLS; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 1faf620..8c74fff 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2684,6 +2684,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) > case KVM_CAP_ADJUST_CLOCK: > r = KVM_CLOCK_TSC_STABLE; > break; > + case KVM_CAP_X86_GUEST_MWAIT: > + r = kvm_mwait_in_guest(); > + break; > case KVM_CAP_X86_SMM: > /* SMBASE is usually relocated above 1M on modern chipsets, > * and SMM handlers might indeed rely on 4G segment limits, > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h > index e8ff3e4..a2d8964 100644 > --- a/arch/x86/kvm/x86.h > +++ b/arch/x86/kvm/x86.h > @@ -1,6 +1,8 @@ > #ifndef ARCH_X86_KVM_X86_H > #define ARCH_X86_KVM_X86_H > > +#include > +#include > #include > #include > #include "kvm_cache_regs.h" > @@ -212,4 +214,30 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec) > __rem; \ > }) > > +static inline bool kvm_mwait_in_guest(void) > +{ > + unsigned int eax, ebx, ecx, edx; > + > + if (!cpu_has(&boot_cpu_data, X86_FEATURE_MWAIT)) > + return false; > + > + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) > + return false; > + > + /* > + * Intel CPUs without CPUID5_ECX_INTERRUPT_BREAK are problematic as > + * they would allow guest to stop the CPU completely by disabling > + * interrupts then invoking MWAIT. > + */ > + if (boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF) > + return false; > + > + cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx); > + > + if (!(ecx & CPUID5_ECX_INTERRUPT_BREAK)) > + return false; > + > + return true; > +} > + > #endif > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index f51d508..8b6bc06 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -883,6 +883,7 @@ struct kvm_ppc_resize_hpt { > #define KVM_CAP_PPC_MMU_RADIX 134 > #define KVM_CAP_PPC_MMU_HASH_V3 135 > #define KVM_CAP_IMMEDIATE_EXIT 136 > +#define KVM_CAP_X86_GUEST_MWAIT 137 > > #ifdef KVM_CAP_IRQ_ROUTING > > -- > MST