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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.13 at 20:58, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> On 28/03/2013 15:24, "Jan Beulich" <JBeulich@xxxxxxxx> 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@xxxxxxxx>
> 
> Acked-by: Keir Fraser <keir@xxxxxxx>

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;
>> 
>> 
>> 




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