[PATCH] MIPS: Fix vmlinuz to flush the caches after kernel decompression

[PATCH] MIPS: Fix vmlinuz to flush the caches after kernel decompression

[Shmulik Ladkani]

Flush caches after kernel decompression.

When writing instructions, the D-cache should be written-back, and I-cache
should be invalidated.

The patch implements L1 cache flushing, for r4k style caches - suitable for
all MIPS32 CPUs (and probably for other CPUs too).

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
diff --git a/arch/mips/boot/compressed/Makefile b/arch/mips/boot/compressed/Makefile
index ed9bb70..9a8d2da 100644
--- a/arch/mips/boot/compressed/Makefile
+++ b/arch/mips/boot/compressed/Makefile
@@ -30,6 +30,9 @@ targets := head.o decompress.o dbg.o uart-16550.o uart-alchemy.o
 # decompressor objects (linked with vmlinuz)
 vmlinuzobjs-y := $(obj)/head.o $(obj)/decompress.o $(obj)/dbg.o
 
+targets += $(obj)/c-r4k.o
+vmlinuzobjs-$(CONFIG_CPU_MIPS32) += $(obj)/c-r4k.o
+
 ifdef CONFIG_DEBUG_ZBOOT
 vmlinuzobjs-$(CONFIG_SYS_SUPPORTS_ZBOOT_UART16550) += $(obj)/uart-16550.o
 vmlinuzobjs-$(CONFIG_MIPS_ALCHEMY)		   += $(obj)/uart-alchemy.o
diff --git a/arch/mips/boot/compressed/c-r4k.c b/arch/mips/boot/compressed/c-r4k.c
new file mode 100644
index 0000000..1959cdc
--- /dev/null
+++ b/arch/mips/boot/compressed/c-r4k.c
@@ -0,0 +1,92 @@
+#include <linux/types.h>
+#include <linux/kernel.h>
+
+#include <asm/addrspace.h>
+#include <asm/asm.h>
+#include <asm/cacheops.h>
+#include <asm/mipsregs.h>
+
+#define INDEX_BASE CKSEG0
+
+extern void puts(const char *s);
+extern void puthex(unsigned long long val);
+
+#define cache_op(op, addr)			\
+	__asm__ __volatile__(			\
+	"	.set push		\n"	\
+	"	.set noreorder		\n"	\
+	"	.set mips3		\n"	\
+	"	cache %1, 0(%0)		\n"	\
+	"	.set pop		\n"	\
+	:					\
+	: "r" (addr), "i" (op))
+
+#define cache_all_index_op(cachesz, linesz, op) do {			\
+	unsigned long addr = INDEX_BASE;				\
+	for (; addr < INDEX_BASE + (cachesz); addr += (linesz))		\
+		cache_op(op, addr);					\
+} while (0)
+
+static void dcache_writeback(const unsigned long cache_size,
+	const unsigned long line_size)
+{
+#ifdef DEBUG
+	puts("dcache writeback, cachesize ");
+	puthex(cache_size);
+	puts(" linesize ");
+	puthex(line_size);
+	puts("\n");
+#endif
+
+	cache_all_index_op(cache_size, line_size, Index_Writeback_Inv_D);
+}
+
+static void icache_invalidate(const unsigned long cache_size,
+	const unsigned long line_size)
+{
+#ifdef DEBUG
+	puts("icache invalidate, cachesize ");
+	puthex(cache_size);
+	puts(" linesize ");
+	puthex(line_size);
+	puts("\n");
+#endif
+
+	cache_all_index_op(cache_size, line_size, Index_Invalidate_I);
+}
+
+void cache_flush(void)
+{
+	volatile unsigned long config1;
+	unsigned long tmp;
+	unsigned long line_size;
+	unsigned long ways;
+	unsigned long sets;
+	unsigned long cache_size;
+
+	if (!(read_c0_config() & MIPS_CONF_M)) {
+		puts("cache_flush error: Config1 unavailable\n");
+		return;
+	}
+	config1 = read_c0_config1();
+
+	/* calculate D-cache line-size and cache-size, then writeback */
+	tmp = (config1 >> 10) & 7;
+	if (tmp) {
+		line_size = 2 << tmp;
+		sets = 64 << ((config1 >> 13) & 7);
+		ways = 1 + ((config1 >> 7) & 7);
+		cache_size = sets * ways * line_size;
+		dcache_writeback(cache_size, line_size);
+	}
+
+	/* calculate I-cache line-size and cache-size, then invalidate */
+	tmp = (config1 >> 19) & 7;
+	if (tmp) {
+		line_size = 2 << tmp;
+		sets = 64 << ((config1 >> 22) & 7);
+		ways = 1 + ((config1 >> 16) & 7);
+		cache_size = sets * ways * line_size;
+		icache_invalidate(cache_size, line_size);
+	}
+}
diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
index 5cad0fa..c86f9bd 100644
--- a/arch/mips/boot/compressed/decompress.c
+++ b/arch/mips/boot/compressed/decompress.c
@@ -30,6 +30,10 @@ extern unsigned char __image_begin, __image_end;
 extern void puts(const char *s);
 extern void puthex(unsigned long long val);
 
+void __weak cache_flush(void)
+{
+}
+
 void error(char *x)
 {
 	puts("\n\n");
@@ -105,6 +109,7 @@ void decompress_kernel(unsigned long boot_heap_start)
 	decompress((char *)zimage_start, zimage_size, 0, 0,
 		   (void *)VMLINUX_LOAD_ADDRESS_ULL, 0, error);
 
-	/* FIXME: should we flush cache here? */
+	cache_flush();
+
 	puts("Now, booting the kernel...\n");
 }
-- 
Shmulik Ladkani

Re: [PATCH] MIPS: Fix vmlinuz to flush the caches after kernel decompression

[Ralf Baechle]

On Wed, Sep 01, 2010 at 12:17:43PM +0300, Shmulik Ladkani wrote:

> Flush caches after kernel decompression.
> 
> When writing instructions, the D-cache should be written-back, and I-cache
> should be invalidated.

Correct - but it's also a can of worms which is why I intentionally
ignored the issue so far.  An I-cache is refilled from L2/L3 (if available)
or memory.  The large amounts of data written by the CPU during
decompression of the kernel virtually guarantee that all code will be
written back to L2/L3 or memory and the I-cache has been flushed by
firmware before the decompressor was entered.

Does this assumption fail for you?

The real can of worms is SMP - none of the other processors have been
detected etc.  At this early stage we just don't know if and how to flush
caches of other processors.  The only good news is that we know they
don't have any not written back kernel code in their D-caches.

> The patch implements L1 cache flushing, for r4k style caches - suitable for
> all MIPS32 CPUs (and probably for other CPUs too).

No - you only compile the code for MIPS32 CPUs and check for MIPS_CONF_M
which - at least with this meaning - only exists on MIPS32 and MIPS64 CPUs.

> +#define INDEX_BASE CKSEG0
> +
> +extern void puts(const char *s);
> +extern void puthex(unsigned long long val);
> +
> +#define cache_op(op, addr)			\
> +	__asm__ __volatile__(			\
> +	"	.set push		\n"	\
> +	"	.set noreorder		\n"	\
> +	"	.set mips3		\n"	\
> +	"	cache %1, 0(%0)		\n"	\
> +	"	.set pop		\n"	\
> +	:					\
> +	: "r" (addr), "i" (op))

This duplicates the definition of arch/mips/include/asm/r4kcache.h.  Why?

> +#define cache_all_index_op(cachesz, linesz, op) do {			\
> +	unsigned long addr = INDEX_BASE;				\
> +	for (; addr < INDEX_BASE + (cachesz); addr += (linesz))		\
> +		cache_op(op, addr);					\
> +} while (0)

For consistence in formatting please move the "do {" to the beginning of
the next line.

