[PATCH] x86: irq_move_cleanup_interrupt() must ignore legacy vectors

[PATCH] x86: irq_move_cleanup_interrupt() must ignore legacy vectors

[Jan Beulich]

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

Since the main loop in the function includes legacy vectors, and since
vector_irq[] gets set up for legacy vectors regardless of whether those
get handled through the IO-APIC, it must not do anything on this vector
range. In fact, we should never get here for IRQs not handled through
the IO-APIC, so add a respective assertion at once. For that assertion
to not have false positives we however need to suppress setting up IRQ2
as an 8259A interrupt (which wasn't correct anyway).

Furthermore, there's no point iterating over the vectors past
LAST_HIPRIORITY_VECTOR, so terminate the loop accordingly.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -416,6 +416,8 @@ void __init init_IRQ(void)
     for (irq = 0; platform_legacy_irq(irq); irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         
+        if ( irq == 2 ) /* IRQ2 doesn't exist */
+            continue;
         desc->handler = &i8259A_irq_type;
         per_cpu(vector_irq, cpu)[FIRST_LEGACY_VECTOR + irq] = irq;
         cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu));
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -616,7 +616,9 @@ void irq_move_cleanup_interrupt(struct c
     ack_APIC_irq();
 
     me = smp_processor_id();
-    for (vector = FIRST_DYNAMIC_VECTOR; vector < NR_VECTORS; vector++) {
+    for ( vector = FIRST_DYNAMIC_VECTOR;
+          vector <= LAST_HIPRIORITY_VECTOR; vector++)
+    {
         unsigned int irq;
         unsigned int irr;
         struct irq_desc *desc;
@@ -625,6 +627,12 @@ void irq_move_cleanup_interrupt(struct c
         if ((int)irq < 0)
             continue;
 
+        if ( vector >= FIRST_LEGACY_VECTOR && vector <= LAST_LEGACY_VECTOR )
+        {
+            ASSERT(IO_APIC_IRQ(irq));
+            continue;
+        }
+
         desc = irq_to_desc(irq);
         if (!desc)
             continue;




[-- Attachment #2: x86-IRQ-move-cleanup-skip-legacy.patch --]
[-- Type: text/plain, Size: 2010 bytes --]

x86: irq_move_cleanup_interrupt() must ignore legacy vectors

Since the main loop in the function includes legacy vectors, and since
vector_irq[] gets set up for legacy vectors regardless of whether those
get handled through the IO-APIC, it must not do anything on this vector
range. In fact, we should never get here for IRQs not handled through
the IO-APIC, so add a respective assertion at once. For that assertion
to not have false positives we however need to suppress setting up IRQ2
as an 8259A interrupt (which wasn't correct anyway).

Furthermore, there's no point iterating over the vectors past
LAST_HIPRIORITY_VECTOR, so terminate the loop accordingly.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/i8259.c
+++ b/xen/arch/x86/i8259.c
@@ -416,6 +416,8 @@ void __init init_IRQ(void)
     for (irq = 0; platform_legacy_irq(irq); irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         
+        if ( irq == 2 ) /* IRQ2 doesn't exist */
+            continue;
         desc->handler = &i8259A_irq_type;
         per_cpu(vector_irq, cpu)[FIRST_LEGACY_VECTOR + irq] = irq;
         cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu));
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -616,7 +616,9 @@ void irq_move_cleanup_interrupt(struct c
     ack_APIC_irq();
 
     me = smp_processor_id();
-    for (vector = FIRST_DYNAMIC_VECTOR; vector < NR_VECTORS; vector++) {
+    for ( vector = FIRST_DYNAMIC_VECTOR;
+          vector <= LAST_HIPRIORITY_VECTOR; vector++)
+    {
         unsigned int irq;
         unsigned int irr;
         struct irq_desc *desc;
@@ -625,6 +627,12 @@ void irq_move_cleanup_interrupt(struct c
         if ((int)irq < 0)
             continue;
 
+        if ( vector >= FIRST_LEGACY_VECTOR && vector <= LAST_LEGACY_VECTOR )
+        {
+            ASSERT(IO_APIC_IRQ(irq));
+            continue;
+        }
+
         desc = irq_to_desc(irq);
         if (!desc)
             continue;

[-- 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: irq_move_cleanup_interrupt() must ignore legacy vectors

[Andrew Cooper]

On 28/03/2013 15:24, Jan Beulich wrote:
> Since the main loop in the function includes legacy vectors, and since
> vector_irq[] gets set up for legacy vectors regardless of whether those
> get handled through the IO-APIC, it must not do anything on this vector
> range. In fact, we should never get here for IRQs not handled through
> the IO-APIC, so add a respective assertion at once. For that assertion
> to not have false positives we however need to suppress setting up IRQ2
> as an 8259A interrupt (which wasn't correct anyway).
>
> Furthermore, there's no point iterating over the vectors past
> LAST_HIPRIORITY_VECTOR, so terminate the loop accordingly.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

>
> --- a/xen/arch/x86/i8259.c
> +++ b/xen/arch/x86/i8259.c
> @@ -416,6 +416,8 @@ void __init init_IRQ(void)
>      for (irq = 0; platform_legacy_irq(irq); irq++) {
>          struct irq_desc *desc = irq_to_desc(irq);
>          
> +        if ( irq == 2 ) /* IRQ2 doesn't exist */
> +            continue;
>          desc->handler = &i8259A_irq_type;
>          per_cpu(vector_irq, cpu)[FIRST_LEGACY_VECTOR + irq] = irq;
>          cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu));
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -616,7 +616,9 @@ void irq_move_cleanup_interrupt(struct c
>      ack_APIC_irq();
>  
>      me = smp_processor_id();
> -    for (vector = FIRST_DYNAMIC_VECTOR; vector < NR_VECTORS; vector++) {
> +    for ( vector = FIRST_DYNAMIC_VECTOR;
> +          vector <= LAST_HIPRIORITY_VECTOR; vector++)
> +    {
>          unsigned int irq;
>          unsigned int irr;
>          struct irq_desc *desc;
> @@ -625,6 +627,12 @@ void irq_move_cleanup_interrupt(struct c
>          if ((int)irq < 0)
>              continue;
>  
> +        if ( vector >= FIRST_LEGACY_VECTOR && vector <= LAST_LEGACY_VECTOR )
> +        {
> +            ASSERT(IO_APIC_IRQ(irq));
> +            continue;
> +        }
> +
>          desc = irq_to_desc(irq);
>          if (!desc)
>              continue;
>
>
>

Re: [PATCH] x86: irq_move_cleanup_interrupt() must ignore legacy vectors

[Keir Fraser]

On 28/03/2013 15:24, "Jan Beulich" <JBeulich@suse.com> wrote:

> Since the main loop in the function includes legacy vectors, and since
> vector_irq[] gets set up for legacy vectors regardless of whether those
> get handled through the IO-APIC, it must not do anything on this vector
> range. In fact, we should never get here for IRQs not handled through
> the IO-APIC, so add a respective assertion at once. For that assertion
> to not have false positives we however need to suppress setting up IRQ2
> as an 8259A interrupt (which wasn't correct anyway).
> 
> Furthermore, there's no point iterating over the vectors past
> LAST_HIPRIORITY_VECTOR, so terminate the loop accordingly.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

> --- a/xen/arch/x86/i8259.c
> +++ b/xen/arch/x86/i8259.c
> @@ -416,6 +416,8 @@ void __init init_IRQ(void)
>      for (irq = 0; platform_legacy_irq(irq); irq++) {
>          struct irq_desc *desc = irq_to_desc(irq);
>          
> +        if ( irq == 2 ) /* IRQ2 doesn't exist */
> +            continue;
>          desc->handler = &i8259A_irq_type;
>          per_cpu(vector_irq, cpu)[FIRST_LEGACY_VECTOR + irq] = irq;
>          cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu));
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -616,7 +616,9 @@ void irq_move_cleanup_interrupt(struct c
>      ack_APIC_irq();
>  
>      me = smp_processor_id();
> -    for (vector = FIRST_DYNAMIC_VECTOR; vector < NR_VECTORS; vector++) {
> +    for ( vector = FIRST_DYNAMIC_VECTOR;
> +          vector <= LAST_HIPRIORITY_VECTOR; vector++)
> +    {
>          unsigned int irq;
>          unsigned int irr;
>          struct irq_desc *desc;
> @@ -625,6 +627,12 @@ void irq_move_cleanup_interrupt(struct c
>          if ((int)irq < 0)
>              continue;
>  
> +        if ( vector >= FIRST_LEGACY_VECTOR && vector <= LAST_LEGACY_VECTOR )
> +        {
> +            ASSERT(IO_APIC_IRQ(irq));
> +            continue;
> +        }
> +
>          desc = irq_to_desc(irq);
>          if (!desc)
>              continue;
> 
> 
> 

Re: [PATCH] x86: irq_move_cleanup_interrupt() must ignore legacy vectors

[Jan Beulich]

>>> On 28.03.13 at 20:58, Keir Fraser <keir.xen@gmail.com> wrote:
> On 28/03/2013 15:24, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> Since the main loop in the function includes legacy vectors, and since
>> vector_irq[] gets set up for legacy vectors regardless of whether those
>> get handled through the IO-APIC, it must not do anything on this vector
>> range. In fact, we should never get here for IRQs not handled through
>> the IO-APIC, so add a respective assertion at once. For that assertion
>> to not have false positives we however need to suppress setting up IRQ2
>> as an 8259A interrupt (which wasn't correct anyway).
>> 
>> Furthermore, there's no point iterating over the vectors past
>> LAST_HIPRIORITY_VECTOR, so terminate the loop accordingly.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Keir Fraser <keir@xen.org>

I'll commit this with the ASSERT() removed again, as I meanwhile
realized it's wrong after all: As I kept saying (and then ignored
myself here) we get there for the legacy vectors no matter what,
and if one of them happens to be serviced via the 8259A we'd
crash. An assertion would only be valid after having checked that
move_cleanup_count is non-zero, but as that's after already
having taken the lock, the loop iterations for the legacy vectors
can be kept less expensive by keeping the check where it is and
dropping the assertion.

Jan

>> --- a/xen/arch/x86/i8259.c
>> +++ b/xen/arch/x86/i8259.c
>> @@ -416,6 +416,8 @@ void __init init_IRQ(void)
>>      for (irq = 0; platform_legacy_irq(irq); irq++) {
>>          struct irq_desc *desc = irq_to_desc(irq);
>>          
>> +        if ( irq == 2 ) /* IRQ2 doesn't exist */
>> +            continue;
>>          desc->handler = &i8259A_irq_type;
>>          per_cpu(vector_irq, cpu)[FIRST_LEGACY_VECTOR + irq] = irq;
>>          cpumask_copy(desc->arch.cpu_mask, cpumask_of(cpu));
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -616,7 +616,9 @@ void irq_move_cleanup_interrupt(struct c
>>      ack_APIC_irq();
>>  
>>      me = smp_processor_id();
>> -    for (vector = FIRST_DYNAMIC_VECTOR; vector < NR_VECTORS; vector++) {
>> +    for ( vector = FIRST_DYNAMIC_VECTOR;
>> +          vector <= LAST_HIPRIORITY_VECTOR; vector++)
>> +    {
>>          unsigned int irq;
>>          unsigned int irr;
>>          struct irq_desc *desc;
>> @@ -625,6 +627,12 @@ void irq_move_cleanup_interrupt(struct c
>>          if ((int)irq < 0)
>>              continue;
>>  
>> +        if ( vector >= FIRST_LEGACY_VECTOR && vector <= LAST_LEGACY_VECTOR )
>> +        {
>> +            ASSERT(IO_APIC_IRQ(irq));
>> +            continue;
>> +        }
>> +
>>          desc = irq_to_desc(irq);
>>          if (!desc)
>>              continue;
>> 
>> 
>>