On Mon, Jan 25, 2016 at 6:43 PM, Andrew Turner <and...@fubar.geek.nz> wrote: > On Mon, 25 Jan 2016 10:28:46 -0700 > Ian Lepore <i...@freebsd.org> wrote: > >> On Mon, 2016-01-25 at 17:00 +0000, Andrew Turner wrote: >> > On Mon, 25 Jan 2016 17:44:53 +0200 >> > Konstantin Belousov <kostik...@gmail.com> wrote: >> > >> > > On Mon, Jan 25, 2016 at 02:09:36PM +0000, Svatopluk Kraus wrote: >> > > > Author: skra >> > > > Date: Mon Jan 25 14:09:35 2016 >> > > > New Revision: 294727 >> > > > URL: https://svnweb.freebsd.org/changeset/base/294727 >> > > > >> > > > Log: >> > > > Fix an occasional undefined instruction abort during module >> > > > loading. >> > > > Even if data cache maintenance was done by IO code, the >> > > > relocation >> > > > fixup process creates dirty cache entries that we must write >> > > > back >> > > > before doing icache sync. >> > > Does arm64 need the same fix ? >> > > >> > >> > I don't think so. On arm64 we call cpu_icache_sync_range to sync the >> > I >> > and D cache. This will clean the D-Cache to the Point of Coherency, >> > then invalidate the I-Cache. I think this is slightly wrong as we >> > only >> > need to clean to the Point of Unification. >> > >> > Looking at the ARMv7 implementation of cpu_icache_sync_all is wrong, >> > it >> > only invalidates the I-Cache. cpu_icache_sync_range is almost >> > correct, >> > it will clean the D-Cache, with the same issue as arm64, but it is >> > missing a barrier after this operation, and is missing a branch >> > predictor invalidation. >> > >> > This change seems to be working around the brokenness of the >> > existing cache sync operations rather than fixing them, however I >> > don't know the >> > details on why this approach to fixing the issue was taken. >> > >> > Andrew >> > >> >> I disagree that the fact that icache_sync only cleans the icache means >> it's broken. It means that the callers have to do the right thing >> with the data cache before calling the icache ops depending on the >> situation, and that's what arm code has always done. > > If it's not broken then we are needlessly issuing extra cache handling > operations in other places. > > If we expect these function to sync the icache with an already clean > dcache then armv7_icache_sync_all looks correct (other than a missing > branch predictor invalidation). However in this case why are we > cleaning the dcache in armv7_icache_sync_range? > > In arm9_icache_sync_all and arm9_icache_sync_range we flush the icache > before cleaning the dcache. This also seems wrong given the above, > however I'm unsure on the details of cache handling on these older CPUs. > > Andrew
Well, looking at patch, there is #if __ARM_ARCH >= 6 in the patch. For armv6/v7, the patch is perfectly correct. It does dcache_wb_pou() and then icache_inv_all(). According to arm arm manual, icache-invalidate-all does invalidate branch predictor cache too. I believe that cpu specific function (like armv7_icache_sync_all) are not used anymore for ARM_NEW_PMAP, which means armv6/v7. WRT armv4, I'm not familiar with their implementation, but the function calling sequence should be okay too. Svata _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"