[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Tony Lindgren]

Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
to not boot when enabled. The ARM core on it is an earlier r1p2:

CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d

Unfortunately I don't have the details of errata 751472, but I'm
guessing we need to disable it for r1p*.

Signed-off-by: Tony Lindgren <tony@atomide.com>

---

Can somebody with access to the errata check if it has more
info on which revisions this should be set on? Maybe it's just
r2p* - r3p0?

--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -236,6 +236,8 @@ __v7_setup:
 	mcreq	p15, 0, r10, c15, c0, 1		@ write diagnostic register
 #endif
 #if defined(CONFIG_ARM_ERRATA_751472) && defined(CONFIG_SMP)
+	teq	r5, #0x00100000			@ fails at least on r1p2
+	beq	1f
 	ALT_SMP(cmp r6, #0x30)			@ present prior to r3p0
 	ALT_UP_B(1f)
 	mrclt	p15, 0, r10, c15, c0, 1		@ read diagnostic register

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Wed, Nov 14, 2012 at 10:53:35AM -0800, Tony Lindgren wrote:
> Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
> to not boot when enabled. The ARM core on it is an earlier r1p2:
> 
> CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
> 
> Unfortunately I don't have the details of errata 751472, but I'm
> guessing we need to disable it for r1p*.

Fails because it can't write the diagnostic register from non-secure
mode or fails later?

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [121114 11:03]:
> On Wed, Nov 14, 2012 at 10:53:35AM -0800, Tony Lindgren wrote:
> > Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
> > to not boot when enabled. The ARM core on it is an earlier r1p2:
> > 
> > CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
> > 
> > Unfortunately I don't have the details of errata 751472, but I'm
> > guessing we need to disable it for r1p*.
> 
> Fails because it can't write the diagnostic register from non-secure
> mode or fails later?

Hmm good question, it fails a bit later on during the boot just
before __enable_mmu.

But I just tried commenting out the setting of bit 11:

@orrlt	r10, r10, #1 << 11              @ set bit #11

And just trying to write the unmodified diagnostic register
also makes it fail.

So yeah it seems to be related to the secure crap instead :(

Regards,

Tony

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Jon Hunter]

On 11/14/2012 12:53 PM, Tony Lindgren wrote:
> Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
> to not boot when enabled. The ARM core on it is an earlier r1p2:
> 
> CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
> 
> Unfortunately I don't have the details of errata 751472, but I'm
> guessing we need to disable it for r1p*.

I checked the CA9MP errata document and this erratum impacts all
r0/r1/r2 CPUs. I am wondering if the problem is because the workaround
requires you to set a bit in the Diagnostic Control register and the
read-modify-write sequence provided in the workaround is for secure
mode. Not sure if there is a non-secure workaround available :-(

Cheers
Jon

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [121114 11:09]:
> 
> On 11/14/2012 12:53 PM, Tony Lindgren wrote:
> > Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
> > to not boot when enabled. The ARM core on it is an earlier r1p2:
> > 
> > CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
> > 
> > Unfortunately I don't have the details of errata 751472, but I'm
> > guessing we need to disable it for r1p*.
> 
> I checked the CA9MP errata document and this erratum impacts all
> r0/r1/r2 CPUs. I am wondering if the problem is because the workaround
> requires you to set a bit in the Diagnostic Control register and the
> read-modify-write sequence provided in the workaround is for secure
> mode. Not sure if there is a non-secure workaround available :-(

So it seems :( And I guess we still don't have a generic way to
check if the core has secure mode or not, and what registers are
accessible in secure mode.

Regards,

Tony

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Wed, Nov 14, 2012 at 01:06:53PM -0600, Jon Hunter wrote:
> 
> On 11/14/2012 12:53 PM, Tony Lindgren wrote:
> > Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
> > to not boot when enabled. The ARM core on it is an earlier r1p2:
> > 
> > CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
> > 
> > Unfortunately I don't have the details of errata 751472, but I'm
> > guessing we need to disable it for r1p*.
> 
> I checked the CA9MP errata document and this erratum impacts all
> r0/r1/r2 CPUs. I am wondering if the problem is because the workaround
> requires you to set a bit in the Diagnostic Control register and the
> read-modify-write sequence provided in the workaround is for secure
> mode. Not sure if there is a non-secure workaround available :-(

Most likely, and there's not a lot that the kernel can sanely do about
that.  We have ended up deciding (through being forced to because of
how the security stuff works) that the stages prior to the kernel will
implement the work-around enables because those stages are already
platform specific, and the kernel will implement a "test for the
work-around already enabled."

The net result is, if you enable an Errata in the kernel which your
earlier boot stages has not already configured, the kernel will hang.
Not much we can do about the hanging aspect, because the kernel takes
an exception which we can't trap at those early stages in the boot
process.

I'm not particularly happy about that design, but that's what we've
ended up with through the 'design' of the security stuff forced onto
us.

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [121114 12:24]:
> On Wed, Nov 14, 2012 at 01:06:53PM -0600, Jon Hunter wrote:
> > 
> > On 11/14/2012 12:53 PM, Tony Lindgren wrote:
> > > Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
> > > to not boot when enabled. The ARM core on it is an earlier r1p2:
> > > 
> > > CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
> > > 
> > > Unfortunately I don't have the details of errata 751472, but I'm
> > > guessing we need to disable it for r1p*.
> > 
> > I checked the CA9MP errata document and this erratum impacts all
> > r0/r1/r2 CPUs. I am wondering if the problem is because the workaround
> > requires you to set a bit in the Diagnostic Control register and the
> > read-modify-write sequence provided in the workaround is for secure
> > mode. Not sure if there is a non-secure workaround available :-(
> 
> Most likely, and there's not a lot that the kernel can sanely do about
> that.  We have ended up deciding (through being forced to because of
> how the security stuff works) that the stages prior to the kernel will
> implement the work-around enables because those stages are already
> platform specific, and the kernel will implement a "test for the
> work-around already enabled."
> 
> The net result is, if you enable an Errata in the kernel which your
> earlier boot stages has not already configured, the kernel will hang.
> Not much we can do about the hanging aspect, because the kernel takes
> an exception which we can't trap at those early stages in the boot
> process.
> 
> I'm not particularly happy about that design, but that's what we've
> ended up with through the 'design' of the security stuff forced onto
> us.

Checking for the bit already set should work in this case, I'll post
a patch for that shortly.

Regards,

Tony

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Rob Herring]

On 11/14/2012 02:32 PM, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [121114 12:24]:
>> On Wed, Nov 14, 2012 at 01:06:53PM -0600, Jon Hunter wrote:
>>>
>>> On 11/14/2012 12:53 PM, Tony Lindgren wrote:
>>>> Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
>>>> to not boot when enabled. The ARM core on it is an earlier r1p2:
>>>>
>>>> CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
>>>>
>>>> Unfortunately I don't have the details of errata 751472, but I'm
>>>> guessing we need to disable it for r1p*.
>>>
>>> I checked the CA9MP errata document and this erratum impacts all
>>> r0/r1/r2 CPUs. I am wondering if the problem is because the workaround
>>> requires you to set a bit in the Diagnostic Control register and the
>>> read-modify-write sequence provided in the workaround is for secure
>>> mode. Not sure if there is a non-secure workaround available :-(
>>
>> Most likely, and there's not a lot that the kernel can sanely do about
>> that.  We have ended up deciding (through being forced to because of
>> how the security stuff works) that the stages prior to the kernel will
>> implement the work-around enables because those stages are already
>> platform specific, and the kernel will implement a "test for the
>> work-around already enabled."
>>
>> The net result is, if you enable an Errata in the kernel which your
>> earlier boot stages has not already configured, the kernel will hang.
>> Not much we can do about the hanging aspect, because the kernel takes
>> an exception which we can't trap at those early stages in the boot
>> process.
>>
>> I'm not particularly happy about that design, but that's what we've
>> ended up with through the 'design' of the security stuff forced onto
>> us.
> 
> Checking for the bit already set should work in this case, I'll post
> a patch for that shortly.

Can you actually read the state of the diagnostic register in non-secure
mode? If you can on the A9, is the same true on A8 or others?

Multi-platform kernels present a new problem in that we basically need
to enable all errata work-arounds. I've been meaning to look thru the
errata work-arounds to figure out which ones can be selected for
multi-platform kernels without side effects on unaffected parts (i.e.
set a chicken bit based on core revision). For any in runtime paths, we
may need to do runtime patching if they have performance impact.

Rob

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Tony Lindgren]

* Rob Herring <robherring2@gmail.com> [121114 13:59]:
> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
> > 
> > Checking for the bit already set should work in this case, I'll post
> > a patch for that shortly.
> 
> Can you actually read the state of the diagnostic register in non-secure
> mode? If you can on the A9, is the same true on A8 or others?

Looks like it can be read on at least TI omap 4430 which is A9.
But it reads as zero, so the below patch is what I came up with.

No idea if assuming that zero value for the diagnostic register
is safe.. What's the default value of the diagnostic register supposed
to be?
 
> Multi-platform kernels present a new problem in that we basically need
> to enable all errata work-arounds. I've been meaning to look thru the
> errata work-arounds to figure out which ones can be selected for
> multi-platform kernels without side effects on unaffected parts (i.e.
> set a chicken bit based on core revision). For any in runtime paths, we
> may need to do runtime patching if they have performance impact.

Yeah that's how I ran into it as multiplatform enabled omap booted
on other omaps but not on omap4.

Regards,

Tony


From: Tony Lindgren <tony@atomide.com>
Date: Tue, 13 Nov 2012 16:57:42 -0800
Subject: [PATCH] ARM: Fix errata 751472 handling on Cortex-A9 for secure mode

Looks like enabling CONFIG_ARM_ERRATA_751472 causes TI omap4 blaze
to not boot when enabled. The ARM core on it is an earlier r1p2.

This seems to be caused by the write to the diagnostic register
that shortly after causes an exception as writing to the diagnostic
register on systems with secure mode is not allowed.

Also it seems that reading the diagnostic register results zero
so we may not be able to check if bit #11 is already set.

Let's assume that if the diagnostic register is zero, we don't
want to touch it as it seems to hint secure mode at least on
TI omaps. If it's non-zero, check if bit #11 is set and only
attempt to set it if not set.

--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -238,9 +238,13 @@ __v7_setup:
 #if defined(CONFIG_ARM_ERRATA_751472) && defined(CONFIG_SMP)
 	ALT_SMP(cmp r6, #0x30)			@ present prior to r3p0
 	ALT_UP_B(1f)
-	mrclt	p15, 0, r10, c15, c0, 1		@ read diagnostic register
-	orrlt	r10, r10, #1 << 11		@ set bit #11
-	mcrlt	p15, 0, r10, c15, c0, 1		@ write diagnostic register
+	bge	1f				@ not needed for r3p0 and later
+	mrc	p15, 0, r10, c15, c0, 1		@ read diagnostic register
+	teq	r10, #0				@ zero for secure mode?
+	beq	1f				@ bail out for secure mode
+	tst	r10, #1 << 11			@ bit #11 already set?
+	orreq	r10, r10, #1 << 11		@ set bit #11 if not set
+	mcreq	p15, 0, r10, c15, c0, 1		@ write diagnostic register
 1:
 #endif
 

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Rob Herring]

On 11/14/2012 04:21 PM, Tony Lindgren wrote:
> * Rob Herring <robherring2@gmail.com> [121114 13:59]:
>> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
>>>
>>> Checking for the bit already set should work in this case, I'll post
>>> a patch for that shortly.
>>
>> Can you actually read the state of the diagnostic register in non-secure
>> mode? If you can on the A9, is the same true on A8 or others?
> 
> Looks like it can be read on at least TI omap 4430 which is A9.
> But it reads as zero, so the below patch is what I came up with.
> 
> No idea if assuming that zero value for the diagnostic register
> is safe.. What's the default value of the diagnostic register supposed
> to be?

RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
even be talking about it.

It could vary by rev, but I see 0 for the reset value, so this would not
work if the bootloader did not do any setup of the diagnostic register.

One way to determine secure mode on the A9 would be seeing if you can
change the auxcr register. Something like this (untested):

mrc	p15, 0, r0, c1, c0, 1; Read ACTLR
eor	r1, r0, #0x100		; Modify alloc in 1 way
mcr	p15, 0, r1, c1, c0, 1
mrc	p15, 0, r2, c1, c0, 1; Read ACTLR
mcr	p15, 0, r0, c1, c0, 1	; Restore original value
cmp	r1, r2
bne	skip_errata


It would be good to do this test for all the errata rather than just
this one.

Rob

>> Multi-platform kernels present a new problem in that we basically need
>> to enable all errata work-arounds. I've been meaning to look thru the
>> errata work-arounds to figure out which ones can be selected for
>> multi-platform kernels without side effects on unaffected parts (i.e.
>> set a chicken bit based on core revision). For any in runtime paths, we
>> may need to do runtime patching if they have performance impact.
> 
> Yeah that's how I ran into it as multiplatform enabled omap booted
> on other omaps but not on omap4.
> 
> Regards,
> 
> Tony
> 
> 
> From: Tony Lindgren <tony@atomide.com>
> Date: Tue, 13 Nov 2012 16:57:42 -0800
> Subject: [PATCH] ARM: Fix errata 751472 handling on Cortex-A9 for secure mode
> 
> Looks like enabling CONFIG_ARM_ERRATA_751472 causes TI omap4 blaze
> to not boot when enabled. The ARM core on it is an earlier r1p2.
> 
> This seems to be caused by the write to the diagnostic register
> that shortly after causes an exception as writing to the diagnostic
> register on systems with secure mode is not allowed.
> 
> Also it seems that reading the diagnostic register results zero
> so we may not be able to check if bit #11 is already set.
> 
> Let's assume that if the diagnostic register is zero, we don't
> want to touch it as it seems to hint secure mode at least on
> TI omaps. If it's non-zero, check if bit #11 is set and only
> attempt to set it if not set.
> 
> --- a/arch/arm/mm/proc-v7.S
> +++ b/arch/arm/mm/proc-v7.S
> @@ -238,9 +238,13 @@ __v7_setup:
>  #if defined(CONFIG_ARM_ERRATA_751472) && defined(CONFIG_SMP)
>  	ALT_SMP(cmp r6, #0x30)			@ present prior to r3p0
>  	ALT_UP_B(1f)
> -	mrclt	p15, 0, r10, c15, c0, 1		@ read diagnostic register
> -	orrlt	r10, r10, #1 << 11		@ set bit #11
> -	mcrlt	p15, 0, r10, c15, c0, 1		@ write diagnostic register
> +	bge	1f				@ not needed for r3p0 and later
> +	mrc	p15, 0, r10, c15, c0, 1		@ read diagnostic register
> +	teq	r10, #0				@ zero for secure mode?
> +	beq	1f				@ bail out for secure mode
> +	tst	r10, #1 << 11			@ bit #11 already set?
> +	orreq	r10, r10, #1 << 11		@ set bit #11 if not set
> +	mcreq	p15, 0, r10, c15, c0, 1		@ write diagnostic register
>  1:
>  #endif
>  
> 

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Tony Lindgren]

* Rob Herring <robherring2@gmail.com> [121114 16:56]:
> On 11/14/2012 04:21 PM, Tony Lindgren wrote:
> > * Rob Herring <robherring2@gmail.com> [121114 13:59]:
> >> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
> >>>
> >>> Checking for the bit already set should work in this case, I'll post
> >>> a patch for that shortly.
> >>
> >> Can you actually read the state of the diagnostic register in non-secure
> >> mode? If you can on the A9, is the same true on A8 or others?
> > 
> > Looks like it can be read on at least TI omap 4430 which is A9.
> > But it reads as zero, so the below patch is what I came up with.
> > 
> > No idea if assuming that zero value for the diagnostic register
> > is safe.. What's the default value of the diagnostic register supposed
> > to be?
> 
> RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
> even be talking about it.

WITFM? :)
 
> It could vary by rev, but I see 0 for the reset value, so this would not
> work if the bootloader did not do any setup of the diagnostic register.

OK
 
> One way to determine secure mode on the A9 would be seeing if you can
> change the auxcr register. Something like this (untested):
> 
> mrc	p15, 0, r0, c1, c0, 1; Read ACTLR
> eor	r1, r0, #0x100		; Modify alloc in 1 way
> mcr	p15, 0, r1, c1, c0, 1
> mrc	p15, 0, r2, c1, c0, 1; Read ACTLR
> mcr	p15, 0, r0, c1, c0, 1	; Restore original value
> cmp	r1, r2
> bne	skip_errata
> 
> 
> It would be good to do this test for all the errata rather than just
> this one.

I recall writes to the aux control registers producing an exception
on secure A8 based omaps, but I'll give that a try when I have a
chance.

Regards,

Tony

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Catalin Marinas]

On Thu, Nov 15, 2012 at 12:54:48AM +0000, Rob Herring wrote:
> On 11/14/2012 04:21 PM, Tony Lindgren wrote:
> > * Rob Herring <robherring2@gmail.com> [121114 13:59]:
> >> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
> >>>
> >>> Checking for the bit already set should work in this case, I'll post
> >>> a patch for that shortly.
> >>
> >> Can you actually read the state of the diagnostic register in non-secure
> >> mode? If you can on the A9, is the same true on A8 or others?
> > 
> > Looks like it can be read on at least TI omap 4430 which is A9.
> > But it reads as zero, so the below patch is what I came up with.
> > 
> > No idea if assuming that zero value for the diagnostic register
> > is safe.. What's the default value of the diagnostic register supposed
> > to be?
> 
> RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
> even be talking about it.
> 
> It could vary by rev, but I see 0 for the reset value, so this would not
> work if the bootloader did not do any setup of the diagnostic register.
> 
> One way to determine secure mode on the A9 would be seeing if you can
> change the auxcr register. Something like this (untested):
> 
> mrc	p15, 0, r0, c1, c0, 1; Read ACTLR
> eor	r1, r0, #0x100		; Modify alloc in 1 way
> mcr	p15, 0, r1, c1, c0, 1
> mrc	p15, 0, r2, c1, c0, 1; Read ACTLR
> mcr	p15, 0, r0, c1, c0, 1	; Restore original value
> cmp	r1, r2
> bne	skip_errata

This would fail on platforms where Linux runs in non-secure mode. What
we do for some errata workarounds is to test whether the bit was already
set and avoid writing the register. But this assumes that, for a given
workaround in the kernel, there is a corresponding workaround in the
code running before the kernel (boot-loader, firmware) which sets that
bit.

Since the kernel will run more often in non-secure mode (on Cortex-A15
you need this for the virtualisation extensions) I strongly suggest that
the workaround (usually undocumented bit setting) is done before the
kernel is started and we simply remove it from Linux (or add a clear
comment that it only works if running in secure mode; if unsure say
'N').

I don't think it's worth the hassle detecting whether the kernel runs in
secure or non-secure mode, just assume the latter and get SoC vendors to
update the boot loaders or firmware (if possible) with any errata
workarounds.

Having a common SMC API for errata workarounds is not feasible since not
all registers are public, most are implementation specific and it could
have secure implications with exposing them.

-- 
Catalin

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Siarhei Siamashka]

On Thu, Nov 15, 2012 at 1:01 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Thu, Nov 15, 2012 at 12:54:48AM +0000, Rob Herring wrote:
>> On 11/14/2012 04:21 PM, Tony Lindgren wrote:
>> > * Rob Herring <robherring2@gmail.com> [121114 13:59]:
>> >> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
>> >>>
>> >>> Checking for the bit already set should work in this case, I'll post
>> >>> a patch for that shortly.
>> >>
>> >> Can you actually read the state of the diagnostic register in non-secure
>> >> mode? If you can on the A9, is the same true on A8 or others?
>> >
>> > Looks like it can be read on at least TI omap 4430 which is A9.
>> > But it reads as zero, so the below patch is what I came up with.
>> >
>> > No idea if assuming that zero value for the diagnostic register
>> > is safe.. What's the default value of the diagnostic register supposed
>> > to be?
>>
>> RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
>> even be talking about it.
>>
>> It could vary by rev, but I see 0 for the reset value, so this would not
>> work if the bootloader did not do any setup of the diagnostic register.
>>
>> One way to determine secure mode on the A9 would be seeing if you can
>> change the auxcr register. Something like this (untested):
>>
>> mrc   p15, 0, r0, c1, c0, 1; Read ACTLR
>> eor   r1, r0, #0x100          ; Modify alloc in 1 way
>> mcr   p15, 0, r1, c1, c0, 1
>> mrc   p15, 0, r2, c1, c0, 1; Read ACTLR
>> mcr   p15, 0, r0, c1, c0, 1   ; Restore original value
>> cmp   r1, r2
>> bne   skip_errata
>
> This would fail on platforms where Linux runs in non-secure mode. What
> we do for some errata workarounds is to test whether the bit was already
> set and avoid writing the register. But this assumes that, for a given
> workaround in the kernel, there is a corresponding workaround in the
> code running before the kernel (boot-loader, firmware) which sets that
> bit.
>
> Since the kernel will run more often in non-secure mode (on Cortex-A15
> you need this for the virtualisation extensions) I strongly suggest that
> the workaround (usually undocumented bit setting) is done before the
> kernel is started and we simply remove it from Linux (or add a clear
> comment that it only works if running in secure mode; if unsure say
> 'N').
>
> I don't think it's worth the hassle detecting whether the kernel runs in
> secure or non-secure mode, just assume the latter and get SoC vendors to
> update the boot loaders or firmware (if possible) with any errata
> workarounds.
>
> Having a common SMC API for errata workarounds is not feasible since not
> all registers are public, most are implementation specific and it could
> have secure implications with exposing them.

BTW, I always wondered about what could be preventing TI and the other
silicon vendors from using something like an SMC API based on
asymmetric cryptography? My understanding is that for OMAP4 GP chips,
ROM code switches to non-secure mode before passing control to the
bootloader and there is simply no way to workaround bugs like this.
The users are screwed.

Having some way to pass a small signed chunk of code through SMC API
could be a life saver if the erratum is particularly nasty. The vendor
could just contribute the signed piece of code with a few instructions
to set the needed bits in the needed registers in the case of
emergency.

-- 
Best regards,
Siarhei Siamashka

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Thu, Nov 15, 2012 at 02:41:43PM +0200, Siarhei Siamashka wrote:
> BTW, I always wondered about what could be preventing TI and the other
> silicon vendors from using something like an SMC API based on
> asymmetric cryptography?

The answer is... nothing.  But it didn't happen and we're stuck with the
consequences of that.

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Catalin Marinas]

On Thu, Nov 15, 2012 at 12:41:43PM +0000, Siarhei Siamashka wrote:
> On Thu, Nov 15, 2012 at 1:01 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Thu, Nov 15, 2012 at 12:54:48AM +0000, Rob Herring wrote:
> >> On 11/14/2012 04:21 PM, Tony Lindgren wrote:
> >> > * Rob Herring <robherring2@gmail.com> [121114 13:59]:
> >> >> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
> >> >>>
> >> >>> Checking for the bit already set should work in this case, I'll post
> >> >>> a patch for that shortly.
> >> >>
> >> >> Can you actually read the state of the diagnostic register in non-secure
> >> >> mode? If you can on the A9, is the same true on A8 or others?
> >> >
> >> > Looks like it can be read on at least TI omap 4430 which is A9.
> >> > But it reads as zero, so the below patch is what I came up with.
> >> >
> >> > No idea if assuming that zero value for the diagnostic register
> >> > is safe.. What's the default value of the diagnostic register supposed
> >> > to be?
> >>
> >> RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
> >> even be talking about it.
> >>
> >> It could vary by rev, but I see 0 for the reset value, so this would not
> >> work if the bootloader did not do any setup of the diagnostic register.
> >>
> >> One way to determine secure mode on the A9 would be seeing if you can
> >> change the auxcr register. Something like this (untested):
> >>
> >> mrc   p15, 0, r0, c1, c0, 1; Read ACTLR
> >> eor   r1, r0, #0x100          ; Modify alloc in 1 way
> >> mcr   p15, 0, r1, c1, c0, 1
> >> mrc   p15, 0, r2, c1, c0, 1; Read ACTLR
> >> mcr   p15, 0, r0, c1, c0, 1   ; Restore original value
> >> cmp   r1, r2
> >> bne   skip_errata
> >
> > This would fail on platforms where Linux runs in non-secure mode. What
> > we do for some errata workarounds is to test whether the bit was already
> > set and avoid writing the register. But this assumes that, for a given
> > workaround in the kernel, there is a corresponding workaround in the
> > code running before the kernel (boot-loader, firmware) which sets that
> > bit.
> >
> > Since the kernel will run more often in non-secure mode (on Cortex-A15
> > you need this for the virtualisation extensions) I strongly suggest that
> > the workaround (usually undocumented bit setting) is done before the
> > kernel is started and we simply remove it from Linux (or add a clear
> > comment that it only works if running in secure mode; if unsure say
> > 'N').
> >
> > I don't think it's worth the hassle detecting whether the kernel runs in
> > secure or non-secure mode, just assume the latter and get SoC vendors to
> > update the boot loaders or firmware (if possible) with any errata
> > workarounds.
> >
> > Having a common SMC API for errata workarounds is not feasible since not
> > all registers are public, most are implementation specific and it could
> > have secure implications with exposing them.
> 
> BTW, I always wondered about what could be preventing TI and the other
> silicon vendors from using something like an SMC API based on
> asymmetric cryptography? My understanding is that for OMAP4 GP chips,
> ROM code switches to non-secure mode before passing control to the
> bootloader and there is simply no way to workaround bugs like this.

AFAIK, there are some SMCs to the OMAP secure firmware that allow such
bits to be set (see omap4_l2x0_set_debug() for example). I'm not sure
they are documented.

But we can't have a standard SMC API since the registers affected are
not standard (nor documented). Which means that such workarounds must be
applied in the bootloader specific to that platform. Even if it is
running in non-secure mode, it can be made aware of the SMC API provided
by the secure firmware.

-- 
Catalin

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Måns Rullgård]

