Re: [PATCH 5/6] KVM: x86: do not scan IRR twice on APICv vmentry

[Radim KrÄmÃÅ]

2017-02-08 15:10+0100, Paolo Bonzini:
> 
> 
> On 07/02/2017 21:19, Radim KrÄmÃÅ wrote:
> > 2016-12-19 17:17+0100, Paolo Bonzini:
> >> Calls to apic_find_highest_irr are scanning IRR twice, once
> >> in vmx_sync_pir_from_irr and once in apic_search_irr.  Change
> >> sync_pir_from_irr to get the new maximum IRR from kvm_apic_update_irr;
> >> now that it does the computation, it can also do the RVI write.
> >>
> >> In order to avoid complications in svm.c, make the callback optional.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> >> ---
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> @@ -8734,20 +8736,24 @@ static void vmx_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
> >>  	}
> >>  }
> >>  
> >> -static void vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu)
> >> +static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu)
> >>  {
> >>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> >> +	int max_irr;
> >>  
> >> -	if (!pi_test_on(&vmx->pi_desc))
> >> -		return;
> >> -
> >> -	pi_clear_on(&vmx->pi_desc);
> >> -	/*
> >> -	 * IOMMU can write to PIR.ON, so the barrier matters even on UP.
> >> -	 * But on x86 this is just a compiler barrier anyway.
> >> -	 */
> >> -	smp_mb__after_atomic();
> >> -	kvm_apic_update_irr(vcpu, vmx->pi_desc.pir);
> >> +	if (vcpu->arch.apicv_active && pi_test_on(&vmx->pi_desc)) {
> >> +		pi_clear_on(&vmx->pi_desc);
> >> +		/*
> >> +		 * IOMMU can write to PIR.ON, so the barrier matters even on UP.
> >> +		 * But on x86 this is just a compiler barrier anyway.
> >> +		 */
> >> +		smp_mb__after_atomic();
> >> +		max_irr = kvm_apic_update_irr(vcpu, vmx->pi_desc.pir);
> >> +	} else {
> >> +		max_irr = kvm_lapic_find_highest_irr(vcpu);
> >> +	}
> >> +	vmx_hwapic_irr_update(vcpu, max_irr);
> > 
> > Btw. a v1 discussion revolved about the need to have
> > vmx_hwapic_irr_update() here when the maximal IRR should always be in
> > RVI, and, uh, I didn't follow up (negligible attention span) ...
> >
> > There is one place where that doesn't hold: we don't update RVI after a
> > EXTERNAL_INTERRUPT nested VM exit without VM_EXIT_ACK_INTR_ON_EXIT, but
> > IRR has likely changed.  Isn't that the problem?
> 
> I'm not sure... there shouldn't be any issue with missed RVI updates in
> this series, since it does

> 	if (kvm_lapic_enabled(vcpu)) {
> 		/*
> 		 * This handles the case where a posted interrupt was
> 		 * notified with kvm_vcpu_kick.
> 		 */
> 		if (kvm_x86_ops->sync_pir_to_irr)
> 			kvm_x86_ops->sync_pir_to_irr(vcpu);
> 	}
> 
> on every VM entry (and kvm_lapic_find_highest_irr inside the callback).
> That is not something I really like, but it's no worse than what was
> there before
> 
>                 if (vcpu->arch.apicv_active)
>                         kvm_x86_ops->hwapic_irr_update(vcpu,
>                                 kvm_lapic_find_highest_irr(vcpu));
>         }
> 
> and obviously better than going unnecessarily through KVM_REQ_EVENT
> processing.

I agree.  I wanted to point out that we could get rid of the RVI update
on VM entry when PI.ON is clear, like you originally planned (because
RVI should already be the max IRR).

And the reason why v1 didn't work out was likely in missing the RVI
update on nested VM exit, which forced v2 to have this work-around.
.