Re: [Xen-devel] [PATCH, v2] x86: adjust handling of interrupts coming in via legacy vectors

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.05.12 at 17:53, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> The debugging code added in c/s 24707:96987c324a4f was hit a (small)
> number of times (one report being
> http://lists.xen.org/archives/html/xen-devel/2012-05/msg00332.html),
> apparently always with a vector within the legacy range. Obviously,
> besides legacy vectors not normally expected to be in use on systems
> with IO-APIC(s), they should never make it to the IRQ migration logic.
> 
> This wasn't being prevented so far: Since we don't have a one-to-one
> mapping between vectors and IRQs - legacy IRQs may have two vectors
> associated with them (one used in either 8259A, the other used in one
> of the IO-APICs) -, vector-to-IRQ translations for legacy vectors (as
> used in do_IRQ()) would yield a valid IRQ number despite the IRQ
> really being handled via an IO-APIC.
> 
> This gets changed here - disable_8259A_irq() zaps the legacy vector-to-
> IRQ mapping, and enable_8259A_irq(), should it ever be called for a
> particular interrupts, restores it.
> 
> Additionally, the spurious interrupt logic in do_IRQ() gets adjusted
> too: Interrupts coming in via legacy vectors obviously didn't get
> reported through the IO-APIC/LAPIC pair (as we never program these
> vectors into any RTE), and hence shouldn't get ack_APIC_irq() called on
> them. Instead, a new function (pointer) bogus_8259A_irq() gets used to
> have the 8259A driver take care of the bogus interrupt (as outside of
> automatice EOI mode it may need an EOI to be issued for it to prevent
> other interrupts that may legitimately go through the 8259As from
> getting masked out).

Just realized that this last paragraph doesn't really match the
implementation, so I'm intending to replace it with:

The spurious interrupt logic in do_IRQ() gets adjusted too: Interrupts
coming in via legacy vectors presumably didn't get reported through the
IO-APIC/LAPIC pair (as we never program these vectors into any RTE or
LVT). Call ack_APIC_irq() only when the LAPIC's ISR bit says an
interrupt is pending at the given vector. Plus, a new function (pointer)
bogus_8259A_irq() gets used to have the 8259A driver take care of the
bogus interrupt (as outside of automatic EOI mode it may need an EOI to
be issued for it to prevent other interrupts legitimately going through
the 8259As from getting masked out).

Jan

> Changes in v2: Check LAPIC's ISR to decide whether to call
> ack_APIC_irq(), and use the vector only to decide whether to call
> bogus_8259A_irq(). Use the newly introduced helper function also in the
> two places in apic.c where this so far was open-coded.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1317,15 +1317,12 @@ void smp_send_state_dump(unsigned int cp
>   */
>  void spurious_interrupt(struct cpu_user_regs *regs)
>  {
> -    unsigned long v;
> -
>      /*
>       * Check if this is a vectored interrupt (most likely, as this is 
> probably
>       * a request to dump local CPU state). Vectored interrupts are ACKed;
>       * spurious interrupts are not.
>       */
> -    v = apic_read(APIC_ISR + ((SPURIOUS_APIC_VECTOR & ~0x1f) >> 1));
> -    if (v & (1 << (SPURIOUS_APIC_VECTOR & 0x1f))) {
> +    if (apic_isr_read(SPURIOUS_APIC_VECTOR)) {
>          ack_APIC_irq();
>          if (this_cpu(state_dump_pending)) {
>              this_cpu(state_dump_pending) = 0;
> @@ -1491,6 +1488,5 @@ enum apic_mode current_local_apic_mode(v
>  
>  void check_for_unexpected_msi(unsigned int vector)
>  {
> -    unsigned long v = apic_read(APIC_ISR + ((vector & ~0x1f) >> 1));
> -    BUG_ON(v & (1 << (vector & 0x1f)));
> +    BUG_ON(apic_isr_read(vector));
>  }
> --- a/xen/arch/x86/i8259.c
> +++ b/xen/arch/x86/i8259.c
> @@ -85,7 +85,15 @@ BUILD_16_IRQS(0xc) BUILD_16_IRQS(0xd) BU
>  
>  static DEFINE_SPINLOCK(i8259A_lock);
>  
> -static void mask_and_ack_8259A_irq(struct irq_desc *);
> +static void _mask_and_ack_8259A_irq(unsigned int irq);
> +
> +void (*__read_mostly bogus_8259A_irq)(unsigned int irq) =
> +    _mask_and_ack_8259A_irq;
> +
> +static void mask_and_ack_8259A_irq(struct irq_desc *desc)
> +{
> +    _mask_and_ack_8259A_irq(desc->irq);
> +}
>  
>  static unsigned int startup_8259A_irq(struct irq_desc *desc)
>  {
> @@ -133,20 +141,26 @@ static unsigned int cached_irq_mask = 0x
>   */
>  unsigned int __read_mostly io_apic_irqs;
>  
> -void disable_8259A_irq(struct irq_desc *desc)
> +static void _disable_8259A_irq(unsigned int irq)
>  {
> -    unsigned int mask = 1 << desc->irq;
> +    unsigned int mask = 1 << irq;
>      unsigned long flags;
>  
>      spin_lock_irqsave(&i8259A_lock, flags);
>      cached_irq_mask |= mask;
> -    if (desc->irq & 8)
> +    if (irq & 8)
>          outb(cached_A1,0xA1);
>      else
>          outb(cached_21,0x21);
> +    per_cpu(vector_irq, 0)[LEGACY_VECTOR(irq)] = -1;
>      spin_unlock_irqrestore(&i8259A_lock, flags);
>  }
>  
> +void disable_8259A_irq(struct irq_desc *desc)
> +{
> +    _disable_8259A_irq(desc->irq);
> +}
> +
>  void enable_8259A_irq(struct irq_desc *desc)
>  {
>      unsigned int mask = ~(1 << desc->irq);
> @@ -154,6 +168,7 @@ void enable_8259A_irq(struct irq_desc *d
>  
>      spin_lock_irqsave(&i8259A_lock, flags);
>      cached_irq_mask &= mask;
> +    per_cpu(vector_irq, 0)[LEGACY_VECTOR(desc->irq)] = desc->irq;
>      if (desc->irq & 8)
>          outb(cached_A1,0xA1);
>      else
> @@ -226,9 +241,9 @@ static inline int i8259A_irq_real(unsign
>   * first, _then_ send the EOI, and the order of EOI
>   * to the two 8259s is important!
>   */
> -static void mask_and_ack_8259A_irq(struct irq_desc *desc)
> +static void _mask_and_ack_8259A_irq(unsigned int irq)
>  {
> -    unsigned int irqmask = 1 << desc->irq;
> +    unsigned int irqmask = 1 << irq;
>      unsigned long flags;
>  
>      spin_lock_irqsave(&i8259A_lock, flags);
> @@ -252,15 +267,15 @@ static void mask_and_ack_8259A_irq(struc
>      cached_irq_mask |= irqmask;
>  
>   handle_real_irq:
> -    if (desc->irq & 8) {
> +    if (irq & 8) {
>          inb(0xA1);              /* DUMMY - (do we need this?) */
>          outb(cached_A1,0xA1);
> -        outb(0x60 + (desc->irq & 7), 0xA0);/* 'Specific EOI' to slave */
> +        outb(0x60 + (irq & 7), 0xA0);/* 'Specific EOI' to slave */
>          outb(0x62,0x20);        /* 'Specific EOI' to master-IRQ2 */
>      } else {
>          inb(0x21);              /* DUMMY - (do we need this?) */
>          outb(cached_21,0x21);
> -        outb(0x60 + desc->irq, 0x20);/* 'Specific EOI' to master */
> +        outb(0x60 + irq, 0x20);/* 'Specific EOI' to master */
>      }
>      spin_unlock_irqrestore(&i8259A_lock, flags);
>      return;
> @@ -269,7 +284,7 @@ static void mask_and_ack_8259A_irq(struc
>      /*
>       * this is the slow path - should happen rarely.
>       */
> -    if (i8259A_irq_real(desc->irq))
> +    if (i8259A_irq_real(irq))
>          /*
>           * oops, the IRQ _is_ in service according to the
>           * 8259A - not spurious, go handle it.
> @@ -283,7 +298,7 @@ static void mask_and_ack_8259A_irq(struc
>           * lets ACK and report it. [once per IRQ]
>           */
>          if (!(spurious_irq_mask & irqmask)) {
> -            printk("spurious 8259A interrupt: IRQ%d.\n", desc->irq);
> +            printk("spurious 8259A interrupt: IRQ%d.\n", irq);
>              spurious_irq_mask |= irqmask;
>          }
>          /*
> @@ -352,13 +367,19 @@ void __devinit init_8259A(int auto_eoi)
>                                 is to be investigated) */
>  
>      if (auto_eoi)
> +    {
>          /*
>           * in AEOI mode we just have to mask the interrupt
>           * when acking.
>           */
>          i8259A_irq_type.ack = disable_8259A_irq;
> +        bogus_8259A_irq = _disable_8259A_irq;
> +    }
>      else
> +    {
>          i8259A_irq_type.ack = mask_and_ack_8259A_irq;
> +        bogus_8259A_irq = _mask_and_ack_8259A_irq;
> +    }
>  
>      udelay(100);            /* wait for 8259A to initialize */
>  
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -811,9 +811,17 @@ void do_IRQ(struct cpu_user_regs *regs)
>          if (direct_apic_vector[vector] != NULL) {
>              (*direct_apic_vector[vector])(regs);
>          } else {
> -            ack_APIC_irq();
> -            printk("%s: %d.%d No irq handler for vector (irq %d)\n",
> -                   __func__, smp_processor_id(), vector, irq);
> +            const char *kind = ", LAPIC";
> +
> +            if ( apic_isr_read(vector) )
> +                ack_APIC_irq();
> +            else
> +                kind = "";
> +            if ( vector >= FIRST_LEGACY_VECTOR &&
> +                 vector <= LAST_LEGACY_VECTOR )
> +                bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR);
> +            printk("CPU%u: No irq handler for vector %02x (IRQ %d%s)\n",
> +                   smp_processor_id(), vector, irq, kind);
>              TRACE_1D(TRC_HW_IRQ_UNMAPPED_VECTOR, vector);
>          }
>          goto out_no_unlock;
> --- a/xen/include/asm-x86/apic.h
> +++ b/xen/include/asm-x86/apic.h
> @@ -146,6 +146,12 @@ static __inline void apic_icr_write(u32 
>      }
>  }
>  
> +static __inline bool_t apic_isr_read(u8 vector)
> +{
> +    return (apic_read(APIC_ISR + ((vector & ~0x1f) >> 1)) >>
> +            (vector & 0x1f)) & 1;
> +}
> +
>  static __inline u32 get_apic_id(void) /* Get the physical APIC id */
>  {
>      u32 id = apic_read(APIC_ID);
> --- a/xen/include/asm-x86/irq.h
> +++ b/xen/include/asm-x86/irq.h
> @@ -104,6 +104,7 @@ void mask_8259A(void);
>  void unmask_8259A(void);
>  void init_8259A(int aeoi);
>  void make_8259A_irq(unsigned int irq);
> +extern void (*bogus_8259A_irq)(unsigned int irq);
>  int i8259A_suspend(void);
>  int i8259A_resume(void);
>  



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