From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Nadav Har'El" Subject: Re: [PATCH 18/31] nVMX: Implement VMLAUNCH and VMRESUME Date: Tue, 24 May 2011 12:45:16 +0300 Message-ID: <20110524094516.GC136@fermat.math.technion.ac.il> References: <1305575004-nyh@il.ibm.com> <201105161953.p4GJr8Jo001858@rice.haifa.ibm.com> <625BA99ED14B2D499DC4E29D8138F1505C9BFA3443@shsmsx502.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "kvm@vger.kernel.org" , "gleb@redhat.com" , "avi@redhat.com" To: "Tian, Kevin" Return-path: Received: from mailgw12.technion.ac.il ([132.68.225.12]:12475 "EHLO mailgw12.technion.ac.il" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755787Ab1EXJpU (ORCPT ); Tue, 24 May 2011 05:45:20 -0400 Content-Disposition: inline In-Reply-To: <625BA99ED14B2D499DC4E29D8138F1505C9BFA3443@shsmsx502.ccr.corp.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, May 24, 2011, Tian, Kevin wrote about "RE: [PATCH 18/31] nVMX: Implement VMLAUNCH and VMRESUME": > > + /* > > + * Switch from L1's VMCS (vmcs01), to L2's VMCS (vmcs02). Remember > > + * vmcs01, on which CPU it was last loaded, and whether it was launched > > + * (we need all these values next time we will use L1). Then recall > > + * these values from the last time vmcs02 was used. > > + */ > > + saved_vmcs02 = nested_get_current_vmcs02(vmx); > > + if (!saved_vmcs02) > > + return -ENOMEM; > > + > > + cpu = get_cpu(); > > + vmx->nested.saved_vmcs01.vmcs = vmx->vmcs; > > + vmx->nested.saved_vmcs01.cpu = vcpu->cpu; > > + vmx->nested.saved_vmcs01.launched = vmx->launched; > > + vmx->vmcs = saved_vmcs02->vmcs; > > + vcpu->cpu = saved_vmcs02->cpu; > > this may be another valid reason for your check on cpu_online in your > latest [08/31] local_vcpus_link fix, since cpu may be offlined after > this assignment. :-) I believe that wrapping this part of the code with get_cpu()/put_cpu() protected me from these kinds of race conditions. By the way, please note that this part of the code was changed after my latest loaded_vmcs overhaul. It now looks like this: vmcs02 = nested_get_current_vmcs02(vmx); if (!vmcs02) return -ENOMEM; cpu = get_cpu(); vmx->loaded_vmcs = vmcs02; vmx_vcpu_put(vcpu); vmx_vcpu_load(vcpu, cpu); vcpu->cpu = cpu; put_cpu(); (if Avi gives me the green light, I'll send the entire, up-to-date, patch set again). > > + vmcs12->launch_state = 1; > > + > > + prepare_vmcs02(vcpu, vmcs12); > > Since prepare_vmcs may fail, add a check here and move launch_state > assignment after its success? prepare_vmcs02() cannot fail. All the checks that need to be done on vmcs12 are done before calling it, in nested_vmx_run(). Currently, there's a single case where prepare_vmcs02 "fails" when it fails to access apic_access_addr memory. This is wrong - the check should have been done earlier. I'll fix that, and make prepare_vmcs02() void. -- Nadav Har'El | Tuesday, May 24 2011, 20 Iyyar 5771 nyh@math.technion.ac.il |----------------------------------------- Phone +972-523-790466, ICQ 13349191 |It's no use crying over spilt milk -- it http://nadav.harel.org.il |only makes it salty for the cat.