Date: Mon, 25 Jan 2016 10:28:46 -0700 From: Ian Lepore <ian@freebsd.org> To: Andrew Turner <andrew@fubar.geek.nz>, Konstantin Belousov <kostikbel@gmail.com> Cc: Svatopluk Kraus <skra@FreeBSD.org>, 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: <1453742926.42081.8.camel@freebsd.org> In-Reply-To: <20160125170053.7ae20536@zapp.Home> References: <201601251409.u0PE9abE013306@repo.freebsd.org> <20160125154453.GA3942@kib.kiev.ua> <20160125170053.7ae20536@zapp.Home>
next in thread | previous in thread | raw e-mail | index | archive | help
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. In this case, we weren't failing to do the right thing because of broken code, we were failing due to my flawed analysis of the situation. As the original comment indicated, I believed the data caches were already clean due to the IO code cache operations. I negle cted to take into account the relocation fixup activity after the IO which re-dirtied some of the cache lines. -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1453742926.42081.8.camel>