| Message ID | 1456841675-7928-1-git-send-email-evan.lloyd@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Evan, On Tue, Mar 01, 2016 at 02:14:35PM +0000, evan.lloyd@arm.com wrote: > From: Evan Lloyd <evan.lloyd@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.Lloyd@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. 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". > + // 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."? > + // > + 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
On 1 March 2016 at 16:11, Leif Lindholm <leif.lindholm@linaro.org> wrote: > Hi Evan, > > On Tue, Mar 01, 2016 at 02:14:35PM +0000, evan.lloyd@arm.com wrote: >> From: Evan Lloyd <evan.lloyd@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.Lloyd@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
On Wed, Mar 02, 2016 at 10:12:37AM +0100, Ard Biesheuvel wrote: > On 1 March 2016 at 16:11, Leif Lindholm <leif.lindholm@linaro.org> wrote: > > Hi Evan, > > > > On Tue, Mar 01, 2016 at 02:14:35PM +0000, evan.lloyd@arm.com wrote: > >> From: Evan Lloyd <evan.lloyd@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.Lloyd@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. 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 2 March 2016 at 11:27, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Wed, Mar 02, 2016 at 10:12:37AM +0100, Ard Biesheuvel wrote: >> On 1 March 2016 at 16:11, Leif Lindholm <leif.lindholm@linaro.org> wrote: >> > Hi Evan, >> > >> > On Tue, Mar 01, 2016 at 02:14:35PM +0000, evan.lloyd@arm.com wrote: >> >> From: Evan Lloyd <evan.lloyd@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.Lloyd@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. > > 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. + // Clearing PD0 and PD1: Prevents translation faults from non-secure page + // lookups. + // Clearing N: 0 is the default reset value, and is again compatible with + // current page table set up. + // + 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)