From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH v2] x86: irq_comm: Add check for RH bit in kvm_set_msi_irq Date: Thu, 12 Mar 2015 16:39:45 +0100 Message-ID: <20150312153945.GA30148@potion.brq.redhat.com> References: <54FE7B9E.8020202@gmail.com> <550059B6.6050708@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, gleb@kernel.org, pbonzini@redhat.com To: James Sullivan Return-path: Received: from mx1.redhat.com ([209.132.183.28]:36744 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754391AbbCLPjw (ORCPT ); Thu, 12 Mar 2015 11:39:52 -0400 Content-Disposition: inline In-Reply-To: <550059B6.6050708@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 2015-03-11 09:05-0600, James Sullivan: > This is a revised patch to the previous submission, adding a check fo= r RH=3D1/DM=3D1 > in kvm_set_msi_irq. (Btw. you can put it below the /^---$/ line, where it gets automaticall= y trimmed -- information on changes since the last version is very important in review, but of little use later.) > This patch doesn't completely handle RH=3D1; if RH=3D1 then the deliv= ery will behave > as in low priority mode (deliver the interrupt to only the lowest pri= ority processor), > but the delivery mode is still used to specify the semantics of the d= elivery beyond > its destination. We can't just set irq->delivery_mode to APIC_DM_LOWP= RI if RH=3D1 since > this squashes the other delivery semantics. I've documented this in t= he patch. It's possible that hardware designers didn't want to waste silicon either, thus overwriting delivery mode would be the correct, and easy, thing to do. (Or have I missed its definition?) > A future fix may be to implement a separate interrupt delivery functi= on for MSI > interrupts in kvm_set_msi, which is a bit hacky but probably the only= way to do this > without modifying the kvm_lapic_irq struct. I'll write this up and se= e how it looks, > unless there are major objections. Thank you; no objections without patches :) I think that exploring other options makes sense here. We would probably re-implement most of the code, so adding extra parameter to interrupt delivery functions would be easier to maintain. Extending 'struct kvm_lapic_irq' seems acceptable too: it's an internal structure and we already have kvm_is_dm_lowest_prio() helper. > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c > @@ -97,18 +97,34 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, str= uct kvm_lapic *src, > static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_ent= ry *e, > struct kvm_lapic_irq *irq) > { > + bool phys; > trace_kvm_msi_set_irq(e->msi.address_lo, e->msi.data); (We usually have have an empty line after declarations. I've seen patches that fixed just this, because some tool complains.) > + /* > + * Set dest_mode to logical just in case both RH=3D1/DH=3D1, > + * otherwise default to physical > + */ This comment contains the same algorithm written in other (less defined= ) language, right next to a similarly complex implementation ... > + phys =3D ((e->msi.address_lo & (MSI_ADDR_REDIRECTION_LOWPRI | > + MSI_ADDR_DEST_MODE_LOGICAL)) !=3D > + (MSI_ADDR_REDIRECTION_LOWPRI | > + MSI_ADDR_DEST_MODE_LOGICAL)); > + irq->dest_mode =3D phys ? APIC_DEST_PHYSICAL : APIC_DEST_LOGICAL; Wouldn't changing white spacing, using 'if', defining a union mask, abstracting the all-bits-in-mask-are-set operation, or something help t= o make this code clear without a comment? > irq->delivery_mode =3D e->msi.data & 0x700; A system that configured DM and RH might not handle delivery to multipl= e APICs. Please add a warning (once), to ease debugging if/when this feature gets its first user. Thanks. > +=C2=B7 /* > +=C2=B7 * TODO Deal with RH bit of MSI message address > +=C2=B7 * IF RH=3D1, then MSI delivers only to the processor wit= h the > +=C2=B7 * lowest interrupt priority among processors that can re= ceive > +=C2=B7 * the interrupt. However other delivery semantics are st= ill > +=C2=B7 * specified by the delivery mode, so we can't default to > +=C2=B7 * APIC_DM_LOWPRI if RH=3D1. > +=C2=B7 */ (We would, if there was some benefit in erring that way.)