From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liran Alon Subject: Re: [PATCH v2 1/5] KVM: nVMX: Remove pi_pending as signal to process nested posted-interrupts Date: Mon, 11 Dec 2017 16:20:23 -0800 (PST) Message-ID: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: , , , , To: , , Return-path: Received: from aserp2130.oracle.com ([141.146.126.79]:54893 "EHLO aserp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751281AbdLLAU2 (ORCPT ); Mon, 11 Dec 2017 19:20:28 -0500 Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: On 12/05/17 10:16, Liran Alon wrote:=20 > When L1 wants to send a posted-interrupt to another L1 CPU which runs > L2, it does the following operations: > 1. Sets the relevant bit in vmx->nested.pi_desc PIR. > 2. Set ON bit in vmx->nested.pi_desc->control field. > 3. Sends an IPI to dest L1 CPU by writing the the posted-interrupt > notification-vector into the LAPIC ICR. >=20 > Step (3) will exit to L0 on APIC_WRITE which will eventually reach > vmx_deliver_nested_posted_interrupt(). If dest L0 CPU is in guest, > then a physical IPI of notification-vector will be sent which will > trigger evaluation of posted-interrupts and clear > vmx->nested.pi_desc->control ON bit. > Otherwise (or if dest L0 CPU exited from guest just before sending the > physical IPI), the nested-posted-interrupts will be evaluated on next > vmentry by vmx_complete_nested_posted_interrupt(). >=20 > In order for vmx_complete_nested_posted_interrupt() to know if it > should do any work, the flag vmx->nested.pi_pending was used which > was set by vmx_deliver_nested_posted_interrupt(). >=20 > However, this seems unnecessary because if posted-interrupts was not > processed yet, vmx->nested.pi_desc->control ON bit should still be > set. Therefore, it should suffice to use it in order to know if work > should be done. >=20 > Signed-off-by: Liran Alon > Reviewed-by: Nikita Leshenko > Reviewed-by: Krish Sadhukhan > Signed-off-by: Krish Sadhukhan > Signed-off-by: Konrad Rzeszutek Wilk > --- > arch/x86/kvm/vmx.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) >=20 > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 714a0673ec3c..f5074ec5701b 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -453,7 +453,6 @@ struct nested_vmx { > =09struct page *virtual_apic_page; > =09struct page *pi_desc_page; > =09struct pi_desc *pi_desc; > -=09bool pi_pending; > =09u16 posted_intr_nv; > =20 > =09unsigned long *msr_bitmap; > @@ -5050,10 +5049,9 @@ static void vmx_complete_nested_posted_interrupt(s= truct kvm_vcpu *vcpu) > =09void *vapic_page; > =09u16 status; > =20 > -=09if (!vmx->nested.pi_desc || !vmx->nested.pi_pending) > +=09if (!vmx->nested.pi_desc) > =09=09return; > =20 > -=09vmx->nested.pi_pending =3D false; > =09if (!pi_test_and_clear_on(vmx->nested.pi_desc)) > =09=09return; > =20 > @@ -5126,7 +5124,6 @@ static int vmx_deliver_nested_posted_interrupt(stru= ct kvm_vcpu *vcpu, > =09=09 * If a posted intr is not recognized by hardware, > =09=09 * we will accomplish it in the next vmentry. > =09=09 */ > -=09=09vmx->nested.pi_pending =3D true; > =09=09kvm_make_request(KVM_REQ_EVENT, vcpu); > =09=09return 0; > =09} > @@ -10488,7 +10485,6 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, = struct vmcs12 *vmcs12, > =09/* Posted interrupts setting is only taken from vmcs12. */ > =09if (nested_cpu_has_posted_intr(vmcs12)) { > =09=09vmx->nested.posted_intr_nv =3D vmcs12->posted_intr_nv; > -=09=09vmx->nested.pi_pending =3D false; > =09=09vmcs_write16(POSTED_INTR_NV, POSTED_INTR_NESTED_VECTOR); > =09} else { > =09=09exec_control &=3D ~PIN_BASED_POSTED_INTR; >=20 It turns out this patch is incorrect and should be removed from series. I will remove it from next version of this patch series. Just didn't want y= ou to queue it by mistake. :) I figured that pi_pending is used as a flag to indicate if L1 has sent a po= sted-interrupt notification-vector such that KVM won't process the nested-p= osted-interrupts on vmentry in case L1 has already set the ON bit in vmx->n= ested.pi_desc->control field but not sent the notification-vector yet. That= will break vCPU semantics. (This is in contrast to how vmx_sync_pir_to_irr() does ignore whether a pos= ted-interrupt notification-vector was sent or not and just consume posted-i= nterrupts on each vmentry if the vmx->pi_desc.control ON bit is set. That i= s fine because in this case L0 KVM defines how it wishes to handle posted-i= nterrupts).=20 Regards, -Liran