On 2 March 2016 at 11:27, Leif Lindholm <[email protected]> wrote: > On Wed, Mar 02, 2016 at 10:12:37AM +0100, Ard Biesheuvel wrote: >> On 1 March 2016 at 16:11, Leif Lindholm <[email protected]> wrote: >> > Hi Evan, >> > >> > On Tue, Mar 01, 2016 at 02:14:35PM +0000, [email protected] wrote: >> >> From: Evan Lloyd <[email protected]> >> >> >> >> 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 <[email protected]> >> >> --- >> >> 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. > > OK, I guess if we're not talking RAM, and have plenty of virtual space > left for i/o. We really just update the spec to mention EAE > explicitly. > >> > It is the specification that matters more than the code, so how about >> > "Use short descriptors, as mandated by specification."? > > Any issue with the actual suggested comment? >
Nope >> >> + // 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
