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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

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 )
     {




[-- Attachment #2: x86-initial-IRQ-affinity.patch --]
[-- Type: text/plain, Size: 1155 bytes --]

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 )
     {

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Konrad Rzeszutek Wilk]

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

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

[Jan Beulich]

>>> On 23.03.15 at 19:46, <konrad.wilk@oracle.com> wrote:
> 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."

I'd rather not, as that may result in the wrong impression that only
MSI-X is affected, while MSI is as much. Apart from the potential
caching is only one part of the problem, the other is (as said in the
description) the non-atomic nature of the address/data updates
for both variants of MSI. I guess I'll simply extend what is in
parentheses at the end to include the caching issue.

Jan

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

[Andrew Cooper]

On 20/03/15 16:40, 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).
>
> Signed-off-by: Jan Beulich<jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Changing the affinity of non-maskable MSI IRQs seems bogus too

Agreed.  Their affinity can clearly only be changed safely by a device 
driver which can guarantee that an interrupt will not be generated 
during the vulnerable period.

This further implies that Xen can't even safely set an affinity to start 
with...

> , but I
> can't immediately see what we can do about this (better than disabling
> affinity changes for them).

If we used interrupt remapping properly (which we don't), we could 
update the effective affinity without changing any device configuration, 
but this still doesn't provide a solution for the many systems out there 
without (functional) interrupt remapping.

~Andrew

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

[Jan Beulich]

>>> On 25.03.15 at 19:12, <andrew.cooper3@citrix.com> wrote:
>> Changing the affinity of non-maskable MSI IRQs seems bogus too
> 
> Agreed.  Their affinity can clearly only be changed safely by a device 
> driver which can guarantee that an interrupt will not be generated 
> during the vulnerable period.
> 
> This further implies that Xen can't even safely set an affinity to start 
> with...

No, as long as MSI is disabled (i.e. during initial setup) this would be
fine. Disabling MSI could even be an option to do this, if there
weren't devices that misbehave when MSI gets disabled again after
having once been enabled.

Jan