[PATCH] KVM: SVM: refactor avic VM ID allocation

[Radim KrÄmÃÅ]

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 <dvlasenk@xxxxxxxxxx>
> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> Cc: pbonzini@xxxxxxxxxx
> Cc: rkrcmar@xxxxxxxxxx
> Cc: tglx@xxxxxxxxxxxxx
> Cc: mingo@xxxxxxxxxx
> Cc: hpa@xxxxxxxxx
> Cc: x86@xxxxxxxxxx
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---

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ÃÅ <rkrcmar@xxxxxxxxxx>
---
 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