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 <jbeul...@suse.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> > --- > 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 <jbeul...@suse.com> > --- > 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
