svn commit: r294727 - head/sys/arm/arm

[Andrew Turner]

On Mon, 25 Jan 2016 10:28:46 -0700
Ian Lepore <ian at 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 at 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