[U-Boot] [PATCH] ARMv8: Bug fix of dcache_disable()

[Mark Rutland]

On Thu, Feb 26, 2015 at 03:06:10PM +0000, FengHua wrote:
> 
> hi Mark,

Hi,

>        You did very detailed analysis of the cache beheaviour. Yes, this patch is not perfect.
> But it did fix the actually existed bug. I will try to describe it more clearly in the following.

While this may appear to work on your platform, it simply trades one bug
for another by relying on guarantees that the architecture does not
provide.

Fundamentally, you require a sequence like:

* Clean by VA any data/code you will need after the caches are disabled.
  This may leave clean entries in the cache, but the data/code will be
  visible to the CPU when SCTLR_ELX.{C,M} are clear.

* DSB to complete the maintenance.

* In assembly, without relying on data/code not clean to the PoC:
  - Clear SCTLR_ELx.{C,M}
  - ISB
  - Flush the architected caches by Set/Way
  - (Clean+)Invalidate by VA any region of memory you need to write to
    for which a dirty line could exist in the L3 (e.g. your stack).
  - DSB to complete the maintenance.

* Flush the L3.

To the best of my knowledge, anything short of that relies on guarantees
that the architecture does not provide. The above sequence does assume
that no other masters are active which could allocate into the caches
and/or acquire dirty lines.

> > * Set/Way operations aren't guaranteed to flush data to the PoC in the
> >   presence of a system cache like CCN, so we have no guarantee that
> >   we've pushed any data to the PoC. Per ARMv8 only maintenance by VA
> >   guarantees this (but luckily maintenance by VA is mandated to be
> >   respected by such system caches).
> flush_dcache_all should flush both cache existed in architecture defined cache 
> hierachy and outer cache(such as L3 in CCN), a previous patch did this.

While the SCTLR_ELx.{C,M} bits are set, Set/Way operations may not even
force data out of the CPU's architected caches, so there is no guarantee
that the data is flushed to the L3.

> > * While the cache is enabled lines could theoretically migrate between
> >   set/way slots mid-sequence (e.g. with speculative accesses and an
> >   exclusive L1/L2 configuration). I don't believe this currently happens
> >   in practice, but the architecture does not prevent this.
> > 
> > So I don't see that moving this maintenance solves any existing problem,
> > and it introduces new ones.
> The bug actually exist when flush_dcache_all is after of set_sctlr.
> I try to describe it more detailed.
> flush_dcache_all is a C routine, it will preserve return address in stack. The stack
> memory may be in the cache. If we call set_sctlr to disable cache
> first, then flush_dcache_all will write the return address directly into memory instead of cache.
> But there is another copy of the stack memory in the cache, the correct return address in
> memory will be rewritten by wrong value when flush cache, then flush_dcache_all get wrong return address.
> The best solution is writing flush_dcache_all totally in assembly and make sure no memory access
> between flush_dcache_all and set_sctlr.

Even when written in assembly, the cache flush will have to occur after
the SCTLR_ELx.{C,M} bits are cleared in order to guarantee that the
cache is in a quiescent state.

[...]

> > The above isn't theoretical, we were hit by these issues in Linux. See
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5e051531447259e5df95c44bccb69979537c19e4
> flush_cache_all of linux kernel did not flush outer cache. I think the patch mostly deal with this.
> right?

In the arm64 Linux port we don't always have access to the system cache
interface (which could be secure-only), so we only rely on cache
maintenance by VA, which system caches are mandated to respect.

This does mean that there may be entries in the caches, but we are able
to perform maintenance on the portions of the address space which we
care about.

Thanks,
Mark.