From owner-freebsd-arm@FreeBSD.ORG Tue Sep 6 21:15:59 2011 Return-Path: Delivered-To: freebsd-arm@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 815C91065670 for ; Tue, 6 Sep 2011 21:15:59 +0000 (UTC) (envelope-from freebsd@damnhippie.dyndns.org) Received: from qmta13.emeryville.ca.mail.comcast.net (qmta13.emeryville.ca.mail.comcast.net [76.96.27.243]) by mx1.freebsd.org (Postfix) with ESMTP id 6728B8FC13 for ; Tue, 6 Sep 2011 21:15:58 +0000 (UTC) Received: from omta17.emeryville.ca.mail.comcast.net ([76.96.30.73]) by qmta13.emeryville.ca.mail.comcast.net with comcast id VYmg1h0031afHeLADZ2kVj; Tue, 06 Sep 2011 21:02:44 +0000 Received: from damnhippie.dyndns.org ([24.8.232.202]) by omta17.emeryville.ca.mail.comcast.net with comcast id VYyT1h01R4NgCEG8dYyUeM; Tue, 06 Sep 2011 20:58:28 +0000 Received: from [172.22.42.240] (revolution.hippie.lan [172.22.42.240]) by damnhippie.dyndns.org (8.14.3/8.14.3) with ESMTP id p86L2lwK038796 for ; Tue, 6 Sep 2011 15:02:47 -0600 (MDT) (envelope-from freebsd@damnhippie.dyndns.org) From: Ian Lepore To: freebsd-arm In-Reply-To: <201109031940.p83Je9fo004190@freefall.freebsd.org> References: <201109031940.p83Je9fo004190@freefall.freebsd.org> Content-Type: text/plain Date: Tue, 06 Sep 2011 15:02:47 -0600 Message-Id: <1315342967.1671.21.camel@revolution.hippie.lan> Mime-Version: 1.0 X-Mailer: Evolution 2.26.0 FreeBSD GNOME Team Port Content-Transfer-Encoding: 7bit Subject: Re: arm/160431: [patch] Disable interrupts during busdma cache sync operations. X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting FreeBSD to the StrongARM Processor List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Sep 2011 21:15:59 -0000 After thinking more about Mark's question I've come to the conclusion that interrupts should only be disabled in the partial cachline flush case. Basically it comes down to the fact that only that case is special, in that we temporarily duplicate the state of the hardware into a local buffer then restore it, and we can't let the state of the real hardware change during that time. It also occurs to me that if it's necessary to disable interrupts during all cache operations done by the busdma code, then it should be just as necessary during the operations done by the pmap code, and it's not clear to me that those operations are all currently happening only while interrupts are disabled. So, here is a do-over on the patch that only disables interrupts in the partial cacheline case. Considering that the partial case seems to be pretty rare, and tends to involve buffers smaller a single cacheline, this should result in having interrupts disabled a whole lot less often during buffer syncs. Index: busdma_machdep.c =================================================================== RCS file: /local/base/FreeBSD-CVS/src/sys/arm/arm/busdma_machdep.c,v retrieving revision 1.50 diff -u -p -r1.50 busdma_machdep.c --- busdma_machdep.c 11 Mar 2010 21:16:54 -0000 1.50 +++ busdma_machdep.c 6 Sep 2011 19:06:32 -0000 @@ -1106,28 +1106,44 @@ bus_dmamap_sync_buf(void *buf, int len, cpu_l2cache_wbinv_range((vm_offset_t)buf, len); } } + /* + * Interrupts must be disabled if handling a partial cacheline flush, + * otherwise the interrupt handling code could modify data in the + * non-DMA part of a cacheline while we have it stashed away in the + * temporary stack buffer, then we end up restoring the stale value. As + * unlikely as this seems, it has been observed in the real world. + */ if (op & BUS_DMASYNC_POSTREAD) { - if ((vm_offset_t)buf & arm_dcache_align_mask) { - memcpy(_tmp_cl, (void *)((vm_offset_t)buf & ~ - arm_dcache_align_mask), - (vm_offset_t)buf & arm_dcache_align_mask); - } - if (((vm_offset_t)buf + len) & arm_dcache_align_mask) { - memcpy(_tmp_clend, (void *)((vm_offset_t)buf + len), - arm_dcache_align - (((vm_offset_t)(buf) + len) & - arm_dcache_align_mask)); + uint32_t intr; + int partial = (((vm_offset_t)buf) | len) & arm_dcache_align_mask; + + if (partial) { + intr = intr_disable(); + if ((vm_offset_t)buf & arm_dcache_align_mask) { + memcpy(_tmp_cl, (void *)((vm_offset_t)buf & ~ + arm_dcache_align_mask), + (vm_offset_t)buf & arm_dcache_align_mask); + } + if (((vm_offset_t)buf + len) & arm_dcache_align_mask) { + memcpy(_tmp_clend, (void *)((vm_offset_t)buf + len), + arm_dcache_align - (((vm_offset_t)(buf) + len) & + arm_dcache_align_mask)); + } } cpu_dcache_inv_range((vm_offset_t)buf, len); cpu_l2cache_inv_range((vm_offset_t)buf, len); - if ((vm_offset_t)buf & arm_dcache_align_mask) - memcpy((void *)((vm_offset_t)buf & - ~arm_dcache_align_mask), _tmp_cl, - (vm_offset_t)buf & arm_dcache_align_mask); - if (((vm_offset_t)buf + len) & arm_dcache_align_mask) - memcpy((void *)((vm_offset_t)buf + len), _tmp_clend, - arm_dcache_align - (((vm_offset_t)(buf) + len) & - arm_dcache_align_mask)); + if (partial) { + if ((vm_offset_t)buf & arm_dcache_align_mask) + memcpy((void *)((vm_offset_t)buf & + ~arm_dcache_align_mask), _tmp_cl, + (vm_offset_t)buf & arm_dcache_align_mask); + if (((vm_offset_t)buf + len) & arm_dcache_align_mask) + memcpy((void *)((vm_offset_t)buf + len), _tmp_clend, + arm_dcache_align - (((vm_offset_t)(buf) + len) & + arm_dcache_align_mask)); + intr_restore(intr); + } } }