[PATCH] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[PATCH] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

Fixes the TCM initialisation code to handle TCM banks that are
present but inaccessible due to TrustZone configuration.  This is
the default case when enabling the non-secure world.  It may also
be the case that that the user decided to use TCM for TrustZone.

This change has exposed a bug in handling of TCM where no TCM bank
was usable (the 0 size TCM case).  This change addresses the
resulting hang.

Signed-off-by: Michael van der Westhuizen <michael@smart-africa.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/kernel/tcm.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/tcm.c b/arch/arm/kernel/tcm.c
index 7a3be1d..af1dd5a 100644
--- a/arch/arm/kernel/tcm.c
+++ b/arch/arm/kernel/tcm.c
@@ -17,6 +17,7 @@
 #include <asm/mach/map.h>
 #include <asm/memory.h>
 #include <asm/system_info.h>
+#include <asm/traps.h>
 
 static struct gen_pool *tcm_pool;
 static bool dtcm_present;
@@ -175,6 +176,21 @@ static int __init setup_tcm_bank(u8 type, u8 bank, u8 banks,
 	return 0;
 }
 
+static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
+{
+	regs->uregs[(instr >> 12) & 0xf] = 0;
+	regs->ARM_pc += 4;
+	return 0;
+}
+
+static const struct undef_hook tcm_hook __initconst = {
+	.instr_mask	= 0xffff0fdf,
+	.instr_val	= 0xee190f11,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= SVC_MODE,
+	.fn		= tcm_handler
+};
+
 /*
  * This initializes the TCM memory
  */
@@ -207,6 +223,8 @@ void __init tcm_init(void)
 	dtcm_banks = (tcm_status >> 16) & 0x03;
 	itcm_banks = (tcm_status & 0x03);
 
+	register_undef_hook(&tcm_hook);
+
 	/* Values greater than 2 for D/ITCM banks are "reserved" */
 	if (dtcm_banks > 2)
 		dtcm_banks = 0;
