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

[Svatopluk Kraus]

On Mon, Jan 25, 2016 at 6:43 PM, Andrew Turner <andrew at fubar.geek.nz> wrote:
> 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

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