Jan Beulich
2013-Mar-28 15:24 UTC
[PATCH] 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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Andrew Cooper
2013-Mar-28 15:29 UTC
Re: [PATCH] x86: irq_move_cleanup_interrupt() must ignore legacy vectors
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; > > >
Keir Fraser
2013-Mar-28 19:58 UTC
Re: [PATCH] x86: irq_move_cleanup_interrupt() must ignore legacy vectors
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; > > >
Jan Beulich
2013-Apr-02 06:25 UTC
Re: [PATCH] x86: irq_move_cleanup_interrupt() must ignore legacy vectors
>>> 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; >> >> >>