Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 8 Jun 2010 17:08:13 +0000 (UTC)
From:      John Baldwin <jhb@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r208915 - in head/sys: amd64/amd64 i386/i386
Message-ID:  <201006081708.o58H8DLn061374@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: jhb
Date: Tue Jun  8 17:08:13 2010
New Revision: 208915
URL: http://svn.freebsd.org/changeset/base/208915

Log:
  - Use a bit more care when moving I/O APIC interrupts between CPUs.  Mask
    the interrupt followed by a brief delay if it is not currently masked
    before moving the interrupt.
  - Move the icu_lock out of ioapic_program_intpin() and into callers.  This
    closes a race where ioapic_program_intpin() could use a stale value of
    the masked state to compute the masked bit in the register.
  
  Reviewed by:	mav
  MFC after:	2 weeks

Modified:
  head/sys/amd64/amd64/io_apic.c
  head/sys/i386/i386/io_apic.c

Modified: head/sys/amd64/amd64/io_apic.c
==============================================================================
--- head/sys/amd64/amd64/io_apic.c	Tue Jun  8 16:48:59 2010	(r208914)
+++ head/sys/amd64/amd64/io_apic.c	Tue Jun  8 17:08:13 2010	(r208915)
@@ -261,16 +261,15 @@ ioapic_program_intpin(struct ioapic_ints
 	 * If a pin is completely invalid or if it is valid but hasn't
 	 * been enabled yet, just ensure that the pin is masked.
 	 */
+	mtx_assert(&icu_lock, MA_OWNED);
 	if (intpin->io_irq == IRQ_DISABLED || (intpin->io_irq < NUM_IO_INTS &&
 	    intpin->io_vector == 0)) {
-		mtx_lock_spin(&icu_lock);
 		low = ioapic_read(io->io_addr,
 		    IOAPIC_REDTBL_LO(intpin->io_intpin));
 		if ((low & IOART_INTMASK) == IOART_INTMCLR)
 			ioapic_write(io->io_addr,
 			    IOAPIC_REDTBL_LO(intpin->io_intpin),
 			    low | IOART_INTMSET);
-		mtx_unlock_spin(&icu_lock);
 		return;
 	}
 
@@ -312,14 +311,12 @@ ioapic_program_intpin(struct ioapic_ints
 	}
 
 	/* Write the values to the APIC. */
-	mtx_lock_spin(&icu_lock);
 	intpin->io_lowreg = low;
 	ioapic_write(io->io_addr, IOAPIC_REDTBL_LO(intpin->io_intpin), low);
 	value = ioapic_read(io->io_addr, IOAPIC_REDTBL_HI(intpin->io_intpin));
 	value &= ~IOART_DEST;
 	value |= high;
 	ioapic_write(io->io_addr, IOAPIC_REDTBL_HI(intpin->io_intpin), value);
-	mtx_unlock_spin(&icu_lock);
 }
 
 static int
@@ -352,6 +349,19 @@ ioapic_assign_cpu(struct intsrc *isrc, u
 	if (new_vector == 0)
 		return (ENOSPC);
 
+	/*
+	 * Mask the old intpin if it is enabled while it is migrated.
+	 *
+	 * At least some level-triggered interrupts seem to need the
+	 * extra DELAY() to avoid being stuck in a non-EOI'd state.
+	 */
+	mtx_lock_spin(&icu_lock);
+	if (!intpin->io_masked) {
+		ioapic_write(io->io_addr, IOAPIC_REDTBL_LO(intpin->io_intpin),
+		    intpin->io_lowreg | IOART_INTMSET);
+		DELAY(100);
+	}
+
 	intpin->io_cpu = apic_id;
 	intpin->io_vector = new_vector;
 	if (isrc->is_handlers > 0)
@@ -364,6 +374,8 @@ ioapic_assign_cpu(struct intsrc *isrc, u
 		    intpin->io_vector);
 	}
 	ioapic_program_intpin(intpin);
+	mtx_unlock_spin(&icu_lock);
+
 	/*
 	 * Free the old vector after the new one is established.  This is done
 	 * to prevent races where we could miss an interrupt.
@@ -399,9 +411,11 @@ ioapic_disable_intr(struct intsrc *isrc)
 		/* Mask this interrupt pin and free its APIC vector. */
 		vector = intpin->io_vector;
 		apic_disable_vector(intpin->io_cpu, vector);
+		mtx_lock_spin(&icu_lock);
 		intpin->io_masked = 1;
 		intpin->io_vector = 0;
 		ioapic_program_intpin(intpin);
+		mtx_unlock_spin(&icu_lock);
 		apic_free_vector(intpin->io_cpu, vector, intpin->io_irq);
 	}
 }
@@ -443,6 +457,7 @@ ioapic_config_intr(struct intsrc *isrc, 
 	 * XXX: Should we write to the ELCR if the trigger mode changes for
 	 * an EISA IRQ or an ISA IRQ with the ELCR present?
 	 */
+	mtx_lock_spin(&icu_lock);
 	if (intpin->io_bus == APIC_BUS_EISA)
 		pol = INTR_POLARITY_HIGH;
 	changed = 0;
@@ -464,6 +479,7 @@ ioapic_config_intr(struct intsrc *isrc, 
 	}
 	if (changed)
 		ioapic_program_intpin(intpin);
+	mtx_unlock_spin(&icu_lock);
 	return (0);
 }
 
@@ -473,8 +489,10 @@ ioapic_resume(struct pic *pic)
 	struct ioapic *io = (struct ioapic *)pic;
 	int i;
 
+	mtx_lock_spin(&icu_lock);
 	for (i = 0; i < io->io_numintr; i++)
 		ioapic_program_intpin(&io->io_pins[i]);
+	mtx_unlock_spin(&icu_lock);
 }
 
 /*

Modified: head/sys/i386/i386/io_apic.c
==============================================================================
--- head/sys/i386/i386/io_apic.c	Tue Jun  8 16:48:59 2010	(r208914)
+++ head/sys/i386/i386/io_apic.c	Tue Jun  8 17:08:13 2010	(r208915)
@@ -261,16 +261,15 @@ ioapic_program_intpin(struct ioapic_ints
 	 * If a pin is completely invalid or if it is valid but hasn't
 	 * been enabled yet, just ensure that the pin is masked.
 	 */
+	mtx_assert(&icu_lock, MA_OWNED);
 	if (intpin->io_irq == IRQ_DISABLED || (intpin->io_irq < NUM_IO_INTS &&
 	    intpin->io_vector == 0)) {
-		mtx_lock_spin(&icu_lock);
 		low = ioapic_read(io->io_addr,
 		    IOAPIC_REDTBL_LO(intpin->io_intpin));
 		if ((low & IOART_INTMASK) == IOART_INTMCLR)
 			ioapic_write(io->io_addr,
 			    IOAPIC_REDTBL_LO(intpin->io_intpin),
 			    low | IOART_INTMSET);
-		mtx_unlock_spin(&icu_lock);
 		return;
 	}
 
@@ -312,14 +311,12 @@ ioapic_program_intpin(struct ioapic_ints
 	}
 
 	/* Write the values to the APIC. */
-	mtx_lock_spin(&icu_lock);
 	intpin->io_lowreg = low;
 	ioapic_write(io->io_addr, IOAPIC_REDTBL_LO(intpin->io_intpin), low);
 	value = ioapic_read(io->io_addr, IOAPIC_REDTBL_HI(intpin->io_intpin));
 	value &= ~IOART_DEST;
 	value |= high;
 	ioapic_write(io->io_addr, IOAPIC_REDTBL_HI(intpin->io_intpin), value);
-	mtx_unlock_spin(&icu_lock);
 }
 
 static int
@@ -352,6 +349,19 @@ ioapic_assign_cpu(struct intsrc *isrc, u
 	if (new_vector == 0)
 		return (ENOSPC);
 
+	/*
+	 * Mask the old intpin if it is enabled while it is migrated.
+	 *
+	 * At least some level-triggered interrupts seem to need the
+	 * extra DELAY() to avoid being stuck in a non-EOI'd state.
+	 */
+	mtx_lock_spin(&icu_lock);
+	if (!intpin->io_masked) {
+		ioapic_write(io->io_addr, IOAPIC_REDTBL_LO(intpin->io_intpin),
+		    intpin->io_lowreg | IOART_INTMSET);
+		DELAY(100);
+	}
+
 	intpin->io_cpu = apic_id;
 	intpin->io_vector = new_vector;
 	if (isrc->is_handlers > 0)
@@ -364,6 +374,8 @@ ioapic_assign_cpu(struct intsrc *isrc, u
 		    intpin->io_vector);
 	}
 	ioapic_program_intpin(intpin);
+	mtx_unlock_spin(&icu_lock);
+
 	/*
 	 * Free the old vector after the new one is established.  This is done
 	 * to prevent races where we could miss an interrupt.
@@ -399,9 +411,11 @@ ioapic_disable_intr(struct intsrc *isrc)
 		/* Mask this interrupt pin and free its APIC vector. */
 		vector = intpin->io_vector;
 		apic_disable_vector(intpin->io_cpu, vector);
+		mtx_lock_spin(&icu_lock);
 		intpin->io_masked = 1;
 		intpin->io_vector = 0;
 		ioapic_program_intpin(intpin);
+		mtx_unlock_spin(&icu_lock);
 		apic_free_vector(intpin->io_cpu, vector, intpin->io_irq);
 	}
 }
@@ -443,6 +457,7 @@ ioapic_config_intr(struct intsrc *isrc, 
 	 * XXX: Should we write to the ELCR if the trigger mode changes for
 	 * an EISA IRQ or an ISA IRQ with the ELCR present?
 	 */
+	mtx_lock_spin(&icu_lock);
 	if (intpin->io_bus == APIC_BUS_EISA)
 		pol = INTR_POLARITY_HIGH;
 	changed = 0;
@@ -464,6 +479,7 @@ ioapic_config_intr(struct intsrc *isrc, 
 	}
 	if (changed)
 		ioapic_program_intpin(intpin);
+	mtx_unlock_spin(&icu_lock);
 	return (0);
 }
 
@@ -473,8 +489,10 @@ ioapic_resume(struct pic *pic)
 	struct ioapic *io = (struct ioapic *)pic;
 	int i;
 
+	mtx_lock_spin(&icu_lock);
 	for (i = 0; i < io->io_numintr; i++)
 		ioapic_program_intpin(&io->io_pins[i]);
+	mtx_unlock_spin(&icu_lock);
 }
 
 /*



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201006081708.o58H8DLn061374>