Message ID | 20121114222159.GB6801@atomide.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
--- 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