Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752598AbdHOUMG (ORCPT ); Tue, 15 Aug 2017 16:12:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58346 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751460AbdHOUME (ORCPT ); Tue, 15 Aug 2017 16:12:04 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 54455267C5 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=rkrcmar@redhat.com Date: Tue, 15 Aug 2017 22:12:00 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Denys Vlasenko , Suravee Suthikulpanit Cc: Joerg Roedel , pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] KVM: SVM: refactor avic VM ID allocation Message-ID: <20170815201159.GC5975@flask> References: <20170811201158.24715-1-dvlasenk@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170811201158.24715-1-dvlasenk@redhat.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 15 Aug 2017 20:12:04 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4557 Lines: 158 2017-08-11 22:11+0200, Denys Vlasenko: > With lightly tweaked defconfig: > > text data bss dec hex filename > 11259661 5109408 2981888 19350957 12745ad vmlinux.before > 11259661 5109408 884736 17253805 10745ad vmlinux.after > > Only compile-tested. > > Signed-off-by: Denys Vlasenko > Cc: Joerg Roedel > Cc: pbonzini@redhat.com > Cc: rkrcmar@redhat.com > Cc: tglx@linutronix.de > Cc: mingo@redhat.com > Cc: hpa@zytor.com > Cc: x86@kernel.org > Cc: kvm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- Ah, I suspected my todo wasn't this short; thanks for the patch! > @@ -1468,6 +1433,22 @@ static int avic_vm_init(struct kvm *kvm) > clear_page(page_address(l_page)); > > spin_lock_irqsave(&svm_vm_data_hash_lock, flags); > + again: > + vm_id = next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK; > + if (vm_id == 0) { /* id is 1-based, zero is not okay */ Suravee, did the reserved zero mean something? > + next_vm_id_wrapped = 1; > + goto again; > + } > + /* Is it still in use? Only possible if wrapped at least once */ > + if (next_vm_id_wrapped) { > + hash_for_each_possible(svm_vm_data_hash, ka, hnode, vm_id) { > + struct kvm *k2 = container_of(ka, struct kvm, arch); > + struct kvm_arch *vd2 = &k2->arch; > + if (vd2->avic_vm_id == vm_id) > + goto again; Although hitting the case where all 2^24 ids are already used is practically impossible, I don't like the loose end ... What about applying something like the following on top? ---8<--- Subject: [PATCH] KVM: SVM: refactor avic VM ID allocation We missed protection in case someone deserving a medal allocated 2^24 VMs and got a deadlock instead. I think it is nicer without the goto even if we droppped the error handling. Signed-off-by: Radim Krčmář --- arch/x86/kvm/svm.c | 58 +++++++++++++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index c21b49b5ee96..4d9ee8d76db3 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -985,8 +985,6 @@ static void disable_nmi_singlestep(struct vcpu_svm *svm) */ #define SVM_VM_DATA_HASH_BITS 8 static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS); -static u32 next_vm_id = 0; -static bool next_vm_id_wrapped = 0; static DEFINE_SPINLOCK(svm_vm_data_hash_lock); /* Note: @@ -1385,6 +1383,38 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu) return 0; } +static bool avic_vm_id_is_used(u32 vm_id) +{ + struct kvm_arch *ka; + + hash_for_each_possible(svm_vm_data_hash, ka, hnode, vm_id) + if (ka->avic_vm_id == vm_id) + return true; + + return false; +} + +static u32 avic_get_next_vm_id(void) +{ + static u32 next_vm_id = 0; + static bool next_vm_id_wrapped = false; + unsigned i; + + for (i = 0; i < AVIC_VM_ID_NR; i++) { + next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK; + + if (next_vm_id == 0) { /* id is 1-based, zero is not okay */ + next_vm_id = 1; + next_vm_id_wrapped = true; + } + + if (!next_vm_id_wrapped || !avic_vm_id_is_used(next_vm_id)) + return next_vm_id; + } + + return 0; +} + static void avic_vm_destroy(struct kvm *kvm) { unsigned long flags; @@ -1410,8 +1440,6 @@ static int avic_vm_init(struct kvm *kvm) struct kvm_arch *vm_data = &kvm->arch; struct page *p_page; struct page *l_page; - struct kvm_arch *ka; - u32 vm_id; if (!avic) return 0; @@ -1432,28 +1460,18 @@ static int avic_vm_init(struct kvm *kvm) vm_data->avic_logical_id_table_page = l_page; clear_page(page_address(l_page)); + err = -EAGAIN; spin_lock_irqsave(&svm_vm_data_hash_lock, flags); - again: - vm_id = next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK; - if (vm_id == 0) { /* id is 1-based, zero is not okay */ - next_vm_id_wrapped = 1; - goto again; - } - /* Is it still in use? Only possible if wrapped at least once */ - if (next_vm_id_wrapped) { - hash_for_each_possible(svm_vm_data_hash, ka, hnode, vm_id) { - struct kvm *k2 = container_of(ka, struct kvm, arch); - struct kvm_arch *vd2 = &k2->arch; - if (vd2->avic_vm_id == vm_id) - goto again; - } - } - vm_data->avic_vm_id = vm_id; + vm_data->avic_vm_id = avic_get_next_vm_id(); + if (!vm_data->avic_vm_id) + goto unlock; hash_add(svm_vm_data_hash, &vm_data->hnode, vm_data->avic_vm_id); spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags); return 0; +unlock: + spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags); free_avic: avic_vm_destroy(kvm); return err; -- 2.13.3