Date: Tue, 13 Dec 2016 16:16:21 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Roman Kagan <rkagan@virtuozzo.com>,
        Denis Plotnikov <dplotnikov@virtuozzo.com>, den@virtuozzo.com,
        pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] KVM: x86: avoid redundant REQ_EVENT
Message-ID: <20161213151620.GB2293@potion>
References: <20161212140223.277077-1-dplotnikov@virtuozzo.com>
 <20161212162943.GA2237@potion>
 <20161212202034.GA937@rkaganip.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20161212202034.GA937@rkaganip.lan>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2661
Lines: 56

2016-12-12 23:20+0300, 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"?

Sounds good.

I though about some other solutions and it looks like we actually don't
need KVM_REQ_EVENT almost anywhere when using TPR shadow:
If we didn't get the TPR VM exit, then we know that there is no
interrupt that can be delivered after applying the change from TPR.
(In other words, if we had a queued interrupt that got unmasked by the
 change, then it should have trigerred the TPR threshold VM exit.)

And KVM must change TPR without TPR shadow, so we would learn about the
change earlier, then.

I think we could only trigger KVM_REQ_EVENT when lowering TPR without
TPR shadow.  Your patch is definitely safer. :)