From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH] KVM: nVMX: Fix races when sending nested PI while dest enters/leaves L2 Date: Fri, 10 Nov 2017 22:24:48 +0100 Message-ID: <20171110212447.GA2029@flask> References: <1510252040-5609-1-git-send-email-liran.alon@oracle.com> <20171110180616.GE15561@flask> <5A060EA0.7060507@ORACLE.COM> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: Paolo Bonzini , kvm@vger.kernel.org, jmattson@google.com, wanpeng.li@hotmail.com, idan.brown@ORACLE.COM, Krish Sadhukhan To: Liran Alon Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43664 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750942AbdKJVYv (ORCPT ); Fri, 10 Nov 2017 16:24:51 -0500 Content-Disposition: inline In-Reply-To: <5A060EA0.7060507@ORACLE.COM> Sender: kvm-owner@vger.kernel.org List-ID: 2017-11-10 22:40+0200, Liran Alon: > On 10/11/17 20:06, Radim Krčmář wrote: > > 2017-11-10 18:06+0100, Paolo Bonzini: > > > On 09/11/2017 19:27, Liran Alon wrote: > > > /* > > > * If a posted intr is not recognized by hardware, > > > * we will accomplish it in the next vmentry. > > > */ > > > vmx->nested.pi_pending = true; > > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > > kvm_vcpu_kick(vcpu); > > > } > > > > > > See the comments around the setting of IN_GUEST_MODE, introduced by > > > commit b95234c84004 ("kvm: x86: do not use KVM_REQ_EVENT for APICv > > > interrupt injection", 2017-02-15). > > > > > > Even though nested PI must use KVM_REQ_EVENT, the reasoning behind the > > > ordering of cli and vcpu->mode = IN_GUEST_MODE should hold in both cases. > > > > I think the fix is correct, but the code is using very awkward > > synchronization through vmx->nested.pi_pending and slaps KVM_REQ_EVENT > > on top. > > If we checked vmx->nested.pi_pending after cli, we could get rid of > > KVM_REQ_EVENT and if we want to support nested posted interrupts from > > PCI devices, we should explicitly check for pi.on during VM entry. > I like this suggestion. If I understand correctly, these are the changes I > will do for v2 of this patch: > > 1. I Will change vmx_deliver_nested_posted_interrupt() to first set > pi_pending=true and then call kvm_vcpu_trigger_posted_interrupt(). No need > to set KVM_REQ_EVENT and no need to check > kvm_vcpu_trigger_posted_interrupt() ret-val. > > 2. I will create a kvm_x86_ops->complete_nested_posted_interrupt() that will > be called from vcpu_enter_guest() just after sync_pir_to_irr() is called but > outside the "if" for kvm_lapic_enabled(). I think that we can't have nested posted interrupts without kvm_lapic_enabled() or kvm_vcpu_apicv_active(), so we could add nested handling to sync_pir_to_irr and save x86 op. vmx_complete_nested_posted_interrupt will need some changes as it is not called with disabled interrupts -- kmap() and probably also nested_mark_vmcs12_pages_dirty() should be done outside; I guess that other code is already be taking care of both, but better check. > 3. I will remove call to vmx_complete_nested_posted_interrupt() from > vmx_check_nested_events(). That call-site had a bug anyway that I fixed in > another patch-series I posted here. This change will make that patch (not > the series) redundant. See redundant patch here: > https://marc.info/?l=kvm&m=150989090007281&w=2 I'll ignore that patch, > I will prepare a v2 patch by this suggestion and send it. thanks.