Date: Mon, 25 Jan 2016 19:47:11 +0100 From: Svatopluk Kraus <onwahe@gmail.com> To: Andrew Turner <andrew@fubar.geek.nz> Cc: Ian Lepore <ian@freebsd.org>, Konstantin Belousov <kostikbel@gmail.com>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r294727 - head/sys/arm/arm Message-ID: <CAFHCsPVLO73NGHURZPcWMs%2BU3LAr26wCtxJ7DrYNMZpQ3C%2BMfA@mail.gmail.com> In-Reply-To: <20160125174338.29103e6b@zapp.Home> References: <201601251409.u0PE9abE013306@repo.freebsd.org> <20160125154453.GA3942@kib.kiev.ua> <20160125170053.7ae20536@zapp.Home> <1453742926.42081.8.camel@freebsd.org> <20160125174338.29103e6b@zapp.Home>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jan 25, 2016 at 6:43 PM, Andrew Turner <andrew@fubar.geek.nz> wrote: > On Mon, 25 Jan 2016 10:28:46 -0700 > Ian Lepore <ian@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 <kostikbel@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAFHCsPVLO73NGHURZPcWMs%2BU3LAr26wCtxJ7DrYNMZpQ3C%2BMfA>