Catalin Marinas <catalin.marinas@arm.com> writes:

> On Thu, Nov 15, 2012 at 12:41:43PM +0000, Siarhei Siamashka wrote:
>> BTW, I always wondered about what could be preventing TI and the other
>> silicon vendors from using something like an SMC API based on
>> asymmetric cryptography? My understanding is that for OMAP4 GP chips,
>> ROM code switches to non-secure mode before passing control to the
>> bootloader and there is simply no way to workaround bugs like this.
>
> AFAIK, there are some SMCs to the OMAP secure firmware that allow such
> bits to be set (see omap4_l2x0_set_debug() for example). I'm not sure
> they are documented.

The trouble with OMAP is that the secure ROM API only allows access to a
tiny subset of the registers we'd need.  In part this can be explained
by the important OMAP customers all using the HS chips with full access
to secure mode.

-- 
M?ns Rullg?rd
mans at mansr.com

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Rob Herring]

On 11/15/2012 05:01 AM, Catalin Marinas wrote:
> On Thu, Nov 15, 2012 at 12:54:48AM +0000, Rob Herring wrote:
>> On 11/14/2012 04:21 PM, Tony Lindgren wrote:
>>> * Rob Herring <robherring2@gmail.com> [121114 13:59]:
>>>> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
>>>>>
>>>>> Checking for the bit already set should work in this case, I'll post
>>>>> a patch for that shortly.
>>>>
>>>> Can you actually read the state of the diagnostic register in non-secure
>>>> mode? If you can on the A9, is the same true on A8 or others?
>>>
>>> Looks like it can be read on at least TI omap 4430 which is A9.
>>> But it reads as zero, so the below patch is what I came up with.
>>>
>>> No idea if assuming that zero value for the diagnostic register
>>> is safe.. What's the default value of the diagnostic register supposed
>>> to be?
>>
>> RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
>> even be talking about it.
>>
>> It could vary by rev, but I see 0 for the reset value, so this would not
>> work if the bootloader did not do any setup of the diagnostic register.
>>
>> One way to determine secure mode on the A9 would be seeing if you can
>> change the auxcr register. Something like this (untested):
>>
>> mrc	p15, 0, r0, c1, c0, 1; Read ACTLR
>> eor	r1, r0, #0x100		; Modify alloc in 1 way
>> mcr	p15, 0, r1, c1, c0, 1
>> mrc	p15, 0, r2, c1, c0, 1; Read ACTLR
>> mcr	p15, 0, r0, c1, c0, 1	; Restore original value
>> cmp	r1, r2
>> bne	skip_errata
> 
> This would fail on platforms where Linux runs in non-secure mode. What
> we do for some errata workarounds is to test whether the bit was already
> set and avoid writing the register. But this assumes that, for a given
> workaround in the kernel, there is a corresponding workaround in the
> code running before the kernel (boot-loader, firmware) which sets that
> bit.
> 
> Since the kernel will run more often in non-secure mode (on Cortex-A15
> you need this for the virtualisation extensions) I strongly suggest that
> the workaround (usually undocumented bit setting) is done before the
> kernel is started and we simply remove it from Linux (or add a clear
> comment that it only works if running in secure mode; if unsure say
> 'N').
> 
> I don't think it's worth the hassle detecting whether the kernel runs in
> secure or non-secure mode, just assume the latter and get SoC vendors to
> update the boot loaders or firmware (if possible) with any errata
> workarounds.

