| Message ID | 1432821287-31436-1-git-send-email-michael@smart-africa.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
> 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
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.
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); } /*
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(-)