Re: [PATCH v1] KVM: x86: avoid redundant REQ_EVENT

[Roman Kagan]

On Mon, Dec 12, 2016 at 05:29:43PM +0100, Radim KrÄmÃÅ wrote:
> 2016-12-12 17:02+0300, Denis Plotnikov:
> > When processing KVM_REQ_EVENT, apic_update_ppr is called which may set
> > KVM_REQ_EVENT again if the recalculated value of PPR becomes smaller
> > than the previous one. This results in cancelling the guest entry and
> > reiterating in vcpu_enter_guest.
> > 
> > However this is unnecessary because at this point KVM_REQ_EVENT is
> > already being processed and there are no other changes in the lapic
> > that may require full-fledged state recalculation.
> > 
> > This situation is often hit on systems with TPR shadow, where the
> > TPR can be updated by the guest without a vmexit, so that the first
> > apic_update_ppr to notice it is exactly the one called while
> > processing KVM_REQ_EVENT.
> > 
> > To avoid it, introduce a parameter in apic_update_ppr allowing to
> > suppress setting of KVM_REQ_EVENT, and use it on the paths called from
> > KVM_REQ_EVENT processing.
> 
> We also call:
> 
>   kvm_cpu_get_interrupt() in nested_vmx_vmexit()
>     - that path is intended without KVM_REQ_EVENT
>   kvm_cpu_has_interrupt() in vmx_check_nested_events(),
>     - I think it does no harm
>   kvm_cpu_has_interrupt() in kvm_vcpu_has_events()
>   kvm_cpu_has_interrupt() in kvm_vcpu_ready_for_interrupt_injection()
>     - both seem safe as we should not have an interrupt between TPR
>       threshold and the new PPR value, so the KVM_REQ_EVENT was useless.
> 
> I would prefer we made sure that only callers from KVM_REQ_EVENT used
> the function we are changing -- it is really easy to make a hard-to-find
> mistake in interrupt delivery.

Indeed, that was my concern as well.  How about introducing a parameter
to kvm_cpu_{has,get}_interrupt() with the same meaning, and pass it down
to apic_update_ppr()?  Then only the call sites under KVM_REQ_EVENT
processing would pass "false" there, and the rest would remain with
"true"?

Roman.