@@ -218,7 +236,7 @@ void __init tcm_init(void)
 		for (i = 0; i < dtcm_banks; i++) {
 			ret = setup_tcm_bank(0, i, dtcm_banks, &dtcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into DTCM */
 		if (dtcm_code_sz > (dtcm_end - DTCM_OFFSET)) {
@@ -227,6 +245,10 @@ void __init tcm_init(void)
 				dtcm_code_sz, (dtcm_end - DTCM_OFFSET));
 			goto no_dtcm;
 		}
+		/* This means that the DTCM sizes were 0 or the DTCM banks
+		 * were inaccessible due to TrustZone configuration */
+		if (!(dtcm_end - DTCM_OFFSET))
+			goto no_dtcm;
 		dtcm_res.end = dtcm_end - 1;
 		request_resource(&iomem_resource, &dtcm_res);
 		dtcm_iomap[0].length = dtcm_end - DTCM_OFFSET;
@@ -250,15 +272,19 @@ no_dtcm:
 		for (i = 0; i < itcm_banks; i++) {
 			ret = setup_tcm_bank(1, i, itcm_banks, &itcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into ITCM */
 		if (itcm_code_sz > (itcm_end - ITCM_OFFSET)) {
 			pr_info("CPU ITCM: %u bytes of code compiled to "
 				"ITCM but only %lu bytes of ITCM present\n",
 				itcm_code_sz, (itcm_end - ITCM_OFFSET));
-			return;
+			goto unregister;
 		}
+		/* This means that the ITCM sizes were 0 or the ITCM banks
+		 * were inaccessible due to TrustZone configuration */
+		if (!(itcm_end - ITCM_OFFSET))
+			goto unregister;
 		itcm_res.end = itcm_end - 1;
 		request_resource(&iomem_resource, &itcm_res);
 		itcm_iomap[0].length = itcm_end - ITCM_OFFSET;
@@ -275,6 +301,9 @@ no_dtcm:
 		pr_info("CPU ITCM: %u bytes of code compiled to ITCM but no "
 			"ITCM banks present in CPU\n", itcm_code_sz);
 	}
+
+unregister:
+	unregister_undef_hook(&tcm_hook);
 }
 
 /*
-- 
2.1.4

[PATCH] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Dave Martin]

On Thu, May 28, 2015 at 11:36:00AM +0200, Michael van der Westhuizen wrote:
> Fixes the TCM initialisation code to handle TCM banks that are
> present but inaccessible due to TrustZone configuration.  This is
> the default case when enabling the non-secure world.  It may also
> be the case that that the user decided to use TCM for TrustZone.
> 
> This change has exposed a bug in handling of TCM where no TCM bank
> was usable (the 0 size TCM case).  This change addresses the
> resulting hang.

The TCM registers in CP15 are not part of the architecture -- behaviour
is IMP DEF in v7.

This is a problem for multiplatform kernels in particular.  In a v6/v7
multiplatform kernel with a TCM-enabled platform built in, it is
unsafe to probe for TCM by accessing these registers if we are running
on v7.  No Undef exception is guaranteed -- anything might happen.

We could add a DT binding for TCM, but it should only describe whether
the CP15 TCM registers are accessible (node present in DT) or not (node
present, but disabled).

For backwards compatiblity with old DTs we could maybe fall back to
unconditional probing for single-platform kernels only, when no tcm
node is present in the DT.

Cheers
---Dave

[...]

[PATCH] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

> On 28 May 2015, at 12:16 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> 
> On Thu, May 28, 2015 at 11:36:00AM +0200, Michael van der Westhuizen wrote:
>> Fixes the TCM initialisation code to handle TCM banks that are
>> present but inaccessible due to TrustZone configuration.  This is
>> the default case when enabling the non-secure world.  It may also
>> be the case that that the user decided to use TCM for TrustZone.
>> 
>> This change has exposed a bug in handling of TCM where no TCM bank
>> was usable (the 0 size TCM case).  This change addresses the
>> resulting hang.
> 
> The TCM registers in CP15 are not part of the architecture -- behaviour
> is IMP DEF in v7.

My reading of DDI0406C_C is that the register is defined (CP15, c0, c0, 2), but the format is either v6 format or implementation defined.

The manual explicitly states that in v7 the register must be implemented and that when v7 format is used that the meaning of bits 28:0 is implementation defined (this is all in B4.1.132).

The ARM goes on to state that when no TCMs are implemented the TCMTR register must be implemented in ARMv6 format, indicating no TCM banks (i.e. all defined bits must be 0).

So, since this code assumes v6 format should I just add a check that bits 31:29 or 0b000?  If I do this, then my reading is that this will continue to work reliably in the face of v7 implementations that use v7 (implementation defined) format.

Michael

> 
> This is a problem for multiplatform kernels in particular.  In a v6/v7
> multiplatform kernel with a TCM-enabled platform built in, it is
> unsafe to probe for TCM by accessing these registers if we are running
> on v7.  No Undef exception is guaranteed -- anything might happen.
> 
> We could add a DT binding for TCM, but it should only describe whether
> the CP15 TCM registers are accessible (node present in DT) or not (node
> present, but disabled).
> 
> For backwards compatiblity with old DTs we could maybe fall back to
> unconditional probing for single-platform kernels only, when no tcm
> node is present in the DT.
> 
> Cheers
> ---Dave
> 
> [...]
> 

[PATCH] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Dave Martin]

On Thu, May 28, 2015 at 01:32:10PM +0200, Michael van der Westhuizen wrote:
> 
> > On 28 May 2015, at 12:16 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> > 
> > On Thu, May 28, 2015 at 11:36:00AM +0200, Michael van der Westhuizen wrote:
> >> Fixes the TCM initialisation code to handle TCM banks that are
> >> present but inaccessible due to TrustZone configuration.  This is
> >> the default case when enabling the non-secure world.  It may also
> >> be the case that that the user decided to use TCM for TrustZone.
> >> 
> >> This change has exposed a bug in handling of TCM where no TCM bank
> >> was usable (the 0 size TCM case).  This change addresses the
> >> resulting hang.
> > 
> > The TCM registers in CP15 are not part of the architecture -- behaviour
> > is IMP DEF in v7.
> 
> My reading of DDI0406C_C is that the register is defined (CP15, c0,
> c0, 2), but the format is either v6 format or implementation defined.
> 
> The manual explicitly states that in v7 the register must be
> implemented and that when v7 format is used that the meaning of bits
> 28:0 is implementation defined (this is all in B4.1.132).
> 
> The ARM goes on to state that when no TCMs are implemented the TCMTR
> register must be implemented in ARMv6 format, indicating no TCM banks
> (i.e. all defined bits must be 0).
> 
> So, since this code assumes v6 format should I just add a check that
> bits 31:29 or 0b000?  If I do this, then my reading is that this will
> continue to work reliably in the face of v7 implementations that use
> v7 (implementation defined) format.

You're right, that looks sound.

Providing that TCMTR is read first and reports v6 format, then access
to the region registers will either succeed safely, or Undef (when
disallowed by the Secure World).

TCMTR itself is guaranteed to be readable even in the ARMv6 base
architecture.

ARMv8 gives mixed messages on this point[1], but it appears[2] that
the intention is for the above check to continue to work.

[1] DDI0487A.e, G6.2.126 (TCMTR, TCM Type Register)
[2] DDI0487A.e, G3.5 (System register support for IMPLEMENTATION DEFINED
memory features)


It would be good to see testing of this in a multiplatform kernel,
assuming you haven't tried it already.

[...]

Cheers
---Dave

[PATCH] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

> On 28 May 2015, at 3:32 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> 
> On Thu, May 28, 2015 at 01:32:10PM +0200, Michael van der Westhuizen wrote:
>> 
>>> On 28 May 2015, at 12:16 PM, Dave Martin <Dave.Martin@arm.com> wrote:
>>> 
>>> On Thu, May 28, 2015 at 11:36:00AM +0200, Michael van der Westhuizen wrote:
>>>> Fixes the TCM initialisation code to handle TCM banks that are
>>>> present but inaccessible due to TrustZone configuration.  This is
>>>> the default case when enabling the non-secure world.  It may also
>>>> be the case that that the user decided to use TCM for TrustZone.
>>>> 
>>>> This change has exposed a bug in handling of TCM where no TCM bank
>>>> was usable (the 0 size TCM case).  This change addresses the
>>>> resulting hang.
>>> 
>>> The TCM registers in CP15 are not part of the architecture -- behaviour
>>> is IMP DEF in v7.
>> 
>> My reading of DDI0406C_C is that the register is defined (CP15, c0,
>> c0, 2), but the format is either v6 format or implementation defined.
>> 
>> The manual explicitly states that in v7 the register must be
>> implemented and that when v7 format is used that the meaning of bits
>> 28:0 is implementation defined (this is all in B4.1.132).
>> 
>> The ARM goes on to state that when no TCMs are implemented the TCMTR
>> register must be implemented in ARMv6 format, indicating no TCM banks
>> (i.e. all defined bits must be 0).
>> 
>> So, since this code assumes v6 format should I just add a check that
>> bits 31:29 or 0b000?  If I do this, then my reading is that this will
>> continue to work reliably in the face of v7 implementations that use
>> v7 (implementation defined) format.
> 
> You're right, that looks sound.

I?ll resend with this check added.

> 
> Providing that TCMTR is read first and reports v6 format, then access
> to the region registers will either succeed safely, or Undef (when
> disallowed by the Secure World).
> 
> TCMTR itself is guaranteed to be readable even in the ARMv6 base
> architecture.
> 
> ARMv8 gives mixed messages on this point[1], but it appears[2] that
> the intention is for the above check to continue to work.
> 
> [1] DDI0487A.e, G6.2.126 (TCMTR, TCM Type Register)
> [2] DDI0487A.e, G3.5 (System register support for IMPLEMENTATION DEFINED
> memory features)
> 
> 
> It would be good to see testing of this in a multiplatform kernel,
> assuming you haven't tried it already.

I?m afraid that I only have one (V6K) board to test against.

Michael

> 
> [...]
> 
> Cheers
> ---Dave
> 

[PATCH v2] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

Fixes the TCM initialisation code to handle TCM banks that are
present but inaccessible due to TrustZone configuration.  This is
the default case when enabling the non-secure world.  It may also
be the case that that the user decided to use TCM for TrustZone.

This change has exposed a bug in handling of TCM where no TCM bank
was usable (the 0 size TCM case).  This change addresses the
resulting hang.

This code only handles the ARMv6 TCMTR register format, and will not
work correctly on boards that use the ARMv7 (or any other) format.
This is handled by performing an early exit from the initialisation
function when the TCMTR reports any format other than v6.

Signed-off-by: Michael van der Westhuizen <michael@smart-africa.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Dave Martin <Dave.Martin@arm.com>
---

Changes in v2:
  - Mark the TCM undefined hook data structure as __initdata, since
    that's what it is.
  - Ensure that we're dealing with an ARMv6 TCMTR format.

 arch/arm/kernel/tcm.c | 44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/tcm.c b/arch/arm/kernel/tcm.c
index 7a3be1d..d1905d9 100644
--- a/arch/arm/kernel/tcm.c
+++ b/arch/arm/kernel/tcm.c
@@ -17,6 +17,9 @@
 #include <asm/mach/map.h>
 #include <asm/memory.h>
 #include <asm/system_info.h>
+#include <asm/traps.h>
+
+#define TCMTR_FORMAT_MASK 0xe0000000U
 
 static struct gen_pool *tcm_pool;
 static bool dtcm_present;
@@ -175,6 +178,21 @@ static int __init setup_tcm_bank(u8 type, u8 bank, u8 banks,
 	return 0;
 }
 
+static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
+{
+	regs->uregs[(instr >> 12) & 0xf] = 0;
+	regs->ARM_pc += 4;
+	return 0;
+}
+
+static struct undef_hook tcm_hook __initdata = {
+	.instr_mask	= 0xffff0fdf,
+	.instr_val	= 0xee190f11,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= SVC_MODE,
+	.fn		= tcm_handler
+};
+
 /*
  * This initializes the TCM memory
  */
@@ -204,9 +222,18 @@ void __init tcm_init(void)
 	}
 
 	tcm_status = read_cpuid_tcmstatus();
