Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753478AbdCOSQl (ORCPT ); Wed, 15 Mar 2017 14:16:41 -0400 Received: from mail-qt0-f194.google.com ([209.85.216.194]:36118 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751310AbdCOSQT (ORCPT ); Wed, 15 Mar 2017 14:16:19 -0400 Date: Wed, 15 Mar 2017 14:14:26 -0400 From: "Gabriel L. Somlo" To: Radim =?utf-8?B?S3LEjW3DocWZ?= Cc: "Michael S. Tsirkin" , linux-kernel@vger.kernel.org, Paolo Bonzini , Jonathan Corbet , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, kvm@vger.kernel.org, linux-doc@vger.kernel.org Subject: Re: [PATCH] kvm: better MWAIT emulation for guests Message-ID: <20170315181425.GA2239@HEDWIG.INI.CMU.EDU> References: <1489098555-23856-1-git-send-email-mst@redhat.com> <20170313154618.GA4547@potion> <20170313180046-mutt-send-email-mst@kernel.org> <20170313193910.GB4547@potion> <20170313215016-mutt-send-email-mst@kernel.org> <20170313214355.GC4547@potion> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170313214355.GC4547@potion> 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: 3641 Lines: 87 Michael, I tested this on OS X 10.7 (Lion), the last version that doesn't check CPUID for MWAIT support. I used the latest kvm from git://git.kernel.org/pub/scm/virt/kvm/kvm.git first as-is, then with your v2 MWAIT patch applied. Single-(V)CPU guest works as expected (but then again, single-vcpu guests worked even back when I tried emulating MWAIT the same as HLT). When I try starting a SMP guest (with "-smp 4,cores=2"), the guest OS hangs after generating some output in text/verbose boot mode -- I gave up waiting for it after about 5 minutes. Works fine before your patch, which leads me to suspect that, as I feared, MWAIT doesn't wake immediately upon another VCPU writing to the MONITOR-ed memory location. Tangentially, I remember back in the days of OS X 10.7, the alternative to exiting guest mode and emulating MWAIT and MONITOR as NOPs was to allow them both to run in guest mode. While poorly documented by Intel at the time, MWAIT at L>0 effectively behaves as a NOP (i.e., doesn't actually put the physical core into low-power mode, because doing that would allow a guest to effectively DOS the host hardware). Given how unusual it is for a guest to use MONITOR/MWAIT in the first place, what's wrong with leaving it all as is (i.e., emulated as NOP)? Thanks, --Gabriel On Mon, Mar 13, 2017 at 10:43:55PM +0100, Radim Krčmář wrote: > 2017-03-13 22:03+0200, Michael S. Tsirkin: > > On Mon, Mar 13, 2017 at 08:39:11PM +0100, Radim Krčmář wrote: > > > 2017-03-13 18:08+0200, Michael S. Tsirkin: > > > > On Mon, Mar 13, 2017 at 04:46:20PM +0100, Radim Krčmář wrote: > >> >> What about keeping just the last hunk to improve OS X, for now? > >> > > >> > IMHO if we have a new functionality we are better of creating > >> > some way for guests to discover it is there. > >> > > >> > Do we really have to argue about a single bit in HV leaf? > >> > What harm does it do? > >> > >> It adds code to both guest and hosts and needs documentation ... > >> The bit is acceptable. I just see no point in having it when there > >> already is a detection mechanism for mwait. > > > > We don't want to use that standard detection mechanism IMHO at least > > not in all cases. > > Enabling mwait by default would make sense if the guest OS monitored its > steal time and disabled mwait when it detects that it is not the main > user of the CPU, because mwait then hurts the host as well as the guest. > > This would warrant some kind of paravirt as we still wouldn't want to > have standard mwait by default. My problem is that the paravirt flag > alone is not enough for a normal mwait use on Intel. > > >> In any case, this patch should also remove VM exits under SVM > > > > AMD does not have MWAIT AFAIK. In any case, I don't see > > why can't SVM be a separate patch. > > AMD just doesn't have MWAIT hints. (AMD has even MWAIT in userspace and > MWAITX, but they are not supported by KVM.) > > The separate patch would have to be part of the same series as we don't > want to have vendor-specific detection, so I'd just remove these two in > the same patch to simplify handling: > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index d1efe2c62b3f..18e53bc185d6 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; > > Thanks.