Re: [PATCH] x86: don't change affinity with interrupt unmasked

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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 <jbeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@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 <jbeulich@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