| Message ID | 1465831567-10819-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 469e1e1e4203b5d369fdce790883cb0aa035a744 |
| Headers | show |
On Mon, Jun 13, 2016 at 05:26:07PM +0200, Ard Biesheuvel wrote: > On some platforms, performing cache maintenance on regions that are backed > by NOR flash result in SErrors. Since cache maintenance is unnecessary in > that case, create a PEIM specific version that only performs said cache > maintenance in its constructor if the module is shadowed in RAM. To avoid > performing the cache maintenance if the MMU code is not used to begin with, > check that explicitly in the constructor. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > > As discussed in the thread dedicated to this subject, the preferred way of > addressing this to split off the MMU manipulation code from ArmLib into a > separate ArmMmuLib, but this affects other packages and platforms. So in the > mean time, let's merge this patch so that D02 can use the upstream ArmLib > unmodified. I'm missing something here (and couldn't figure out which thread covered this earlier), and this feels pretty suspect. Why does cache maintenance to these regions result in SError in the first place? Is the SRAM not accepting some writeback or fetch that occurs as a result? How are we protected against natural evictions and/or fetches? Does the SRAM respond badly to a CMO itself for some reason? Thanks, Mark. > ArmPkg/Library/ArmLib/AArch64/{AArch64LibConstructor.c => AArch64BaseLibConstructor.c} | 0 > ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 2 +- > ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf | 43 +++++++++++ > ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 2 + > ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c | 75 ++++++++++++++++++++ > 5 files changed, 121 insertions(+), 1 deletion(-) > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c > similarity index 100% > rename from ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c > rename to ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > index 58684e8492f2..ef9d261b910d 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > @@ -32,7 +32,7 @@ [Sources.AARCH64] > > ../Common/AArch64/ArmLibSupport.S > ../Common/ArmLib.c > - AArch64LibConstructor.c > + AArch64BaseLibConstructor.c > > [Packages] > ArmPkg/ArmPkg.dec > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf > new file mode 100644 > index 000000000000..c8f0b97750d4 > --- /dev/null > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf > @@ -0,0 +1,43 @@ > +#/** @file > +# > +# Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR> > +# Portions copyright (c) 2011 - 2014, 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 > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = AArch64Lib > + FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138 > + MODULE_TYPE = PEIM > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = ArmLib|PEIM PEI_CORE > + CONSTRUCTOR = AArch64LibConstructor > + > +[Sources.AARCH64] > + AArch64Lib.c > + AArch64Mmu.c > + AArch64ArchTimer.c > + ArmLibSupportV8.S > + AArch64Support.S > + AArch64ArchTimerSupport.S > + > + ../Common/AArch64/ArmLibSupport.S > + ../Common/ArmLib.c > + AArch64PeiLibConstructor.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + MemoryAllocationLib > + CacheMaintenanceLib > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > index cf9b7222b47b..07864bac28e6 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > @@ -26,6 +26,8 @@ > // We use this index definition to define an invalid block entry > #define TT_ATTR_INDX_INVALID ((UINT32)~0) > > +INT32 HaveMmuRoutines = 1; > + > STATIC > UINT64 > ArmMemoryAttributeToPageAttribute ( > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c > new file mode 100644 > index 000000000000..2de9c7c54ed9 > --- /dev/null > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c > @@ -0,0 +1,75 @@ > +#/* @file > +# > +# Copyright (c) 2016, Linaro Limited. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +#*/ > + > +#include <Base.h> > + > +#include <Library/ArmLib.h> > +#include <Library/CacheMaintenanceLib.h> > +#include <Library/DebugLib.h> > + > +// > +// This is a hack. We define a weak symbol with external linkage, which may or > +// may not be overridden by a non-weak alternative that is defined with a non > +// zero value in the object that contains the MMU routines. Since static > +// libraries are pulled in on a per-object basis, and since the MMU object will > +// only be pulled in if any of its other symbols are referenced by the client > +// module, we can use the value below to figure out whether the MMU routines are > +// in use by this module, and decide whether cache maintenance of the function > +// ArmReplaceLiveTranslationEntry () is required. > +// > +INT32 __attribute__((weak)) HaveMmuRoutines; > + > +EFI_STATUS > +EFIAPI > +AArch64LibConstructor ( > + IN EFI_PEI_FILE_HANDLE FileHandle, > + IN CONST EFI_PEI_SERVICES **PeiServices > + ) > +{ > + extern UINT32 ArmReplaceLiveTranslationEntrySize; > + > + EFI_FV_FILE_INFO FileInfo; > + EFI_STATUS Status; > + > + if (HaveMmuRoutines == 0) { > + return RETURN_SUCCESS; > + } > + > + ASSERT (FileHandle != NULL); > + > + Status = (*PeiServices)->FfsGetFileInfo (FileHandle, &FileInfo); > + ASSERT_EFI_ERROR (Status); > + > + // > + // Some platforms do not cope very well with cache maintenance being > + // performed on regions backed by NOR flash. Since the cache maintenance > + // is unnecessary to begin with in that case, perform it only when not > + // executing in place. > + // > + if ((UINTN)FileInfo.Buffer <= (UINTN)ArmReplaceLiveTranslationEntry && > + ((UINTN)FileInfo.Buffer + FileInfo.BufferSize >= > + (UINTN)ArmReplaceLiveTranslationEntry + ArmReplaceLiveTranslationEntrySize)) { > + DEBUG ((EFI_D_INFO, "ArmLib: skipping cache maintence on XIP PEIM\n")); > + } else { > + DEBUG ((EFI_D_INFO, "ArmLib: performing cache maintence on shadowed PEIM\n")); > + // > + // The ArmReplaceLiveTranslationEntry () helper function may be invoked > + // with the MMU off so we have to ensure that it gets cleaned to the PoC > + // > + WriteBackDataCacheRange (ArmReplaceLiveTranslationEntry, > + ArmReplaceLiveTranslationEntrySize); > + } > + > + return RETURN_SUCCESS; > +} > -- > 1.9.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 13 June 2016 at 17:45, Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Jun 13, 2016 at 05:26:07PM +0200, Ard Biesheuvel wrote: >> On some platforms, performing cache maintenance on regions that are backed >> by NOR flash result in SErrors. Since cache maintenance is unnecessary in >> that case, create a PEIM specific version that only performs said cache >> maintenance in its constructor if the module is shadowed in RAM. To avoid >> performing the cache maintenance if the MMU code is not used to begin with, >> check that explicitly in the constructor. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> >> As discussed in the thread dedicated to this subject, the preferred way of >> addressing this to split off the MMU manipulation code from ArmLib into a >> separate ArmMmuLib, but this affects other packages and platforms. So in the >> mean time, let's merge this patch so that D02 can use the upstream ArmLib >> unmodified. > > I'm missing something here (and couldn't figure out which thread covered > this earlier), and this feels pretty suspect. > > Why does cache maintenance to these regions result in SError in the > first place? > > Is the SRAM not accepting some writeback or fetch that occurs as a > result? How are we protected against natural evictions and/or fetches? > > Does the SRAM respond badly to a CMO itself for some reason? > It's NOR flash, not SRAM, and it is basically a quirk of the D02 non-DRAM memory bus implementation. I will let Heyi respond with more details, but this patch is basically a stop gap solution until we split off the MMU code from ArmLib, which will allow us to deal with this more elegantly (Currently, the only PEI phase module that manipulates the page tables with the MMU on, and is likely to split entries is DxeIpl, which maps the DXE phase stack non-executable. The only other user is the module that creates the page tables in the first place, and so does not require the extra caution when splitting block entries) _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Mon, Jun 13, 2016 at 05:51:23PM +0200, Ard Biesheuvel wrote: > On 13 June 2016 at 17:45, Mark Rutland <mark.rutland@arm.com> wrote: > > On Mon, Jun 13, 2016 at 05:26:07PM +0200, Ard Biesheuvel wrote: > >> On some platforms, performing cache maintenance on regions that are backed > >> by NOR flash result in SErrors. Since cache maintenance is unnecessary in > >> that case, create a PEIM specific version that only performs said cache > >> maintenance in its constructor if the module is shadowed in RAM. To avoid > >> performing the cache maintenance if the MMU code is not used to begin with, > >> check that explicitly in the constructor. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> --- > >> > >> As discussed in the thread dedicated to this subject, the preferred way of > >> addressing this to split off the MMU manipulation code from ArmLib into a > >> separate ArmMmuLib, but this affects other packages and platforms. So in the > >> mean time, let's merge this patch so that D02 can use the upstream ArmLib > >> unmodified. > > > > I'm missing something here (and couldn't figure out which thread covered > > this earlier), and this feels pretty suspect. > > > > Why does cache maintenance to these regions result in SError in the > > first place? > > > > Is the SRAM not accepting some writeback or fetch that occurs as a > > result? How are we protected against natural evictions and/or fetches? > > > > Does the SRAM respond badly to a CMO itself for some reason? > > > > It's NOR flash, not SRAM, and it is basically a quirk of the D02 > non-DRAM memory bus implementation. Ok. I take it that means the CMO itself is the issue (i.e. it gets forwarded to the memory controller endpoint, which barfs), rather than asynchronous writebacks or fetches being a potential issue. If Heyi can confirm that, then this looks fine to me. For the sake of future reference, it would be nice to note the specific issue in the commit message. > I will let Heyi respond with more details, but this patch is basically > a stop gap solution until we split off the MMU code from ArmLib, which > will allow us to deal with this more elegantly (Currently, the only > PEI phase module that manipulates the page tables with the MMU on, and > is likely to split entries is DxeIpl, which maps the DXE phase stack > non-executable. The only other user is the module that creates the > page tables in the first place, and so does not require the extra > caution when splitting block entries) Understood! My only concern was whether this was masking an issue with asynchronous behaviour. Thanks, Mark. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi Mark, The reason of SERR by flushing flash address is as below: 1. Flash controller and other devices except DRAM are all behind bus agents called DISP; 2. The DISPs only accept certain types of request, not including CMO; 3. When the DISPs receive requests of unsupported types, like CMO, they will response an error; 4. To processor, the error response triggers a system error exception. From SOC design perspective, the error response is only a warning, because it is not expected to have device regions configured as cacheable. On D03 and future platforms, we can disable such warning so that there will be no SERR for this operation. But on D02 we cannot disable it and need software to avoid such operation. Thanks. Heyi On 06/14/2016 12:48 AM, Mark Rutland wrote: > On Mon, Jun 13, 2016 at 05:51:23PM +0200, Ard Biesheuvel wrote: >> On 13 June 2016 at 17:45, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Mon, Jun 13, 2016 at 05:26:07PM +0200, Ard Biesheuvel wrote: >>>> On some platforms, performing cache maintenance on regions that are backed >>>> by NOR flash result in SErrors. Since cache maintenance is unnecessary in >>>> that case, create a PEIM specific version that only performs said cache >>>> maintenance in its constructor if the module is shadowed in RAM. To avoid >>>> performing the cache maintenance if the MMU code is not used to begin with, >>>> check that explicitly in the constructor. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> --- >>>> >>>> As discussed in the thread dedicated to this subject, the preferred way of >>>> addressing this to split off the MMU manipulation code from ArmLib into a >>>> separate ArmMmuLib, but this affects other packages and platforms. So in the >>>> mean time, let's merge this patch so that D02 can use the upstream ArmLib >>>> unmodified. >>> I'm missing something here (and couldn't figure out which thread covered >>> this earlier), and this feels pretty suspect. >>> >>> Why does cache maintenance to these regions result in SError in the >>> first place? >>> >>> Is the SRAM not accepting some writeback or fetch that occurs as a >>> result? How are we protected against natural evictions and/or fetches? >>> >>> Does the SRAM respond badly to a CMO itself for some reason? >>> >> It's NOR flash, not SRAM, and it is basically a quirk of the D02 >> non-DRAM memory bus implementation. > Ok. I take it that means the CMO itself is the issue (i.e. it gets > forwarded to the memory controller endpoint, which barfs), rather than > asynchronous writebacks or fetches being a potential issue. > > If Heyi can confirm that, then this looks fine to me. For the sake of > future reference, it would be nice to note the specific issue in the > commit message. > >> I will let Heyi respond with more details, but this patch is basically >> a stop gap solution until we split off the MMU code from ArmLib, which >> will allow us to deal with this more elegantly (Currently, the only >> PEI phase module that manipulates the page tables with the MMU on, and >> is likely to split entries is DxeIpl, which maps the DXE phase stack >> non-executable. The only other user is the module that creates the >> page tables in the first place, and so does not require the extra >> caution when splitting block entries) > Understood! > > My only concern was whether this was masking an issue with asynchronous > behaviour. > > Thanks, > Mark. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Tested-by: Heyi Guo <heyi.guo@linaro.org> On 06/13/2016 11:26 PM, Ard Biesheuvel wrote: > On some platforms, performing cache maintenance on regions that are backed > by NOR flash result in SErrors. Since cache maintenance is unnecessary in > that case, create a PEIM specific version that only performs said cache > maintenance in its constructor if the module is shadowed in RAM. To avoid > performing the cache maintenance if the MMU code is not used to begin with, > check that explicitly in the constructor. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > > As discussed in the thread dedicated to this subject, the preferred way of > addressing this to split off the MMU manipulation code from ArmLib into a > separate ArmMmuLib, but this affects other packages and platforms. So in the > mean time, let's merge this patch so that D02 can use the upstream ArmLib > unmodified. > > > ArmPkg/Library/ArmLib/AArch64/{AArch64LibConstructor.c => AArch64BaseLibConstructor.c} | 0 > ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 2 +- > ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf | 43 +++++++++++ > ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 2 + > ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c | 75 ++++++++++++++++++++ > 5 files changed, 121 insertions(+), 1 deletion(-) > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c > similarity index 100% > rename from ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c > rename to ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > index 58684e8492f2..ef9d261b910d 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > @@ -32,7 +32,7 @@ [Sources.AARCH64] > > ../Common/AArch64/ArmLibSupport.S > ../Common/ArmLib.c > - AArch64LibConstructor.c > + AArch64BaseLibConstructor.c > > [Packages] > ArmPkg/ArmPkg.dec > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf > new file mode 100644 > index 000000000000..c8f0b97750d4 > --- /dev/null > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf > @@ -0,0 +1,43 @@ > +#/** @file > +# > +# Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR> > +# Portions copyright (c) 2011 - 2014, 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 > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = AArch64Lib > + FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138 > + MODULE_TYPE = PEIM > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = ArmLib|PEIM PEI_CORE > + CONSTRUCTOR = AArch64LibConstructor > + > +[Sources.AARCH64] > + AArch64Lib.c > + AArch64Mmu.c > + AArch64ArchTimer.c > + ArmLibSupportV8.S > + AArch64Support.S > + AArch64ArchTimerSupport.S > + > + ../Common/AArch64/ArmLibSupport.S > + ../Common/ArmLib.c > + AArch64PeiLibConstructor.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + MemoryAllocationLib > + CacheMaintenanceLib > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > index cf9b7222b47b..07864bac28e6 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > @@ -26,6 +26,8 @@ > // We use this index definition to define an invalid block entry > #define TT_ATTR_INDX_INVALID ((UINT32)~0) > > +INT32 HaveMmuRoutines = 1; > + > STATIC > UINT64 > ArmMemoryAttributeToPageAttribute ( > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c > new file mode 100644 > index 000000000000..2de9c7c54ed9 > --- /dev/null > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c > @@ -0,0 +1,75 @@ > +#/* @file > +# > +# Copyright (c) 2016, Linaro Limited. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +#*/ > + > +#include <Base.h> > + > +#include <Library/ArmLib.h> > +#include <Library/CacheMaintenanceLib.h> > +#include <Library/DebugLib.h> > + > +// > +// This is a hack. We define a weak symbol with external linkage, which may or > +// may not be overridden by a non-weak alternative that is defined with a non > +// zero value in the object that contains the MMU routines. Since static > +// libraries are pulled in on a per-object basis, and since the MMU object will > +// only be pulled in if any of its other symbols are referenced by the client > +// module, we can use the value below to figure out whether the MMU routines are > +// in use by this module, and decide whether cache maintenance of the function > +// ArmReplaceLiveTranslationEntry () is required. > +// > +INT32 __attribute__((weak)) HaveMmuRoutines; > + > +EFI_STATUS > +EFIAPI > +AArch64LibConstructor ( > + IN EFI_PEI_FILE_HANDLE FileHandle, > + IN CONST EFI_PEI_SERVICES **PeiServices > + ) > +{ > + extern UINT32 ArmReplaceLiveTranslationEntrySize; > + > + EFI_FV_FILE_INFO FileInfo; > + EFI_STATUS Status; > + > + if (HaveMmuRoutines == 0) { > + return RETURN_SUCCESS; > + } > + > + ASSERT (FileHandle != NULL); > + > + Status = (*PeiServices)->FfsGetFileInfo (FileHandle, &FileInfo); > + ASSERT_EFI_ERROR (Status); > + > + // > + // Some platforms do not cope very well with cache maintenance being > + // performed on regions backed by NOR flash. Since the cache maintenance > + // is unnecessary to begin with in that case, perform it only when not > + // executing in place. > + // > + if ((UINTN)FileInfo.Buffer <= (UINTN)ArmReplaceLiveTranslationEntry && > + ((UINTN)FileInfo.Buffer + FileInfo.BufferSize >= > + (UINTN)ArmReplaceLiveTranslationEntry + ArmReplaceLiveTranslationEntrySize)) { > + DEBUG ((EFI_D_INFO, "ArmLib: skipping cache maintence on XIP PEIM\n")); > + } else { > + DEBUG ((EFI_D_INFO, "ArmLib: performing cache maintence on shadowed PEIM\n")); > + // > + // The ArmReplaceLiveTranslationEntry () helper function may be invoked > + // with the MMU off so we have to ensure that it gets cleaned to the PoC > + // > + WriteBackDataCacheRange (ArmReplaceLiveTranslationEntry, > + ArmReplaceLiveTranslationEntrySize); > + } > + > + return RETURN_SUCCESS; > +} _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 13 June 2016 at 16:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On some platforms, performing cache maintenance on regions that are backed > by NOR flash result in SErrors. Since cache maintenance is unnecessary in > that case, create a PEIM specific version that only performs said cache > maintenance in its constructor if the module is shadowed in RAM. To avoid > performing the cache maintenance if the MMU code is not used to begin with, > check that explicitly in the constructor. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > > As discussed in the thread dedicated to this subject, the preferred way of > addressing this to split off the MMU manipulation code from ArmLib into a > separate ArmMmuLib, but this affects other packages and platforms. So in the > mean time, let's merge this patch so that D02 can use the upstream ArmLib > unmodified. > > > ArmPkg/Library/ArmLib/AArch64/{AArch64LibConstructor.c => AArch64BaseLibConstructor.c} | 0 > ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 2 +- > ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf | 43 +++++++++++ > ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 2 + > ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c | 75 ++++++++++++++++++++ > 5 files changed, 121 insertions(+), 1 deletion(-) > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c > similarity index 100% > rename from ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c > rename to ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > index 58684e8492f2..ef9d261b910d 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf > @@ -32,7 +32,7 @@ [Sources.AARCH64] > > ../Common/AArch64/ArmLibSupport.S > ../Common/ArmLib.c > - AArch64LibConstructor.c > + AArch64BaseLibConstructor.c > > [Packages] > ArmPkg/ArmPkg.dec > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf > new file mode 100644 > index 000000000000..c8f0b97750d4 > --- /dev/null > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf > @@ -0,0 +1,43 @@ > +#/** @file > +# > +# Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR> > +# Portions copyright (c) 2011 - 2014, 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 > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = AArch64Lib > + FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138 > + MODULE_TYPE = PEIM > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = ArmLib|PEIM PEI_CORE > + CONSTRUCTOR = AArch64LibConstructor > + > +[Sources.AARCH64] > + AArch64Lib.c > + AArch64Mmu.c > + AArch64ArchTimer.c > + ArmLibSupportV8.S > + AArch64Support.S > + AArch64ArchTimerSupport.S > + > + ../Common/AArch64/ArmLibSupport.S > + ../Common/ArmLib.c > + AArch64PeiLibConstructor.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + MemoryAllocationLib > + CacheMaintenanceLib > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > index cf9b7222b47b..07864bac28e6 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > @@ -26,6 +26,8 @@ > // We use this index definition to define an invalid block entry > #define TT_ATTR_INDX_INVALID ((UINT32)~0) > > +INT32 HaveMmuRoutines = 1; > + > STATIC > UINT64 > ArmMemoryAttributeToPageAttribute ( > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c > new file mode 100644 > index 000000000000..2de9c7c54ed9 > --- /dev/null > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c > @@ -0,0 +1,75 @@ > +#/* @file > +# > +# Copyright (c) 2016, Linaro Limited. All rights reserved. > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +#*/ > + > +#include <Base.h> > + > +#include <Library/ArmLib.h> > +#include <Library/CacheMaintenanceLib.h> > +#include <Library/DebugLib.h> > + > +// > +// This is a hack. We define a weak symbol with external linkage, which may or > +// may not be overridden by a non-weak alternative that is defined with a non > +// zero value in the object that contains the MMU routines. Since static > +// libraries are pulled in on a per-object basis, and since the MMU object will > +// only be pulled in if any of its other symbols are referenced by the client > +// module, we can use the value below to figure out whether the MMU routines are > +// in use by this module, and decide whether cache maintenance of the function > +// ArmReplaceLiveTranslationEntry () is required. > +// > +INT32 __attribute__((weak)) HaveMmuRoutines; > + > +EFI_STATUS > +EFIAPI > +AArch64LibConstructor ( > + IN EFI_PEI_FILE_HANDLE FileHandle, > + IN CONST EFI_PEI_SERVICES **PeiServices > + ) > +{ > + extern UINT32 ArmReplaceLiveTranslationEntrySize; > + > + EFI_FV_FILE_INFO FileInfo; > + EFI_STATUS Status; > + > + if (HaveMmuRoutines == 0) { > + return RETURN_SUCCESS; > + } > + > + ASSERT (FileHandle != NULL); > + > + Status = (*PeiServices)->FfsGetFileInfo (FileHandle, &FileInfo); > + ASSERT_EFI_ERROR (Status); > + > + // > + // Some platforms do not cope very well with cache maintenance being > + // performed on regions backed by NOR flash. Since the cache maintenance > + // is unnecessary to begin with in that case, perform it only when not > + // executing in place. > + // > + if ((UINTN)FileInfo.Buffer <= (UINTN)ArmReplaceLiveTranslationEntry && > + ((UINTN)FileInfo.Buffer + FileInfo.BufferSize >= > + (UINTN)ArmReplaceLiveTranslationEntry + ArmReplaceLiveTranslationEntrySize)) { > + DEBUG ((EFI_D_INFO, "ArmLib: skipping cache maintence on XIP PEIM\n")); > + } else { > + DEBUG ((EFI_D_INFO, "ArmLib: performing cache maintence on shadowed PEIM\n")); > + // > + // The ArmReplaceLiveTranslationEntry () helper function may be invoked > + // with the MMU off so we have to ensure that it gets cleaned to the PoC > + // > + WriteBackDataCacheRange (ArmReplaceLiveTranslationEntry, > + ArmReplaceLiveTranslationEntrySize); > + } > + > + return RETURN_SUCCESS; > +} > -- > 1.9.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 15 June 2016 at 17:10, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On 13 June 2016 at 16:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: >> On some platforms, performing cache maintenance on regions that are backed >> by NOR flash result in SErrors. Since cache maintenance is unnecessary in >> that case, create a PEIM specific version that only performs said cache >> maintenance in its constructor if the module is shadowed in RAM. To avoid >> performing the cache maintenance if the MMU code is not used to begin with, >> check that explicitly in the constructor. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > Thanks Pushed as 469e1e1e4203b5d369fdce790883cb0aa035a744 -- Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c similarity index 100% rename from ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c rename to ArmPkg/Library/ArmLib/AArch64/AArch64BaseLibConstructor.c diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf index 58684e8492f2..ef9d261b910d 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf @@ -32,7 +32,7 @@ [Sources.AARCH64] ../Common/AArch64/ArmLibSupport.S ../Common/ArmLib.c - AArch64LibConstructor.c + AArch64BaseLibConstructor.c [Packages] ArmPkg/ArmPkg.dec diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf new file mode 100644 index 000000000000..c8f0b97750d4 --- /dev/null +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf @@ -0,0 +1,43 @@ +#/** @file +# +# Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR> +# Portions copyright (c) 2011 - 2014, 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 +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# +# +#**/ + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = AArch64Lib + FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138 + MODULE_TYPE = PEIM + VERSION_STRING = 1.0 + LIBRARY_CLASS = ArmLib|PEIM PEI_CORE + CONSTRUCTOR = AArch64LibConstructor + +[Sources.AARCH64] + AArch64Lib.c + AArch64Mmu.c + AArch64ArchTimer.c + ArmLibSupportV8.S + AArch64Support.S + AArch64ArchTimerSupport.S + + ../Common/AArch64/ArmLibSupport.S + ../Common/ArmLib.c + AArch64PeiLibConstructor.c + +[Packages] + ArmPkg/ArmPkg.dec + MdePkg/MdePkg.dec + +[LibraryClasses] + MemoryAllocationLib + CacheMaintenanceLib diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c index cf9b7222b47b..07864bac28e6 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c @@ -26,6 +26,8 @@ // We use this index definition to define an invalid block entry #define TT_ATTR_INDX_INVALID ((UINT32)~0) +INT32 HaveMmuRoutines = 1; + STATIC UINT64 ArmMemoryAttributeToPageAttribute ( diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c new file mode 100644 index 000000000000..2de9c7c54ed9 --- /dev/null +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c @@ -0,0 +1,75 @@ +#/* @file +# +# Copyright (c) 2016, Linaro Limited. All rights reserved. +# +# This program and the accompanying materials +# are licensed and made available under the terms and conditions of the BSD License +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# +#*/ + +#include <Base.h> + +#include <Library/ArmLib.h> +#include <Library/CacheMaintenanceLib.h> +#include <Library/DebugLib.h> + +// +// This is a hack. We define a weak symbol with external linkage, which may or +// may not be overridden by a non-weak alternative that is defined with a non +// zero value in the object that contains the MMU routines. Since static +// libraries are pulled in on a per-object basis, and since the MMU object will +// only be pulled in if any of its other symbols are referenced by the client +// module, we can use the value below to figure out whether the MMU routines are +// in use by this module, and decide whether cache maintenance of the function +// ArmReplaceLiveTranslationEntry () is required. +// +INT32 __attribute__((weak)) HaveMmuRoutines; + +EFI_STATUS +EFIAPI +AArch64LibConstructor ( + IN EFI_PEI_FILE_HANDLE FileHandle, + IN CONST EFI_PEI_SERVICES **PeiServices + ) +{ + extern UINT32 ArmReplaceLiveTranslationEntrySize; + + EFI_FV_FILE_INFO FileInfo; + EFI_STATUS Status; + + if (HaveMmuRoutines == 0) { + return RETURN_SUCCESS; + } + + ASSERT (FileHandle != NULL); + + Status = (*PeiServices)->FfsGetFileInfo (FileHandle, &FileInfo); + ASSERT_EFI_ERROR (Status); + + // + // Some platforms do not cope very well with cache maintenance being + // performed on regions backed by NOR flash. Since the cache maintenance + // is unnecessary to begin with in that case, perform it only when not + // executing in place. + // + if ((UINTN)FileInfo.Buffer <= (UINTN)ArmReplaceLiveTranslationEntry && + ((UINTN)FileInfo.Buffer + FileInfo.BufferSize >= + (UINTN)ArmReplaceLiveTranslationEntry + ArmReplaceLiveTranslationEntrySize)) { + DEBUG ((EFI_D_INFO, "ArmLib: skipping cache maintence on XIP PEIM\n")); + } else { + DEBUG ((EFI_D_INFO, "ArmLib: performing cache maintence on shadowed PEIM\n")); + // + // The ArmReplaceLiveTranslationEntry () helper function may be invoked + // with the MMU off so we have to ensure that it gets cleaned to the PoC + // + WriteBackDataCacheRange (ArmReplaceLiveTranslationEntry, + ArmReplaceLiveTranslationEntrySize); + } + + return RETURN_SUCCESS; +}
On some platforms, performing cache maintenance on regions that are backed by NOR flash result in SErrors. Since cache maintenance is unnecessary in that case, create a PEIM specific version that only performs said cache maintenance in its constructor if the module is shadowed in RAM. To avoid performing the cache maintenance if the MMU code is not used to begin with, check that explicitly in the constructor. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- As discussed in the thread dedicated to this subject, the preferred way of addressing this to split off the MMU manipulation code from ArmLib into a separate ArmMmuLib, but this affects other packages and platforms. So in the mean time, let's merge this patch so that D02 can use the upstream ArmLib unmodified. ArmPkg/Library/ArmLib/AArch64/{AArch64LibConstructor.c => AArch64BaseLibConstructor.c} | 0 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 2 +- ArmPkg/Library/ArmLib/AArch64/AArch64LibPei.inf | 43 +++++++++++ ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 2 + ArmPkg/Library/ArmLib/AArch64/AArch64PeiLibConstructor.c | 75 ++++++++++++++++++++ 5 files changed, 121 insertions(+), 1 deletion(-) -- 1.9.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel