From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liran Alon Subject: Re: [PATCH v3 10/11] KVM: nVMX: Wake halted L2 on nested posted-interrupt Date: Wed, 27 Dec 2017 14:52:42 +0200 Message-ID: <5A43979A.5050400@ORACLE.COM> References: <1514131983-24305-1-git-send-email-liran.alon@oracle.com> <1514131983-24305-11-git-send-email-liran.alon@oracle.com> <5A438B8C.3080109@ORACLE.COM> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: jmattson@google.com, wanpeng.li@hotmail.com, idan.brown@ORACLE.COM, Liam Merwick , Konrad Rzeszutek Wilk To: Paolo Bonzini , rkrcmar@redhat.com, kvm@vger.kernel.org Return-path: Received: from userp2130.oracle.com ([156.151.31.86]:37285 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750919AbdL0Mwy (ORCPT ); Wed, 27 Dec 2017 07:52:54 -0500 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 27/12/17 14:27, Paolo Bonzini wrote: > On 27/12/2017 13:01, Liran Alon wrote: >> I think that current patch series makes more sense than reordering them. >> >> This is because the patch you are suggesting will actually, as a >> side-effect, fix the bug that is fixed in commit "KVM: nVMX: Deliver >> missed nested-PI notification-vector via self-IPI while interrupts >> disabled". This is actually the same as the first patch I suggested for >> fixing the bug in v1 of this series: >> https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_1510252040-2D5609-2D1-2Dgit-2Dsend-2Demail-2Dliran.alon-40oracle.com&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=Yu_AVwFNqMnoYNj9fVZO9MHUe9-GwD5Ar2DHsbmBus4&s=VxOx9jL1jMWvw4WWvn3RCeRe2GwzTIlaBgZ5nJpch8Q&e= >> >> After I submitted v1, Radim mentioned that even though the fix is >> correct, it is awkward and error-prone. I agreed and therefore we have >> written another fix using self-IPI. > > It's not exactly the same. My suggested version checks the return value > of kvm_vcpu_trigger_posted_interrupt and calls kvm_vcpu_kick. > >> Therefore, to preserve each commit fixing only one problem, I prefer to >> keep the orders of commits as they are now. > > It's true that it would hide the bug but it makes no sense to say you > are fixing a bug in patch 9, and then fix that bug in the next patch. There are 2 separate bugs here. One is fixed in patch 9 and the other in patches 10 & 11. The race-condition described in commit message of patch 9 is fixed in patch 9. It is fixed because we get rid of the dependency on KVM_REQ_EVENT. If the target vCPU thread passes the check for pending kvm requests, it means it is already running with interrupts disabled and therefore the physical IPI of POSTED_INTR_NESTED_VECTOR will be received in guest which will process nested posted-interrupts correctly. If guest will exit because of another external-interrupt before the physical IPI will be received, on next VMEntry, code will note there are pending nested posted-interrupts and re-trigger physical IPI of POSTED_INTR_NESTED_VECTOR. Therefore, nested posted-interrupts will eventually be processed in guest. This is in contrast to what happens before patch 9 where L2 guest will continue running until next time KVM_REQ_EVENT will be consumed. Therefore, bug is indeed fixed in patch 9. Patch 10 fixes another bug: Consider the case target vCPU thread is actually blocked because it executed HLT and it was not intercepted by L1. When some other L1 vCPU post to it a posted-interrupt, it should wake the target vCPU (as it is HLT in L2) and process nested posted-interrupt. Patches 10 & 11 makes sure to fix this issue. > > So I think this patch should definitely go first (in fact I'm tempted to > install it together with patches 1-4...) and the rest should be done as > a broader cleanup of vmx->nested.pi_pending. I agree with Radim that as > of now it's awkward and needs some work. I was about to reply about > that so I'll leave it for another email. Note that this series is not a clean-up of vmx->nested.pi_pending. That variable remained untouched. It is used as a signal to know that a nested posted-interrupt notification-vector has indeed been sent to vCPU. This is required to not process nested posted-interrupts in case vmx.nested.pi_desc->control ON bit is set but no nested posted-interrupt notification-vector was actually sent by L1. This is in contrast to how KVM process posted-interrupts where it only treats vmx.pi_desc->control ON bit as indicator if it should process posted-interrupts. Instead, this series just fixes a bunch of unrelated nVMX issues which all revolve around posted-interrupts. Regards, -Liran > > Paolo >