[U-Boot] [Resend RFC PATCH 1/2] armv8: Fix dcache disable function

[Mark Rutland]

Hi,

Sorry for joining this a bit late; apologies if the below re-treads
ground already covered.

On Wed, Oct 19, 2016 at 09:25:02AM -0600, Stephen Warren wrote:
> On 10/14/2016 02:17 PM, York Sun wrote:
> >Current code turns off d-cache first, then flush all levels of cache.
> >This results data loss. As soon as d-cache is off, the dirty cache
> >is discarded according to the test on LS2080A. This issue was not
> >seen as long as external L3 cache was flushed to push the data to
> >main memory. However, external L3 cache is not guaranteed to have
> >the data. To fix this, flush the d-cache by way/set first to make
> >sure cache is clean before turning it off.
> 
> >diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> 
> >@@ -478,9 +478,9 @@ void dcache_disable(void)
> 
> >+	flush_dcache_all();
> > 	set_sctlr(sctlr & ~(CR_C|CR_M));
> >
> >-	flush_dcache_all();
> > 	__asm_invalidate_tlb_all();
> 
> I talked to Mark Rutland at ARM, and I believe the current code is
> correct.

Well, almost, but not quite. It's a long story... ;)

I gave a primer [1,2] on the details at ELC earlier this year, which may
or may not be useful.

The big details are:

* Generaly "Flush" is ambiguous/meaningless. Here you seem to want
  clean+invalidate.

* Set/Way operations are for IMPLEMENTATION DEFINED (i.e. SoC-specific)
  cache maintenance sequences, and are not truly portable (e.g. not
  affecting system caches).
  
  I assume that an earlier boot stage initialised the caches prior to
  U-Boot. Given that, you *only* need to perform maintenance for the
  memory you have (at any point) mapped with cacheable attrbiutes, which
  should be a small subset of the PA space. With ARMv8-A, broadcast
  maintenance to the PoC should affect all relevant caches (assuming you
  use the correct shareability attributes).

* You *cannot* write a dcache disable routine in C, as the compiler can
  perform a number of implicit memory accesses (e.g. stack, globals,
  GOT). For that alone, I do not believe the code above is correct.

  Note that we have seen this being an issue in practice, before we got
  rid of Set/Way ops from arm64 Linux (see commit 5e051531447259e5).

* Your dcache disable code *must* be clean to the PoC, prior to
  execution, or instruction fetches could see stale data. You can first
  *clean* this to the PoC, which is sufficient to avoid the problems
  above.

* The SCTLR_ELx.{C,I} bits do not enable/disable caches; they merely
  activate/deactiveate cacheable attributes on data/instruction fetches.

  Note that cacheable instruction fetches can allocate into unified/data
  caches.
  
  Also, note that the I bit is independent of the C bit, and the
  attributes it provides differ when the M bit is clear. Generally, I
  would advise that at all times M == C == I, as that leads to the least
  surprise.

Thanks,
Mark.

[1] http://events.linuxfoundation.org/sites/events/files/slides/slides_17.pdf
[2] https://www.youtube.com/watch?v=F0SlIMHRnLk