On 10/24/2016 04:44 AM, Mark Rutland wrote:
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... ;)
To be clear, I was referring specifically/only to the relative ordering
of the call to flush_dcache_all() and the SCTLR register modification,
rather than wider aspects of the code:-)
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.
Yes. I think the naming of this code is driven by U-Boot's
cross-architecture compatibility and history, hence why it doesn't use
the expected ARM terminology.
* 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).
There is another thread (or fork of this thread) where York suggests
replacing this code with operations by VA instead.
* 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).
I agree. In practice, at least with the compiler I happen to be using,
we are currently getting lucky and there aren't any RAM accesses emitted
by the compiler in this part of the code. Admittedly that won't hold
true for everyone or across all of time though, so this should certainly
be fixed...
* 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.
I'm not sure if we have any documented rules/assumptions regarding
cache/... state upon U-Boot entry like the Linux kernel does. It would
probably be a good idea to do so...
I believe that for this particular piece of code, since it's always
executed late in U-Boot's operation, the requirement is covered by
U-Boot's code relocation routine, since that memcpy()s' .text/.data to
the top of usable RAM, and hence hopefully is already fully cleaning the
dcache and invalidating the icache afterwards.
* 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.
I think M==C==I is generally true in U-Boot, except for a limited time
right before it jumps to the OS, where icache and dcache "are
disabled"[1] separately, but one right after the other.
[1] to abuse the terminology that you pointed out was incorrect:-)
Thanks,
Mark.
[1] http://events.linuxfoundation.org/sites/events/files/slides/slides_17.pdf
[2] https://www.youtube.com/watch?v=F0SlIMHRnLk
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