There's other places we need to know secure vs. non-secure mode like
whether we can do smc calls or not.

So we should make all these work-arounds depend on !MULTI_PLATFORM then.
Does that work for Versatile Express CA9? It needs ARM_ERRATA_751472.

Rob

> 
> Having a common SMC API for errata workarounds is not feasible since not
> all registers are public, most are implementation specific and it could
> have secure implications with exposing them.
> 

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Catalin Marinas]

On Thu, Nov 15, 2012 at 02:31:33PM +0000, Rob Herring wrote:
> On 11/15/2012 05:01 AM, Catalin Marinas wrote:
> > On Thu, Nov 15, 2012 at 12:54:48AM +0000, Rob Herring wrote:
> >> On 11/14/2012 04:21 PM, Tony Lindgren wrote:
> >>> * Rob Herring <robherring2@gmail.com> [121114 13:59]:
> >>>> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
> >>>>>
> >>>>> Checking for the bit already set should work in this case, I'll post
> >>>>> a patch for that shortly.
> >>>>
> >>>> Can you actually read the state of the diagnostic register in non-secure
> >>>> mode? If you can on the A9, is the same true on A8 or others?
> >>>
> >>> Looks like it can be read on at least TI omap 4430 which is A9.
> >>> But it reads as zero, so the below patch is what I came up with.
> >>>
> >>> No idea if assuming that zero value for the diagnostic register
> >>> is safe.. What's the default value of the diagnostic register supposed
> >>> to be?
> >>
> >> RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
> >> even be talking about it.
> >>
> >> It could vary by rev, but I see 0 for the reset value, so this would not
> >> work if the bootloader did not do any setup of the diagnostic register.
> >>
> >> One way to determine secure mode on the A9 would be seeing if you can
> >> change the auxcr register. Something like this (untested):
> >>
> >> mrc	p15, 0, r0, c1, c0, 1; Read ACTLR
> >> eor	r1, r0, #0x100		; Modify alloc in 1 way
> >> mcr	p15, 0, r1, c1, c0, 1
> >> mrc	p15, 0, r2, c1, c0, 1; Read ACTLR
> >> mcr	p15, 0, r0, c1, c0, 1	; Restore original value
> >> cmp	r1, r2
> >> bne	skip_errata
> > 
> > This would fail on platforms where Linux runs in non-secure mode. What
> > we do for some errata workarounds is to test whether the bit was already
> > set and avoid writing the register. But this assumes that, for a given
> > workaround in the kernel, there is a corresponding workaround in the
> > code running before the kernel (boot-loader, firmware) which sets that
> > bit.
> > 
> > Since the kernel will run more often in non-secure mode (on Cortex-A15
> > you need this for the virtualisation extensions) I strongly suggest that
> > the workaround (usually undocumented bit setting) is done before the
> > kernel is started and we simply remove it from Linux (or add a clear
> > comment that it only works if running in secure mode; if unsure say
> > 'N').
> > 
> > I don't think it's worth the hassle detecting whether the kernel runs in
> > secure or non-secure mode, just assume the latter and get SoC vendors to
> > update the boot loaders or firmware (if possible) with any errata
> > workarounds.
> 
> There's other places we need to know secure vs. non-secure mode like
> whether we can do smc calls or not.
> 
> So we should make all these work-arounds depend on !MULTI_PLATFORM then.