> +void cache_flush(void)
> +{
> +	volatile unsigned long config1;

I don't know why you're using volatile here - but it won't work as you
intended.  Just drop the keyword.

> +	unsigned long tmp;
> +	unsigned long line_size;
> +	unsigned long ways;
> +	unsigned long sets;
> +	unsigned long cache_size;

Make these int variables.  The code here is fine for MIPS64 as well but
there is no point in having 64-bit variables and multiplies.

> +	if (!(read_c0_config() & MIPS_CONF_M)) {
> +		puts("cache_flush error: Config1 unavailable\n");
> +		return;
> +	}
> +	config1 = read_c0_config1();
> +
> +	/* calculate D-cache line-size and cache-size, then writeback */
> +	tmp = (config1 >> 10) & 7;
> +	if (tmp) {
> +		line_size = 2 << tmp;
> +		sets = 64 << ((config1 >> 13) & 7);
> +		ways = 1 + ((config1 >> 7) & 7);
> +		cache_size = sets * ways * line_size;
> +		dcache_writeback(cache_size, line_size);
> +	}
> +
> +	/* calculate I-cache line-size and cache-size, then invalidate */
> +	tmp = (config1 >> 19) & 7;
> +	if (tmp) {
> +		line_size = 2 << tmp;
> +		sets = 64 << ((config1 >> 22) & 7);
> +		ways = 1 + ((config1 >> 16) & 7);
> +		cache_size = sets * ways * line_size;
> +		icache_invalidate(cache_size, line_size);
> +	}

Eww...  You copied (my ...) old sin from c-r4k.c and use all the magic
numbers.

Anyway, does this actually fix a bug for you or is it more a theoretical
convern?

  Ralf

Re: [PATCH] MIPS: Fix vmlinuz to flush the caches after kernel decompression

[Shmulik Ladkani]

On Tue, 21 Sep 2010 11:11:05 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:

> 
> Correct - but it's also a can of worms which is why I intentionally
> ignored the issue so far.  An I-cache is refilled from L2/L3 (if available)
> or memory.  The large amounts of data written by the CPU during
> decompression of the kernel virtually guarantee that all code will be
> written back to L2/L3 or memory and the I-cache has been flushed by
> firmware before the decompressor was entered.
> 
> Does this assumption fail for you?

Yes.
Indeed, decompressed code has been written-back to memory.
However, there was no flushing of the I-cache by the firmware; actually, there
was no firmware involved - I've tried to boot a kernel from a running kernel
(the "first" kernel acting as a bootloader).
The first kernel could have flushed the I-cache before passing execution to
the decompressor; however I thought it's more appropriate if the decompressor
takes care of that, as the decompressor is the one who actually writes the
instructions of the "second" kernel.

> > The patch implements L1 cache flushing, for r4k style caches - suitable for
> > all MIPS32 CPUs (and probably for other CPUs too).
> 
> No - you only compile the code for MIPS32 CPUs and check for MIPS_CONF_M
> which - at least with this meaning - only exists on MIPS32 and MIPS64 CPUs.

Ok, will fix Makefile and git log appropriately in V2.

> > +#define INDEX_BASE CKSEG0
> > +
> > +extern void puts(const char *s);
> > +extern void puthex(unsigned long long val);
> > +
> > +#define cache_op(op, addr)			\
> > +	__asm__ __volatile__(			\
> > +	"	.set push		\n"	\
> > +	"	.set noreorder		\n"	\
> > +	"	.set mips3		\n"	\
> > +	"	cache %1, 0(%0)		\n"	\
> > +	"	.set pop		\n"	\
> > +	:					\
> > +	: "r" (addr), "i" (op))
> 
> This duplicates the definition of arch/mips/include/asm/r4kcache.h.  Why?

Ok, will include r4kcache.h instead.

Actually, I really wanted to use the blast_XXX macros of r4kcache.h instead
of re-implementing, but it required an initialized 'cpu_data' structure
and overloaded 'smp_processor_id' (or some other trick to have a valid
'current_cpu_data'); IMO this was not suitable for a small decompressor
executable.

> > +#define cache_all_index_op(cachesz, linesz, op) do {			\
> > +	unsigned long addr = INDEX_BASE;				\
> > +	for (; addr < INDEX_BASE + (cachesz); addr += (linesz))		\
> > +		cache_op(op, addr);					\
> > +} while (0)
> 
> For consistence in formatting please move the "do {" to the beginning of
> the next line.

Ok.

> > +void cache_flush(void)
> > +{
> > +	volatile unsigned long config1;
> 
> I don't know why you're using volatile here - but it won't work as you
> intended.  Just drop the keyword.

Ok, will remove volatile keyword.
My intention was to avoid any compilation optimization of read_c0_config1,
as sometimes, when code was organized differently, I got 0xffffffff.

> > +	unsigned long tmp;
> > +	unsigned long line_size;
> > +	unsigned long ways;
> > +	unsigned long sets;
> > +	unsigned long cache_size;
> 
> Make these int variables.  The code here is fine for MIPS64 as well but
> there is no point in having 64-bit variables and multiplies.

Ok.

> Eww...  You copied (my ...) old sin from c-r4k.c and use all the magic
> numbers.

I didn't feel well about it either; there's no simple way to refactor the
c-r4k.c code.

> Anyway, does this actually fix a bug for you or is it more a theoretical
> convern?

Yes, this was necessary, but could have been solved differently - as described
in my first comment. 
If it's guaranteed that the entity executing the decompressor (firmware,
bootloader, etc...) is resposible for flushing the caches, then this fix is
not needed.
Please let me know your approach on this; I'll resubmit a V2 of the patch if
necessary.

-- 
Shmulik Ladkani