From patchwork Mon Dec 18 08:45:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Hildenbrand X-Patchwork-Id: 10118469 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 1758360390 for ; Mon, 18 Dec 2017 08:45:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 066252888C for ; Mon, 18 Dec 2017 08:45:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id EE946288A8; Mon, 18 Dec 2017 08:45:48 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 822512888C for ; Mon, 18 Dec 2017 08:45:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758145AbdLRIpg (ORCPT ); Mon, 18 Dec 2017 03:45:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40936 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757935AbdLRIpf (ORCPT ); Mon, 18 Dec 2017 03:45:35 -0500 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 365BB5F7B1; Mon, 18 Dec 2017 08:45:35 +0000 (UTC) Received: from [10.36.116.251] (ovpn-116-251.ams2.redhat.com [10.36.116.251]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1954E7A3D8; Mon, 18 Dec 2017 08:45:30 +0000 (UTC) Subject: Re: [PATCH] KVM: vmx: speed up MSR bitmap merge To: Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org Cc: bsd@redhat.com, jmattson@google.com References: <1513171828-5130-1-git-send-email-pbonzini@redhat.com> From: David Hildenbrand Organization: Red Hat GmbH Message-ID: Date: Mon, 18 Dec 2017 09:45:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <1513171828-5130-1-git-send-email-pbonzini@redhat.com> Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 18 Dec 2017 08:45:35 +0000 (UTC) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 13.12.2017 14:30, Paolo Bonzini wrote: > The bulk of the MSR bitmap is either immutable, or can be copied from > the L1 bitmap. By initializing it at VMXON time, and copying the mutable > parts one long at a time on vmentry (rather than one bit), about 4000 > clock cycles (30%) can be saved on a nested VMLAUNCH/VMRESUME. > > The resulting for loop only has four iterations, so it is cheap enough > to reinitialize the MSR write bitmaps on every iteration, and it makes > the code simpler. > > Suggested-by: Jim Mattson > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/vmx.c | 57 ++++++++++++++++++++++++++++-------------------------- > 1 file changed, 30 insertions(+), 27 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 1458cb52de68..ee214b4112af 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -5217,11 +5217,6 @@ static void nested_vmx_disable_intercept_for_msr(unsigned long *msr_bitmap_l1, > { > int f = sizeof(unsigned long); > > - if (!cpu_has_vmx_msr_bitmap()) { > - WARN_ON(1); > - return; > - } > - > /* > * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals > * have the write-low and read-high bitmap offsets the wrong way round. > @@ -7493,6 +7488,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu) > (unsigned long *)__get_free_page(GFP_KERNEL); > if (!vmx->nested.msr_bitmap) > goto out_msr_bitmap; > + memset(vmx->nested.msr_bitmap, 0xff, PAGE_SIZE); > } > > vmx->nested.cached_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL); > @@ -10325,36 +10321,43 @@ static inline bool nested_vmx_merge_msr_bitmap(struct kvm_vcpu *vcpu, > /* This shortcut is ok because we support only x2APIC MSRs so far. */ > if (!nested_cpu_has_virt_x2apic_mode(vmcs12)) > return false; > + if (WARN_ON_ONCE(!cpu_has_vmx_msr_bitmap())) > + return false; IMHO it would be nicer to always call nested_vmx_merge_msr_bitmap() and make calling code less ugly: } @@ -10318,6 +10314,10 @@ static inline bool nested_vmx_merge_msr_bitmap(struct kvm_vcpu *vcpu, unsigned long *msr_bitmap_l1; unsigned long *msr_bitmap_l0 = to_vmx(vcpu)->nested.msr_bitmap; + if (!cpu_has_vmx_msr_bitmap()) + return false; + if (!nested_cpu_has(vmcs12, CPU_BASED_USE_MSR_BITMAPS)) + return false; /* This shortcut is ok because we support only x2APIC MSRs so far. */ if (!nested_cpu_has_virt_x2apic_mode(vmcs12)) return false; > > page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->msr_bitmap); > if (is_error_page(page)) > return false; > - msr_bitmap_l1 = (unsigned long *)kmap(page); > > - memset(msr_bitmap_l0, 0xff, PAGE_SIZE); > + msr_bitmap_l1 = (unsigned long *)kmap(page); Wouldn't it be easier to simply set everything to 0xff as before and then only handle the one special case where you don't do that? e.g. the complete else part would be gone. > + if (nested_cpu_has_apic_reg_virt(vmcs12)) { > + /* Disable read intercept for all MSRs between 0x800 and 0x8ff. */ > + for (msr = 0x800; msr <= 0x8ff; msr += BITS_PER_LONG) { > + unsigned word = msr / BITS_PER_LONG; > + msr_bitmap_l0[word] = msr_bitmap_l1[word]; > + msr_bitmap_l0[word + (0x800 / sizeof(long))] = ~0; > + } > + } else { > + for (msr = 0x800; msr <= 0x8ff; msr += BITS_PER_LONG) { > + unsigned word = msr / BITS_PER_LONG; > + msr_bitmap_l0[word] = ~0; > + msr_bitmap_l0[word + (0x800 / sizeof(long))] = ~0; > + } > + } > > - if (nested_cpu_has_virt_x2apic_mode(vmcs12)) { > - if (nested_cpu_has_apic_reg_virt(vmcs12)) > - for (msr = 0x800; msr <= 0x8ff; msr++) > - nested_vmx_disable_intercept_for_msr( > - msr_bitmap_l1, msr_bitmap_l0, > - msr, MSR_TYPE_R); > + nested_vmx_disable_intercept_for_msr( > + msr_bitmap_l1, msr_bitmap_l0, > + APIC_BASE_MSR + (APIC_TASKPRI >> 4), > + MSR_TYPE_W); I'd vote for indenting the parameters properly (even though we exceed 80 chars by 1 then :) ) > > + if (nested_cpu_has_vid(vmcs12)) { > nested_vmx_disable_intercept_for_msr( > - msr_bitmap_l1, msr_bitmap_l0, > - APIC_BASE_MSR + (APIC_TASKPRI >> 4), > - MSR_TYPE_R | MSR_TYPE_W); > - > - if (nested_cpu_has_vid(vmcs12)) { > - nested_vmx_disable_intercept_for_msr( > - msr_bitmap_l1, msr_bitmap_l0, > - APIC_BASE_MSR + (APIC_EOI >> 4), > - MSR_TYPE_W); > - nested_vmx_disable_intercept_for_msr( > - msr_bitmap_l1, msr_bitmap_l0, > - APIC_BASE_MSR + (APIC_SELF_IPI >> 4), > - MSR_TYPE_W); > - } > + msr_bitmap_l1, msr_bitmap_l0, > + APIC_BASE_MSR + (APIC_EOI >> 4), > + MSR_TYPE_W); > + nested_vmx_disable_intercept_for_msr( > + msr_bitmap_l1, msr_bitmap_l0, > + APIC_BASE_MSR + (APIC_SELF_IPI >> 4), > + MSR_TYPE_W); > } > kunmap(page); > kvm_release_page_clean(page); > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index ee214b4112af..d4f06fc643ae 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10238,11 +10238,7 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu, (unsigned long)(vmcs12->posted_intr_desc_addr & (PAGE_SIZE - 1))); } - if (cpu_has_vmx_msr_bitmap() && - nested_cpu_has(vmcs12, CPU_BASED_USE_MSR_BITMAPS) && - nested_vmx_merge_msr_bitmap(vcpu, vmcs12)) - ; - else + if (!nested_vmx_merge_msr_bitmap(vcpu, vmcs12)) vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL, CPU_BASED_USE_MSR_BITMAPS);