From 11d47032eec59ee3e57c84217839741d54ddcf5a Mon Sep 17 00:00:00 2001 From: Ian Lepore Date: Sat, 24 May 2014 16:21:16 +0000 Subject: [PATCH] Eliminate one of the causes of spurious interrupts on armv6. The arm weak memory ordering model allows writes to different devices to complete out of order, leading to a situation where the write that clears an interrupt source at a device can complete after a write that unmasks and EOIs the interrupt at the interrupt controller, leading to a spurious re-interrupt. This adds a generic barrier function specific to the needs of interrupt controllers, and calls that function from the GIC and TI AINTC controllers. There may still be other soc-specific controllers that need to make the call. Reviewed by: cognet, Svatopluk Kraus MFC after: 3 days --- sys/arm/arm/gic.c | 10 +++++-- sys/arm/arm/intr.c | 64 ++++++++++++++++++++++++++++++++++++++++++ sys/arm/include/intr.h | 2 ++ sys/arm/ti/aintc.c | 2 ++ 4 files changed, 76 insertions(+), 2 deletions(-) diff --git a/sys/arm/arm/gic.c b/sys/arm/arm/gic.c index 9472edb92a78..4d523c335c64 100644 --- a/sys/arm/arm/gic.c +++ b/sys/arm/arm/gic.c @@ -83,6 +83,8 @@ __FBSDID("$FreeBSD$"); #define GICC_ABPR 0x001C /* v1 ICCABPR */ #define GICC_IIDR 0x00FC /* v1 ICCIIDR*/ +#define GIC_LAST_IPI 15 /* Irqs 0-15 are IPIs. */ + /* First bit is a polarity bit (0 - low, 1 - high) */ #define GICD_ICFGR_POL_LOW (0 << 0) #define GICD_ICFGR_POL_HIGH (1 << 0) @@ -268,6 +270,8 @@ gic_post_filter(void *arg) { uintptr_t irq = (uintptr_t) arg; + if (irq > GIC_LAST_IPI) + arm_irq_memory_barrier(irq); gic_c_write_4(GICC_EOIR, irq); } @@ -284,13 +288,13 @@ arm_get_next_irq(int last_irq) * have this information later. */ - if ((active_irq & 0x3ff) < 16) + if ((active_irq & 0x3ff) <= GIC_LAST_IPI) gic_c_write_4(GICC_EOIR, active_irq); active_irq &= 0x3FF; if (active_irq == 0x3FF) { if (last_irq == -1) - printf("Spurious interrupt detected [0x%08x]\n", active_irq); + printf("Spurious interrupt detected\n"); return -1; } @@ -309,6 +313,8 @@ void arm_unmask_irq(uintptr_t nb) { + if (nb > GIC_LAST_IPI) + arm_irq_memory_barrier(nb); gic_d_write_4(GICD_ISENABLER(nb >> 5), (1UL << (nb & 0x1F))); } diff --git a/sys/arm/arm/intr.c b/sys/arm/arm/intr.c index ac93cbdfeb14..ba151276d332 100644 --- a/sys/arm/arm/intr.c +++ b/sys/arm/arm/intr.c @@ -149,3 +149,67 @@ arm_irq_handler(struct trapframe *frame) } } } + +/* + * arm_irq_memory_barrier() + * + * Ensure all writes to device memory have reached devices before proceeding. + * + * This is intended to be called from the post-filter and post-thread routines + * of an interrupt controller implementation. A peripheral device driver should + * use bus_space_barrier() if it needs to ensure a write has reached the + * hardware for some reason other than clearing interrupt conditions. + * + * The need for this function arises from the ARM weak memory ordering model. + * Writes to locations mapped with the Device attribute bypass any caches, but + * are buffered. Multiple writes to the same device will be observed by that + * device in the order issued by the cpu. Writes to different devices may + * appear at those devices in a different order than issued by the cpu. That + * is, if the cpu writes to device A then device B, the write to device B could + * complete before the write to device A. + * + * Consider a typical device interrupt handler which services the interrupt and + * writes to a device status-acknowledge register to clear the interrupt before + * returning. That write is posted to the L2 controller which "immediately" + * places it in a store buffer and automatically drains that buffer. This can + * be less immediate than you'd think... There may be no free slots in the store + * buffers, so an existing buffer has to be drained first to make room. The + * target bus may be busy with other traffic (such as DMA for various devices), + * delaying the drain of the store buffer for some indeterminate time. While + * all this delay is happening, execution proceeds on the CPU, unwinding its way + * out of the interrupt call stack to the point where the interrupt driver code + * is ready to EOI and unmask the interrupt. The interrupt controller may be + * accessed via a faster bus than the hardware whose handler just ran; the write + * to unmask and EOI the interrupt may complete quickly while the device write + * to ack and clear the interrupt source is still lingering in a store buffer + * waiting for access to a slower bus. With the interrupt unmasked at the + * interrupt controller but still active at the device, as soon as interrupts + * are enabled on the core the device re-interrupts immediately: now you've got + * a spurious interrupt on your hands. + * + * The right way to fix this problem is for every device driver to use the + * proper bus_space_barrier() calls in its interrupt handler. For ARM a single + * barrier call at the end of the handler would work. This would have to be + * done to every driver in the system, not just arm-specific drivers. + * + * Another potential fix is to map all device memory as Strongly-Ordered rather + * than Device memory, which takes the store buffers out of the picture. This + * has a pretty big impact on overall system performance, because each strongly + * ordered memory access causes all L2 store buffers to be drained. + * + * A compromise solution is to have the interrupt controller implementation call + * this function to establish a barrier between writes to the interrupt-source + * device and writes to the interrupt controller device. + * + * This takes the interrupt number as an argument, and currently doesn't use it. + * The plan is that maybe some day there is a way to flag certain interrupts as + * "memory barrier safe" and we can avoid this overhead with them. + */ +void +arm_irq_memory_barrier(uintptr_t irq) +{ + + dsb(); + cpu_l2cache_drain_writebuf(); +} + diff --git a/sys/arm/include/intr.h b/sys/arm/include/intr.h index 49d6c0594d3b..e2d0feb3c626 100644 --- a/sys/arm/include/intr.h +++ b/sys/arm/include/intr.h @@ -79,6 +79,8 @@ extern void (*arm_post_filter)(void *); extern int (*arm_config_irq)(int irq, enum intr_trigger trig, enum intr_polarity pol); +void arm_irq_memory_barrier(uintptr_t); + void gic_init_secondary(void); #endif /* _MACHINE_INTR_H */ diff --git a/sys/arm/ti/aintc.c b/sys/arm/ti/aintc.c index 659db15c820f..213a1d366c71 100644 --- a/sys/arm/ti/aintc.c +++ b/sys/arm/ti/aintc.c @@ -180,5 +180,7 @@ arm_mask_irq(uintptr_t nb) void arm_unmask_irq(uintptr_t nb) { + + arm_irq_memory_barrier(nb); aintc_write_4(INTC_MIR_CLEAR(nb >> 5), (1UL << (nb & 0x1F))); }