Date: Mon, 12 Dec 2016 23:20:35 +0300
From: Roman Kagan <rkagan@virtuozzo.com>
To: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
CC: 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: <20161212202034.GA937@rkaganip.lan>
Mail-Followup-To: Roman Kagan <rkagan@virtuozzo.com>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Denis Plotnikov <dplotnikov@virtuozzo.com>, den@virtuozzo.com,
        pbonzini@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20161212140223.277077-1-dplotnikov@virtuozzo.com>
 <20161212162943.GA2237@potion>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20161212162943.GA2237@potion>
User-Agent: Mutt/1.7.1 (2016-10-04)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2016 20:20:41.8406 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0802MB2467
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1957
Lines: 42

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.