+
+	/*
+	 * This code only supports v6-compatible TCMTR implementations.
+	 */
+	if (tcm_status & TCMTR_FORMAT_MASK)
+		return;
+
 	dtcm_banks = (tcm_status >> 16) & 0x03;
 	itcm_banks = (tcm_status & 0x03);
 
+	register_undef_hook(&tcm_hook);
+
 	/* Values greater than 2 for D/ITCM banks are "reserved" */
 	if (dtcm_banks > 2)
 		dtcm_banks = 0;
@@ -218,7 +245,7 @@ void __init tcm_init(void)
 		for (i = 0; i < dtcm_banks; i++) {
 			ret = setup_tcm_bank(0, i, dtcm_banks, &dtcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into DTCM */
 		if (dtcm_code_sz > (dtcm_end - DTCM_OFFSET)) {
@@ -227,6 +254,10 @@ void __init tcm_init(void)
 				dtcm_code_sz, (dtcm_end - DTCM_OFFSET));
 			goto no_dtcm;
 		}
+		/* This means that the DTCM sizes were 0 or the DTCM banks
+		 * were inaccessible due to TrustZone configuration */
+		if (!(dtcm_end - DTCM_OFFSET))
+			goto no_dtcm;
 		dtcm_res.end = dtcm_end - 1;
 		request_resource(&iomem_resource, &dtcm_res);
 		dtcm_iomap[0].length = dtcm_end - DTCM_OFFSET;
@@ -250,15 +281,19 @@ no_dtcm:
 		for (i = 0; i < itcm_banks; i++) {
 			ret = setup_tcm_bank(1, i, itcm_banks, &itcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into ITCM */
 		if (itcm_code_sz > (itcm_end - ITCM_OFFSET)) {
 			pr_info("CPU ITCM: %u bytes of code compiled to "
 				"ITCM but only %lu bytes of ITCM present\n",
 				itcm_code_sz, (itcm_end - ITCM_OFFSET));
-			return;
+			goto unregister;
 		}
+		/* This means that the ITCM sizes were 0 or the ITCM banks
+		 * were inaccessible due to TrustZone configuration */
+		if (!(itcm_end - ITCM_OFFSET))
+			goto unregister;
 		itcm_res.end = itcm_end - 1;
 		request_resource(&iomem_resource, &itcm_res);
 		itcm_iomap[0].length = itcm_end - ITCM_OFFSET;
@@ -275,6 +310,9 @@ no_dtcm:
 		pr_info("CPU ITCM: %u bytes of code compiled to ITCM but no "
 			"ITCM banks present in CPU\n", itcm_code_sz);
 	}
+
+unregister:
+	unregister_undef_hook(&tcm_hook);
 }
 
 /*
-- 
2.1.4

[PATCH v2] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Linus Walleij]

On Thu, May 28, 2015 at 3:54 PM, Michael van der Westhuizen
<michael@smart-africa.com> wrote:

> Fixes the TCM initialisation code to handle TCM banks that are
> present but inaccessible due to TrustZone configuration.  This is
> the default case when enabling the non-secure world.  It may also
> be the case that that the user decided to use TCM for TrustZone.
>
> This change has exposed a bug in handling of TCM where no TCM bank
> was usable (the 0 size TCM case).  This change addresses the
> resulting hang.
>
> This code only handles the ARMv6 TCMTR register format, and will not
> work correctly on boards that use the ARMv7 (or any other) format.
> This is handled by performing an early exit from the initialisation
> function when the TCMTR reports any format other than v6.
>
> Signed-off-by: Michael van der Westhuizen <michael@smart-africa.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Dave Martin <Dave.Martin@arm.com>
> ---
>
> Changes in v2:
>   - Mark the TCM undefined hook data structure as __initdata, since
>     that's what it is.
>   - Ensure that we're dealing with an ARMv6 TCMTR format.

OK...

> +#define TCMTR_FORMAT_MASK 0xe0000000U
>
>  static struct gen_pool *tcm_pool;
>  static bool dtcm_present;
> @@ -175,6 +178,21 @@ static int __init setup_tcm_bank(u8 type, u8 bank, u8 banks,
>         return 0;
>  }
>
> +static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
> +{
> +       regs->uregs[(instr >> 12) & 0xf] = 0;
> +       regs->ARM_pc += 4;
> +       return 0;
> +}

Is the action here totally obvious to everyone except me?
I guess it is masking off something and advancing past a
failed instruction but whatdoIknow. Can you put in a small comment
above the function or something?

> +static struct undef_hook tcm_hook __initdata = {
> +       .instr_mask     = 0xffff0fdf,
> +       .instr_val      = 0xee190f11,
> +       .cpsr_mask      = MODE_MASK,
> +       .cpsr_val       = SVC_MODE,
> +       .fn             = tcm_handler
> +};

Likewise here. Why not #define instruction 0xee190f11
so it is a bit more readable? I guess this instruction will
be trapped and handled by the hook but it'd be nice
to know what instruction it is and how it comes to
be issued.

> +               /* This means that the DTCM sizes were 0 or the DTCM banks
> +                * were inaccessible due to TrustZone configuration */

Nitpick:

/*
 * I prefer the comments like so: blank line after the
 * first slash-star, and sole star-slash on the last line.
 */

> +               /* This means that the ITCM sizes were 0 or the ITCM banks
> +                * were inaccessible due to TrustZone configuration */

Dito

Apart from that it looks good to me.

Yours,
Linus Walleij

[PATCH v2] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 01:16:38PM +0200, Linus Walleij wrote:
> > +static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
> > +{
> > +       regs->uregs[(instr >> 12) & 0xf] = 0;
> > +       regs->ARM_pc += 4;
> > +       return 0;
> > +}
> 
> Is the action here totally obvious to everyone except me?
> I guess it is masking off something and advancing past a
> failed instruction but whatdoIknow. Can you put in a small comment
> above the function or something?

I know what it's doing :)

It's storing '0' into the destination register of the faulted MRC
instruction, and advancing past the instruction.  So, an undef fault
on either:

	mrc	15, 0, rX, cr9, cr1, {0}
	mrc	15, 0, rX, cr9, cr1, {1}

causes them to return zero.

> > +static struct undef_hook tcm_hook __initdata = {
> > +       .instr_mask     = 0xffff0fdf,
> > +       .instr_val      = 0xee190f11,
> > +       .cpsr_mask      = MODE_MASK,
> > +       .cpsr_val       = SVC_MODE,
> > +       .fn             = tcm_handler
> > +};
> 
> Likewise here. Why not #define instruction 0xee190f11
> so it is a bit more readable? I guess this instruction will
> be trapped and handled by the hook but it'd be nice
> to know what instruction it is and how it comes to
> be issued.

We typically don't do that for instructions, otherwise we end up with
loads of problems such as how to represent the difference between these:

	ldr	rd, [rn, #4]
	ldr	rd, [rn], #4

You could use: LDR_Rd_Rn_4 but then how do you indicate the difference
between the pre-indexed and the post-indexed instruction.

In this case, it's these _two_ instructions being trapped:

	mrc	15, 0, rX, cr9, cr1, {0}
	mrc	15, 0, rX, cr9, cr1, {1}

where we don't care what register 'rX' is, and we only care about those
two, and not:

	mrc	15, 0, rX, cr9, cr1, {2}
	mrc	15, 0, rX, cr9, cr1, {3}
	...

So, if you can come up with some #define which represents just two
instructions in a class but not the others... err, nope, didn't think so.

Since the test is also:

	(actual_instruction & mask) == value

encoding the value without its mask value is pretty pointless and is
in fact ambiguous.  For example, a definition for a STR instruction
with the load bit clear in the mask matches both LDR and STR, so
while a definition for the STR instruction may seem useful, unless you
also have some way to encode the mask as well, it's pretty pointless,
and is in fact misleading if used with a mask which clears bit 20.

So it's quite reasonable to use the numeric encoding here, since
interpreting the instruction value without taking account of the mask
is pretty stupid.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

> On 02 Jun 2015, at 4:52 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> On Tue, Jun 02, 2015 at 01:16:38PM +0200, Linus Walleij wrote:
>>> +static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
>>> +{
>>> +       regs->uregs[(instr >> 12) & 0xf] = 0;
>>> +       regs->ARM_pc += 4;
>>> +       return 0;
>>> +}
>> 
>> Is the action here totally obvious to everyone except me?
>> I guess it is masking off something and advancing past a
>> failed instruction but whatdoIknow. Can you put in a small comment
>> above the function or something?
> 
> I know what it's doing :)
> 
> It's storing '0' into the destination register of the faulted MRC
> instruction, and advancing past the instruction.  So, an undef fault
> on either:
> 
> 	mrc	15, 0, rX, cr9, cr1, {0}
> 	mrc	15, 0, rX, cr9, cr1, {1}
> 
> causes them to return zero.

It?s almost depressing how quickly you grokked that!

I?ve addressed the comments from Linus is a v3 patch that crossed this mail in-flight.

Given your explanation I now fear I may have over-documented!

Michael

[PATCH v2] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 05:21:11PM +0200, Michael van der Westhuizen wrote:
> It?s almost depressing how quickly you grokked that!

Quite simple really:

$ cat > t.s
.word 0xee190f11
.word 0xee190f31
$ arm-linux-as -o t.o t.s
$ arm-linux-objdump -d t.o
...
00000000 <.text>:
   0:   ee190f11        mrc     15, 0, r0, cr9, cr1, {0}
   4:   ee190f31        mrc     15, 0, r0, cr9, cr1, {1}

Those two words being the values which fit the word and mask you
specified, omitting the usual location for Rd (which is normally bits
12:16 in ARM instructions).  Of course, a couple more words would
confirm that.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v3] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

Fixes the TCM initialisation code to handle TCM banks that are
present but inaccessible due to TrustZone configuration.  This is
the default case when enabling the non-secure world.  It may also
be the case that that the user decided to use TCM for TrustZone.

This change has exposed a bug in handling of TCM where no TCM bank
was usable (the 0 size TCM case).  This change addresses the
resulting hang.

This code only handles the ARMv6 TCMTR register format, and will not
work correctly on boards that use the ARMv7 (or any other) format.
This is handled by performing an early exit from the initialisation
function when the TCMTR reports any format other than v6.

Signed-off-by: Michael van der Westhuizen <michael@smart-africa.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Dave Martin <Dave.Martin@arm.com>
---

Changes in v3:
  - Add some documentation about the undefined handler hook and
    how the values used in that hook are derived.
  - Use symbolic values for the handler instruction and mask.
  - Use symbolic values for the shift-and-mask operation that
    extracts the destination register for the trapped operation.
  - Reformat added comments to match the style in the remainder
    of the file.

Changes in v2:
  - Mark the TCM undefined hook data structure as __initdata, since
    that's what it is.
  - Ensure that we're dealing with an ARMv6 TCMTR format.

 arch/arm/kernel/tcm.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 97 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/tcm.c b/arch/arm/kernel/tcm.c
index 7a3be1d..c0210ca 100644
--- a/arch/arm/kernel/tcm.c
+++ b/arch/arm/kernel/tcm.c
@@ -17,6 +17,9 @@
 #include <asm/mach/map.h>
 #include <asm/memory.h>
 #include <asm/system_info.h>
+#include <asm/traps.h>
+
+#define TCMTR_FORMAT_MASK	0xe0000000U
 
 static struct gen_pool *tcm_pool;
 static bool dtcm_present;
@@ -176,6 +179,73 @@ static int __init setup_tcm_bank(u8 type, u8 bank, u8 banks,
 }
 
 /*
+ * When we are running in the non-secure world and the secure world
+ * has not explicitly given us access to the TCM we will get an
+ * undefined error when reading the TCM region register in the
+ * setup_tcm_bank function (above).
+ *
+ * There are two variants of this register read that we need to trap,
+ * the read for the data TCM and the read for the instruction TCM:
+ *  c0370628:       ee196f11        mrc     15, 0, r6, cr9, cr1, {0}
+ *  c0370674:       ee196f31        mrc     15, 0, r6, cr9, cr1, {1}
+ *
+ * Our undef hook mask explicitly matches all fields of the encoded
+ * instruction other than the destination register.  The mask also
+ * only allows operand 2 to have the values 0 or 1.
+ *
+ * The undefined hook is defined as __init and __initdata, and therefore
+ * must be removed before tcm_init returns.
+ *
+ * See A8.8.107, DDI0406C_C ARM Architecture Reference Manual, Encoding
+ * T1/A1 for the bit-by-bit details.
+ *
+ *  mrc   p15, 0, XX, c9, c1, 0
+ *  mrc   p15, 0, XX, c9, c1, 1
+ *   |  |  |   |   |   |   |  +---- opc2           0|1 = 000|001
+ *   |  |  |   |   |   |   +------- CRm              0 = 0001
+ *   |  |  |   |   |   +----------- CRn              0 = 1001
+ *   |  |  |   |   +--------------- Rt               ? = ????
+ *   |  |  |   +------------------- opc1             0 =  000
+ *   |  |  +----------------------- coproc          15 = 1111
+ *   |  +-------------------------- condition   ALways = 1110
+ *   +----------------------------- instruction    MRC = 1110
+ *
+ * Encoding this as per A8.8.107 of DDI0406C, Encoding T1/A1, yields:
+ *  1111 1111 1111 1111 0000 1111 1101 1111 Required Mask
+ *  1110 1110 0001 1001 ???? 1111 0001 0001 mrc p15, 0, XX, c9, c1, 0
+ *  1110 1110 0001 1001 ???? 1111 0011 0001 mrc p15, 0, XX, c9, c1, 1
+ *  [  ] [  ] [ ]| [  ] [  ] [  ] [ ]| +--- CRm
+ *    |    |   | |   |    |    |   | +----- SBO
+ *    |    |   | |   |    |    |   +------- opc2
+ *    |    |   | |   |    |    +----------- coproc
+ *    |    |   | |   |    +---------------- Rt
+ *    |    |   | |   +--------------------- CRn
+ *    |    |   | +------------------------- SBO
+ *    |    |   +--------------------------- opc1
+ *    |    +------------------------------- instruction
+ *    +------------------------------------ condition
+ */
+#define TCM_REGION_READ_MASK		0xffff0fdf
+#define TCM_REGION_READ_INSTR		0xee190f11
+#define DEST_REG_SHIFT			12
+#define DEST_REG_MASK			0xf
+
+static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
+{
+	regs->uregs[(instr >> DEST_REG_SHIFT) & DEST_REG_MASK] = 0;
+	regs->ARM_pc += 4;
+	return 0;
+}
+
+static struct undef_hook tcm_hook __initdata = {
+	.instr_mask	= TCM_REGION_READ_MASK,
+	.instr_val	= TCM_REGION_READ_INSTR,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= SVC_MODE,
+	.fn		= tcm_handler
+};
+
+/*
  * This initializes the TCM memory
  */
 void __init tcm_init(void)
@@ -204,9 +274,18 @@ void __init tcm_init(void)
 	}
 
 	tcm_status = read_cpuid_tcmstatus();
