[RESEND,2/2] arm: socfpga: Set share override bit of the l2 cache controller

Commit Message

[e-mail] Feb. 19, 2015, 5:06 p.m. UTC

From: Dinh Nguyen <[e-mail]>

By not having bit 22 set in the PL310 Auxiliary Control register (shared
attribute override enable) has the side effect of transforming Normal
Shared Non-cacheable reads into Cacheable no-allocate reads.

Coherent DMA buffers in Linux always have a Cacheable alias via the
kernel linear mapping and the processor can speculatively load cache
lines into the PL310 controller. With bit 22 cleared, Non-cacheable
reads would unexpectedly hit such cache lines leading to buffer
corruption.

Signed-off-by: Dinh Nguyen <[e-mail]>
---
 arch/arm/mach-socfpga/socfpga.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Rob Herring Feb. 19, 2015, 6:13 p.m. UTC | #1

On Thu, Feb 19, 2015 at 11:06 AM,  <[e-mail]> wrote:
> From: Dinh Nguyen <[e-mail]>
>
> By not having bit 22 set in the PL310 Auxiliary Control register (shared
> attribute override enable) has the side effect of transforming Normal
> Shared Non-cacheable reads into Cacheable no-allocate reads.
>
> Coherent DMA buffers in Linux always have a Cacheable alias via the
> kernel linear mapping and the processor can speculatively load cache
> lines into the PL310 controller. With bit 22 cleared, Non-cacheable
> reads would unexpectedly hit such cache lines leading to buffer
> corruption.

You really should be doing this in your bootloader.

Rob

>
> Signed-off-by: Dinh Nguyen <[e-mail]>
> ---
>  arch/arm/mach-socfpga/socfpga.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index a5f1fda..4ce2100 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -105,7 +105,8 @@ static const char *altera_dt_match[] = {
>
>  DT_MACHINE_START(SOCFPGA, "Altera SOCFPGA")
>         .l2c_aux_val    = L310_AUX_CTRL_DATA_PREFETCH |
> -                         L310_AUX_CTRL_INSTR_PREFETCH,
> +                         L310_AUX_CTRL_INSTR_PREFETCH |
> +                         L2C_AUX_CTRL_SHARED_OVERRIDE,
>         .l2c_aux_mask   = ~0,
>         .smp            = smp_ops(socfpga_smp_ops),
>         .map_io         = socfpga_map_io,
> --
> 2.2.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Dinh Nguyen Feb. 20, 2015, 7:15 a.m. UTC | #2

Hi Rob,

On 2/19/15 12:13 PM, Rob Herring wrote:
> On Thu, Feb 19, 2015 at 11:06 AM,  <[e-mail]> wrote:
>> From: Dinh Nguyen <[e-mail]>
>>
>> By not having bit 22 set in the PL310 Auxiliary Control register (shared
>> attribute override enable) has the side effect of transforming Normal
>> Shared Non-cacheable reads into Cacheable no-allocate reads.
>>
>> Coherent DMA buffers in Linux always have a Cacheable alias via the
>> kernel linear mapping and the processor can speculatively load cache
>> lines into the PL310 controller. With bit 22 cleared, Non-cacheable
>> reads would unexpectedly hit such cache lines leading to buffer
>> corruption.
> 
> You really should be doing this in your bootloader.
> 

Can I ask what is your reasoning for doing this in the bootloader? It's
seems like this is such a nice mechanism to do it here.

Dinh

Rob Herring Feb. 20, 2015, 1:53 p.m. UTC | #3

On Fri, Feb 20, 2015 at 1:15 AM, Dinh Nguyen <[e-mail]> wrote:
> Hi Rob,
>
> On 2/19/15 12:13 PM, Rob Herring wrote:
>> On Thu, Feb 19, 2015 at 11:06 AM,  <[e-mail]> wrote:
>>> From: Dinh Nguyen <[e-mail]>
>>>
>>> By not having bit 22 set in the PL310 Auxiliary Control register (shared
>>> attribute override enable) has the side effect of transforming Normal
>>> Shared Non-cacheable reads into Cacheable no-allocate reads.
>>>
>>> Coherent DMA buffers in Linux always have a Cacheable alias via the
>>> kernel linear mapping and the processor can speculatively load cache
>>> lines into the PL310 controller. With bit 22 cleared, Non-cacheable
>>> reads would unexpectedly hit such cache lines leading to buffer
>>> corruption.
>>
>> You really should be doing this in your bootloader.
>>
>
> Can I ask what is your reasoning for doing this in the bootloader? It's
> seems like this is such a nice mechanism to do it here.

Primarily, this register is secure only and we try to avoid secure
mode setup in the kernel.

Russell also has had a patch to do this generically in his patch queue
forever. If we want this in the kernel, then we should apply that.

Rob

Russell King - ARM Linux Feb. 20, 2015, 1:57 p.m. UTC | #4

On Fri, Feb 20, 2015 at 07:53:50AM -0600, Rob Herring wrote:
> On Fri, Feb 20, 2015 at 1:15 AM, Dinh Nguyen <[e-mail]> wrote:
> > Can I ask what is your reasoning for doing this in the bootloader? It's
> > seems like this is such a nice mechanism to do it here.
> 
> Primarily, this register is secure only and we try to avoid secure
> mode setup in the kernel.
> 
> Russell also has had a patch to do this generically in his patch queue
> forever. If we want this in the kernel, then we should apply that.

I discarded it.  In general, we want boot loaders or firmware to
configure the basic properties of the caches, rather than having the
kernel do it for exactly the reasons you say above.

Yes, there are some cache features which can only be enabled in
combination with CPU features, and those the kernel _has_ to know
about, but the basic setup should be done outside the kernel.

Pavel Machek Feb. 23, 2015, 12:13 p.m. UTC | #5

On Thu 2015-02-19 12:13:13, Rob Herring wrote:
> On Thu, Feb 19, 2015 at 11:06 AM,  <[e-mail]> wrote:
> > From: Dinh Nguyen <[e-mail]>
> >
> > By not having bit 22 set in the PL310 Auxiliary Control register (shared
> > attribute override enable) has the side effect of transforming Normal
> > Shared Non-cacheable reads into Cacheable no-allocate reads.
> >
> > Coherent DMA buffers in Linux always have a Cacheable alias via the
> > kernel linear mapping and the processor can speculatively load cache
> > lines into the PL310 controller. With bit 22 cleared, Non-cacheable
> > reads would unexpectedly hit such cache lines leading to buffer
> > corruption.
> 
> You really should be doing this in your bootloader.
>

You mean... in all your bootloaders? Because there's more
than one.

And as both bootloaders need it, it makes sense to do it 
in kernel, afaict.
							Pavel
> >  DT_MACHINE_START(SOCFPGA, "Altera SOCFPGA")
> >         .l2c_aux_val    = L310_AUX_CTRL_DATA_PREFETCH |
> > -                         L310_AUX_CTRL_INSTR_PREFETCH,
> > +                         L310_AUX_CTRL_INSTR_PREFETCH |
> > +                         L2C_AUX_CTRL_SHARED_OVERRIDE,
> >         .l2c_aux_mask   = ~0,
> >         .smp            = smp_ops(socfpga_smp_ops),
> >         .map_io         = socfpga_map_io,
> > --
> > 2.2.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Patch

diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index a5f1fda..4ce2100 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -105,7 +105,8 @@  static const char *altera_dt_match[] = {
 
 DT_MACHINE_START(SOCFPGA, "Altera SOCFPGA")
 	.l2c_aux_val	= L310_AUX_CTRL_DATA_PREFETCH |
-			  L310_AUX_CTRL_INSTR_PREFETCH,
+			  L310_AUX_CTRL_INSTR_PREFETCH |
+			  L2C_AUX_CTRL_SHARED_OVERRIDE,
 	.l2c_aux_mask	= ~0,
 	.smp		= smp_ops(socfpga_smp_ops),
 	.map_io		= socfpga_map_io,