Only the workarounds that set bits in secure-only registers.

> Does that work for Versatile Express CA9? It needs ARM_ERRATA_751472.

On VE Linux runs in secure mode, so it's fine.

-- 
Catalin

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Rob Herring]

On 11/15/2012 08:37 AM, Catalin Marinas wrote:
> On Thu, Nov 15, 2012 at 02:31:33PM +0000, Rob Herring wrote:
>> On 11/15/2012 05:01 AM, Catalin Marinas wrote:
>>> On Thu, Nov 15, 2012 at 12:54:48AM +0000, Rob Herring wrote:
>>>> On 11/14/2012 04:21 PM, Tony Lindgren wrote:
>>>>> * Rob Herring <robherring2@gmail.com> [121114 13:59]:
>>>>>> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
>>>>>>>
>>>>>>> Checking for the bit already set should work in this case, I'll post
>>>>>>> a patch for that shortly.
>>>>>>
>>>>>> Can you actually read the state of the diagnostic register in non-secure
>>>>>> mode? If you can on the A9, is the same true on A8 or others?
>>>>>
>>>>> Looks like it can be read on at least TI omap 4430 which is A9.
>>>>> But it reads as zero, so the below patch is what I came up with.
>>>>>
>>>>> No idea if assuming that zero value for the diagnostic register
>>>>> is safe.. What's the default value of the diagnostic register supposed
>>>>> to be?
>>>>
>>>> RTFM. Oh, wait it's a super secret, undocumented register. We shouldn't
>>>> even be talking about it.
>>>>
>>>> It could vary by rev, but I see 0 for the reset value, so this would not
>>>> work if the bootloader did not do any setup of the diagnostic register.
>>>>
>>>> One way to determine secure mode on the A9 would be seeing if you can
>>>> change the auxcr register. Something like this (untested):
>>>>
>>>> mrc	p15, 0, r0, c1, c0, 1; Read ACTLR
>>>> eor	r1, r0, #0x100		; Modify alloc in 1 way
>>>> mcr	p15, 0, r1, c1, c0, 1
>>>> mrc	p15, 0, r2, c1, c0, 1; Read ACTLR
>>>> mcr	p15, 0, r0, c1, c0, 1	; Restore original value
>>>> cmp	r1, r2
>>>> bne	skip_errata
>>>
>>> This would fail on platforms where Linux runs in non-secure mode. What
>>> we do for some errata workarounds is to test whether the bit was already
>>> set and avoid writing the register. But this assumes that, for a given
>>> workaround in the kernel, there is a corresponding workaround in the
>>> code running before the kernel (boot-loader, firmware) which sets that
>>> bit.
>>>
>>> Since the kernel will run more often in non-secure mode (on Cortex-A15
>>> you need this for the virtualisation extensions) I strongly suggest that
>>> the workaround (usually undocumented bit setting) is done before the
>>> kernel is started and we simply remove it from Linux (or add a clear
>>> comment that it only works if running in secure mode; if unsure say
>>> 'N').
>>>
>>> I don't think it's worth the hassle detecting whether the kernel runs in
>>> secure or non-secure mode, just assume the latter and get SoC vendors to
>>> update the boot loaders or firmware (if possible) with any errata
>>> workarounds.
>>
>> There's other places we need to know secure vs. non-secure mode like
>> whether we can do smc calls or not.
>>
>> So we should make all these work-arounds depend on !MULTI_PLATFORM then.
> 
> Only the workarounds that set bits in secure-only registers.

