Fixes bug with reading LAPIC ISR/IRR

Not sure what I was thinking there.

Be sure to read the documentation if you're using ipi_is_pending().

diff --git a/kern/arch/i686/apic.c b/kern/arch/i686/apic.c
index 4a930b9..84245f5 100644 (file)
--- a/kern/arch/i686/apic.c
+++ b/kern/arch/i686/apic.c
@@ -16,6 +16,7 @@
 #include <time.h>
 #include <assert.h>
 #include <stdio.h>
+#include <bitmask.h>
 
 system_timing_t RO system_timing = {0, 0, 0xffff, 0};
 
@@ -87,6 +88,20 @@ uint16_t pic_get_isr(void)
        return __pic_get_irq_reg(PIC_READ_ISR);
 }
 
+/* This works for any interrupt that goes through the LAPIC, but not things like
+ * ExtInts.  To prevent abuse, we'll use it just for IPIs for now (which only
+ * come via the APIC).
+ *
+ * Note that if you call this from an interrupt handler for 'vector' before you
+ * EOI, the ISR will show your bit as set.  It is the EOI that clears the bit
+ * from the ISR. */
+bool ipi_is_pending(uint8_t vector)
+{
+       /* The ISR/IRR are 256 bits long.  We want to check if 'vector' is set. */
+       return GET_BITMASK_BIT((uint8_t*)LAPIC_ISR, vector) ||
+              GET_BITMASK_BIT((uint8_t*)LAPIC_IRR, vector);
+}
+
 /*
  * Sets the LAPIC timer to go off after a certain number of ticks.  The primary
  * clock freq is actually the bus clock, which we figure out during timer_init
@@ -256,4 +271,3 @@ uint64_t getfreq(void)
 {
        return system_timing.tsc_freq;
 }
-

diff --git a/kern/arch/i686/apic.h b/kern/arch/i686/apic.h
index 1d43985..b11fb5b 100644 (file)
--- a/kern/arch/i686/apic.h
+++ b/kern/arch/i686/apic.h
@@ -108,6 +108,7 @@ void pic_unmask_irq(uint8_t irq);
 uint16_t pic_get_mask(void);
 uint16_t pic_get_irr(void);
 uint16_t pic_get_isr(void);
+bool ipi_is_pending(uint8_t vector);
 void __lapic_set_timer(uint32_t ticks, uint8_t vec, bool periodic, uint8_t div);
 void lapic_set_timer(uint32_t usec, bool periodic);
 uint32_t lapic_get_default_id(void);
@@ -139,7 +140,6 @@ static inline void send_all_others_ipi(uint8_t vector);
 static inline void send_ipi(uint8_t hw_coreid, uint8_t vector);
 static inline void send_group_ipi(uint8_t hw_groupid, uint8_t vector);
 static inline void __send_nmi(uint8_t os_coreid);
-static inline bool ipi_is_pending(uint8_t vector);
 
 #define mask_lapic_lvt(entry) \
        write_mmreg32(entry, read_mmreg32(entry) | LAPIC_LVT_MASK)
@@ -281,15 +281,6 @@ static inline void __send_nmi(uint8_t hw_coreid)
        lapic_wait_to_send();
 }
 
-/* This works for any interrupt that goes through the LAPIC, but not things like
- * ExtInts.  To prevent abuse, we'll use it just for IPIs for now. */
-static inline bool ipi_is_pending(uint8_t vector)
-{
-       /* TODO: look at this, it's so fucked up.  also, there's a bit that needs to
-        * be set to allow us to read from the IRR. */
-       return (LAPIC_ISR & vector) || (LAPIC_IRR & vector);
-}
-
 /* To change the LAPIC Base (not recommended):
        msr_val = read_msr(IA32_APIC_BASE);
        msr_val = msr_val & ~MSR_APIC_BASE_ADDRESS | 0xfaa00000;

diff --git a/kern/arch/i686/trap.c b/kern/arch/i686/trap.c
index 48ae3e8..e84de91 100644 (file)
--- a/kern/arch/i686/trap.c
+++ b/kern/arch/i686/trap.c
@@ -555,6 +555,9 @@ void __kernel_message(struct trapframe *tf, void *data)
        per_cpu_info_t *myinfo = &per_cpu_info[core_id()];
        kernel_message_t msg_cp, *k_msg;
 
+       /* Important that we send the EOI first, so that the ipi_is_pending check
+        * doesn't see the irq we're servicing (which it would see if it was still
+        * 'inside' the IRQ handler (which to the APIC ends upon EOI)). */
        lapic_send_eoi();
        while (1) { // will break out when there are no more messages
                /* Try to get an immediate message.  Exec and free it. */
@@ -574,7 +577,7 @@ void __kernel_message(struct trapframe *tf, void *data)
                        msg_cp = *k_msg;
                        kmem_cache_free(kernel_msg_cache, (void*)k_msg);
                        /* make sure an IPI is pending if we have more work */
-                       /* techincally, we don't need to lock when checking */
+                       /* technically, we don't need to lock when checking */
                        if (!STAILQ_EMPTY(&myinfo->routine_amsgs) &&
                               !ipi_is_pending(I_KERNEL_MSG))
                                send_self_ipi(I_KERNEL_MSG);