+
+	/*
+	 * This code only supports v6-compatible TCMTR implementations.
+	 */
+	if (tcm_status & TCMTR_FORMAT_MASK)
+		return;
+
 	dtcm_banks = (tcm_status >> 16) & 0x03;
 	itcm_banks = (tcm_status & 0x03);
 
+	register_undef_hook(&tcm_hook);
+
 	/* Values greater than 2 for D/ITCM banks are "reserved" */
 	if (dtcm_banks > 2)
 		dtcm_banks = 0;
@@ -218,7 +297,7 @@ void __init tcm_init(void)
 		for (i = 0; i < dtcm_banks; i++) {
 			ret = setup_tcm_bank(0, i, dtcm_banks, &dtcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into DTCM */
 		if (dtcm_code_sz > (dtcm_end - DTCM_OFFSET)) {
@@ -227,6 +306,12 @@ void __init tcm_init(void)
 				dtcm_code_sz, (dtcm_end - DTCM_OFFSET));
 			goto no_dtcm;
 		}
+		/*
+		 * This means that the DTCM sizes were 0 or the DTCM banks
+		 * were inaccessible due to TrustZone configuration.
+		 */
+		if (!(dtcm_end - DTCM_OFFSET))
+			goto no_dtcm;
 		dtcm_res.end = dtcm_end - 1;
 		request_resource(&iomem_resource, &dtcm_res);
 		dtcm_iomap[0].length = dtcm_end - DTCM_OFFSET;
@@ -250,15 +335,21 @@ no_dtcm:
 		for (i = 0; i < itcm_banks; i++) {
 			ret = setup_tcm_bank(1, i, itcm_banks, &itcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into ITCM */
 		if (itcm_code_sz > (itcm_end - ITCM_OFFSET)) {
 			pr_info("CPU ITCM: %u bytes of code compiled to "
 				"ITCM but only %lu bytes of ITCM present\n",
 				itcm_code_sz, (itcm_end - ITCM_OFFSET));
-			return;
+			goto unregister;
 		}
+		/*
+		 * This means that the ITCM sizes were 0 or the ITCM banks
+		 * were inaccessible due to TrustZone configuration.
+		 */
+		if (!(itcm_end - ITCM_OFFSET))
+			goto unregister;
 		itcm_res.end = itcm_end - 1;
 		request_resource(&iomem_resource, &itcm_res);
 		itcm_iomap[0].length = itcm_end - ITCM_OFFSET;
@@ -275,6 +366,9 @@ no_dtcm:
 		pr_info("CPU ITCM: %u bytes of code compiled to ITCM but no "
 			"ITCM banks present in CPU\n", itcm_code_sz);
 	}
+
+unregister:
+	unregister_undef_hook(&tcm_hook);
 }
 
 /*
-- 
2.1.4

[PATCH v3] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Dave Martin]

On Tue, Jun 02, 2015 at 05:10:03PM +0200, Michael van der Westhuizen wrote:
> Fixes the TCM initialisation code to handle TCM banks that are
> present but inaccessible due to TrustZone configuration.  This is
> the default case when enabling the non-secure world.  It may also
> be the case that that the user decided to use TCM for TrustZone.
> 
> This change has exposed a bug in handling of TCM where no TCM bank
> was usable (the 0 size TCM case).  This change addresses the
> resulting hang.

[...]

> + *
> + * Encoding this as per A8.8.107 of DDI0406C, Encoding T1/A1, yields:
> + *  1111 1111 1111 1111 0000 1111 1101 1111 Required Mask
> + *  1110 1110 0001 1001 ???? 1111 0001 0001 mrc p15, 0, XX, c9, c1, 0
> + *  1110 1110 0001 1001 ???? 1111 0011 0001 mrc p15, 0, XX, c9, c1, 1
> + *  [  ] [  ] [ ]| [  ] [  ] [  ] [ ]| +--- CRm
> + *    |    |   | |   |    |    |   | +----- SBO
> + *    |    |   | |   |    |    |   +------- opc2
> + *    |    |   | |   |    |    +----------- coproc
> + *    |    |   | |   |    +---------------- Rt
> + *    |    |   | |   +--------------------- CRn
> + *    |    |   | +------------------------- SBO
> + *    |    |   +--------------------------- opc1
> + *    |    +------------------------------- instruction
> + *    +------------------------------------ condition
> + */
> +#define TCM_REGION_READ_MASK		0xffff0fdf
> +#define TCM_REGION_READ_INSTR		0xee190f11
> +#define DEST_REG_SHIFT			12
> +#define DEST_REG_MASK			0xf

What happens in a Thumb-2 kernel?

I think it is safe, since Thumb-2 kernel implies v7, and v7 mandates the
TCMTR.  Even if we did get an Undef, the code in traps.c rearranges the
bits of the offending insn into the "correct" order, and it so happens
that this also makes the encoding for MRC/MCR instructions identical
between ARM and Thumb.

For some other random instruction these assumptions may not hold, so
it is worth adding brief comment in case people blindly use this code
as a template for something else.

[...]

Cheers
---Dave

[PATCH v3] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

Hi,

> On 04 Jun 2015, at 12:40 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> 
> On Tue, Jun 02, 2015 at 05:10:03PM +0200, Michael van der Westhuizen wrote:
>> Fixes the TCM initialisation code to handle TCM banks that are
>> present but inaccessible due to TrustZone configuration.  This is
>> the default case when enabling the non-secure world.  It may also
>> be the case that that the user decided to use TCM for TrustZone.
>> 
>> This change has exposed a bug in handling of TCM where no TCM bank
>> was usable (the 0 size TCM case).  This change addresses the
>> resulting hang.
> 
> [...]
> 
>> + *
>> + * Encoding this as per A8.8.107 of DDI0406C, Encoding T1/A1, yields:
>> + *  1111 1111 1111 1111 0000 1111 1101 1111 Required Mask
>> + *  1110 1110 0001 1001 ???? 1111 0001 0001 mrc p15, 0, XX, c9, c1, 0
>> + *  1110 1110 0001 1001 ???? 1111 0011 0001 mrc p15, 0, XX, c9, c1, 1
>> + *  [  ] [  ] [ ]| [  ] [  ] [  ] [ ]| +--- CRm
>> + *    |    |   | |   |    |    |   | +----- SBO
>> + *    |    |   | |   |    |    |   +------- opc2
>> + *    |    |   | |   |    |    +----------- coproc
>> + *    |    |   | |   |    +---------------- Rt
>> + *    |    |   | |   +--------------------- CRn
>> + *    |    |   | +------------------------- SBO
>> + *    |    |   +--------------------------- opc1
>> + *    |    +------------------------------- instruction
>> + *    +------------------------------------ condition
>> + */
>> +#define TCM_REGION_READ_MASK		0xffff0fdf
>> +#define TCM_REGION_READ_INSTR		0xee190f11
>> +#define DEST_REG_SHIFT			12
>> +#define DEST_REG_MASK			0xf
> 
> What happens in a Thumb-2 kernel?
> 
> I think it is safe, since Thumb-2 kernel implies v7, and v7 mandates the
> TCMTR.  Even if we did get an Undef, the code in traps.c rearranges the
> bits of the offending insn into the "correct" order, and it so happens
> that this also makes the encoding for MRC/MCR instructions identical
> between ARM and Thumb.
> 
> For some other random instruction these assumptions may not hold, so
> it is worth adding brief comment in case people blindly use this code
> as a template for something else.

In this particular case, the T1 and A1 encodings are identical.

I?ll make that clear, and also make it clear that this is not necessarily a general case.

Michael

[PATCH v4] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Michael van der Westhuizen]