Right.

>> Does that work for Versatile Express CA9? It needs ARM_ERRATA_751472.
> 
> On VE Linux runs in secure mode, so it's fine.

WTF? You are contradicting yourself. Don't determine secure mode or not,
but apply work-arounds only in secure mode? How does a kernel built to
boot on secure and non-secure chips know that? The requirement would be
that every platform have proper work-arounds setup by the bootloader
regardless of running in secure or non-secure mode.

Rob

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Catalin Marinas]

On Thu, Nov 15, 2012 at 03:37:08PM +0000, Rob Herring wrote:
> On 11/15/2012 08:37 AM, Catalin Marinas wrote:
> > On Thu, Nov 15, 2012 at 02:31:33PM +0000, Rob Herring wrote:
> >> Does that work for Versatile Express CA9? It needs ARM_ERRATA_751472.
> > 
> > On VE Linux runs in secure mode, so it's fine.
> 
> WTF? You are contradicting yourself.

No, it was just a statement that you can enable this workaround on VE
CA9. Didn't realise you were referring to the MULTI_PLATFORM case (it's
afternoon and coffee here not that great).

> Don't determine secure mode or not,
> but apply work-arounds only in secure mode? How does a kernel built to
> boot on secure and non-secure chips know that? The requirement would be
> that every platform have proper work-arounds setup by the bootloader
> regardless of running in secure or non-secure mode.

