From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [Patch v4] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq Date: Fri, 13 Mar 2015 15:39:58 +0100 Message-ID: <20150313143957.GA17349@potion.brq.redhat.com> References: <55022423.7050101@gmail.com> <5502506C.6000005@gmail.com> <550254C6.6010103@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, pbonzini@redhat.com, gleb@kernel.org To: James Sullivan Return-path: Received: from mx1.redhat.com ([209.132.183.28]:35799 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754169AbbCMOkE (ORCPT ); Fri, 13 Mar 2015 10:40:04 -0400 Content-Disposition: inline In-Reply-To: <550254C6.6010103@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 2015-03-12 21:08-0600, James Sullivan: > --- > Changes since v2: > * Added one time warning message when RH=3D1 > * Documented conflict between RH=3D1 and delivery mode > * Tidied code to check RH=3D1/DM=3D1 (remove bool phys, use if/el= se) > Changes since v3: > * Fixed logical error in RH=3D1/DM=3D1 check > * Aligned quotation blocks in multiline pr_warn_once argument >=20 > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c > @@ -103,12 +103,24 @@ static inline void kvm_set_msi_irq(struct kvm_k= ernel_irq_routing_entry *e, > MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT; > irq->vector =3D (e->msi.data & > MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT; > - irq->dest_mode =3D (1 << MSI_ADDR_DEST_MODE_SHIFT) & e->msi.address= _lo; > + /* > + * TODO Deal with RH bit of MSI message address > + * IF RH=3D1, then MSI delivers only to the processor with the > + * lowest interrupt priority among processors that can receive > + * the interrupt. > + */ > + if ((e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI) && > + (e->msi.address_lo & MSI_ADDR_DEST_MODE_LOGICAL)) > + irq->dest_mode =3D APIC_DEST_LOGICAL; > + else > + irq->dest_mode =3D APIC_DEST_PHYSICAL; > irq->trig_mode =3D (1 << MSI_DATA_TRIGGER_SHIFT) & e->msi.data; > irq->delivery_mode =3D e->msi.data & 0x700; > + if (e->msi.address_lo & MSI_ADDR_REDIRECTION_LOWPRI) > + pr_warn_once("KVM may not correctly deliver MSIs " > + "with RH set.\n"); Please begin the warning with "kvm: ". It's better to have a line bit longer than 80 columns than to break a string that we might want to `grep` for; reword if possible, or you might prefer something like pr_warn_once( "long"); (Documentation/CodingStyle, Chapter 2. It's nitpicking, sorry, but we recently had a patch that fixed just that too ... http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/133110) Then, Reviewed-by: Radim Kr=C4=8Dm=C3=A1=C5=99 Thanks. --- The warning message is very clever: - it contains the magical "may" qualifier and being protected only by RH=3D1 creates weird-looking code structure, but it is technically ri= ght 1) lowest-priority delivery may be set in msi.data, which avoids our otherwise incorrect behavior with RH=3D1/DM=3D1 2) RH=3D1/DM=3D0 can't deliver to multiple APICs (broadcast is forbid= den), but real hardware may overwrite delivery mode from msi.data - being two lines apart adds to suspicion, yet it can be hint to those possible problems I only fear it is too clever :)