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

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 14/05/2012 13:39, "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).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Looks sensible, and I suppose good to have for 4.2.

Acked-by: Keir Fraser <keir@xxxxxxx>

> --- 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,12 @@ 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);
> +            if ( vector < FIRST_LEGACY_VECTOR || vector > LAST_LEGACY_VECTOR
> )
> +                ack_APIC_irq();
> +            else
> +                bogus_8259A_irq(vector - FIRST_LEGACY_VECTOR);
> +            printk("CPU%u: No handler for vector %02x (IRQ %d)\n",
> +                   smp_processor_id(), vector, irq);
>              TRACE_1D(TRC_HW_IRQ_UNMAPPED_VECTOR, vector);
>          }
>          goto out_no_unlock;
> --- 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



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