From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 83D74C433F5 for ; Thu, 18 Nov 2021 09:57:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6C91961B64 for ; Thu, 18 Nov 2021 09:57:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245360AbhKRKAI (ORCPT ); Thu, 18 Nov 2021 05:00:08 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:51412 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S245369AbhKRJ7T (ORCPT ); Thu, 18 Nov 2021 04:59:19 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1637229379; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wEk0Ix6QqXBgVWnz008QmDW0QekiZrGZyqYEU0wf5p4=; b=R1IVZ+gw68ytCyB3rtw5ziFoQjoDgGTHXTNx7+fHbDiEPoRatPdaXOndkZBNiOVpZ+6s+k hK2CWHNvXRNUuIi7C6lmMMwW2YaXvwy/6F2hDITgvQE17CHEl/gs6ZLSXr46e7uIB0XN3G nSbhHw2zKLUTLkZsBvZnK48Wvz/3CPg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-363-uqecaitHPRmsAKjQpphpBg-1; Thu, 18 Nov 2021 04:56:16 -0500 X-MC-Unique: uqecaitHPRmsAKjQpphpBg-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1D81587D541; Thu, 18 Nov 2021 09:56:14 +0000 (UTC) Received: from starship (unknown [10.40.194.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5D8CB5FC13; Thu, 18 Nov 2021 09:56:12 +0000 (UTC) Message-ID: <8ad47d43a7c8ae19f09cc6ada73665d6e348e213.camel@redhat.com> Subject: Re: [PATCH v2] KVM: x86: check PIR even for vCPUs with disabled APICv From: Maxim Levitsky To: Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org Cc: stable@vger.kernel.org, Sean Christopherson Date: Thu, 18 Nov 2021 11:56:11 +0200 In-Reply-To: <20211118072531.1534938-1-pbonzini@redhat.com> References: <20211118072531.1534938-1-pbonzini@redhat.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Thu, 2021-11-18 at 02:25 -0500, Paolo Bonzini wrote: > The IRTE for an assigned device can trigger a POSTED_INTR_VECTOR even > if APICv is disabled on the vCPU that receives it. In that case, the > interrupt will just cause a vmexit and leave the ON bit set together > with the PIR bit corresponding to the interrupt. 100% true. > > Right now, the interrupt would not be delivered until APICv is re-enabled. > However, fixing this is just a matter of always doing the PIR->IRR > synchronization, even if the vCPU has temporarily disabled APICv. > > This is not a problem for performance, or if anything it is an > improvement. First, in the common case where vcpu->arch.apicv_active is > true, one fewer check has to be performed. Second, static_call_cond will > elide the function call if APICv is not present or disabled. Finally, > in the case for AMD hardware we can remove the sync_pir_to_irr callback: > it is only needed for apic_has_interrupt_for_ppr, and that function > already has a fallback for !APICv. > > Cc: stable@vger.kernel.org > Co-developed-by: Sean Christopherson > Signed-off-by: Paolo Bonzini > --- > arch/x86/kvm/lapic.c | 2 +- > arch/x86/kvm/svm/svm.c | 1 - > arch/x86/kvm/x86.c | 18 +++++++++--------- > 3 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 759952dd1222..f206fc35deff 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -707,7 +707,7 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu) > static int apic_has_interrupt_for_ppr(struct kvm_lapic *apic, u32 ppr) > { > int highest_irr; > - if (apic->vcpu->arch.apicv_active) > + if (kvm_x86_ops.sync_pir_to_irr) > highest_irr = static_call(kvm_x86_sync_pir_to_irr)(apic->vcpu); > else > highest_irr = apic_find_highest_irr(apic); > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 5630c241d5f6..d0f68d11ec70 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -4651,7 +4651,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = { > .load_eoi_exitmap = svm_load_eoi_exitmap, > .hwapic_irr_update = svm_hwapic_irr_update, > .hwapic_isr_update = svm_hwapic_isr_update, > - .sync_pir_to_irr = kvm_lapic_find_highest_irr, > .apicv_post_state_restore = avic_post_state_restore, > > .set_tss_addr = svm_set_tss_addr, > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 627c955101a0..a8f12c83db4b 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -4448,8 +4448,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) > static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu, > struct kvm_lapic_state *s) > { > - if (vcpu->arch.apicv_active) > - static_call(kvm_x86_sync_pir_to_irr)(vcpu); > + static_call_cond(kvm_x86_sync_pir_to_irr)(vcpu); > > return kvm_apic_get_state(vcpu, s); > } > @@ -9528,8 +9527,7 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu) > if (irqchip_split(vcpu->kvm)) > kvm_scan_ioapic_routes(vcpu, vcpu->arch.ioapic_handled_vectors); > else { > - if (vcpu->arch.apicv_active) > - static_call(kvm_x86_sync_pir_to_irr)(vcpu); > + static_call_cond(kvm_x86_sync_pir_to_irr)(vcpu); > if (ioapic_in_kernel(vcpu->kvm)) > kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors); > } > @@ -9802,10 +9800,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > /* > * This handles the case where a posted interrupt was > - * notified with kvm_vcpu_kick. > + * notified with kvm_vcpu_kick. Assigned devices can > + * use the POSTED_INTR_VECTOR even if APICv is disabled, > + * so do it even if !kvm_vcpu_apicv_active(vcpu). > */ > - if (kvm_lapic_enabled(vcpu) && vcpu->arch.apicv_active) > - static_call(kvm_x86_sync_pir_to_irr)(vcpu); > + if (kvm_lapic_enabled(vcpu)) > + static_call_cond(kvm_x86_sync_pir_to_irr)(vcpu); > > if (kvm_vcpu_exit_request(vcpu)) { > vcpu->mode = OUTSIDE_GUEST_MODE; > @@ -9849,8 +9849,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > if (likely(exit_fastpath != EXIT_FASTPATH_REENTER_GUEST)) > break; > > - if (kvm_lapic_enabled(vcpu) && kvm->arch.apicv_active) > - static_call(kvm_x86_sync_pir_to_irr)(vcpu); > + if (kvm_lapic_enabled(vcpu)) > + static_call_cond(kvm_x86_sync_pir_to_irr)(vcpu); > > if (unlikely(kvm_vcpu_exit_request(vcpu))) { > exit_fastpath = EXIT_FASTPATH_EXIT_HANDLED; vmx_sync_pir_to_irr has 'if (KVM_BUG_ON(!vcpu->arch.apicv_active, vcpu->kvm))' That has to be removed I think for this to work. Plus the above calls now can happen when APICv is fully disabled (and not just inhibited), which is also something that I think that vmx_sync_pir_to_irr should be fixed to be aware of. Also note that VMX has code that sets vmx_x86_ops.sync_pir_to_irr to NULL in its 'hardware_setup' if APICv is disabled. I wonder if that done befor or after the static_call_cond sites are updated. I think that this code should be removed as well, and vmx_sync_pir_to_irr should just do nothing when APICv is fully disabled. I haven't run tested this code so I might be wrong of course. Best regards, Maxim Levitsky