From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH] x86: don't change affinity with interrupt unmasked Date: Mon, 23 Mar 2015 14:46:49 -0400 Message-ID: <20150323184649.GA19464@l.oracle.com> References: <550C4D62020000780006C258@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Ya7NJ-0004Rc-Ug for xen-devel@lists.xenproject.org; Mon, 23 Mar 2015 18:46:58 +0000 Content-Disposition: inline In-Reply-To: <550C4D62020000780006C258@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Andrew Cooper List-Id: xen-devel@lists.xenproject.org On Fri, Mar 20, 2015 at 03:40:02PM +0000, Jan Beulich wrote: > With ->startup unmasking the IRQ, setting the affinity afterwards > without masking the IRQ again is invalid namely for MSI (which can't > have their affinity updated atomically). That took a bit of verification :-) Could you include this in the commit please: Per 6.8.3.5. Per-vector Masking and Function Masking from https://www.pcisig.com/specifications/conventional/msi-x_ecn.pdf ".. anytime software unmasks a currently masked MSI-X Table entry either by clearing its Mask bit or by clearing the Function Mask bit, the function must update any Address or Data values that it cached from that entry. If software changes the Address or Data value of an entry while the entry is unmasked, the result is undefined." Ouch! Good catch! > > Signed-off-by: Jan Beulich Reviewed-by: Konrad Rzeszutek Wilk > --- > Changing the affinity of non-maskable MSI IRQs seems bogus too, but I > can't immediately see what we can do about this (better than disabling > affinity changes for them). > > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1608,12 +1608,13 @@ int pirq_guest_bind(struct vcpu *v, stru > init_timer(&action->eoi_timer, irq_guest_eoi_timer_fn, desc, 0); > > desc->status |= IRQ_GUEST; > - desc->status &= ~IRQ_DISABLED; > - desc->handler->startup(desc); > > /* Attempt to bind the interrupt target to the correct CPU. */ > if ( !opt_noirqbalance && (desc->handler->set_affinity != NULL) ) > desc->handler->set_affinity(desc, cpumask_of(v->processor)); > + > + desc->status &= ~IRQ_DISABLED; > + desc->handler->startup(desc); > } > else if ( !will_share || !action->shareable ) > { > > > > x86: don't change affinity with interrupt unmasked > > With ->startup unmasking the IRQ, setting the affinity afterwards > without masking the IRQ again is invalid namely for MSI (which can't > have their affinity updated atomically). > > Signed-off-by: Jan Beulich > --- > Changing the affinity of non-maskable MSI IRQs seems bogus too, but I > can't immediately see what we can do about this (better than disabling > affinity changes for them). > > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1608,12 +1608,13 @@ int pirq_guest_bind(struct vcpu *v, stru > init_timer(&action->eoi_timer, irq_guest_eoi_timer_fn, desc, 0); > > desc->status |= IRQ_GUEST; > - desc->status &= ~IRQ_DISABLED; > - desc->handler->startup(desc); > > /* Attempt to bind the interrupt target to the correct CPU. */ > if ( !opt_noirqbalance && (desc->handler->set_affinity != NULL) ) > desc->handler->set_affinity(desc, cpumask_of(v->processor)); > + > + desc->status &= ~IRQ_DISABLED; > + desc->handler->startup(desc); > } > else if ( !will_share || !action->shareable ) > { > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel