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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/05/12 17:07, Jan Beulich wrote:
>>>> 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

Yes - that looks better.

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


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