Fixes the TCM initialisation code to handle TCM banks that are
present but inaccessible due to TrustZone configuration.  This is
the default case when enabling the non-secure world.  It may also
be the case that that the user decided to use TCM for TrustZone.

This change has exposed a bug in handling of TCM where no TCM bank
was usable (the 0 size TCM case).  This change addresses the
resulting hang.

This code only handles the ARMv6 TCMTR register format, and will not
work correctly on boards that use the ARMv7 (or any other) format.
This is handled by performing an early exit from the initialisation
function when the TCMTR reports any format other than v6.

Signed-off-by: Michael van der Westhuizen <michael@smart-africa.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Dave Martin <Dave.Martin@arm.com>
---

Changes in v4:
  - Clarify that the instruction encoding for unconditional MRC
    happens to be identical between ARM and Thumb-2 modes.

Changes in v3:
  - Add some documentation about the undefined handler hook and
    how the values used in that hook are derived.
  - Use symbolic values for the handler instruction and mask.
  - Use symbolic values for the shift-and-mask operation that
    extracts the destination register for the trapped operation.
  - Reformat added comments to match the style in the remainder
    of the file.

Changes in v2:
  - Mark the TCM undefined hook data structure as __initdata, since
    that's what it is.
  - Ensure that we're dealing with an ARMv6 TCMTR format.

 arch/arm/kernel/tcm.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/tcm.c b/arch/arm/kernel/tcm.c
