From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751327AbdJCRxq (ORCPT ); Tue, 3 Oct 2017 13:53:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53096 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751190AbdJCRxp (ORCPT ); Tue, 3 Oct 2017 13:53:45 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C68008763A Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=rkrcmar@redhat.com Date: Tue, 3 Oct 2017 19:53:41 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Wanpeng Li Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paolo Bonzini , Wanpeng Li Subject: Re: [PATCH v2 4/4] KVM: LAPIC: Don't silently accept bad vectors Message-ID: <20171003175341.GD21107@flask> References: <1506647099-2688-1-git-send-email-wanpeng.li@hotmail.com> <1506647099-2688-5-git-send-email-wanpeng.li@hotmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1506647099-2688-5-git-send-email-wanpeng.li@hotmail.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 03 Oct 2017 17:53:44 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2017-09-28 18:04-0700, Wanpeng Li: > From: Wanpeng Li > > Vectors 0-15 are reserved, and a physical LAPIC - upon sending or > receiving one - would generate an APIC error instead of doing the > requested action. Make our emulation behave similarly. > > Cc: Paolo Bonzini > Cc: Radim Krčmář > Signed-off-by: Wanpeng Li > --- > arch/x86/kvm/lapic.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 6bafd06..a779ba9 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -935,6 +935,25 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq, > return ret; > } > > +static void apic_error(struct kvm_lapic *apic, unsigned long errmask) > +{ > + uint32_t esr; > + > + esr = kvm_lapic_get_reg(apic, APIC_ESR); > + > + if ((esr & errmask) != errmask) { The spec makes me think that there is going to be only 1 interrupt (regardless of the number errors) until the software writes 0 to APIC_ESR. Is there a better description than the following 10.5.3? The ESR is a write/read register. Before attempt to read from the ESR, software should first write to it. (The value written does not affect the values read subsequently; only zero may be written in x2APIC mode.) This write clears any previously logged errors and updates the ESR with any errors detected since the last write to the ESR. This write also rearms the APIC error interrupt triggering mechanism. This also describes a different handling of APIC_ESR -- APIC_ESR is updated only on software writes to APIC_ESR. All errors in between seem to be logged internally (not sure where to migrate it). > + uint32_t lvterr = kvm_lapic_get_reg(apic, APIC_LVTERR); > + > + kvm_lapic_set_reg(apic, APIC_ESR, esr | errmask); > + if (!(lvterr & APIC_LVT_MASKED)) { > + struct kvm_lapic_irq irq; > + > + irq.vector = lvterr & 0xff; > + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > + } > + } > +} > + > /* > * Add a pending IRQ into lapic. > * Return 1 if successfully added and 0 if discarded. > @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, > int result = 0; > struct kvm_vcpu *vcpu = apic->vcpu; > > + if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) { > + apic_error(apic, APIC_ESR_RECVILL); The error is also triggered if lowest priority is supported and tries to deliver an invalid vector. > + return 0; > + } > + > trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, > trig_mode, vector); > switch (delivery_mode) { > @@ -1146,7 +1170,10 @@ static void apic_send_ipi(struct kvm_lapic *apic) > irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode, > irq.vector, irq.msi_redir_hint); > > - kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > + if (unlikely(irq.vector < 16 && irq.delivery_mode == APIC_DM_FIXED)) Please check how APICv self-IPI acceleration behaves, so we're consistent. Thanks. > + apic_error(apic, APIC_ESR_SENDILL); > + else > + kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL); > } > > static u32 apic_get_tmcct(struct kvm_lapic *apic) > @@ -1734,7 +1761,6 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > case APIC_LVTPC: > case APIC_LVT1: > case APIC_LVTERR: > - /* TODO: Check vector */ > if (!kvm_apic_sw_enabled(apic)) > val |= APIC_LVT_MASKED; > > -- > 2.7.4 >