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? > >> + // 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