index 7a3be1d..b10e136 100644
--- a/arch/arm/kernel/tcm.c
+++ b/arch/arm/kernel/tcm.c
@@ -17,6 +17,9 @@
 #include <asm/mach/map.h>
 #include <asm/memory.h>
 #include <asm/system_info.h>
+#include <asm/traps.h>
+
+#define TCMTR_FORMAT_MASK	0xe0000000U
 
 static struct gen_pool *tcm_pool;
 static bool dtcm_present;
@@ -176,6 +179,77 @@ static int __init setup_tcm_bank(u8 type, u8 bank, u8 banks,
 }
 
 /*
+ * When we are running in the non-secure world and the secure world
+ * has not explicitly given us access to the TCM we will get an
+ * undefined error when reading the TCM region register in the
+ * setup_tcm_bank function (above).
+ *
+ * There are two variants of this register read that we need to trap,
+ * the read for the data TCM and the read for the instruction TCM:
+ *  c0370628:       ee196f11        mrc     15, 0, r6, cr9, cr1, {0}
+ *  c0370674:       ee196f31        mrc     15, 0, r6, cr9, cr1, {1}
+ *
+ * Our undef hook mask explicitly matches all fields of the encoded
+ * instruction other than the destination register.  The mask also
+ * only allows operand 2 to have the values 0 or 1.
+ *
+ * The undefined hook is defined as __init and __initdata, and therefore
+ * must be removed before tcm_init returns.
+ *
+ * In this particular case (MRC with ARM condition code ALways) the
+ * Thumb-2 and ARM instruction encoding are identical, so this hook
+ * will work on a Thumb-2 kernel.
+ *
+ * See A8.8.107, DDI0406C_C ARM Architecture Reference Manual, Encoding
+ * T1/A1 for the bit-by-bit details.
+ *
+ *  mrc   p15, 0, XX, c9, c1, 0
+ *  mrc   p15, 0, XX, c9, c1, 1
+ *   |  |  |   |   |   |   |  +---- opc2           0|1 = 000|001
+ *   |  |  |   |   |   |   +------- CRm              0 = 0001
+ *   |  |  |   |   |   +----------- CRn              0 = 1001
+ *   |  |  |   |   +--------------- Rt               ? = ????
+ *   |  |  |   +------------------- opc1             0 =  000
+ *   |  |  +----------------------- coproc          15 = 1111
+ *   |  +-------------------------- condition   ALways = 1110
+ *   +----------------------------- instruction    MRC = 1110
+ *
+ * Encoding this as per A8.8.107 of DDI0406C, Encoding T1/A1, yields:
+ *  1111 1111 1111 1111 0000 1111 1101 1111 Required Mask
+ *  1110 1110 0001 1001 ???? 1111 0001 0001 mrc p15, 0, XX, c9, c1, 0
+ *  1110 1110 0001 1001 ???? 1111 0011 0001 mrc p15, 0, XX, c9, c1, 1
+ *  [  ] [  ] [ ]| [  ] [  ] [  ] [ ]| +--- CRm
+ *    |    |   | |   |    |    |   | +----- SBO
+ *    |    |   | |   |    |    |   +------- opc2
+ *    |    |   | |   |    |    +----------- coproc
+ *    |    |   | |   |    +---------------- Rt
+ *    |    |   | |   +--------------------- CRn
+ *    |    |   | +------------------------- SBO
+ *    |    |   +--------------------------- opc1
+ *    |    +------------------------------- instruction
+ *    +------------------------------------ condition
+ */
+#define TCM_REGION_READ_MASK		0xffff0fdf
+#define TCM_REGION_READ_INSTR		0xee190f11
+#define DEST_REG_SHIFT			12
+#define DEST_REG_MASK			0xf
+
+static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
+{
+	regs->uregs[(instr >> DEST_REG_SHIFT) & DEST_REG_MASK] = 0;
+	regs->ARM_pc += 4;
+	return 0;
+}
+
+static struct undef_hook tcm_hook __initdata = {
+	.instr_mask	= TCM_REGION_READ_MASK,
+	.instr_val	= TCM_REGION_READ_INSTR,
+	.cpsr_mask	= MODE_MASK,
+	.cpsr_val	= SVC_MODE,
+	.fn		= tcm_handler
+};
+
+/*
  * This initializes the TCM memory
  */
 void __init tcm_init(void)
