Messages in this thread Patch in this message | | | From | Paolo Bonzini <> | Subject | Re: [PATCH v3 11/12] KVM: SVM: call avic_vcpu_load/avic_vcpu_put when enabling/disabling AVIC | Date | Tue, 3 Aug 2021 11:00:10 +0200 |
| |
On 02/08/21 20:33, Maxim Levitsky wrote: > @@ -651,6 +673,7 @@ void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu) > } > vmcb_mark_dirty(vmcb, VMCB_AVIC); > > + __avic_set_running(vcpu, activated); > svm_set_pi_irte_mode(vcpu, activated); > } >
I'd rather have calls to avic_vcpu_load/avic_vcpu_put directly inside the "if (activated)", and leaving avic_set_running to its current implementation. That way you don't need __avic_set_running (which is a confusing name, because it does more than just setting the running bit).
> void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu) > { > + bool activate; > + > if (!lapic_in_kernel(vcpu)) > return; > > mutex_lock(&vcpu->kvm->arch.apicv_update_lock); > > - vcpu->arch.apicv_active = kvm_apicv_activated(vcpu->kvm); > + activate = kvm_apicv_activated(vcpu->kvm); > + if (vcpu->arch.apicv_active == activate) > + goto out; > + > + vcpu->arch.apicv_active = activate; > kvm_apic_update_apicv(vcpu); > static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu); > > @@ -9257,6 +9263,7 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu) > if (!vcpu->arch.apicv_active) > kvm_make_request(KVM_REQ_EVENT, vcpu); > > +out: > mutex_unlock(&vcpu->kvm->arch.apicv_update_lock); > } > EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
Should this be a separate patch?
As an aside, we have
static inline bool kvm_vcpu_apicv_active(struct kvm_vcpu *vcpu) { return vcpu->arch.apic && vcpu->arch.apicv_active; }
but really vcpu->arch.apicv_active should never be true if vcpu->arch.apic is. So it should be possible to change this to "return vcpu->arch.apicv_active" with a comment that the serialization between apicv_inhibit_reasons and apicv_active happens via apicv_update_lock.
Thanks, Paolo
| |