Re: [boot-wrapper PATCH 03/12] Remove cache maintenance

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Jul 30, 2021 at 04:12:23PM +0100, Andre Przywara wrote:
> On Thu, 29 Jul 2021 16:20:41 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > For models, we assume that out-of-reset caches ar invalid and no cache
> 
> Is that "caches are invalid out-of-reset" an architecture guarantee or
> just a model property? Since the bootwrapper is officially targeting
> the model, that doesn't really matter, but some people (ab)use it for
> other platforms, so it might be worth noting.

Just a model property; in general implementations can require an
IMPLEMENTATION DEFINED initialization sequence out-of-reset.

I'd like to document the assumptions the boot-wrapper makes, but at the
moment those aren't entirely consistent, so I'm holding back until teh
cleanup's done.

Practically speaking, it's unlikely people would build something weaker
as it would wreak havoc upon SMP and hotplug, etc.

> > maintenance is required.
> > 
> > We added cache maintenance to the boot-wrapper in commit:
> > 
> >   28ec269a22c8dc14 ("Add code to clean and invalidate caches")
> > 
> > ... because the boot-wrapper would teansiently use cacheable mappings,
> > and could allocate into caches. As we were using Set/Way operations, we
> > were on somewhat shaky ground (e.g. due to system-level caches, or
> > dirty line migration). Further, we never took FEAT_CCIDX into account,
> > and so would not necessarily invalidate all potential levels of cache
> > 
> > However, since commit:
> > 
> >   0bb7b2545582accf ("Remove MMU identity map setup")
> > 
> > ... we no longer enable the MMU within the boot-wrapper, and so no
> > longer have any reason to perform cache maintenance.
> > 
> > This patch removes the redundant and incomplete cache maintenance.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  Makefile.am   |  2 +-
> >  boot_common.c |  3 ---
> >  cache.c       | 58 ----------------------------------------------------------
> 
> Love that ^^^^, also it removes the *flush*_cache() name ;-)

:)

> Indeed I couldn't find anything setting SCTLR.M, so:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> Cheers,
> Andre

Thanks!

Mark.

> 
> >  3 files changed, 1 insertion(+), 62 deletions(-)
> >  delete mode 100644 cache.c
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index ef6b793..8334049 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -123,7 +123,7 @@ CFLAGS		+= -Wall -fomit-frame-pointer
> >  CFLAGS		+= -ffunction-sections -fdata-sections
> >  LDFLAGS		+= --gc-sections
> >  
> > -OFILES		+= boot_common.o bakery_lock.o platform.o $(GIC) cache.o lib.o
> > +OFILES		+= boot_common.o bakery_lock.o platform.o $(GIC) lib.o
> >  OFILES		+= $(addprefix $(ARCH_SRC),boot.o stack.o $(BOOTMETHOD) utils.o)
> >  
> >  # Don't lookup all prerequisites in $(top_srcdir), only the source files. When
> > diff --git a/boot_common.c b/boot_common.c
> > index e7b8e1d..d48b7e1 100644
> > --- a/boot_common.c
> > +++ b/boot_common.c
> > @@ -13,7 +13,6 @@ extern unsigned long entrypoint;
> >  extern unsigned long dtb;
> >  
> >  void init_platform(void);
> > -void flush_caches(void);
> >  
> >  void __noreturn jump_kernel(unsigned long address,
> >  			    unsigned long a0,
> > @@ -62,8 +61,6 @@ void __noreturn spin(unsigned long *mbox, unsigned long invalid, int is_entry)
> >  void __noreturn first_spin(unsigned int cpu, unsigned long *mbox,
> >  			   unsigned long invalid)
> >  {
> > -	flush_caches();
> > -
> >  	if (cpu == 0) {
> >  		init_platform();
> >  
> > diff --git a/cache.c b/cache.c
> > deleted file mode 100644
> > index 9d71248..0000000
> > --- a/cache.c
> > +++ /dev/null
> > @@ -1,58 +0,0 @@
> > -/*
> > - * cache.c - simple cache clean+invalidate code
> > - *
> > - * Copyright (C) 2015 ARM Limited. All rights reserved.
> > - *
> > - * Use of this source code is governed by a BSD-style license that can be
> > - * found in the LICENSE.txt file.
> > - */
> > -
> > -#include <cpu.h>
> > -
> > -void flush_caches(void)
> > -{
> > -	unsigned int level;
> > -	uint32_t clidr = read_clidr();
> > -	unsigned int max_level = (clidr >> 24) & 0x7;
> > -
> > -	uint32_t ccsidr;
> > -
> > -	if (max_level == 0)
> > -		return;
> > -
> > -	for (level = 0; level < max_level; level++) {
> > -		uint32_t cache_type = (clidr >> (level * 3)) & 0x7;
> > -		unsigned int line_size, num_ways, num_sets, way_shift;
> > -		unsigned int way, set;
> > -
> > -		if (cache_type == 1)
> > -			/* No dcache at this level */
> > -			continue;
> > -
> > -		write_csselr(level << 1);
> > -		isb();
> > -		ccsidr = read_ccsidr();
> > -
> > -		line_size = (ccsidr & 0x7) + 4; /* log2 line size */
> > -		num_ways = ((ccsidr >> 3) & 0x3ff) + 1;
> > -		num_sets = ((ccsidr >> 13) & 0x7fff) + 1;
> > -
> > -		way_shift = clz(num_ways - 1);
> > -		for (way = 0; way < num_ways; way++) {
> > -			for (set = 0; set < num_sets; set++) {
> > -				uint32_t command = level << 1;
> > -				command |= way << way_shift;
> > -				command |= set << line_size;
> > -
> > -				dccisw(command);
> > -				dsb(sy);
> > -			}
> > -		}
> > -
> > -		dsb(sy);
> > -	}
> > -	dsb(sy);
> > -	iciallu();
> > -	dsb(sy);
> > -	isb();
> > -}
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel