[patch 0/2] [patch 0/2] ARM: Disable outer cache before kexec call -V2

[patch 0/2] [patch 0/2] ARM: Disable outer cache before kexec call -V2

[Thomas Gleixner]

The following patch series addresses the problem, that the kexec code
does not disable the outer cache before disabling the inner cache and
jumping into the new kernel. This results in random crashes of the new
kernel.

Changes since version 1:

 - added flush_all() and inv_all() functions to outer_cache_fns
   so OMAP can override the disable() function easily.

 - changed the implementation from inv_all() -> disable() to
   flush_all() -> disable() -> inv_all() according to feedback from
   Russell and Catalin.

Thanks,

	tglx

[patch 1/2] arm: Disable outer (L2) cache in kexec

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: arm-outer-cache-disable-for-kexec.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100705/76e5d455/attachment.ksh>

[patch 2/2] arm: Implement l2x0 cache disable functions

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: arm-l2x0-cache-disable.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100705/e4216032/attachment.ksh>

[patch 1/2] arm: Disable outer (L2) cache in kexec

[Catalin Marinas]

On Mon, 2010-07-05 at 10:25 +0100, Thomas Gleixner wrote:
> kexec does not disable the outer cache before disabling the inner
> caches in cpu_proc_fin(). So L2 is enabled across the kexec jump. When
> the new kernel enables chaches again, it randomly crashes.
> 
> Disabling L2 before calling cpu_proc_fin() cures the problem.
> 
> Disabling L2 requires the following new functions: flush_all(),
> inv_all() and disable(). Add them to outer_cache_fns and call them
> from the kexec code.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Index: linux-2.6/arch/arm/include/asm/outercache.h
> ===================================================================
> --- linux-2.6.orig/arch/arm/include/asm/outercache.h
> +++ linux-2.6/arch/arm/include/asm/outercache.h
> @@ -25,6 +25,9 @@ struct outer_cache_fns {
>         void (*inv_range)(unsigned long, unsigned long);
>         void (*clean_range)(unsigned long, unsigned long);
>         void (*flush_range)(unsigned long, unsigned long);
> +       void (*flush_all)(void);
> +       void (*inv_all)(void);
> +       void (*disable)(void);

Why don't we handle the flush_all/inv_all in the disable function and
avoid exporting them? Once exported, people may start using them in
drivers and they should use the DMA API for this. The inv_all is also
dangerous if called when the L2 is enabled.

> +static inline void outer_cache_disable(void)
> +{
> +       if (outer_cache.disable)
> +               outer_cache.disable();
> +}

Personal preference - "outer_disable()" as we don't call any of the
other functions outer_cache_*().

> --- linux-2.6.orig/arch/arm/kernel/machine_kexec.c
> +++ linux-2.6/arch/arm/kernel/machine_kexec.c
> @@ -74,7 +74,10 @@ void machine_kexec(struct kimage *image)
>                            (unsigned long) reboot_code_buffer + KEXEC_CONTROL_PAGE_SIZE);
>         printk(KERN_INFO "Bye!\n");
> 
> +       outer_flush_all();
> +       outer_cache_disable();
>         cpu_proc_fin();
> +       outer_inv_all();

As I said above, we could have just outer_disable() here which does
outer_flush_all, disabling and outer_inv_all. I don't think the
invalidation needs to be done after cup_proc_fin().

Thanks.

-- 
Catalin

[patch 2/2] arm: Implement l2x0 cache disable functions

[Catalin Marinas]

On Mon, 2010-07-05 at 10:25 +0100, Thomas Gleixner wrote:
> --- linux-2.6.orig/arch/arm/mm/cache-l2x0.c
> +++ linux-2.6/arch/arm/mm/cache-l2x0.c
> @@ -103,7 +103,19 @@ static void l2x0_cache_sync(void)
>         spin_unlock_irqrestore(&l2x0_lock, flags);
>  }
> 
> -static inline void l2x0_inv_all(void)
> +static void l2x0_flush_all(void)
> +{
> +       unsigned long flags;
> +
> +       /* clean all ways */
> +       spin_lock_irqsave(&l2x0_lock, flags);
> +       writel(l2x0_way_mask, l2x0_base + L2X0_CLEAN_WAY);
> +       cache_wait(l2x0_base + L2X0_CLEAN_WAY, l2x0_way_mask);
> +       cache_sync();
> +       spin_unlock_irqrestore(&l2x0_lock, flags);
> +}

Since it only does cleaning, we should probably call it l2x0_clean_all
(the same for the outer_cache_fns entry). I think on ARM we
traditionally referred to flush as clean+invalidate.

-- 
Catalin

[patch 2/2] arm: Implement l2x0 cache disable functions

[Thomas Gleixner]

On Mon, 5 Jul 2010, Catalin Marinas wrote:

> On Mon, 2010-07-05 at 10:25 +0100, Thomas Gleixner wrote:
> > --- linux-2.6.orig/arch/arm/mm/cache-l2x0.c
> > +++ linux-2.6/arch/arm/mm/cache-l2x0.c
> > @@ -103,7 +103,19 @@ static void l2x0_cache_sync(void)
> >         spin_unlock_irqrestore(&l2x0_lock, flags);
> >  }
> > 
> > -static inline void l2x0_inv_all(void)
> > +static void l2x0_flush_all(void)
> > +{
> > +       unsigned long flags;
> > +
> > +       /* clean all ways */
> > +       spin_lock_irqsave(&l2x0_lock, flags);
> > +       writel(l2x0_way_mask, l2x0_base + L2X0_CLEAN_WAY);
> > +       cache_wait(l2x0_base + L2X0_CLEAN_WAY, l2x0_way_mask);
> > +       cache_sync();
> > +       spin_unlock_irqrestore(&l2x0_lock, flags);
> > +}
> 
> Since it only does cleaning, we should probably call it l2x0_clean_all
> (the same for the outer_cache_fns entry). I think on ARM we
> traditionally referred to flush as clean+invalidate.

So CLEAN_INV_WAY is the right opcode ?

Thanks,

	tglx

[patch 1/2] arm: Disable outer (L2) cache in kexec

[Thomas Gleixner]

On Mon, 5 Jul 2010, Catalin Marinas wrote:

> On Mon, 2010-07-05 at 10:25 +0100, Thomas Gleixner wrote:
> > kexec does not disable the outer cache before disabling the inner
> > caches in cpu_proc_fin(). So L2 is enabled across the kexec jump. When
> > the new kernel enables chaches again, it randomly crashes.
> > 
> > Disabling L2 before calling cpu_proc_fin() cures the problem.
> > 
> > Disabling L2 requires the following new functions: flush_all(),
> > inv_all() and disable(). Add them to outer_cache_fns and call them
> > from the kexec code.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Index: linux-2.6/arch/arm/include/asm/outercache.h
> > ===================================================================
> > --- linux-2.6.orig/arch/arm/include/asm/outercache.h
> > +++ linux-2.6/arch/arm/include/asm/outercache.h
> > @@ -25,6 +25,9 @@ struct outer_cache_fns {
> >         void (*inv_range)(unsigned long, unsigned long);
> >         void (*clean_range)(unsigned long, unsigned long);
> >         void (*flush_range)(unsigned long, unsigned long);
> > +       void (*flush_all)(void);
> > +       void (*inv_all)(void);
> > +       void (*disable)(void);
> 
> Why don't we handle the flush_all/inv_all in the disable function and
> avoid exporting them? Once exported, people may start using them in

Because OMAP needs them to avoid reimplementing everything in the OMAP
part. So OMAP only needs to override the disable function. And if
Russell wants to add his cleanup patch which he posted last week, then
the split and the export needs to be done anyway.

Thanks,

	tglx

[patch 2/2] arm: Implement l2x0 cache disable functions

[Catalin Marinas]

On Mon, 2010-07-05 at 12:10 +0100, Thomas Gleixner wrote:
> 
> On Mon, 5 Jul 2010, Catalin Marinas wrote:
> 
> > On Mon, 2010-07-05 at 10:25 +0100, Thomas Gleixner wrote:
> > > --- linux-2.6.orig/arch/arm/mm/cache-l2x0.c
> > > +++ linux-2.6/arch/arm/mm/cache-l2x0.c
> > > @@ -103,7 +103,19 @@ static void l2x0_cache_sync(void)
> > >         spin_unlock_irqrestore(&l2x0_lock, flags);
> > >  }
> > >
> > > -static inline void l2x0_inv_all(void)
> > > +static void l2x0_flush_all(void)
> > > +{
> > > +       unsigned long flags;
> > > +
> > > +       /* clean all ways */
> > > +       spin_lock_irqsave(&l2x0_lock, flags);
> > > +       writel(l2x0_way_mask, l2x0_base + L2X0_CLEAN_WAY);
> > > +       cache_wait(l2x0_base + L2X0_CLEAN_WAY, l2x0_way_mask);
> > > +       cache_sync();
> > > +       spin_unlock_irqrestore(&l2x0_lock, flags);
> > > +}
> >
> > Since it only does cleaning, we should probably call it l2x0_clean_all
> > (the same for the outer_cache_fns entry). I think on ARM we
> > traditionally referred to flush as clean+invalidate.
> 
> So CLEAN_INV_WAY is the right opcode ?

