Reviewing this patch and the next one together.
On 02/08/21 20:33, Maxim Levitsky wrote:
+static int avic_alloc_access_page(struct kvm *kvm)
{
void __user *ret;
int r = 0;
mutex_lock(&kvm->slots_lock);
+
+ if (kvm->arch.apic_access_memslot_enabled)
goto out;
This variable is overloaded between "is access enabled" and "is the
memslot allocated". I think you should check
kvm->arch.apicv_inhibit_reasons instead in kvm_faultin_pfn.
+ if (!activate)
+ kvm_zap_gfn_range(kvm, gpa_to_gfn(APIC_DEFAULT_PHYS_BASE),
+ gpa_to_gfn(APIC_DEFAULT_PHYS_BASE + PAGE_SIZE));
+
Off by one, the last argument of kvm_zap_gfn_range is inclusive:
Also, checking "activate" is a bit ugly when we have "new" available as
well. Yes, they are the same if !!old != !!new, but we care about the
global state, not the single bit.
Putting everything together, this could become something like
trace_kvm_apicv_update_request(activate, bit);
if (!!old != !!new) {
/*
* Kick all CPUs out of guest mode. When
* kvm_vcpu_update_apicv succeeds in taking
* apicv_update_lock, it will see the
* new apicv_inhibit_reasons that we set below.
*/
kvm_make_all_cpus_request(kvm, KVM_REQ_APICV_UPDATE);
if (new) {
unsigned long gfn = gpa_to_gfn(APIC_DEFAULT_PHYS_BASE);
kvm_zap_gfn_range(kvm, gfn, gfn);
}
}
kvm->arch.apicv_inhibit_reasons = new;
mutex_unlock(&kvm->arch.apicv_update_lock);
Paolo
