From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 08/31] nVMX: Fix local_vcpus_link handling Date: Mon, 23 May 2011 18:49:17 +0300 Message-ID: <4DDA81FD.5050203@redhat.com> References: <20110517131918.GA3809@amt.cnet> <4DD27998.1040105@redhat.com> <20110517143532.GA2490@fermat.math.technion.ac.il> <4DD2902C.9050403@redhat.com> <20110517181132.GA16262@fermat.math.technion.ac.il> <20110517184336.GA10394@amt.cnet> <20110517193030.GA21656@fermat.math.technion.ac.il> <20110517195253.GB11065@amt.cnet> <20110518055236.GA1230@fermat.math.technion.ac.il> <20110518120801.GA9176@amt.cnet> <20110522085732.GB1116@fermat.math.technion.ac.il> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org, gleb@redhat.com, "Roedel, Joerg" To: "Nadav Har'El" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:18574 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755291Ab1EWPt0 (ORCPT ); Mon, 23 May 2011 11:49:26 -0400 In-Reply-To: <20110522085732.GB1116@fermat.math.technion.ac.il> Sender: kvm-owner@vger.kernel.org List-ID: On 05/22/2011 11:57 AM, Nadav Har'El wrote: > Hi Avi and Marcelo, here is a the new first patch to the nvmx patch set, > which overhauls the handling of vmcss on cpus, as you asked. > > As you guessed, the nested entry and exit code becomes much simpler and > cleaner, with the whole VMCS switching code on entry, for example, reduced > to: > cpu = get_cpu(); > vmx->loaded_vmcs = vmcs02; > vmx_vcpu_put(vcpu); > vmx_vcpu_load(vcpu, cpu); > vcpu->cpu = cpu; > put_cpu(); That's wonderful, it indicates the code is much better integrated. Perhaps later we can refine it to have separate _load and _put for host-related and guest-related parts (I think they already exist in the code, except they are always called together), but that is an optimization, and not the most important one by far. > You can apply this patch separately from the rest of the patch set, if you > wish. I'm sending just this one, like you asked - and can send the rest of > the patches when you ask me to. > > > Subject: [PATCH 01/31] nVMX: Keep list of loaded VMCSs, instead of vcpus. > > In VMX, before we bring down a CPU we must VMCLEAR all VMCSs loaded on it > because (at least in theory) the processor might not have written all of its > content back to memory. Since a patch from June 26, 2008, this is done using > a per-cpu "vcpus_on_cpu" linked list of vcpus loaded on each CPU. > > The problem is that with nested VMX, we no longer have the concept of a > vcpu being loaded on a cpu: A vcpu has multiple VMCSs (one for L1, a pool for > L2s), and each of those may be have been last loaded on a different cpu. > > So instead of linking the vcpus, we link the VMCSs, using a new structure > loaded_vmcs. This structure contains the VMCS, and the information pertaining > to its loading on a specific cpu (namely, the cpu number, and whether it > was already launched on this cpu once). In nested we will also use the same > structure to hold L2 VMCSs, and vmx->loaded_vmcs is a pointer to the > currently active VMCS. > > --- .before/arch/x86/kvm/x86.c 2011-05-22 11:41:57.000000000 +0300 > +++ .after/arch/x86/kvm/x86.c 2011-05-22 11:41:57.000000000 +0300 > @@ -2119,7 +2119,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu > if (need_emulate_wbinvd(vcpu)) { > if (kvm_x86_ops->has_wbinvd_exit()) > cpumask_set_cpu(cpu, vcpu->arch.wbinvd_dirty_mask); > - else if (vcpu->cpu != -1&& vcpu->cpu != cpu) > + else if (vcpu->cpu != -1&& vcpu->cpu != cpu > + && cpu_online(vcpu->cpu)) > smp_call_function_single(vcpu->cpu, > wbinvd_ipi, NULL, 1); > } Is this a necessary part of this patch? Or an semi-related bugfix? I think that it can't actually trigger before this patch due to luck. svm doesn't clear vcpu->cpu on cpu offline, but on the other hand it ->has_wbinvd_exit(). Joerg, is if (unlikely(cpu != vcpu->cpu)) { svm->asid_generation = 0; mark_all_dirty(svm->vmcb); } susceptible to cpu offline/online? > @@ -971,22 +992,22 @@ static void vmx_vcpu_load(struct kvm_vcp > > if (!vmm_exclusive) > kvm_cpu_vmxon(phys_addr); > - else if (vcpu->cpu != cpu) > - vcpu_clear(vmx); > + else if (vmx->loaded_vmcs->cpu != cpu) > + loaded_vmcs_clear(vmx->loaded_vmcs); > > - if (per_cpu(current_vmcs, cpu) != vmx->vmcs) { > - per_cpu(current_vmcs, cpu) = vmx->vmcs; > - vmcs_load(vmx->vmcs); > + if (per_cpu(current_vmcs, cpu) != vmx->loaded_vmcs->vmcs) { > + per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs; > + vmcs_load(vmx->loaded_vmcs->vmcs); > } > > - if (vcpu->cpu != cpu) { > + if (vmx->loaded_vmcs->cpu != cpu) { > struct desc_ptr *gdt =&__get_cpu_var(host_gdt); > unsigned long sysenter_esp; > > kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu); > local_irq_disable(); > - list_add(&vmx->local_vcpus_link, > - &per_cpu(vcpus_on_cpu, cpu)); > + list_add(&vmx->loaded_vmcs->loaded_vmcss_on_cpu_link, > + &per_cpu(loaded_vmcss_on_cpu, cpu)); > local_irq_enable(); > > /* > @@ -999,13 +1020,15 @@ static void vmx_vcpu_load(struct kvm_vcp > rdmsrl(MSR_IA32_SYSENTER_ESP, sysenter_esp); > vmcs_writel(HOST_IA32_SYSENTER_ESP, sysenter_esp); /* 22.2.3 */ > } > + vmx->loaded_vmcs->cpu = cpu; This should be within the if () block. > @@ -4344,11 +4369,13 @@ static struct kvm_vcpu *vmx_create_vcpu( > goto uninit_vcpu; > } > > - vmx->vmcs = alloc_vmcs(); > - if (!vmx->vmcs) > + vmx->loaded_vmcs =&vmx->vmcs01; > + vmx->loaded_vmcs->vmcs = alloc_vmcs(); > + if (!vmx->loaded_vmcs->vmcs) > goto free_msrs; > - > - vmcs_init(vmx->vmcs); > + vmcs_init(vmx->loaded_vmcs->vmcs); > + vmx->loaded_vmcs->cpu = -1; > + vmx->loaded_vmcs->launched = 0; Perhaps a loaded_vmcs_init() to encapsulate initialization of these three fields, you'll probably reuse it later. Please repost separately after the fix, I'd like to apply it before the rest of the series. (regarding interrupts, I think we can do that work post-merge. But I'd like to see Kevin's comments addressed) -- error compiling committee.c: too many arguments to function