From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751309AbeAPRig (ORCPT + 1 other); Tue, 16 Jan 2018 12:38:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60862 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750812AbeAPRif (ORCPT ); Tue, 16 Jan 2018 12:38:35 -0500 Subject: Re: [PATCH 1/3] KVM: VMX: introduce alloc_loaded_vmcs To: David Hildenbrand , linux-kernel@vger.kernel.org, kvm@vger.kernel.org References: <1516121545-30847-1-git-send-email-pbonzini@redhat.com> <1516121545-30847-2-git-send-email-pbonzini@redhat.com> From: Paolo Bonzini Message-ID: Date: Tue, 16 Jan 2018 18:38:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 16 Jan 2018 17:38:35 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On 16/01/2018 18:27, David Hildenbrand wrote: > On 16.01.2018 17:52, Paolo Bonzini wrote: >> Group together the calls to alloc_vmcs and loaded_vmcs_init. Soon we'll also >> allocate an MSR bitmap there. >> >> Signed-off-by: Paolo Bonzini >> --- >> arch/x86/kvm/vmx.c | 36 ++++++++++++++++++++++-------------- >> 1 file changed, 22 insertions(+), 14 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 00d76292a5d2..d81fd4a87f2b 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -3799,11 +3799,6 @@ static struct vmcs *alloc_vmcs_cpu(int cpu) >> return vmcs; >> } >> >> -static struct vmcs *alloc_vmcs(void) >> -{ >> - return alloc_vmcs_cpu(raw_smp_processor_id()); >> -} >> - >> static void free_vmcs(struct vmcs *vmcs) >> { >> free_pages((unsigned long)vmcs, vmcs_config.order); >> @@ -3835,6 +3830,22 @@ static void vmx_nested_free_vmcs02(struct vcpu_vmx *vmx) >> free_loaded_vmcs(loaded_vmcs); >> } >> >> +static struct vmcs *alloc_vmcs(void) >> +{ >> + return alloc_vmcs_cpu(raw_smp_processor_id()); >> +} >> + > > don't understand why that was moved :) Because later alloc_loaded_vmcs needs to call free_loaded_vmcs so I wanted to put all free before alloc. :) >> +static int alloc_loaded_vmcs(struct loaded_vmcs *loaded_vmcs) > > maybe s/alloc_/init_/ would be a better name? > >> +{ >> + loaded_vmcs->vmcs = alloc_vmcs(); >> + if (!loaded_vmcs->vmcs) >> + return -ENOMEM; >> + >> + loaded_vmcs->shadow_vmcs = NULL; >> + loaded_vmcs_init(loaded_vmcs); >> + return 0; >> +} >> + >> static void free_kvm_area(void) >> { >> int cpu; >> @@ -7168,12 +7179,11 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu) >> { >> struct vcpu_vmx *vmx = to_vmx(vcpu); >> struct vmcs *shadow_vmcs; >> + int r; >> >> - vmx->nested.vmcs02.vmcs = alloc_vmcs(); >> - vmx->nested.vmcs02.shadow_vmcs = NULL; >> - if (!vmx->nested.vmcs02.vmcs) >> + r = alloc_loaded_vmcs(&vmx->nested.vmcs02); >> + if (r < 0) >> goto out_vmcs02; > > local variable r is not needed > > if (alloc_ ...) Yes, but it's less readable. Paolo