Re: [Linaro-uefi] [PATCH v4] ArmPkg/AArch64Mmu: disable MMU during page table manipulations
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below: 1. We have a PEIM, which invokes HobLib -> PeiServicesLib -> PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I'm not sure if it is a common issue of generating SERR when flush flash space, but, as ArmLib is such a fundamental library, do we really need to make the function as a library constructor instead of being called on demand?
Regards.
Heyi
On 04/14/2016 11:48 PM, Ard Biesheuvel wrote:
On ARM, manipulating live page tables is cumbersome since the architecture mandates the use of break-before-make, i.e., replacing a block entry with a table entry requires an intermediate step via an invalid entry, or TLB conflicts may occur.
Since it is not generally feasible to decide in the page table manipulation routines whether such an invalid entry will result in those routines themselves to become unavailable, use a function that is callable with the MMU off (i.e., a leaf function that does not access the stack) to perform the change of a block entry into a table entry.
Note that the opposite should never occur, i.e., table entries are never coalesced into block entries.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
ArmPkg/Include/Library/ArmLib.h | 6 ++ ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +- ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36 ++++++++++++ ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 17 +++++- ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 62 ++++++++++++++++++++ 5 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h index 15f610d82e1d..1689f0072db6 100644 --- a/ArmPkg/Include/Library/ArmLib.h +++ b/ArmPkg/Include/Library/ArmLib.h @@ -613,4 +613,10 @@ ArmClearMemoryRegionReadOnly ( IN UINT64 Length ); +VOID +ArmReplaceLiveTranslationEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- );
- #endif // __ARM_LIB__
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf index dd585dea91fb..58684e8492f2 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf @@ -17,9 +17,10 @@ [Defines] INF_VERSION = 0x00010005 BASE_NAME = AArch64Lib FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138
- MODULE_TYPE = DXE_DRIVER
- MODULE_TYPE = BASE VERSION_STRING = 1.0 LIBRARY_CLASS = ArmLib
- CONSTRUCTOR = AArch64LibConstructor
[Sources.AARCH64] AArch64Lib.c @@ -31,6 +32,7 @@ [Sources.AARCH64] ../Common/AArch64/ArmLibSupport.S ../Common/ArmLib.c
- AArch64LibConstructor.c
[Packages] ArmPkg/ArmPkg.dec @@ -38,6 +40,7 @@ [Packages] [LibraryClasses] MemoryAllocationLib
- CacheMaintenanceLib
[Protocols] gEfiCpuArchProtocolGuid diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c new file mode 100644 index 000000000000..d2d0d3c15ee3 --- /dev/null +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c @@ -0,0 +1,36 @@ +#/* @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>
+RETURN_STATUS +EFIAPI +AArch64LibConstructor (
- VOID
- )
+{
- extern UINT32 ArmReplaceLiveTranslationEntrySize;
- //
- // 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/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c index b7d23c6f3286..2cc6fc45aecf 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c @@ -169,6 +169,20 @@ GetRootTranslationTableInfo ( STATIC VOID +ReplaceLiveEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- )
+{
- if (!ArmMmuEnabled ()) {
- *Entry = Value;
- } else {
- ArmReplaceLiveTranslationEntry (Entry, Value);
- }
+}
+STATIC +VOID LookupAddresstoRootTable ( IN UINT64 MaxAddress, OUT UINTN *T0SZ, @@ -330,7 +344,8 @@ GetBlockEntryListFromAddress ( } // Fill the BlockEntry with the new TranslationTable
*BlockEntry = ((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY;
ReplaceLiveEntry (BlockEntry,((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY); } } else { if (IndexLevel != PageLevel) {diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S index 1a3023b79487..43f7a795acec 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S @@ -56,6 +56,8 @@ GCC_ASM_EXPORT (ArmReadIdPfr1) GCC_ASM_EXPORT (ArmWriteHcr) GCC_ASM_EXPORT (ArmReadHcr) GCC_ASM_EXPORT (ArmReadCurrentEL) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntry) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntrySize) .set CTRL_M_BIT, (1 << 0) .set CTRL_A_BIT, (1 << 1) @@ -481,4 +483,64 @@ ASM_PFX(ArmReadCurrentEL): mrs x0, CurrentEL ret
- .macro __replace_entry, el
- // disable the MMU
- mrs x8, sctlr_el\el
- bic x9, x8, #CTRL_M_BIT
- msr sctlr_el\el, x9
- isb
- // write updated entry
- str x1, [x0]
- // invalidate again to get rid of stale clean cachelines that may
- // have been filled speculatively since the last invalidate
- dmb sy
- dc ivac, x0
- // flush the TLBs
- .if \el == 1
- tlbi vmalle1
- .else
- tlbi alle\el
- .endif
- dsb sy
- // re-enable the MMU
- msr sctlr_el\el, x8
- isb
- .endm
+//VOID +//ArmReplaceLiveTranslationEntry ( +// IN UINT64 *Entry, +// IN UINT64 Value +// ) +ASM_PFX(ArmReplaceLiveTranslationEntry):
- // disable interrupts
- mrs x2, daif
- msr daifset, #0xf
- isb
- // clean and invalidate first so that we don't clobber
- // adjacent entries that are dirty in the caches
- dc civac, x0
- dsb ish
- EL1_OR_EL2_OR_EL3(x3)
+1:__replace_entry 1
- b 4f
+2:__replace_entry 2
- b 4f
+3:__replace_entry 3
+4:msr daif, x2
- ret
+ASM_PFX(ArmReplaceLiveTranslationEntrySize):
- .long . - ArmReplaceLiveTranslationEntry
- ASM_FUNCTION_REMOVE_IF_UNREFERENCED
On 22 April 2016 at 11:13, Heyi Guo heyi.guo@linaro.org wrote:
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below:
- We have a PEIM, which invokes HobLib -> PeiServicesLib ->
PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I'm not sure if it is a common issue of generating SERR when flush flash space, but, as ArmLib is such a fundamental library, do we really need to make the function as a library constructor instead of being called on demand?
I think the issue here is that the ordinary ArmLib may not be suitable for PEIMs. There is also AArch64LibSec.inf, which is the same library but without the constructor. Could you try that please?
On 04/14/2016 11:48 PM, Ard Biesheuvel wrote:
On ARM, manipulating live page tables is cumbersome since the architecture mandates the use of break-before-make, i.e., replacing a block entry with a table entry requires an intermediate step via an invalid entry, or TLB conflicts may occur.
Since it is not generally feasible to decide in the page table manipulation routines whether such an invalid entry will result in those routines themselves to become unavailable, use a function that is callable with the MMU off (i.e., a leaf function that does not access the stack) to perform the change of a block entry into a table entry.
Note that the opposite should never occur, i.e., table entries are never coalesced into block entries.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
ArmPkg/Include/Library/ArmLib.h | 6 ++ ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +- ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36 ++++++++++++ ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 17 +++++- ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 62 ++++++++++++++++++++ 5 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h index 15f610d82e1d..1689f0072db6 100644 --- a/ArmPkg/Include/Library/ArmLib.h +++ b/ArmPkg/Include/Library/ArmLib.h @@ -613,4 +613,10 @@ ArmClearMemoryRegionReadOnly ( IN UINT64 Length ); +VOID +ArmReplaceLiveTranslationEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- );
- #endif // __ARM_LIB__
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf index dd585dea91fb..58684e8492f2 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf @@ -17,9 +17,10 @@ [Defines] INF_VERSION = 0x00010005 BASE_NAME = AArch64Lib FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138
- MODULE_TYPE = DXE_DRIVER
- MODULE_TYPE = BASE VERSION_STRING = 1.0 LIBRARY_CLASS = ArmLib
- CONSTRUCTOR = AArch64LibConstructor [Sources.AARCH64] AArch64Lib.c
@@ -31,6 +32,7 @@ [Sources.AARCH64] ../Common/AArch64/ArmLibSupport.S ../Common/ArmLib.c
- AArch64LibConstructor.c [Packages] ArmPkg/ArmPkg.dec
@@ -38,6 +40,7 @@ [Packages] [LibraryClasses] MemoryAllocationLib
- CacheMaintenanceLib [Protocols] gEfiCpuArchProtocolGuid
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c new file mode 100644 index 000000000000..d2d0d3c15ee3 --- /dev/null +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c @@ -0,0 +1,36 @@ +#/* @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>
+RETURN_STATUS +EFIAPI +AArch64LibConstructor (
- VOID
- )
+{
- extern UINT32 ArmReplaceLiveTranslationEntrySize;
- //
- // 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/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c index b7d23c6f3286..2cc6fc45aecf 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c @@ -169,6 +169,20 @@ GetRootTranslationTableInfo ( STATIC VOID +ReplaceLiveEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- )
+{
- if (!ArmMmuEnabled ()) {
- *Entry = Value;
- } else {
- ArmReplaceLiveTranslationEntry (Entry, Value);
- }
+}
+STATIC +VOID LookupAddresstoRootTable ( IN UINT64 MaxAddress, OUT UINTN *T0SZ, @@ -330,7 +344,8 @@ GetBlockEntryListFromAddress ( } // Fill the BlockEntry with the new TranslationTable
*BlockEntry = ((UINTN)TranslationTable &TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY;
ReplaceLiveEntry (BlockEntry,((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) |TableAttributes | TT_TYPE_TABLE_ENTRY); } } else { if (IndexLevel != PageLevel) { diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S index 1a3023b79487..43f7a795acec 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S @@ -56,6 +56,8 @@ GCC_ASM_EXPORT (ArmReadIdPfr1) GCC_ASM_EXPORT (ArmWriteHcr) GCC_ASM_EXPORT (ArmReadHcr) GCC_ASM_EXPORT (ArmReadCurrentEL) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntry) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntrySize) .set CTRL_M_BIT, (1 << 0) .set CTRL_A_BIT, (1 << 1) @@ -481,4 +483,64 @@ ASM_PFX(ArmReadCurrentEL): mrs x0, CurrentEL ret
- .macro __replace_entry, el
- // disable the MMU
- mrs x8, sctlr_el\el
- bic x9, x8, #CTRL_M_BIT
- msr sctlr_el\el, x9
- isb
- // write updated entry
- str x1, [x0]
- // invalidate again to get rid of stale clean cachelines that may
- // have been filled speculatively since the last invalidate
- dmb sy
- dc ivac, x0
- // flush the TLBs
- .if \el == 1
- tlbi vmalle1
- .else
- tlbi alle\el
- .endif
- dsb sy
- // re-enable the MMU
- msr sctlr_el\el, x8
- isb
- .endm
+//VOID +//ArmReplaceLiveTranslationEntry ( +// IN UINT64 *Entry, +// IN UINT64 Value +// ) +ASM_PFX(ArmReplaceLiveTranslationEntry):
- // disable interrupts
- mrs x2, daif
- msr daifset, #0xf
- isb
- // clean and invalidate first so that we don't clobber
- // adjacent entries that are dirty in the caches
- dc civac, x0
- dsb ish
- EL1_OR_EL2_OR_EL3(x3)
+1:__replace_entry 1
- b 4f
+2:__replace_entry 2
- b 4f
+3:__replace_entry 3
+4:msr daif, x2
- ret
+ASM_PFX(ArmReplaceLiveTranslationEntrySize):
- .long . - ArmReplaceLiveTranslationEntry
- ASM_FUNCTION_REMOVE_IF_UNREFERENCED
On 04/22/2016 05:16 PM, Ard Biesheuvel wrote:
On 22 April 2016 at 11:13, Heyi Guo heyi.guo@linaro.org wrote:
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below:
- We have a PEIM, which invokes HobLib -> PeiServicesLib ->
PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I'm not sure if it is a common issue of generating SERR when flush flash space, but, as ArmLib is such a fundamental library, do we really need to make the function as a library constructor instead of being called on demand?
I think the issue here is that the ordinary ArmLib may not be suitable for PEIMs. There is also AArch64LibSec.inf, which is the same library but without the constructor. Could you try that please?
We have memory initialization PEIM, which needs to configure MMU after memory initialization is done. So AArch64LibSec is not sufficient, and it will still cause issue on our platform.
Regards.
Heyi
On 04/14/2016 11:48 PM, Ard Biesheuvel wrote:
On ARM, manipulating live page tables is cumbersome since the architecture mandates the use of break-before-make, i.e., replacing a block entry with a table entry requires an intermediate step via an invalid entry, or TLB conflicts may occur.
Since it is not generally feasible to decide in the page table manipulation routines whether such an invalid entry will result in those routines themselves to become unavailable, use a function that is callable with the MMU off (i.e., a leaf function that does not access the stack) to perform the change of a block entry into a table entry.
Note that the opposite should never occur, i.e., table entries are never coalesced into block entries.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
ArmPkg/Include/Library/ArmLib.h | 6 ++ ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +- ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36 ++++++++++++ ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 17 +++++- ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 62 ++++++++++++++++++++ 5 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h index 15f610d82e1d..1689f0072db6 100644 --- a/ArmPkg/Include/Library/ArmLib.h +++ b/ArmPkg/Include/Library/ArmLib.h @@ -613,4 +613,10 @@ ArmClearMemoryRegionReadOnly ( IN UINT64 Length ); +VOID +ArmReplaceLiveTranslationEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- );
- #endif // __ARM_LIB__
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf index dd585dea91fb..58684e8492f2 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf @@ -17,9 +17,10 @@ [Defines] INF_VERSION = 0x00010005 BASE_NAME = AArch64Lib FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138
- MODULE_TYPE = DXE_DRIVER
- MODULE_TYPE = BASE VERSION_STRING = 1.0 LIBRARY_CLASS = ArmLib
- CONSTRUCTOR = AArch64LibConstructor [Sources.AARCH64] AArch64Lib.c
@@ -31,6 +32,7 @@ [Sources.AARCH64] ../Common/AArch64/ArmLibSupport.S ../Common/ArmLib.c
- AArch64LibConstructor.c [Packages] ArmPkg/ArmPkg.dec
@@ -38,6 +40,7 @@ [Packages] [LibraryClasses] MemoryAllocationLib
- CacheMaintenanceLib [Protocols] gEfiCpuArchProtocolGuid
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c new file mode 100644 index 000000000000..d2d0d3c15ee3 --- /dev/null +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c @@ -0,0 +1,36 @@ +#/* @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>
+RETURN_STATUS +EFIAPI +AArch64LibConstructor (
- VOID
- )
+{
- extern UINT32 ArmReplaceLiveTranslationEntrySize;
- //
- // 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/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c index b7d23c6f3286..2cc6fc45aecf 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c @@ -169,6 +169,20 @@ GetRootTranslationTableInfo ( STATIC VOID +ReplaceLiveEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- )
+{
- if (!ArmMmuEnabled ()) {
- *Entry = Value;
- } else {
- ArmReplaceLiveTranslationEntry (Entry, Value);
- }
+}
+STATIC +VOID LookupAddresstoRootTable ( IN UINT64 MaxAddress, OUT UINTN *T0SZ, @@ -330,7 +344,8 @@ GetBlockEntryListFromAddress ( } // Fill the BlockEntry with the new TranslationTable
*BlockEntry = ((UINTN)TranslationTable &TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY;
ReplaceLiveEntry (BlockEntry,((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) |TableAttributes | TT_TYPE_TABLE_ENTRY); } } else { if (IndexLevel != PageLevel) { diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S index 1a3023b79487..43f7a795acec 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S @@ -56,6 +56,8 @@ GCC_ASM_EXPORT (ArmReadIdPfr1) GCC_ASM_EXPORT (ArmWriteHcr) GCC_ASM_EXPORT (ArmReadHcr) GCC_ASM_EXPORT (ArmReadCurrentEL) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntry) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntrySize) .set CTRL_M_BIT, (1 << 0) .set CTRL_A_BIT, (1 << 1) @@ -481,4 +483,64 @@ ASM_PFX(ArmReadCurrentEL): mrs x0, CurrentEL ret
- .macro __replace_entry, el
- // disable the MMU
- mrs x8, sctlr_el\el
- bic x9, x8, #CTRL_M_BIT
- msr sctlr_el\el, x9
- isb
- // write updated entry
- str x1, [x0]
- // invalidate again to get rid of stale clean cachelines that may
- // have been filled speculatively since the last invalidate
- dmb sy
- dc ivac, x0
- // flush the TLBs
- .if \el == 1
- tlbi vmalle1
- .else
- tlbi alle\el
- .endif
- dsb sy
- // re-enable the MMU
- msr sctlr_el\el, x8
- isb
- .endm
+//VOID +//ArmReplaceLiveTranslationEntry ( +// IN UINT64 *Entry, +// IN UINT64 Value +// ) +ASM_PFX(ArmReplaceLiveTranslationEntry):
- // disable interrupts
- mrs x2, daif
- msr daifset, #0xf
- isb
- // clean and invalidate first so that we don't clobber
- // adjacent entries that are dirty in the caches
- dc civac, x0
- dsb ish
- EL1_OR_EL2_OR_EL3(x3)
+1:__replace_entry 1
- b 4f
+2:__replace_entry 2
- b 4f
+3:__replace_entry 3
+4:msr daif, x2
- ret
+ASM_PFX(ArmReplaceLiveTranslationEntrySize):
- .long . - ArmReplaceLiveTranslationEntry
- ASM_FUNCTION_REMOVE_IF_UNREFERENCED
On 22 April 2016 at 11:31, Heyi Guo heyi.guo@linaro.org wrote:
On 04/22/2016 05:16 PM, Ard Biesheuvel wrote:
On 22 April 2016 at 11:13, Heyi Guo heyi.guo@linaro.org wrote:
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below:
- We have a PEIM, which invokes HobLib -> PeiServicesLib ->
PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I'm not sure if it is a common issue of generating SERR when flush flash space, but, as ArmLib is such a fundamental library, do we really need to make the function as a library constructor instead of being called on demand?
I think the issue here is that the ordinary ArmLib may not be suitable for PEIMs. There is also AArch64LibSec.inf, which is the same library but without the constructor. Could you try that please?
We have memory initialization PEIM, which needs to configure MMU after memory initialization is done. So AArch64LibSec is not sufficient, and it will still cause issue on our platform.
Ah yes, that doesn't have the MMU code. In any case, the cache maintenance is only necessary if not executing in place, so we should be able to create a PEI version without the constructor, but with the MMU routines. We should probably have an alternate version of ArmReplaceLiveTranslationEntry that asserts that the MMU is disabled.
On Fri, Apr 22, 2016 at 05:13:08PM +0800, Heyi Guo wrote:
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below:
- We have a PEIM, which invokes HobLib -> PeiServicesLib ->
PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I take it by 'flush' you mean the Clean+Invalidate (i.e. the DC CIVAC).
That should only cause a writeback if data is dirty, and should be a NOP otherwise, so if this is causing problems then there is a latent issue regardless.
Dirty data can be naturally evicted from the cache at any time, for any reason, regardless of explicit cache maintenance.
I'm not sure if it is a common issue of generating SERR when flush flash space, but, as ArmLib is such a fundamental library, do we really need to make the function as a library constructor instead of being called on demand?
Sorry for my ignorance, but would this actually prevent dirtying of cache lines for PAs covering the flash? It doesn't sound to me like it would, and thus it doesn't sound like it solves the underlying problem.
Thanks, Mark.
Regards.
Heyi
On 04/14/2016 11:48 PM, Ard Biesheuvel wrote:
On ARM, manipulating live page tables is cumbersome since the architecture mandates the use of break-before-make, i.e., replacing a block entry with a table entry requires an intermediate step via an invalid entry, or TLB conflicts may occur.
Since it is not generally feasible to decide in the page table manipulation routines whether such an invalid entry will result in those routines themselves to become unavailable, use a function that is callable with the MMU off (i.e., a leaf function that does not access the stack) to perform the change of a block entry into a table entry.
Note that the opposite should never occur, i.e., table entries are never coalesced into block entries.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
ArmPkg/Include/Library/ArmLib.h | 6 ++ ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +- ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36 ++++++++++++ ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 17 +++++- ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 62 ++++++++++++++++++++ 5 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h index 15f610d82e1d..1689f0072db6 100644 --- a/ArmPkg/Include/Library/ArmLib.h +++ b/ArmPkg/Include/Library/ArmLib.h @@ -613,4 +613,10 @@ ArmClearMemoryRegionReadOnly ( IN UINT64 Length ); +VOID +ArmReplaceLiveTranslationEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- );
#endif // __ARM_LIB__ diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf index dd585dea91fb..58684e8492f2 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf @@ -17,9 +17,10 @@ [Defines] INF_VERSION = 0x00010005 BASE_NAME = AArch64Lib FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138
- MODULE_TYPE = DXE_DRIVER
- MODULE_TYPE = BASE VERSION_STRING = 1.0 LIBRARY_CLASS = ArmLib
- CONSTRUCTOR = AArch64LibConstructor
[Sources.AARCH64] AArch64Lib.c @@ -31,6 +32,7 @@ [Sources.AARCH64] ../Common/AArch64/ArmLibSupport.S ../Common/ArmLib.c
- AArch64LibConstructor.c
[Packages] ArmPkg/ArmPkg.dec @@ -38,6 +40,7 @@ [Packages] [LibraryClasses] MemoryAllocationLib
- CacheMaintenanceLib
[Protocols] gEfiCpuArchProtocolGuid diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c new file mode 100644 index 000000000000..d2d0d3c15ee3 --- /dev/null +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c @@ -0,0 +1,36 @@ +#/* @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>
+RETURN_STATUS +EFIAPI +AArch64LibConstructor (
- VOID
- )
+{
- extern UINT32 ArmReplaceLiveTranslationEntrySize;
- //
- // 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/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c index b7d23c6f3286..2cc6fc45aecf 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c @@ -169,6 +169,20 @@ GetRootTranslationTableInfo ( STATIC VOID +ReplaceLiveEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- )
+{
- if (!ArmMmuEnabled ()) {
- *Entry = Value;
- } else {
- ArmReplaceLiveTranslationEntry (Entry, Value);
- }
+}
+STATIC +VOID LookupAddresstoRootTable ( IN UINT64 MaxAddress, OUT UINTN *T0SZ, @@ -330,7 +344,8 @@ GetBlockEntryListFromAddress ( } // Fill the BlockEntry with the new TranslationTable
*BlockEntry = ((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY;
ReplaceLiveEntry (BlockEntry,((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY); }diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S index 1a3023b79487..43f7a795acec 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S @@ -56,6 +56,8 @@ GCC_ASM_EXPORT (ArmReadIdPfr1) GCC_ASM_EXPORT (ArmWriteHcr) GCC_ASM_EXPORT (ArmReadHcr) GCC_ASM_EXPORT (ArmReadCurrentEL) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntry) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntrySize) .set CTRL_M_BIT, (1 << 0) .set CTRL_A_BIT, (1 << 1) @@ -481,4 +483,64 @@ ASM_PFX(ArmReadCurrentEL): mrs x0, CurrentEL ret
- .macro __replace_entry, el
- // disable the MMU
- mrs x8, sctlr_el\el
- bic x9, x8, #CTRL_M_BIT
- msr sctlr_el\el, x9
- isb
- // write updated entry
- str x1, [x0]
- // invalidate again to get rid of stale clean cachelines that may
- // have been filled speculatively since the last invalidate
- dmb sy
- dc ivac, x0
- // flush the TLBs
- .if \el == 1
- tlbi vmalle1
- .else
- tlbi alle\el
- .endif
- dsb sy
- // re-enable the MMU
- msr sctlr_el\el, x8
- isb
- .endm
+//VOID +//ArmReplaceLiveTranslationEntry ( +// IN UINT64 *Entry, +// IN UINT64 Value +// ) +ASM_PFX(ArmReplaceLiveTranslationEntry):
- // disable interrupts
- mrs x2, daif
- msr daifset, #0xf
- isb
- // clean and invalidate first so that we don't clobber
- // adjacent entries that are dirty in the caches
- dc civac, x0
- dsb ish
- EL1_OR_EL2_OR_EL3(x3)
+1:__replace_entry 1
- b 4f
+2:__replace_entry 2
- b 4f
+3:__replace_entry 3
+4:msr daif, x2
- ret
+ASM_PFX(ArmReplaceLiveTranslationEntrySize):
- .long . - ArmReplaceLiveTranslationEntry
ASM_FUNCTION_REMOVE_IF_UNREFERENCED
On 22 April 2016 at 11:32, Mark Rutland mark.rutland@arm.com wrote:
On Fri, Apr 22, 2016 at 05:13:08PM +0800, Heyi Guo wrote:
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below:
- We have a PEIM, which invokes HobLib -> PeiServicesLib ->
PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I take it by 'flush' you mean the Clean+Invalidate (i.e. the DC CIVAC).
That should only cause a writeback if data is dirty, and should be a NOP otherwise, so if this is causing problems then there is a latent issue regardless.
Dirty data can be naturally evicted from the cache at any time, for any reason, regardless of explicit cache maintenance.
Assuming that this code executes with the MMU off, it probably means there is junk in the caches?
I'm not sure if it is a common issue of generating SERR when flush flash space, but, as ArmLib is such a fundamental library, do we really need to make the function as a library constructor instead of being called on demand?
Sorry for my ignorance, but would this actually prevent dirtying of cache lines for PAs covering the flash? It doesn't sound to me like it would, and thus it doesn't sound like it solves the underlying problem.
Thanks, Mark.
Regards.
Heyi
On 04/14/2016 11:48 PM, Ard Biesheuvel wrote:
On ARM, manipulating live page tables is cumbersome since the architecture mandates the use of break-before-make, i.e., replacing a block entry with a table entry requires an intermediate step via an invalid entry, or TLB conflicts may occur.
Since it is not generally feasible to decide in the page table manipulation routines whether such an invalid entry will result in those routines themselves to become unavailable, use a function that is callable with the MMU off (i.e., a leaf function that does not access the stack) to perform the change of a block entry into a table entry.
Note that the opposite should never occur, i.e., table entries are never coalesced into block entries.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org
ArmPkg/Include/Library/ArmLib.h | 6 ++ ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf | 5 +- ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c | 36 ++++++++++++ ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 17 +++++- ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 62 ++++++++++++++++++++ 5 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h index 15f610d82e1d..1689f0072db6 100644 --- a/ArmPkg/Include/Library/ArmLib.h +++ b/ArmPkg/Include/Library/ArmLib.h @@ -613,4 +613,10 @@ ArmClearMemoryRegionReadOnly ( IN UINT64 Length ); +VOID +ArmReplaceLiveTranslationEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- );
#endif // __ARM_LIB__ diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf index dd585dea91fb..58684e8492f2 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf @@ -17,9 +17,10 @@ [Defines] INF_VERSION = 0x00010005 BASE_NAME = AArch64Lib FILE_GUID = ef20ddf5-b334-47b3-94cf-52ff44c29138
- MODULE_TYPE = DXE_DRIVER
- MODULE_TYPE = BASE VERSION_STRING = 1.0 LIBRARY_CLASS = ArmLib
- CONSTRUCTOR = AArch64LibConstructor
[Sources.AARCH64] AArch64Lib.c @@ -31,6 +32,7 @@ [Sources.AARCH64] ../Common/AArch64/ArmLibSupport.S ../Common/ArmLib.c
- AArch64LibConstructor.c
[Packages] ArmPkg/ArmPkg.dec @@ -38,6 +40,7 @@ [Packages] [LibraryClasses] MemoryAllocationLib
- CacheMaintenanceLib
[Protocols] gEfiCpuArchProtocolGuid diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c new file mode 100644 index 000000000000..d2d0d3c15ee3 --- /dev/null +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64LibConstructor.c @@ -0,0 +1,36 @@ +#/* @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>
+RETURN_STATUS +EFIAPI +AArch64LibConstructor (
- VOID
- )
+{
- extern UINT32 ArmReplaceLiveTranslationEntrySize;
- //
- // 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/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c index b7d23c6f3286..2cc6fc45aecf 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c @@ -169,6 +169,20 @@ GetRootTranslationTableInfo ( STATIC VOID +ReplaceLiveEntry (
- IN UINT64 *Entry,
- IN UINT64 Value
- )
+{
- if (!ArmMmuEnabled ()) {
- *Entry = Value;
- } else {
- ArmReplaceLiveTranslationEntry (Entry, Value);
- }
+}
+STATIC +VOID LookupAddresstoRootTable ( IN UINT64 MaxAddress, OUT UINTN *T0SZ, @@ -330,7 +344,8 @@ GetBlockEntryListFromAddress ( } // Fill the BlockEntry with the new TranslationTable
*BlockEntry = ((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY;
ReplaceLiveEntry (BlockEntry,((UINTN)TranslationTable & TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY); }diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S index 1a3023b79487..43f7a795acec 100644 --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S @@ -56,6 +56,8 @@ GCC_ASM_EXPORT (ArmReadIdPfr1) GCC_ASM_EXPORT (ArmWriteHcr) GCC_ASM_EXPORT (ArmReadHcr) GCC_ASM_EXPORT (ArmReadCurrentEL) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntry) +GCC_ASM_EXPORT (ArmReplaceLiveTranslationEntrySize) .set CTRL_M_BIT, (1 << 0) .set CTRL_A_BIT, (1 << 1) @@ -481,4 +483,64 @@ ASM_PFX(ArmReadCurrentEL): mrs x0, CurrentEL ret
- .macro __replace_entry, el
- // disable the MMU
- mrs x8, sctlr_el\el
- bic x9, x8, #CTRL_M_BIT
- msr sctlr_el\el, x9
- isb
- // write updated entry
- str x1, [x0]
- // invalidate again to get rid of stale clean cachelines that may
- // have been filled speculatively since the last invalidate
- dmb sy
- dc ivac, x0
- // flush the TLBs
- .if \el == 1
- tlbi vmalle1
- .else
- tlbi alle\el
- .endif
- dsb sy
- // re-enable the MMU
- msr sctlr_el\el, x8
- isb
- .endm
+//VOID +//ArmReplaceLiveTranslationEntry ( +// IN UINT64 *Entry, +// IN UINT64 Value +// ) +ASM_PFX(ArmReplaceLiveTranslationEntry):
- // disable interrupts
- mrs x2, daif
- msr daifset, #0xf
- isb
- // clean and invalidate first so that we don't clobber
- // adjacent entries that are dirty in the caches
- dc civac, x0
- dsb ish
- EL1_OR_EL2_OR_EL3(x3)
+1:__replace_entry 1
- b 4f
+2:__replace_entry 2
- b 4f
+3:__replace_entry 3
+4:msr daif, x2
- ret
+ASM_PFX(ArmReplaceLiveTranslationEntrySize):
- .long . - ArmReplaceLiveTranslationEntry
ASM_FUNCTION_REMOVE_IF_UNREFERENCED
On Fri, Apr 22, 2016 at 11:39:48AM +0200, Ard Biesheuvel wrote:
On 22 April 2016 at 11:32, Mark Rutland mark.rutland@arm.com wrote:
On Fri, Apr 22, 2016 at 05:13:08PM +0800, Heyi Guo wrote:
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below:
- We have a PEIM, which invokes HobLib -> PeiServicesLib ->
PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I take it by 'flush' you mean the Clean+Invalidate (i.e. the DC CIVAC).
That should only cause a writeback if data is dirty, and should be a NOP otherwise, so if this is causing problems then there is a latent issue regardless.
Dirty data can be naturally evicted from the cache at any time, for any reason, regardless of explicit cache maintenance.
Assuming that this code executes with the MMU off, it probably means there is junk in the caches?
Perhaps.
That itself is a major/worse problem; platform-specific initialisation code on the secure side must initialise (i.e. invalidate) caches out-of-reset if they reset to garbage, or the caches are effectively unusable even ignoring issues from natural evictions.
Thanks, Mark.
On 04/22/2016 06:03 PM, Mark Rutland wrote:
On Fri, Apr 22, 2016 at 11:39:48AM +0200, Ard Biesheuvel wrote:
On 22 April 2016 at 11:32, Mark Rutland mark.rutland@arm.com wrote:
On Fri, Apr 22, 2016 at 05:13:08PM +0800, Heyi Guo wrote:
Hi Ard,
I actually found this patch caused SERR on D02, for the library constructor it is using.
The function AArch64LibConstructor will be called in every module if it directly or indirectly invokes ArmLib.
Our case is like below:
- We have a PEIM, which invokes HobLib -> PeiServicesLib ->
PeiServicesTablePointerLib -> ArmLib. 2. So it will integrate this constructor and flush the code at the beginning of module entry. 3. For the code is running on Flash, we actually flush the flash area, and then a SERR occurs.
I take it by 'flush' you mean the Clean+Invalidate (i.e. the DC CIVAC).
That should only cause a writeback if data is dirty, and should be a NOP otherwise, so if this is causing problems then there is a latent issue regardless.
Dirty data can be naturally evicted from the cache at any time, for any reason, regardless of explicit cache maintenance.
Assuming that this code executes with the MMU off, it probably means there is junk in the caches?
Perhaps.
That itself is a major/worse problem; platform-specific initialisation code on the secure side must initialise (i.e. invalidate) caches out-of-reset if they reset to garbage, or the caches are effectively unusable even ignoring issues from natural evictions.
So I don't think there will be junk in the caches, or else we would have had problems before. I will check with our silicon developers to find out the reason.
Thanks and Regards.
Heyi
Thanks, Mark.
-
Ard Biesheuvel -
Heyi Guo -
Mark Rutland