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

[Ian Lepore]

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.

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