From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: Heads-up: Nested VMX got broken by commit Date: Tue, 06 Mar 2012 14:07:56 +0200 Message-ID: <4F55FE1C.1050600@redhat.com> References: <20120306083322.GA24844@fermat.math.technion.ac.il> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, gleb@redhat.com To: "Nadav Har'El" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:12902 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030402Ab2CFMIG (ORCPT ); Tue, 6 Mar 2012 07:08:06 -0500 In-Reply-To: <20120306083322.GA24844@fermat.math.technion.ac.il> Sender: kvm-owner@vger.kernel.org List-ID: On 03/06/2012 10:33 AM, Nadav Har'El wrote: > Hi, > > I just noticed that Nested VMX got broken (at least in my tests) by commit > 46199f33c29533e7ad2a7d2128dc30175d1d4157. > > The specific change causing the problem was: > > @@ -2220,7 +2216,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 > msr_index, u64 data) > break; > msr = find_msr_entry(vmx, msr_index); > if (msr) { > - vmx_load_host_state(vmx); > msr->data = data; > break; > } > > And if anyone wants a quick workaround to making nested VMX work again, > returning this line fixes the problem. > > I'm still trying to figure out why this line, which indeed seems unrelated > and unnecessary, is necessary for the correct functioning of nested VMX. > My (unsubstantiated) guess is that it isn't that it is actually necessary > in this point - it's just that it does something that should have been more > properly done in another place, but I've yet to figure out exactly what. > I'll send a patch when I have this figured out. If anybody else has any > guess, I'd love to hear. A side effect of vmx_load_host_state() is that it schedules a vmx_save_host_state() for the next entry. Maybe you're missing something there. Ah, it's probably for (i = 0; i < vmx->save_nmsrs; ++i) kvm_set_shared_msr(vmx->guest_msrs[i].index, vmx->guest_msrs[i].data, vmx->guest_msrs[i].mask); What happened is that nvmx workloads write a lot to MSR_*STAR and friends, and these are stored both in vmx->guest_msrs[] and on the cpu. Without the call to vmx_save_host_state(), the values were written only to memory, not to the cpu register, and the guest saw a corrupted value. I'll post a patch soon. -- error compiling committee.c: too many arguments to function