On Thu, 2021-06-24 at 11:07 +0300, Maxim Levitsky wrote:
> On Wed, 2021-06-23 at 23:50 +0200, Paolo Bonzini wrote:
> > On 23/06/21 13:29, Maxim Levitsky wrote:
> > > + kvm_block_guest_entries(kvm);
> > > +
> > > trace_kvm_apicv_update_request(activate, bit);
> > > if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
> > > static_call(kvm_x86_pre_update_apicv_exec_ctrl)(kvm, activate);
> > > @@ -9243,6 +9245,8 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> > > except = kvm_get_running_vcpu();
> > > kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
> > > except);
> > > +
> > > + kvm_allow_guest_entries(kvm);
> >
> > Doesn't this cause a busy loop during synchronize_rcu?
>
> Hi,
>
> If you mean busy loop on other vcpus, then the answer is sadly yes.
> Other option is to use a mutex, which is what I did in a former
> version of this patch, but at last minute I decided that this
> way it was done in this patch would be simplier.
> AVIC updates don't happen often.
> Also with a request, the KVM_REQ_APICV_UPDATE can be handled in parallel,
> while mutex enforces unneeded mutual execution of it.
>
>
> > It should be
> > possible to request the vmexit of other CPUs from
> > avic_update_access_page, and do a lock/unlock of kvm->slots_lock to wait
> > for the memslot to be updated.
>
> This would still keep the race. The other vCPUs must not enter the guest mode
> from the moment the memslot update was started and until the KVM_REQ_APICV_UPDATE
> is raised.
>
> If I were to do any kind of synchronization in avic_update_access_page, then I will
> have to drop the lock/request there, and from this point and till the common code
> raises the KVM_REQ_APICV_UPDATE there is a possibility of a vCPU reentering the
> guest mode without updating its AVIC.
>
>
> Here is an older version of this patch that does use mutex instead.
> Please let me know if you prefer it.
>
> I copy pasted it here, thus its likely not to apply as my email client
> probably destroys whitespace.
>
> Thanks for the review,
> Best regards,
> Maxim Levitsky
>
>
> --
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fdc6b8a4348f..b7dc7fd7b63d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9183,11 +9183,8 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
> kvm_make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
> }
>
> -void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
> +void __kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
> {
> - if (!lapic_in_kernel(vcpu))
> - return;
> -
> vcpu->arch.apicv_active = kvm_apicv_activated(vcpu->kvm);
> kvm_apic_update_apicv(vcpu);
> static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu);
> @@ -9201,6 +9198,16 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
> if (!vcpu->arch.apicv_active)
> kvm_make_request(KVM_REQ_EVENT, vcpu);
> }
> +
> +void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
> +{
> + if (!lapic_in_kernel(vcpu))
> + return;
> +
> + mutex_lock(&vcpu->kvm->apicv_update_lock);
> + __kvm_vcpu_update_apicv(vcpu);
> + mutex_unlock(&vcpu->kvm->apicv_update_lock);
> +}
> EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
>
> /*
> @@ -9213,30 +9220,26 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
> void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> {
> struct kvm_vcpu *except;
> - unsigned long old, new, expected;
> + unsigned long old, new;
>
> if (!kvm_x86_ops.check_apicv_inhibit_reasons ||
> !static_call(kvm_x86_check_apicv_inhibit_reasons)(bit))
> return;
>
> - old = READ_ONCE(kvm->arch.apicv_inhibit_reasons);
> - do {
> - expected = new = old;
> - if (activate)
> - __clear_bit(bit, &new);
> - else
> - __set_bit(bit, &new);
> - if (new == old)
> - break;
> - old = cmpxchg(&kvm->arch.apicv_inhibit_reasons, expected, new);
> - } while (old != expected);
> + mutex_lock(&kvm->apicv_update_lock);
> +
> + old = new = kvm->arch.apicv_inhibit_reasons;
> + if (activate)
> + __clear_bit(bit, &new);
> + else
> + __set_bit(bit, &new);
> +
> + WRITE_ONCE(kvm->arch.apicv_inhibit_reasons, new);
>
> if (!!old == !!new)
> - return;
> + goto out;
>
> trace_kvm_apicv_update_request(activate, bit);
> - if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
> - static_call(kvm_x86_pre_update_apicv_exec_ctrl)(kvm, activate);
>
> /*
> * Sending request to update APICV for all other vcpus,
> @@ -9244,10 +9247,24 @@ void kvm_request_apicv_update(struct kvm *kvm, bool activate, ulong bit)
> * waiting for another #VMEXIT to handle the request.
> */
> except = kvm_get_running_vcpu();
> +
> + /*
> + * on SVM, raising the KVM_REQ_APICV_UPDATE request while holding the
> + * apicv_update_lock ensures that we kick all vCPUs out of the
> + * guest mode and let them wait until the AVIC memslot update
> + * has completed.
> + */
> +
> kvm_make_all_cpus_request_except(kvm, KVM_REQ_APICV_UPDATE,
> except);
> +
> + if (kvm_x86_ops.pre_update_apicv_exec_ctrl)
> + static_call(kvm_x86_pre_update_apicv_exec_ctrl)(kvm, activate);
> +
> if (except)
> - kvm_vcpu_update_apicv(except);
> + __kvm_vcpu_update_apicv(except);
> +out:
> + mutex_unlock(&kvm->apicv_update_lock);
> }
> EXPORT_SYMBOL_GPL(kvm_request_apicv_update);
>
> @@ -9454,8 +9471,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> */
> if (kvm_check_request(KVM_REQ_HV_STIMER, vcpu))
> kvm_hv_process_stimers(vcpu);
> - if (kvm_check_request(KVM_REQ_APICV_UPDATE, vcpu))
> + if (kvm_check_request(KVM_REQ_APICV_UPDATE, vcpu)) {
> + srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> kvm_vcpu_update_apicv(vcpu);
> + vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
> + }
> if (kvm_check_request(KVM_REQ_APF_READY, vcpu))
> kvm_check_async_pf_completion(vcpu);
> if (kvm_check_request(KVM_REQ_MSR_FILTER_CHANGED, vcpu))
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 37cbb56ccd09..0364d35d43dc 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -524,6 +524,7 @@ struct kvm {
> #endif /* KVM_HAVE_MMU_RWLOCK */
>
> struct mutex slots_lock;
> + struct mutex apicv_update_lock;
>
> /*
> * Protects the arch-specific fields of struct kvm_memory_slots in
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ed4d1581d502..ba5d5d9ebc64 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -943,6 +943,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
> mutex_init(&kvm->irq_lock);
> mutex_init(&kvm->slots_lock);
> mutex_init(&kvm->slots_arch_lock);
> + mutex_init(&kvm->apicv_update_lock);
> INIT_LIST_HEAD(&kvm->devices);
>
> BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
>
>
>
> > (As an aside, I'd like to get rid of KVM_REQ_MCLOCK_IN_PROGRESS in 5.15...).
> >
> > Paolo
> >
Hi!
Any update? should I use a lock for this?
Best regards,
Maxim Levitsky