Yes.

-- 
Catalin

[patch 1/2] arm: Disable outer (L2) cache in kexec

[Catalin Marinas]

On Mon, 2010-07-05 at 12:13 +0100, Thomas Gleixner wrote:
> On Mon, 5 Jul 2010, Catalin Marinas wrote:
> 
> > On Mon, 2010-07-05 at 10:25 +0100, Thomas Gleixner wrote:
> > > kexec does not disable the outer cache before disabling the inner
> > > caches in cpu_proc_fin(). So L2 is enabled across the kexec jump. When
> > > the new kernel enables chaches again, it randomly crashes.
> > >
> > > Disabling L2 before calling cpu_proc_fin() cures the problem.
> > >
> > > Disabling L2 requires the following new functions: flush_all(),
> > > inv_all() and disable(). Add them to outer_cache_fns and call them
> > > from the kexec code.
> > >
> > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > > Index: linux-2.6/arch/arm/include/asm/outercache.h
> > > ===================================================================
> > > --- linux-2.6.orig/arch/arm/include/asm/outercache.h
> > > +++ linux-2.6/arch/arm/include/asm/outercache.h
> > > @@ -25,6 +25,9 @@ struct outer_cache_fns {
> > >         void (*inv_range)(unsigned long, unsigned long);
> > >         void (*clean_range)(unsigned long, unsigned long);
> > >         void (*flush_range)(unsigned long, unsigned long);
> > > +       void (*flush_all)(void);
> > > +       void (*inv_all)(void);
> > > +       void (*disable)(void);
> >
> > Why don't we handle the flush_all/inv_all in the disable function and
> > avoid exporting them? Once exported, people may start using them in
> 
> Because OMAP needs them to avoid reimplementing everything in the OMAP
> part. So OMAP only needs to override the disable function. And if
> Russell wants to add his cleanup patch which he posted last week, then
> the split and the export needs to be done anyway.

OK, good point.

Thanks.

-- 
Catalin

[patch 2/2] arm: Implement l2x0 cache disable functions

[Thomas Gleixner]

An embedded and charset-unspecified text was scrubbed...
Name: arm-l2x0-cache-disable.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100705/b88b1a1a/attachment.ksh>

[patch 2/2] arm: Implement l2x0 cache disable functions

[Catalin Marinas]

On Mon, 2010-07-05 at 12:52 +0100, Thomas Gleixner wrote:
> Add flush_all, inv_all and disable functions to the l2x0 code. These
> functions are called from kexec code to prevent random crashes in the
> new kernel.
> 
> Platforms like OMAP which control L2 enable/disable via SMI mode can
> override the outer_cache.disable() function to implement their own.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[...]
> -static inline void l2x0_inv_all(void)
> +static void l2x0_flush_all(void)
> +{
> +       unsigned long flags;
> +
> +       /* clean all ways */

Minor point - /* clean and invalidate all ways */

But the patch looks good to me:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

[patch 2/2] arm: Implement l2x0 cache disable functions

[Linus Walleij]

2010/7/5 Thomas Gleixner <tglx@linutronix.de>:

> Add flush_all, inv_all and disable functions to the l2x0 code. These
> functions are called from kexec code to prevent random crashes in the
> new kernel.
> (...)
> +static void l2x0_disable(void)
> +{
> + ? ? ? unsigned long flags;
> +
> + ? ? ? spin_lock_irqsave(&l2x0_lock, flags);
> + ? ? ? writel(0, l2x0_base + L2X0_CTRL);
> + ? ? ? spin_unlock_irqrestore(&l2x0_lock, flags);
> +}
> +

Does this take into account this comment from Srinidhi:

[Srinidhi KASAGAR wrote]
> I think it's a problem for PL310 controller where the access to
> L2X0_CTRL is secure, accessing this in non-secure mode will fault.

I'm worried that a writel() into the CTRL register like that will cause
a bus error on platforms that have L2x0 in Trusted World (TrustZone)?
Or that it may be ignored simply on such platforms causing
trouble that way?

Yours,
Linus Walleij