> From: Nadav Har'El [mailto:nyh@xxxxxxxxxxxxxxxxxxx] > Sent: Tuesday, May 24, 2011 5:45 PM > > 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. you're right. > > 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). Generally your new patch looks good. > > > > + 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. > then no problem, as long as you keep this choice clear. Thanks Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