Yes, so for such workarounds that require secure register access just
make them depend on !MULTI_PLATFORM. Of course, VE CA9 would be affected
since neither the boot loader nor boot monitor touch those bits (yet).
But they definitely should as I don't see any other way to support
multi-platform, especially when such bits need to be set long before the
DT is parsed.

-- 
Catalin

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Thu, Nov 15, 2012 at 09:37:08AM -0600, Rob Herring wrote:
> >> Does that work for Versatile Express CA9? It needs ARM_ERRATA_751472.
> > 
> > On VE Linux runs in secure mode, so it's fine.
> 
> WTF? You are contradicting yourself.

No, it's already been explained; the problem is lack of understanding.

Versatile Express does indeed run in secure mode; it doesn't have any
secure monitor present.  OMAP and many other platforms run in non-secure
mode.

The work-arounds are applied to secure mode registers which are sensitive
to writes in the following manner:
- we check the revision of the CPU to see whether the workaround is
  applicable.  If it is, then...
  - the register is read.
  - the bit(s) are checked to see whether the work-around has already been
    applied.
  - the bit(s) is set to the appropriate state.
  - the register is written _if_ the work-around has not already been applied.

That means a platform running in secure mode gets the work-arounds applied
as appropriate for the CPU.  It also means that a platform running in non-
secure mode won't boot if the work-around has not already been applied.
That is a good thing; some work-arounds fix data corrupting bugs, and we
don't want an unsafe kernel running on such platforms.