@@ -204,9 +278,18 @@ void __init tcm_init(void)
 	}
 
 	tcm_status = read_cpuid_tcmstatus();
+
+	/*
+	 * This code only supports v6-compatible TCMTR implementations.
+	 */
+	if (tcm_status & TCMTR_FORMAT_MASK)
+		return;
+
 	dtcm_banks = (tcm_status >> 16) & 0x03;
 	itcm_banks = (tcm_status & 0x03);
 
+	register_undef_hook(&tcm_hook);
+
 	/* Values greater than 2 for D/ITCM banks are "reserved" */
 	if (dtcm_banks > 2)
 		dtcm_banks = 0;
@@ -218,7 +301,7 @@ void __init tcm_init(void)
 		for (i = 0; i < dtcm_banks; i++) {
 			ret = setup_tcm_bank(0, i, dtcm_banks, &dtcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into DTCM */
 		if (dtcm_code_sz > (dtcm_end - DTCM_OFFSET)) {
@@ -227,6 +310,12 @@ void __init tcm_init(void)
 				dtcm_code_sz, (dtcm_end - DTCM_OFFSET));
 			goto no_dtcm;
 		}
+		/*
+		 * This means that the DTCM sizes were 0 or the DTCM banks
+		 * were inaccessible due to TrustZone configuration.
+		 */
+		if (!(dtcm_end - DTCM_OFFSET))
+			goto no_dtcm;
 		dtcm_res.end = dtcm_end - 1;
 		request_resource(&iomem_resource, &dtcm_res);
 		dtcm_iomap[0].length = dtcm_end - DTCM_OFFSET;
@@ -250,15 +339,21 @@ no_dtcm:
 		for (i = 0; i < itcm_banks; i++) {
 			ret = setup_tcm_bank(1, i, itcm_banks, &itcm_end);
 			if (ret)
-				return;
+				goto unregister;
 		}
 		/* This means you compiled more code than fits into ITCM */
 		if (itcm_code_sz > (itcm_end - ITCM_OFFSET)) {
 			pr_info("CPU ITCM: %u bytes of code compiled to "
 				"ITCM but only %lu bytes of ITCM present\n",
 				itcm_code_sz, (itcm_end - ITCM_OFFSET));
-			return;
+			goto unregister;
 		}
+		/*
+		 * This means that the ITCM sizes were 0 or the ITCM banks
+		 * were inaccessible due to TrustZone configuration.
+		 */
+		if (!(itcm_end - ITCM_OFFSET))
+			goto unregister;
 		itcm_res.end = itcm_end - 1;
 		request_resource(&iomem_resource, &itcm_res);
 		itcm_iomap[0].length = itcm_end - ITCM_OFFSET;
@@ -275,6 +370,9 @@ no_dtcm:
 		pr_info("CPU ITCM: %u bytes of code compiled to ITCM but no "
 			"ITCM banks present in CPU\n", itcm_code_sz);
 	}
