On 15/08/2017 22:12, Radim Krčmář wrote:
> 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 ...
I think even the case where 2^16 ids are used deserves a medal. Why
don't we just make the bitmap 8 KiB and call it a day? :)
Paolo
> 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;
>
