Re: [edk2] [PATCH 4/4] ArmPkg/ArmMmuLib: add PEI specific version of ArmMmuLib
On 17 June 2016 at 12:32, Leif Lindholm wrote:
> On Thu, Jun 16, 2016 at 12:29:30PM +0200, Ard Biesheuvel wrote:
>> This introduces a special version of ArmMmuLib for PEIMs that takes care
>> only to perform cache maintenance on the live entry replacement routine
>> if the module is not executing in place. Not only is such cache maintenance
>> unnecessary in that case, it may be actively harmful on some systems that
>> fail to tolerate cache maintenance operations on NOR flash regions.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel
>> ---
>> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c | 60
>>
>> ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf | 36
>> 2 files changed, 96 insertions(+)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
>> b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
>> new file mode 100644
>> index ..91dc1157e79a
>> --- /dev/null
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
>> @@ -0,0 +1,60 @@
>> +#/* @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
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +ArmMmuPeiLibConstructor (
>> + IN EFI_PEI_FILE_HANDLE FileHandle,
>> + IN CONST EFI_PEI_SERVICES **PeiServices
>> + )
>> +{
>> + extern UINT32 ArmReplaceLiveTranslationEntrySize;
>> +
>> + EFI_FV_FILE_INFO FileInfo;
>> + EFI_STATUSStatus;
>> +
>> + 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.
>> + //
>
> I think perhaps the wording of the above is confusing me a little bit;
> the thing that makes it not matter is that it is executing in place,
> not that it is backed by NOR, right? Could the statement be flipped?:
>
Actually, a bit of both. But even when we are executing PEI from DRAM,
we should be able to assume that it is clean to the PoC since it is
entered with the MMU off. So that is why the heuristic is based on XIP
rather than whether it is backed by NOR, since the latter is
impossible to detect.
> "Perform it only when not executing in place, since otherwise the
> cache maintenance is unnecessary."
>
> Someone who is less fond of multiple negations in a singe sentence
> than me may prefer to reword it further...
>
How about
"""
Since the firmware image can be assumed to be clean to the PoC when
running XIP, even when PEI is executing from DRAM, we only need to
perform the cache maintenance when not executing in place.
"""
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 4/4] ArmPkg/ArmMmuLib: add PEI specific version of ArmMmuLib
On Thu, Jun 16, 2016 at 12:29:30PM +0200, Ard Biesheuvel wrote:
> This introduces a special version of ArmMmuLib for PEIMs that takes care
> only to perform cache maintenance on the live entry replacement routine
> if the module is not executing in place. Not only is such cache maintenance
> unnecessary in that case, it may be actively harmful on some systems that
> fail to tolerate cache maintenance operations on NOR flash regions.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel
> ---
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c | 60
>
> ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf | 36
> 2 files changed, 96 insertions(+)
>
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
> b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
> new file mode 100644
> index ..91dc1157e79a
> --- /dev/null
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
> @@ -0,0 +1,60 @@
> +#/* @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
> +
> +#include
> +#include
> +#include
> +#include
> +
> +EFI_STATUS
> +EFIAPI
> +ArmMmuPeiLibConstructor (
> + IN EFI_PEI_FILE_HANDLE FileHandle,
> + IN CONST EFI_PEI_SERVICES **PeiServices
> + )
> +{
> + extern UINT32 ArmReplaceLiveTranslationEntrySize;
> +
> + EFI_FV_FILE_INFO FileInfo;
> + EFI_STATUSStatus;
> +
> + 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.
> + //
I think perhaps the wording of the above is confusing me a little bit;
the thing that makes it not matter is that it is executing in place,
not that it is backed by NOR, right? Could the statement be flipped?:
"Perform it only when not executing in place, since otherwise the
cache maintenance is unnecessary."
Someone who is less fond of multiple negations in a singe sentence
than me may prefer to reword it further...
/
Leif
> + if ((UINTN)FileInfo.Buffer <= (UINTN)ArmReplaceLiveTranslationEntry &&
> + ((UINTN)FileInfo.Buffer + FileInfo.BufferSize >=
> + (UINTN)ArmReplaceLiveTranslationEntry +
> ArmReplaceLiveTranslationEntrySize)) {
> +DEBUG ((EFI_D_INFO, "ArmMmuLib: skipping cache maintenance on XIP
> PEIM\n"));
> + } else {
> +DEBUG ((EFI_D_INFO, "ArmMmuLib: performing cache maintenance 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;
> +}
> diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> new file mode 100644
> index ..14ebf8de673d
> --- /dev/null
> +++ b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
> @@ -0,0 +1,36 @@
> +#/** @file
> +#
> +# Copyright (c) 2016 Linaro Ltd. 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 = ArmMmuPeiLib
> + FILE_GUID = b50d8d53-1ad1-44ea-9e69-8c89d4a6d08b
> + MODULE_TYPE= PEIM
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = ArmMmuLib|PEIM
> + CONSTRUCTOR= ArmMmuPeiLibConstructor
> +
> +[Sources.AARCH64]
> + AArch64/ArmMmuLibCore.c
> + AArch64/ArmMmuPeiLibConstructor.c
> + AArch64/ArmMmuLibReplaceEntry.S
> +
> +[Packages]
> + ArmPkg/ArmPkg.dec
> + MdePkg/MdePkg.dec
> +
> +[Li
[edk2] [PATCH 4/4] ArmPkg/ArmMmuLib: add PEI specific version of ArmMmuLib
This introduces a special version of ArmMmuLib for PEIMs that takes care
only to perform cache maintenance on the live entry replacement routine
if the module is not executing in place. Not only is such cache maintenance
unnecessary in that case, it may be actively harmful on some systems that
fail to tolerate cache maintenance operations on NOR flash regions.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c | 60
ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf | 36
2 files changed, 96 insertions(+)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
new file mode 100644
index ..91dc1157e79a
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuPeiLibConstructor.c
@@ -0,0 +1,60 @@
+#/* @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
+
+#include
+#include
+#include
+#include
+
+EFI_STATUS
+EFIAPI
+ArmMmuPeiLibConstructor (
+ IN EFI_PEI_FILE_HANDLE FileHandle,
+ IN CONST EFI_PEI_SERVICES **PeiServices
+ )
+{
+ extern UINT32 ArmReplaceLiveTranslationEntrySize;
+
+ EFI_FV_FILE_INFO FileInfo;
+ EFI_STATUSStatus;
+
+ 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, "ArmMmuLib: skipping cache maintenance on XIP
PEIM\n"));
+ } else {
+DEBUG ((EFI_D_INFO, "ArmMmuLib: performing cache maintenance 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;
+}
diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
new file mode 100644
index ..14ebf8de673d
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
@@ -0,0 +1,36 @@
+#/** @file
+#
+# Copyright (c) 2016 Linaro Ltd. 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 = ArmMmuPeiLib
+ FILE_GUID = b50d8d53-1ad1-44ea-9e69-8c89d4a6d08b
+ MODULE_TYPE= PEIM
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = ArmMmuLib|PEIM
+ CONSTRUCTOR= ArmMmuPeiLibConstructor
+
+[Sources.AARCH64]
+ AArch64/ArmMmuLibCore.c
+ AArch64/ArmMmuPeiLibConstructor.c
+ AArch64/ArmMmuLibReplaceEntry.S
+
+[Packages]
+ ArmPkg/ArmPkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ ArmLib
+ CacheMaintenanceLib
+ MemoryAllocationLib
--
1.9.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
