[U-Boot] [PATCH 1/2] armv8: caches: Disable dcache after flush

[Mark Rutland]

On Thu, Apr 16, 2015 at 06:17:59AM +0100, Siva Durga Prasad Paladugu wrote:
> Hi Mark.
> 
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland at arm.com]
> > Sent: Wednesday, April 15, 2015 6:41 PM
> > To: Michal Simek
> > Cc: u-boot at lists.denx.de; Tom Rini; Siva Durga Prasad Paladugu; Varun Sethi;
> > Arnab Basu; York Sun
> > Subject: Re: [U-Boot] [PATCH 1/2] armv8: caches: Disable dcache after flush
> > 
> > On Wed, Apr 15, 2015 at 12:33:00PM +0100, Michal Simek wrote:
> > > From: Siva Durga Prasad Paladugu <siva.durga.paladugu at xilinx.com>
> > >
> > > Always disable dcache after the flush operation The following sequence
> > > is advisable while disabling d-cache:
> > > 1. disable_dcache() - flushes and disables d-cache 2.
> > > invalidate_dcache_all() - invalid any entry that came to the cache
> > >    in the short period after the cache was flushed but before the
> > >    cache got disabled
> > 
> > For reasons I have described previously (see [1,2,3]), this is unsafe.
> > The first cache flush may achieve nothing.
> > 
> > If you need data out at the PoC before disabling the cache, then you should
> > first use maintenance by VA to push that data out.
> > 
> > Thanks,
> > Mark.
> > 
> > [1] http://lists.denx.de/pipermail/u-boot/2015-February/204403.html
> > [2] http://lists.denx.de/pipermail/u-boot/2015-February/204407.html
> > [3] http://lists.denx.de/pipermail/u-boot/2015-February/204702.html
> > 
> > >
> > > Signed-off-by: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
> > > Signed-off-by: Michal Simek <michal.simek at xilinx.com>
> > > ---
> > >
> > >  arch/arm/cpu/armv8/cache_v8.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/arm/cpu/armv8/cache_v8.c
> > > b/arch/arm/cpu/armv8/cache_v8.c index c5ec5297cd39..2a0492fbef52
> > > 100644
> > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > @@ -128,10 +128,10 @@ void dcache_disable(void)
> > >  	if (!(sctlr & CR_C))
> > >  		return;
> > >
> > > -	set_sctlr(sctlr & ~(CR_C|CR_M));
> > > -
> > >  	flush_dcache_all();
> > >  	__asm_invalidate_tlb_all();
> > > +
> > > +	set_sctlr(sctlr & ~(CR_C|CR_M));
> 
> I got your point. But here in this scenario also there is an issue
> with disable first and then flush_dcache_all().  This is because when
> we disable the cache and invoke the c routine flush_dcache_all() then
> the return address of this is stored in a stack(in memory as dcache is
> disabled).

Which is why this sequence cannot be written in C, and needs to be
performed in assembly, without any memory accesses between the write to
the SCTLR and the cache flush.

> Now in the flush_dcache_all we are invoking the actual asm call to
> flush dcache which may wipeout the stored return value in stack with
> cahe contents(main memory). Hence the return from the flush_dcahe_all
> will fail.
> 
> To confirm this I modified the dcache_disable in the below way and it worked fine.
> 1. Disable the dcache.
> 2. Now I called the __asm_flush_dcache_all(); and then flush_l3_cache();  instead of calling the flush_dcache_all().

That also is unsafe; implicit (e.g. stack) accesses at any point after
SCTLR.C is cleared and before flush_l3_cache() has completed may see
stale data, or get overwritten by stale data.

Set/Way ops only flush the CPU-local caches, so you only guarantee that
these are clean (and potentially dirty cache lines for the stack could
be sat in L3 and written back at any time). So your flush_l3_cache()
function might not work.

Per ARMv8 the L3 _must_ respect maintenance by VA, so after disabling
the MMU you can flush the memory region corresponding to your stack (and
any other data you need) by VA to the PoC before executing
flush_l3_cache(), in addition to the Set/Way ops used to empty the
CPU-local caches.

Thanks,
Mark.