+
+unregister:
+	unregister_undef_hook(&tcm_hook);
 }
 
 /*
-- 
2.1.4

[PATCH v4] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Linus Walleij]

On Thu, Jun 4, 2015 at 1:58 PM, Michael van der Westhuizen
<michael@smart-africa.com> wrote:

> Fixes the TCM initialisation code to handle TCM banks that are
> present but inaccessible due to TrustZone configuration.  This is
> the default case when enabling the non-secure world.  It may also
> be the case that that the user decided to use TCM for TrustZone.
>
> This change has exposed a bug in handling of TCM where no TCM bank
> was usable (the 0 size TCM case).  This change addresses the
> resulting hang.
>
> This code only handles the ARMv6 TCMTR register format, and will not
> work correctly on boards that use the ARMv7 (or any other) format.
> This is handled by performing an early exit from the initialisation
> function when the TCMTR reports any format other than v6.
>
> Signed-off-by: Michael van der Westhuizen <michael@smart-africa.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Dave Martin <Dave.Martin@arm.com>
> ---
>
> Changes in v4:
>   - Clarify that the instruction encoding for unconditional MRC
>     happens to be identical between ARM and Thumb-2 modes.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

If no further comment appear, I suggest you put this into Russell's
patch tracker.

Yours,
Linus Walleij

[PATCH v4] arm: tcm: Don't crash when TCM banks are protected by TrustZone

[Dave Martin]

On Thu, Jun 04, 2015 at 01:58:41PM +0200, Michael van der Westhuizen wrote:
> Fixes the TCM initialisation code to handle TCM banks that are
> present but inaccessible due to TrustZone configuration.  This is
> the default case when enabling the non-secure world.  It may also
> be the case that that the user decided to use TCM for TrustZone.
> 
> This change has exposed a bug in handling of TCM where no TCM bank
> was usable (the 0 size TCM case).  This change addresses the
> resulting hang.
> 
> This code only handles the ARMv6 TCMTR register format, and will not
> work correctly on boards that use the ARMv7 (or any other) format.
> This is handled by performing an early exit from the initialisation
> function when the TCMTR reports any format other than v6.
> 
> Signed-off-by: Michael van der Westhuizen <michael@smart-africa.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Dave Martin <Dave.Martin@arm.com>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
> 
> Changes in v4:
>   - Clarify that the instruction encoding for unconditional MRC
>     happens to be identical between ARM and Thumb-2 modes.
> 
> Changes in v3:
>   - Add some documentation about the undefined handler hook and
>     how the values used in that hook are derived.
>   - Use symbolic values for the handler instruction and mask.
>   - Use symbolic values for the shift-and-mask operation that
>     extracts the destination register for the trapped operation.
>   - Reformat added comments to match the style in the remainder
>     of the file.
> 
> Changes in v2:
>   - Mark the TCM undefined hook data structure as __initdata, since
>     that's what it is.
>   - Ensure that we're dealing with an ARMv6 TCMTR format.
> 
>  arch/arm/kernel/tcm.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kernel/tcm.c b/arch/arm/kernel/tcm.c
> index 7a3be1d..b10e136 100644
> --- a/arch/arm/kernel/tcm.c
> +++ b/arch/arm/kernel/tcm.c
> @@ -17,6 +17,9 @@
>  #include <asm/mach/map.h>
>  #include <asm/memory.h>
>  #include <asm/system_info.h>
> +#include <asm/traps.h>
> +
> +#define TCMTR_FORMAT_MASK	0xe0000000U
>  
>  static struct gen_pool *tcm_pool;
>  static bool dtcm_present;
> @@ -176,6 +179,77 @@ static int __init setup_tcm_bank(u8 type, u8 bank, u8 banks,
>  }
>  
>  /*
> + * When we are running in the non-secure world and the secure world
> + * has not explicitly given us access to the TCM we will get an
> + * undefined error when reading the TCM region register in the
> + * setup_tcm_bank function (above).
> + *
> + * There are two variants of this register read that we need to trap,
> + * the read for the data TCM and the read for the instruction TCM:
> + *  c0370628:       ee196f11        mrc     15, 0, r6, cr9, cr1, {0}
> + *  c0370674:       ee196f31        mrc     15, 0, r6, cr9, cr1, {1}
> + *
> + * Our undef hook mask explicitly matches all fields of the encoded
> + * instruction other than the destination register.  The mask also
> + * only allows operand 2 to have the values 0 or 1.
> + *
> + * The undefined hook is defined as __init and __initdata, and therefore
> + * must be removed before tcm_init returns.
> + *
> + * In this particular case (MRC with ARM condition code ALways) the
> + * Thumb-2 and ARM instruction encoding are identical, so this hook
> + * will work on a Thumb-2 kernel.
> + *
> + * See A8.8.107, DDI0406C_C ARM Architecture Reference Manual, Encoding
> + * T1/A1 for the bit-by-bit details.
> + *
> + *  mrc   p15, 0, XX, c9, c1, 0
> + *  mrc   p15, 0, XX, c9, c1, 1
> + *   |  |  |   |   |   |   |  +---- opc2           0|1 = 000|001
> + *   |  |  |   |   |   |   +------- CRm              0 = 0001
> + *   |  |  |   |   |   +----------- CRn              0 = 1001
> + *   |  |  |   |   +--------------- Rt               ? = ????
> + *   |  |  |   +------------------- opc1             0 =  000
> + *   |  |  +----------------------- coproc          15 = 1111
> + *   |  +-------------------------- condition   ALways = 1110
> + *   +----------------------------- instruction    MRC = 1110
> + *
> + * Encoding this as per A8.8.107 of DDI0406C, Encoding T1/A1, yields:
> + *  1111 1111 1111 1111 0000 1111 1101 1111 Required Mask
> + *  1110 1110 0001 1001 ???? 1111 0001 0001 mrc p15, 0, XX, c9, c1, 0
> + *  1110 1110 0001 1001 ???? 1111 0011 0001 mrc p15, 0, XX, c9, c1, 1
> + *  [  ] [  ] [ ]| [  ] [  ] [  ] [ ]| +--- CRm
> + *    |    |   | |   |    |    |   | +----- SBO
> + *    |    |   | |   |    |    |   +------- opc2
> + *    |    |   | |   |    |    +----------- coproc
> + *    |    |   | |   |    +---------------- Rt
> + *    |    |   | |   +--------------------- CRn
> + *    |    |   | +------------------------- SBO
> + *    |    |   +--------------------------- opc1
> + *    |    +------------------------------- instruction
> + *    +------------------------------------ condition
> + */
> +#define TCM_REGION_READ_MASK		0xffff0fdf
> +#define TCM_REGION_READ_INSTR		0xee190f11
> +#define DEST_REG_SHIFT			12
> +#define DEST_REG_MASK			0xf
> +
> +static int __init tcm_handler(struct pt_regs *regs, unsigned int instr)
> +{
> +	regs->uregs[(instr >> DEST_REG_SHIFT) & DEST_REG_MASK] = 0;
> +	regs->ARM_pc += 4;
> +	return 0;
> +}
> +
> +static struct undef_hook tcm_hook __initdata = {
> +	.instr_mask	= TCM_REGION_READ_MASK,
> +	.instr_val	= TCM_REGION_READ_INSTR,
> +	.cpsr_mask	= MODE_MASK,
> +	.cpsr_val	= SVC_MODE,
> +	.fn		= tcm_handler
> +};
> +
> +/*
>   * This initializes the TCM memory
>   */
>  void __init tcm_init(void)
> @@ -204,9 +278,18 @@ void __init tcm_init(void)
>  	}
>  
>  	tcm_status = read_cpuid_tcmstatus();
> +
> +	/*
> +	 * This code only supports v6-compatible TCMTR implementations.
> +	 */
> +	if (tcm_status & TCMTR_FORMAT_MASK)
> +		return;
> +
>  	dtcm_banks = (tcm_status >> 16) & 0x03;
>  	itcm_banks = (tcm_status & 0x03);
>  
> +	register_undef_hook(&tcm_hook);
> +
>  	/* Values greater than 2 for D/ITCM banks are "reserved" */
>  	if (dtcm_banks > 2)
>  		dtcm_banks = 0;
> @@ -218,7 +301,7 @@ void __init tcm_init(void)
>  		for (i = 0; i < dtcm_banks; i++) {
>  			ret = setup_tcm_bank(0, i, dtcm_banks, &dtcm_end);
>  			if (ret)
> -				return;
> +				goto unregister;
>  		}
>  		/* This means you compiled more code than fits into DTCM */
>  		if (dtcm_code_sz > (dtcm_end - DTCM_OFFSET)) {
> @@ -227,6 +310,12 @@ void __init tcm_init(void)
>  				dtcm_code_sz, (dtcm_end - DTCM_OFFSET));
>  			goto no_dtcm;
>  		}
> +		/*
> +		 * This means that the DTCM sizes were 0 or the DTCM banks
> +		 * were inaccessible due to TrustZone configuration.
> +		 */
> +		if (!(dtcm_end - DTCM_OFFSET))
> +			goto no_dtcm;
>  		dtcm_res.end = dtcm_end - 1;
>  		request_resource(&iomem_resource, &dtcm_res);
>  		dtcm_iomap[0].length = dtcm_end - DTCM_OFFSET;
> @@ -250,15 +339,21 @@ no_dtcm:
>  		for (i = 0; i < itcm_banks; i++) {
>  			ret = setup_tcm_bank(1, i, itcm_banks, &itcm_end);
>  			if (ret)
> -				return;
> +				goto unregister;
>  		}
>  		/* This means you compiled more code than fits into ITCM */
>  		if (itcm_code_sz > (itcm_end - ITCM_OFFSET)) {
>  			pr_info("CPU ITCM: %u bytes of code compiled to "
>  				"ITCM but only %lu bytes of ITCM present\n",
>  				itcm_code_sz, (itcm_end - ITCM_OFFSET));
> -			return;
> +			goto unregister;
>  		}
> +		/*
> +		 * This means that the ITCM sizes were 0 or the ITCM banks
> +		 * were inaccessible due to TrustZone configuration.
> +		 */
> +		if (!(itcm_end - ITCM_OFFSET))
> +			goto unregister;
>  		itcm_res.end = itcm_end - 1;
>  		request_resource(&iomem_resource, &itcm_res);
>  		itcm_iomap[0].length = itcm_end - ITCM_OFFSET;
> @@ -275,6 +370,9 @@ no_dtcm:
>  		pr_info("CPU ITCM: %u bytes of code compiled to ITCM but no "
>  			"ITCM banks present in CPU\n", itcm_code_sz);
>  	}
> +
> +unregister:
> +	unregister_undef_hook(&tcm_hook);
>  }
>  
>  /*
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel