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