So, we don't detect whether we're running in secure mode or not; as I've
already stated, we don't have a way to do that.  We instead only apply
work-arounds which aren't already enabled prior to the kernel booting.
So, even on a secure mode platform, we will avoid writing the bits if the
work-around has already been applied.

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [121116 02:07]:
> 
> So, we don't detect whether we're running in secure mode or not; as I've
> already stated, we don't have a way to do that.  We instead only apply
> work-arounds which aren't already enabled prior to the kernel booting.
> So, even on a secure mode platform, we will avoid writing the bits if the
> work-around has already been applied.

This all assumes that we can read the value of the diagnostic register,
and on my 4430 blaze the read returns zero. I have no idea if this is
the correct value for the register, or if reads always just returns 0.

If we can verify that the read of the diagnostic register returns the
correct value, then we don't need to test for the secure mode like you
are saying, and can require the bootrom or bootloader set the right bits.

Can somebody confirm that reading of the diagnostic register without
using SMI is supposed to return the correct value?

Regards,

Tony

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Fri, Nov 16, 2012 at 09:13:33AM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [121116 02:07]:
> > 
> > So, we don't detect whether we're running in secure mode or not; as I've
> > already stated, we don't have a way to do that.  We instead only apply
> > work-arounds which aren't already enabled prior to the kernel booting.
> > So, even on a secure mode platform, we will avoid writing the bits if the
> > work-around has already been applied.
> 
> This all assumes that we can read the value of the diagnostic register,
> and on my 4430 blaze the read returns zero. I have no idea if this is
> the correct value for the register, or if reads always just returns 0.

ARM Ltd has made that assumption since the inception of the errata
work-arounds appearing in the kernel for v6+ CPUs...

But your question may prove to be moot if we end up ripping all these
out, like I'm beginning to think we should do.

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Thu, Nov 15, 2012 at 08:31:33AM -0600, Rob Herring wrote:
> So we should make all these work-arounds depend on !MULTI_PLATFORM then.

No, because some of the work-arounds don't require setting bits in magic
registers.

Also realise this: we test for the revision of the CPU to which they're
applicable before attempting to set them.  If you have a work-around
enabled in the kernel, and it fails at boot, _that_ is an indicator not
that the kernel is doing something wrong, but that you need to ensure
that the work-around has been applied by the earlier stages.

It's not about "but platform X doesn't need it" - it's about platform X
not having the earlier stages updated to fix the errata.

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Catalin Marinas]

On Fri, Nov 16, 2012 at 09:59:21AM +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 15, 2012 at 08:31:33AM -0600, Rob Herring wrote:
> > So we should make all these work-arounds depend on !MULTI_PLATFORM then.
> 
> No, because some of the work-arounds don't require setting bits in magic
> registers.
> 
> Also realise this: we test for the revision of the CPU to which they're
> applicable before attempting to set them.  If you have a work-around
> enabled in the kernel, and it fails at boot, _that_ is an indicator not
> that the kernel is doing something wrong, but that you need to ensure
> that the work-around has been applied by the earlier stages.
> 
> It's not about "but platform X doesn't need it" - it's about platform X
> not having the earlier stages updated to fix the errata.

There is another aspect. Many CPUs in the field, even if they are a
certain rxpy revision, have ECO fixes applied for critical bugs and
don't require the workaround. Sometimes those undocumented bits have
considerable performance impact and vendors may apply an ECO (unless
they are very late in the tape-out process). But the ECO fix does not
change the CPU revision, hence the workaround becomes platform specific.

That's why I think it's better if those workarounds are just pushed to
the boot-loader for multi-platform kernels. Linux could still check the
bits and warn about them rather than failing to boot.

-- 
Catalin

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Fri, Nov 16, 2012 at 06:09:28PM +0000, Catalin Marinas wrote:
> On Fri, Nov 16, 2012 at 09:59:21AM +0000, Russell King - ARM Linux wrote:
> > On Thu, Nov 15, 2012 at 08:31:33AM -0600, Rob Herring wrote:
> > > So we should make all these work-arounds depend on !MULTI_PLATFORM then.
> > 
> > No, because some of the work-arounds don't require setting bits in magic
> > registers.
> > 
> > Also realise this: we test for the revision of the CPU to which they're
> > applicable before attempting to set them.  If you have a work-around
> > enabled in the kernel, and it fails at boot, _that_ is an indicator not
> > that the kernel is doing something wrong, but that you need to ensure
> > that the work-around has been applied by the earlier stages.
> > 
> > It's not about "but platform X doesn't need it" - it's about platform X
> > not having the earlier stages updated to fix the errata.
> 
> There is another aspect. Many CPUs in the field, even if they are a
> certain rxpy revision, have ECO fixes applied for critical bugs and
> don't require the workaround. Sometimes those undocumented bits have
> considerable performance impact and vendors may apply an ECO (unless
> they are very late in the tape-out process). But the ECO fix does not
> change the CPU revision, hence the workaround becomes platform specific.
> 
> That's why I think it's better if those workarounds are just pushed to
> the boot-loader for multi-platform kernels. Linux could still check the
> bits and warn about them rather than failing to boot.

... and that's a U-turn if ever there was one... it's ARM Ltd who've been
pushing to have these work-arounds in the kernel in the first place.

Should we just remove all the work-arounds, and require boot loaders,
including the one on Versatile platforms, to implement this?  Why should
we treat secure platforms be any different from the non-secure ones in
this regard?  After all, this _does_ stand in the way of a single kernel
image working properly on secure and non-secure platforms.

The more this thread progresses, the more I think the decision to put
errata into the kernel was the wrong one.

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Catalin Marinas]

On 16 November 2012 18:39, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Nov 16, 2012 at 06:09:28PM +0000, Catalin Marinas wrote:
>> On Fri, Nov 16, 2012 at 09:59:21AM +0000, Russell King - ARM Linux wrote:
>> > On Thu, Nov 15, 2012 at 08:31:33AM -0600, Rob Herring wrote:
>> > > So we should make all these work-arounds depend on !MULTI_PLATFORM then.
>> >
>> > No, because some of the work-arounds don't require setting bits in magic
>> > registers.
>> >
>> > Also realise this: we test for the revision of the CPU to which they're
>> > applicable before attempting to set them.  If you have a work-around
>> > enabled in the kernel, and it fails at boot, _that_ is an indicator not
>> > that the kernel is doing something wrong, but that you need to ensure
>> > that the work-around has been applied by the earlier stages.
>> >
>> > It's not about "but platform X doesn't need it" - it's about platform X
>> > not having the earlier stages updated to fix the errata.
>>
>> There is another aspect. Many CPUs in the field, even if they are a
>> certain rxpy revision, have ECO fixes applied for critical bugs and
>> don't require the workaround. Sometimes those undocumented bits have
>> considerable performance impact and vendors may apply an ECO (unless
>> they are very late in the tape-out process). But the ECO fix does not
>> change the CPU revision, hence the workaround becomes platform specific.
>>
>> That's why I think it's better if those workarounds are just pushed to
>> the boot-loader for multi-platform kernels. Linux could still check the
>> bits and warn about them rather than failing to boot.
>
> ... and that's a U-turn if ever there was one... it's ARM Ltd who've been
> pushing to have these work-arounds in the kernel in the first place.

