From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754719AbdBHOSO (ORCPT ); Wed, 8 Feb 2017 09:18:14 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35232 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753928AbdBHOSL (ORCPT ); Wed, 8 Feb 2017 09:18:11 -0500 Subject: Re: [PATCH 5/6] KVM: x86: do not scan IRR twice on APICv vmentry To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= References: <1482164232-130035-1-git-send-email-pbonzini@redhat.com> <1482164232-130035-6-git-send-email-pbonzini@redhat.com> <20170207201955.GD31091@potion> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org From: Paolo Bonzini Message-ID: <7d5ef520-77b2-abd6-8840-f7b309676aa1@redhat.com> Date: Wed, 8 Feb 2017 15:10:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170207201955.GD31091@potion> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/02/2017 21:19, Radim Krčmář wrote: > 2016-12-19 17:17+0100, Paolo Bonzini: >> Calls to apic_find_highest_irr are scanning IRR twice, once >> in vmx_sync_pir_from_irr and once in apic_search_irr. Change >> sync_pir_from_irr to get the new maximum IRR from kvm_apic_update_irr; >> now that it does the computation, it can also do the RVI write. >> >> In order to avoid complications in svm.c, make the callback optional. >> >> Signed-off-by: Paolo Bonzini >> --- >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> @@ -8734,20 +8736,24 @@ static void vmx_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr) >> } >> } >> >> -static void vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) >> +static int vmx_sync_pir_to_irr(struct kvm_vcpu *vcpu) >> { >> struct vcpu_vmx *vmx = to_vmx(vcpu); >> + int max_irr; >> >> - if (!pi_test_on(&vmx->pi_desc)) >> - return; >> - >> - pi_clear_on(&vmx->pi_desc); >> - /* >> - * IOMMU can write to PIR.ON, so the barrier matters even on UP. >> - * But on x86 this is just a compiler barrier anyway. >> - */ >> - smp_mb__after_atomic(); >> - kvm_apic_update_irr(vcpu, vmx->pi_desc.pir); >> + if (vcpu->arch.apicv_active && pi_test_on(&vmx->pi_desc)) { >> + pi_clear_on(&vmx->pi_desc); >> + /* >> + * IOMMU can write to PIR.ON, so the barrier matters even on UP. >> + * But on x86 this is just a compiler barrier anyway. >> + */ >> + smp_mb__after_atomic(); >> + max_irr = kvm_apic_update_irr(vcpu, vmx->pi_desc.pir); >> + } else { >> + max_irr = kvm_lapic_find_highest_irr(vcpu); >> + } >> + vmx_hwapic_irr_update(vcpu, max_irr); > > Btw. a v1 discussion revolved about the need to have > vmx_hwapic_irr_update() here when the maximal IRR should always be in > RVI, and, uh, I didn't follow up (negligible attention span) ... > > There is one place where that doesn't hold: we don't update RVI after a > EXTERNAL_INTERRUPT nested VM exit without VM_EXIT_ACK_INTR_ON_EXIT, but > IRR has likely changed. Isn't that the problem? I'm not sure... there shouldn't be any issue with missed RVI updates in this series, since it does if (kvm_lapic_enabled(vcpu)) { /* * This handles the case where a posted interrupt was * notified with kvm_vcpu_kick. */ if (kvm_x86_ops->sync_pir_to_irr) kvm_x86_ops->sync_pir_to_irr(vcpu); } on every VM entry (and kvm_lapic_find_highest_irr inside the callback). That is not something I really like, but it's no worse than what was there before if (vcpu->arch.apicv_active) kvm_x86_ops->hwapic_irr_update(vcpu, kvm_lapic_find_highest_irr(vcpu)); } and obviously better than going unnecessarily through KVM_REQ_EVENT processing. Paolo