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
