From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bandan Das Subject: Re: [PATCH v2] KVM: nVMX: Track active shadow VMCSs Date: Wed, 27 Jul 2016 17:53:05 -0400 Message-ID: References: <1469211903-21781-1-git-send-email-jmattson@google.com> <20160727204533.GA29177@potion> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Jim Mattson , kvm@vger.kernel.org, pbonzini@redhat.com, dmatlack@google.com, pfeiner@google.com To: Radim =?utf-8?B?S3LEjW3DocWZ?= Return-path: Received: from mx1.redhat.com ([209.132.183.28]:54158 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932557AbcG0VxH convert rfc822-to-8bit (ORCPT ); Wed, 27 Jul 2016 17:53:07 -0400 Sender: kvm-owner@vger.kernel.org List-ID: Radim Kr=C4=8Dm=C3=A1=C5=99 writes: > 2016-07-22 11:25-0700, Jim Mattson: >> If a shadow VMCS is referenced by the VMCS link pointer in the >> current VMCS, then VM-entry makes the shadow VMCS active on the >> current processor. No VMCS should ever be active on more than one >> processor. If a VMCS is to be migrated from one processor to >> another, software should execute a VMCLEAR for the VMCS on the >> first processor before the VMCS is made active on the second >> processor. >>=20 >> We already keep track of ordinary VMCSs that are made active by >> VMPTRLD. Extend that tracking to handle shadow VMCSs that are >> made active by VM-entry to their parents. >>=20 >> Signed-off-by: Jim Mattson >> --- >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> @@ -2113,6 +2113,15 @@ static void vmx_vcpu_pi_load(struct kvm_vcpu = *vcpu, int cpu) >> new.control) !=3D old.control); >> } >> =20 >> +static void record_loaded_vmcs(struct loaded_vmcs *loaded_vmcs, int= cpu) >> +{ >> + if (loaded_vmcs->vmcs) { >> + list_add(&loaded_vmcs->loaded_vmcss_on_cpu_link, >> + &per_cpu(loaded_vmcss_on_cpu, cpu)); >> + loaded_vmcs->cpu =3D cpu; >> + } >> +} >> + >> /* >> * Switches to specified vcpu, until a matching vcpu_put(), but ass= umes >> * vcpu mutex is already taken. >> @@ -2124,15 +2133,13 @@ static void vmx_vcpu_load(struct kvm_vcpu *v= cpu, int cpu) >> =20 >> if (!vmm_exclusive) >> kvm_cpu_vmxon(phys_addr); >> - else if (vmx->loaded_vmcs->cpu !=3D cpu) >> + else if (vmx->loaded_vmcs->cpu !=3D cpu) { >> loaded_vmcs_clear(vmx->loaded_vmcs); >> + if (vmx->nested.current_shadow_vmcs.vmcs) >> + loaded_vmcs_clear(&vmx->nested.current_shadow_vmcs); > > loaded_vmcs_clear() uses expensive IPI, so would want to do both in o= ne > call in future patches as they are always active on the same CPU. Maybe just move the check for an active shadow vmcs to loaded_vmcs_clea= r and clear it there unconditionally. > Another possible complication is marking current_shadow_vmcs as activ= e > on a cpu only after a successful vmlaunch. > (We don't seem to ever vmptrld shadow vmcs explicitly.) > >> + } > > Incorrect whitespace for indentation. > >> =20 >> if (per_cpu(current_vmcs, cpu) !=3D vmx->loaded_vmcs->vmcs) { >> - per_cpu(current_vmcs, cpu) =3D vmx->loaded_vmcs->vmcs; >> - vmcs_load(vmx->loaded_vmcs->vmcs); >> - } >> - >> - if (vmx->loaded_vmcs->cpu !=3D cpu) { > > This condition is nice for performance because a non-current vmcs is > usually already active on the same CPU, so we skip all the code below= =2E > > (This is the only thing that has to be fixed as it regresses non-nest= ed, > the rest are mostly ideas for simplifications.) I think he wanted to make sure to call vmcs_load after the call to crash_disable_local_vmclear() but that should be possible without removing the check. Bandan >> struct desc_ptr *gdt =3D this_cpu_ptr(&host_gdt); >> unsigned long sysenter_esp; >> =20 >> @@ -2147,11 +2154,15 @@ static void vmx_vcpu_load(struct kvm_vcpu *v= cpu, int cpu) >> */ >> smp_rmb(); >> =20 >> - list_add(&vmx->loaded_vmcs->loaded_vmcss_on_cpu_link, >> - &per_cpu(loaded_vmcss_on_cpu, cpu)); >> + record_loaded_vmcs(vmx->loaded_vmcs, cpu); > > Adding and an element to a list multiple times seems invalid, which t= he > condition was also guarding. > >> + record_loaded_vmcs(&vmx->nested.current_shadow_vmcs, cpu); > > current_shadow_vmcs is always active on the same cpu as loaded_vmcs .= =2E. > I think we could skip the list and just clear current_shadow_vmcs whe= n > clearing its loaded_vmcs. > >> crash_enable_local_vmclear(cpu); >> local_irq_enable(); >> =20 >> + per_cpu(current_vmcs, cpu) =3D vmx->loaded_vmcs->vmcs; >> + vmcs_load(vmx->loaded_vmcs->vmcs); >> + >> /* >> * Linux uses per-cpu TSS and GDT, so set these when switching >> * processors. >> @@ -2161,8 +2172,6 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcp= u, int cpu) >> =20 >> rdmsrl(MSR_IA32_SYSENTER_ESP, sysenter_esp); >> vmcs_writel(HOST_IA32_SYSENTER_ESP, sysenter_esp); /* 22.2.3 */ >> - >> - vmx->loaded_vmcs->cpu =3D cpu; >> } >> =20 >> /* Setup TSC multiplier */ >> @@ -6812,6 +6821,34 @@ static int nested_vmx_check_vmptr(struct kvm_= vcpu *vcpu, int exit_reason, >> return 0; >> } >> =20 >> +static int setup_shadow_vmcs(struct vcpu_vmx *vmx) >> +{ >> + struct vmcs *shadow_vmcs; >> + int cpu; >> + >> + shadow_vmcs =3D alloc_vmcs(); >> + if (!shadow_vmcs) >> + return -ENOMEM; >> + >> + /* mark vmcs as shadow */ >> + shadow_vmcs->revision_id |=3D (1u << 31); >> + /* init shadow vmcs */ >> + vmx->nested.current_shadow_vmcs.vmcs =3D shadow_vmcs; >> + loaded_vmcs_init(&vmx->nested.current_shadow_vmcs); >> + >> + cpu =3D get_cpu(); >> + local_irq_disable(); >> + crash_disable_local_vmclear(cpu); >> + >> + record_loaded_vmcs(&vmx->nested.current_shadow_vmcs, cpu); > > This could be avoided if we assumed that shadow vmcs is always active= on > the same vcpu. The assumption looks rock-solid, because shadow vmcs = is > activated on vmlaunch and its linking vmcs must be active (and curren= t) > on the same CPU. > >> + >> + crash_enable_local_vmclear(cpu); >> + local_irq_enable(); >> + put_cpu(); >> + >> + return 0; >> +} >> + >> /* >> * Emulate the VMXON instruction. >> * Currently, we just remember that VMX is active, and do not save = or even >> @@ -6867,14 +6903,9 @@ static int handle_vmon(struct kvm_vcpu *vcpu) >> } >> =20 >> if (enable_shadow_vmcs) { >> - shadow_vmcs =3D alloc_vmcs(); >> - if (!shadow_vmcs) >> - return -ENOMEM; >> - /* mark vmcs as shadow */ >> - shadow_vmcs->revision_id |=3D (1u << 31); >> - /* init shadow vmcs */ >> - vmcs_clear(shadow_vmcs); >> - vmx->nested.current_shadow_vmcs =3D shadow_vmcs; >> + int ret =3D setup_shadow_vmcs(vmx); >> + if (ret < 0) >> + return ret; >> } >> =20 >> INIT_LIST_HEAD(&(vmx->nested.vmcs02_pool));