I wouldn't say it's a U-turn. ARM Ltd never had an official position
(i.e. document) on where the secure-only workarounds should be
implemented (but I think there should have been one). The most
convenient for me and SoC vendors was to push the workarounds into the
kernel, together with other run-time workarounds (which we must keep
in the kernel anyway).

> Should we just remove all the work-arounds, and require boot loaders,
> including the one on Versatile platforms, to implement this?  Why should
> we treat secure platforms be any different from the non-secure ones in
> this regard?  After all, this _does_ stand in the way of a single kernel
> image working properly on secure and non-secure platforms.

I'll ack this (but not all SoC vendors will agree). ARM Ltd, with the
introduction of TZ in ARMv6, made it clear that a general-purpose OS
should not differentiate between secure and non-secure worlds (no
register to read the state from). Furthermore, with the virtualisation
extensions (ARMv7, ARMv8), the general-purpose OS is supposed to run
in non-secure mode. This means that the place for secure-only
workarounds is outside the OS kernel.

> The more this thread progresses, the more I think the decision to put
> errata into the kernel was the wrong one.

Not entirely. This started at a time when we didn't have TZ (ARM1136).
We may also use Linux as a bootloader (kexec) and it's not clear
whether we need a pre-bootloader. But I think for now we could just
make the secure-only boot-time workarounds depend on
!ARCH_MULTIPLATFORM. For newer cores like Cortex-A15/A7 where we know
that Linux always runs in NS mode, don't add new secure-only
workarounds.

For AArch64 I will not merge any secure-only errata workarounds. I'll
leave this to the firmware (can be UEFI or a bootloader). The same for
other implementation-defined bits like SMP/nAMP which Linux can't
touch while in NS mode.

-- 
Catalin

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Russell King - ARM Linux]

On Wed, Nov 14, 2012 at 02:21:59PM -0800, Tony Lindgren wrote:
> No idea if assuming that zero value for the diagnostic register
> is safe.. What's the default value of the diagnostic register supposed
> to be?

No, that's not safe.  What if your pre-kernel code has asked the secure
monitor to set the work-around bit already?

>  #if defined(CONFIG_ARM_ERRATA_751472) && defined(CONFIG_SMP)
>  	ALT_SMP(cmp r6, #0x30)			@ present prior to r3p0
>  	ALT_UP_B(1f)
> -	mrclt	p15, 0, r10, c15, c0, 1		@ read diagnostic register
> -	orrlt	r10, r10, #1 << 11		@ set bit #11
> -	mcrlt	p15, 0, r10, c15, c0, 1		@ write diagnostic register
> +	bge	1f				@ not needed for r3p0 and later
> +	mrc	p15, 0, r10, c15, c0, 1		@ read diagnostic register
> +	teq	r10, #0				@ zero for secure mode?
> +	beq	1f				@ bail out for secure mode

This test for zero is pointless.  What if some other work-around has
been enabled but not this one?

> +	tst	r10, #1 << 11			@ bit #11 already set?
> +	orreq	r10, r10, #1 << 11		@ set bit #11 if not set
> +	mcreq	p15, 0, r10, c15, c0, 1		@ write diagnostic register
>  1:
>  #endif
>  

[PATCH] ARM: Fix errata 751472 handling on Cortex-A9 r1p*

[Jon Masters]

On 11/14/2012 04:57 PM, Rob Herring wrote:
> On 11/14/2012 02:32 PM, Tony Lindgren wrote:
>> * Russell King - ARM Linux <linux@arm.linux.org.uk> [121114 12:24]:
>>> On Wed, Nov 14, 2012 at 01:06:53PM -0600, Jon Hunter wrote:
>>>>
>>>> On 11/14/2012 12:53 PM, Tony Lindgren wrote:
>>>>> Looks like enabling CONFIG_ARM_ERRATA_751472 causes omap4 blaze
>>>>> to not boot when enabled. The ARM core on it is an earlier r1p2:
>>>>>
>>>>> CPU: ARMv7 Processor [411fc092] revision 2 (ARMv7), cr=10c53c7d
>>>>>
>>>>> Unfortunately I don't have the details of errata 751472, but I'm
>>>>> guessing we need to disable it for r1p*.
>>>>
>>>> I checked the CA9MP errata document and this erratum impacts all
>>>> r0/r1/r2 CPUs. I am wondering if the problem is because the workaround
>>>> requires you to set a bit in the Diagnostic Control register and the
>>>> read-modify-write sequence provided in the workaround is for secure
>>>> mode. Not sure if there is a non-secure workaround available :-(
>>>
>>> Most likely, and there's not a lot that the kernel can sanely do about
>>> that.  We have ended up deciding (through being forced to because of
>>> how the security stuff works) that the stages prior to the kernel will
>>> implement the work-around enables because those stages are already
>>> platform specific, and the kernel will implement a "test for the
>>> work-around already enabled."
>>>
>>> The net result is, if you enable an Errata in the kernel which your
>>> earlier boot stages has not already configured, the kernel will hang.
>>> Not much we can do about the hanging aspect, because the kernel takes
>>> an exception which we can't trap at those early stages in the boot
>>> process.
>>>
>>> I'm not particularly happy about that design, but that's what we've
>>> ended up with through the 'design' of the security stuff forced onto
>>> us.
>>
>> Checking for the bit already set should work in this case, I'll post
>> a patch for that shortly.
> 
> Can you actually read the state of the diagnostic register in non-secure
> mode? If you can on the A9, is the same true on A8 or others?
> 
> Multi-platform kernels present a new problem in that we basically need
> to enable all errata work-arounds. I've been meaning to look thru the
> errata work-arounds to figure out which ones can be selected for
> multi-platform kernels without side effects on unaffected parts (i.e.
> set a chicken bit based on core revision). For any in runtime paths, we
> may need to do runtime patching if they have performance impact.

Rob, per my other email just now, the two that were turned on in
Fedora's initial multiplatform kernel that were failing on highbank are
the following: CONFIG_PL310_ERRATA_588369 and
CONFIG_PL310_ERRATA_727915. Thanks for the initial reference to this
thread, which I had missed. Just want to throw those data points in.

Jon.