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
