On 1 March 2016 at 16:11, Leif Lindholm <leif.lindh...@linaro.org> wrote: > Hi Evan, > > On Tue, Mar 01, 2016 at 02:14:35PM +0000, evan.ll...@arm.com wrote: >> From: Evan Lloyd <evan.ll...@arm.com> >> >> Architecturally, the TTBCR register value is undefined at reset for >> Non-Secure. >> On some platforms the reset value for TTBCR is not zero and >> this causes a data abort exception once the MMU is enabled. >> >> This patch configures the TTBCR register to enable translation table >> walk using TTBR0. >> >> Contributed-under: Tianocore Contribution Agreement 1.0 > > Upper-case C in TianoCore: > Contributed-under: TianoCore Contribution Agreement 1.0 > > I can fix that one on commit, but scanning through history I notice > your previous patch did the same. > > BaseTools/Scripts/PatchCheck.py will pick this up for you. > >> Signed-off-by: Evan Lloyd <evan.ll...@arm.com> >> --- >> ArmPkg/Include/Library/ArmLib.h | 8 +++++++- >> ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c | 14 +++++++++++++- >> ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.S | 8 +++++++- >> ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.asm | 7 ++++++- >> 4 files changed, 33 insertions(+), 4 deletions(-) >> >> diff --git a/ArmPkg/Include/Library/ArmLib.h >> b/ArmPkg/Include/Library/ArmLib.h >> index 85fa1f6..15f610d 100644 >> --- a/ArmPkg/Include/Library/ArmLib.h >> +++ b/ArmPkg/Include/Library/ArmLib.h >> @@ -1,7 +1,7 @@ >> /** @file >> >> Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> >> - Copyright (c) 2011 - 2015, ARM Ltd. All rights reserved.<BR> >> + Copyright (c) 2011 - 2016, ARM Ltd. All rights reserved.<BR> >> >> This program and the accompanying materials >> are licensed and made available under the terms and conditions of the BSD >> License >> @@ -353,6 +353,12 @@ ArmSetTTBR0 ( >> IN VOID *TranslationTableBase >> ); >> >> +VOID >> +EFIAPI >> +ArmSetTTBCR ( >> + IN UINT32 Bits >> + ); >> + >> VOID * >> EFIAPI >> ArmGetTTBR0BaseAddress ( >> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c >> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c >> index fc8ea42..8cc33e3 100644 >> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c >> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Mmu.c >> @@ -1,7 +1,7 @@ >> /** @file >> * File managing the MMU for ARMv7 architecture >> * >> -* Copyright (c) 2011-2013, ARM Limited. All rights reserved. >> +* Copyright (c) 2011-2016, ARM Limited. All rights reserved. >> * >> * This program and the accompanying materials >> * are licensed and made available under the terms and conditions of the >> BSD License >> @@ -347,6 +347,18 @@ ArmConfigureMmu ( >> >> ArmSetTTBR0 ((VOID *)(UINTN)(((UINTN)TranslationTable & >> ~TRANSLATION_TABLE_SECTION_ALIGNMENT_MASK) | (TTBRAttributes & 0x7F))); >> >> + // >> + // The TTBCR register value is undefined at reset in the Non-Secure world. >> + // Writing 0 has the effect of: >> + // Clearing EAE: Important because current page table set up code does >> not >> + // support Extended Addressing, so only uses short >> descriptors. > > ... but it won't pick up on that line being >80 characters (because > that isn't (yet) a hard TianoCore rule). I'll fix that one up as well, > for my own future comfort. > > As for the functionality, yes - the translation table code for AArch32 > will always have to be short descriptors, since the specification > mandates that virtual address equals physical address.
I don't follow. The spec mandates that virtual address equals physical address, but that by itself could easily be implemented using long descriptors. The only reason we cannot support long descriptors is because they imply TRE=1, while the spec mandates TRE=0. > It is the specification that matters more than the code, so how about > "Use short descriptors, as mandated by specification."? > >> + // Clearing PD0 and PD1: Prevents translation faults from non-secure >> page >> + // lookups. > > I would prefer to put this as "Translation Table Walk Disable is off". > Ack > >> + // Clearing N: 0 is the default reset value, and is again compatible >> with >> + // current page table set up. > > As the specification says "TTBR0 must be used solely", how about > "Perform all translation table walks through TTBR0."? > Ack >> + // >> + ArmSetTTBCR (0); >> + >> ArmSetDomainAccessControl (DOMAIN_ACCESS_CONTROL_NONE(15) | >> DOMAIN_ACCESS_CONTROL_NONE(14) | >> DOMAIN_ACCESS_CONTROL_NONE(13) | >> diff --git a/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.S >> b/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.S >> index 085f08b..5d1194e 100644 >> --- a/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.S >> +++ b/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.S >> @@ -1,7 +1,7 @@ >> >> #------------------------------------------------------------------------------ >> # >> # Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> >> -# Copyright (c) 2011 - 2014, ARM Limited. All rights reserved. >> +# Copyright (c) 2011 - 2016, ARM Limited. All rights reserved. >> # >> # This program and the accompanying materials >> # are licensed and made available under the terms and conditions of the BSD >> License >> @@ -23,6 +23,7 @@ GCC_ASM_EXPORT(ArmGetInterruptState) >> GCC_ASM_EXPORT(ArmGetFiqState) >> GCC_ASM_EXPORT(ArmGetTTBR0BaseAddress) >> GCC_ASM_EXPORT(ArmSetTTBR0) >> +GCC_ASM_EXPORT(ArmSetTTBCR) >> GCC_ASM_EXPORT(ArmSetDomainAccessControl) >> GCC_ASM_EXPORT(CPSRMaskInsert) >> GCC_ASM_EXPORT(CPSRRead) >> @@ -111,6 +112,11 @@ ASM_PFX(ArmSetTTBR0): >> isb >> bx lr >> >> +ASM_PFX(ArmSetTTBCR): >> + mcr p15, 0, r0, c2, c0, 2 >> + isb >> + bx lr >> + >> ASM_PFX(ArmGetTTBR0BaseAddress): >> mrc p15,0,r0,c2,c0,0 >> LoadConstantToReg(0xFFFFC000, r1) >> diff --git a/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.asm >> b/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.asm >> index 228d7c8..9b87b7f 100644 >> --- a/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.asm >> +++ b/ArmPkg/Library/ArmLib/Common/Arm/ArmLibSupport.asm >> @@ -1,7 +1,7 @@ >> >> //------------------------------------------------------------------------------ >> // >> // Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> >> -// Copyright (c) 2011 - 2014, ARM Limited. All rights reserved. >> +// Copyright (c) 2011 - 2016, ARM Limited. All rights reserved. >> // >> // This program and the accompanying materials >> // are licensed and made available under the terms and conditions of the >> BSD License >> @@ -85,6 +85,11 @@ >> isb >> bx lr >> >> + RVCT_ASM_EXPORT ArmSetTTBCR >> + mcr p15, 0, r0, c2, c0, 2 >> + isb >> + bx lr >> + >> RVCT_ASM_EXPORT ArmGetTTBR0BaseAddress >> mrc p15,0,r0,c2,c0,0 >> LoadConstantToReg(0xFFFFC000, r1) >> -- >> 2.7.0 > > No comments on code, and it's a clear bugfix, but I would like some > feedback on my suggestions for changes in the comments before pushing. > > Regards, > > Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
