* [PATCH 1/4] ArmPkg: Apply Uncrustify to Non-Compliant Files
2023-06-29 16:17 [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency Taylor Beebe
@ 2023-06-29 16:17 ` Taylor Beebe
2023-06-29 16:17 ` [PATCH 2/4] ArmPkg: Update GetMemoryRegion() to Handle No mapping Taylor Beebe
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Taylor Beebe @ 2023-06-29 16:17 UTC (permalink / raw)
To: devel; +Cc: Taylor Beebe, Leif Lindholm, Ard Biesheuvel, Taylor Beebe
From: Taylor Beebe <tabeebe@microsoft.com>
This patch applies Uncrustify to the following files:
ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
ArmPkg/Include/IndustryStandard/ArmStdSmc.h
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
---
ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c | 6 +++---
ArmPkg/Include/IndustryStandard/ArmStdSmc.h | 8 ++++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
index 5dbe99fc3134..ccb182668d6a 100644
--- a/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
+++ b/ArmPkg/Drivers/MmCommunicationPei/MmCommunicationPei.c
@@ -159,8 +159,8 @@ MmCommunicationPeim (
}
CopyMem (CommBuffer, CommunicateHeader, BufferSize);
- *CommSize = BufferSize;
- Status = EFI_SUCCESS;
+ *CommSize = BufferSize;
+ Status = EFI_SUCCESS;
break;
case ARM_SMC_MM_RET_INVALID_PARAMS:
@@ -197,7 +197,7 @@ STATIC CONST EFI_PEI_MM_COMMUNICATION_PPI mPeiMmCommunication = {
STATIC CONST EFI_PEI_PPI_DESCRIPTOR mPeiMmCommunicationPpi = {
(EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
&gEfiPeiMmCommunicationPpiGuid,
- (VOID*)&mPeiMmCommunication
+ (VOID *)&mPeiMmCommunication
};
/**
diff --git a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
index 616c650d07c8..f3d78d8e789b 100644
--- a/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmStdSmc.h
@@ -248,9 +248,9 @@
* SMC64 SiP Service Calls
*/
-#define SMC_FASTCALL 0x80000000
-#define SMC64_FUNCTION (SMC_FASTCALL | 0x40000000)
-#define SMC_SIP_FUNCTION (SMC64_FUNCTION | 0x02000000)
-#define SMC_SIP_FUNCTION_ID(n) (SMC_SIP_FUNCTION | (n))
+#define SMC_FASTCALL 0x80000000
+#define SMC64_FUNCTION (SMC_FASTCALL | 0x40000000)
+#define SMC_SIP_FUNCTION (SMC64_FUNCTION | 0x02000000)
+#define SMC_SIP_FUNCTION_ID(n) (SMC_SIP_FUNCTION | (n))
#endif // ARM_STD_SMC_H_
--
2.41.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/4] ArmPkg: Update GetMemoryRegion() to Handle No mapping
2023-06-29 16:17 [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency Taylor Beebe
2023-06-29 16:17 ` [PATCH 1/4] ArmPkg: Apply Uncrustify to Non-Compliant Files Taylor Beebe
@ 2023-06-29 16:17 ` Taylor Beebe
2023-06-29 16:17 ` [PATCH 3/4] ArmPkg: Fix Unsafe ASSERTs in MMU Logic Taylor Beebe
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Taylor Beebe @ 2023-06-29 16:17 UTC (permalink / raw)
To: devel; +Cc: Taylor Beebe, Leif Lindholm, Ard Biesheuvel, Taylor Beebe
From: Taylor Beebe <tabeebe@microsoft.com>
This patch updates the GetMemoryRegion() function to handle the case
where there is no mapping for the requested address.
The original logic for the ARM would hit an ASSERT after
GetMemoryRegionPage() returned EFI_SUCCESS but did not update The
RegionLength parameter.
The original logic for the AARCH64 would never initialize the
RegionLength parameter to zero and return EFI_SUCCESS after
traversing an unknown number of pages.
To fix this, the logic for both architecture has updated to
return EFI_NO_MAPPING if the BaseAddress being checked is unmapped.
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 30 +++++++------
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 65 +++++++++++++++++++----------
2 files changed, 60 insertions(+), 35 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 1d02e41e18d8..0d3bc2809682 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -380,10 +380,10 @@ GetMemoryRegionRec (
RegionAttributes
);
- // In case of 'Success', it means the end of the block region has been found into the upper
- // level translation table
- if (!EFI_ERROR (Status)) {
- return EFI_SUCCESS;
+ // EFI_SUCCESS: The end of the end of the region was found.
+ // EFI_NO_MAPPING: The translation entry associated with BaseAddress is invalid.
+ if (Status != EFI_NOT_FOUND) {
+ return Status;
}
// Now we processed the table move to the next entry
@@ -395,12 +395,13 @@ GetMemoryRegionRec (
*RegionLength = 0;
*RegionAttributes = *BlockEntry & TT_ATTRIBUTES_MASK;
} else {
- // We have an 'Invalid' entry
- return EFI_UNSUPPORTED;
+ return EFI_NO_MAPPING;
}
while (BlockEntry <= LastBlockEntry) {
- if ((*BlockEntry & TT_ATTRIBUTES_MASK) == *RegionAttributes) {
+ if (((*BlockEntry & TT_TYPE_MASK) == BlockEntryType) &&
+ ((*BlockEntry & TT_ATTRIBUTES_MASK) == *RegionAttributes))
+ {
*RegionLength = *RegionLength + TT_BLOCK_ENTRY_SIZE_AT_LEVEL (TableLevel);
} else {
// In case we have found the end of the region we return success
@@ -412,7 +413,7 @@ GetMemoryRegionRec (
// If we have reached the end of the TranslationTable and we have not found the end of the region then
// we return EFI_NOT_FOUND.
- // The caller will continue to look for the memory region at its level
+ // The caller will continue to look for the memory region at its level.
return EFI_NOT_FOUND;
}
@@ -433,6 +434,11 @@ GetMemoryRegion (
TranslationTable = ArmGetTTBR0BaseAddress ();
+ // Initialize the output parameters. These paramaters are only valid if the
+ // result is EFI_SUCCESS.
+ *RegionLength = 0;
+ *RegionAttributes = 0;
+
T0SZ = ArmGetTCR () & TCR_T0SZ_MASK;
// Get the Table info from T0SZ
GetRootTranslationTableInfo (T0SZ, &TableLevel, &EntryCount);
@@ -447,10 +453,10 @@ GetMemoryRegion (
);
// If the region continues up to the end of the root table then GetMemoryRegionRec()
- // will return EFI_NOT_FOUND
- if (Status == EFI_NOT_FOUND) {
+ // will return EFI_NOT_FOUND. Check if the region length was updated.
+ if ((Status == EFI_NOT_FOUND) && (*RegionLength > 0)) {
return EFI_SUCCESS;
- } else {
- return Status;
}
+
+ return Status;
}
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index afd6aab60204..268c0bf3f9ae 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -427,17 +427,20 @@ EfiAttributeToArmAttribute (
EFI_STATUS
GetMemoryRegionPage (
IN UINT32 *PageTable,
- IN OUT UINTN *BaseAddress,
- OUT UINTN *RegionLength,
- OUT UINTN *RegionAttributes
+ IN UINTN *BaseAddress,
+ IN UINTN *RegionAttributes,
+ OUT UINTN *RegionLength
)
{
- UINT32 PageAttributes;
- UINT32 TableIndex;
- UINT32 PageDescriptor;
+ UINT32 PageAttributes;
+ UINT32 TableIndex;
+ UINT32 PageDescriptor;
+ EFI_STATUS Status;
// Convert the section attributes into page attributes
PageAttributes = ConvertSectionAttributesToPageAttributes (*RegionAttributes);
+ Status = EFI_NOT_FOUND;
+ *RegionLength = 0;
// Calculate index into first level translation table for start of modification
TableIndex = ((*BaseAddress) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
@@ -449,23 +452,24 @@ GetMemoryRegionPage (
PageDescriptor = PageTable[TableIndex];
if ((PageDescriptor & TT_DESCRIPTOR_PAGE_TYPE_MASK) == TT_DESCRIPTOR_PAGE_TYPE_FAULT) {
- // Case: End of the boundary of the region
- return EFI_SUCCESS;
+ Status = (*RegionLength > 0) ? EFI_SUCCESS : EFI_NO_MAPPING;
+ break;
} else if ((PageDescriptor & TT_DESCRIPTOR_PAGE_TYPE_PAGE) == TT_DESCRIPTOR_PAGE_TYPE_PAGE) {
- if ((PageDescriptor & TT_DESCRIPTOR_PAGE_ATTRIBUTE_MASK) == PageAttributes) {
- *RegionLength = *RegionLength + TT_DESCRIPTOR_PAGE_SIZE;
- } else {
- // Case: End of the boundary of the region
- return EFI_SUCCESS;
+ if ((PageDescriptor & TT_DESCRIPTOR_PAGE_ATTRIBUTE_MASK) != PageAttributes) {
+ Status = EFI_SUCCESS;
+ break;
}
+
+ *RegionLength += TT_DESCRIPTOR_PAGE_SIZE;
} else {
- // We do not support Large Page yet. We return EFI_SUCCESS that means end of the region.
+ // Large pages are unsupported.
+ Status = EFI_UNSUPPORTED;
ASSERT (0);
- return EFI_SUCCESS;
+ break;
}
}
- return EFI_NOT_FOUND;
+ return Status;
}
EFI_STATUS
@@ -482,6 +486,7 @@ GetMemoryRegion (
UINT32 SectionDescriptor;
ARM_FIRST_LEVEL_DESCRIPTOR *FirstLevelTable;
UINT32 *PageTable;
+ UINTN Length;
// Initialize the arguments
*RegionLength = 0;
@@ -491,7 +496,11 @@ GetMemoryRegion (
// Calculate index into first level translation table for start of modification
TableIndex = TT_DESCRIPTOR_SECTION_BASE_ADDRESS (*BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
- ASSERT (TableIndex < TRANSLATION_TABLE_SECTION_COUNT);
+
+ if (TableIndex >= TRANSLATION_TABLE_SECTION_COUNT) {
+ ASSERT (TableIndex < TRANSLATION_TABLE_SECTION_COUNT);
+ return EFI_INVALID_PARAMETER;
+ }
// Get the section at the given index
SectionDescriptor = FirstLevelTable[TableIndex];
@@ -524,6 +533,8 @@ GetMemoryRegion (
TT_DESCRIPTOR_CONVERT_TO_SECTION_AP (PageAttributes);
}
+ Status = EFI_NOT_FOUND;
+
for ( ; TableIndex < TRANSLATION_TABLE_SECTION_COUNT; TableIndex++) {
// Get the section at the given index
SectionDescriptor = FirstLevelTable[TableIndex];
@@ -532,15 +543,18 @@ GetMemoryRegion (
if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE (SectionDescriptor)) {
// Extract the page table location from the descriptor
PageTable = (UINT32 *)(SectionDescriptor & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK);
+ Length = 0;
// Scan the page table to find the end of the region.
- Status = GetMemoryRegionPage (PageTable, BaseAddress, RegionLength, RegionAttributes);
- ASSERT (*RegionLength > 0);
+ Status = GetMemoryRegionPage (PageTable, BaseAddress, RegionAttributes, &Length);
+ *RegionLength += Length;
- // If we have found the end of the region (Status == EFI_SUCCESS) then we exit the for-loop
- if (Status == EFI_SUCCESS) {
- break;
+ // Status == EFI_NOT_FOUND implies we have not reached the end of the region.
+ if ((Status == EFI_NOT_FOUND) && (Length > 0)) {
+ continue;
}
+
+ break;
} else if (((SectionDescriptor & TT_DESCRIPTOR_SECTION_TYPE_MASK) == TT_DESCRIPTOR_SECTION_TYPE_SECTION) ||
((SectionDescriptor & TT_DESCRIPTOR_SECTION_TYPE_MASK) == TT_DESCRIPTOR_SECTION_TYPE_SUPERSECTION))
{
@@ -556,5 +570,10 @@ GetMemoryRegion (
}
}
- return EFI_SUCCESS;
+ // Check if the region length was updated.
+ if (*RegionLength > 0) {
+ Status = EFI_SUCCESS;
+ }
+
+ return Status;
}
--
2.41.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/4] ArmPkg: Fix Unsafe ASSERTs in MMU Logic
2023-06-29 16:17 [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency Taylor Beebe
2023-06-29 16:17 ` [PATCH 1/4] ArmPkg: Apply Uncrustify to Non-Compliant Files Taylor Beebe
2023-06-29 16:17 ` [PATCH 2/4] ArmPkg: Update GetMemoryRegion() to Handle No mapping Taylor Beebe
@ 2023-06-29 16:17 ` Taylor Beebe
2023-06-29 16:17 ` [PATCH 4/4] ArmPkg: Add Function Headers to " Taylor Beebe
2023-07-03 14:17 ` [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency Ard Biesheuvel
4 siblings, 0 replies; 6+ messages in thread
From: Taylor Beebe @ 2023-06-29 16:17 UTC (permalink / raw)
To: devel; +Cc: Taylor Beebe, Leif Lindholm, Ard Biesheuvel, Taylor Beebe
From: Taylor Beebe <tabeebe@microsoft.com>
There are ASSERTs present in the MMU logic to ensure various
functions return successfully, but these ASSERTs may be ignored
on release builds causing unsafe behavior. This patch updates
the logic to handle unexpected return values and branch safely.
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 21 ++++++++++++-----
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 36 ++++++++++++++++++++++++-----
2 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 0d3bc2809682..d9d386dbed6b 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -148,10 +148,12 @@ GetNextEntryAttribute (
// Get the memory space map from GCD
MemorySpaceMap = NULL;
Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap);
- ASSERT_EFI_ERROR (Status);
- // We cannot get more than 3-level page table
- ASSERT (TableLevel <= 3);
+ if (EFI_ERROR (Status) || (TableLevel > 3)) {
+ ASSERT_EFI_ERROR (Status);
+ ASSERT (TableLevel <= 3);
+ return 0;
+ }
// While the top level table might not contain TT_ENTRY_COUNT entries;
// the subsequent ones should be filled up
@@ -243,7 +245,11 @@ SyncCacheConfig (
//
MemorySpaceMap = NULL;
Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap);
- ASSERT_EFI_ERROR (Status);
+
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ return Status;
+ }
// The GCD implementation maintains its own copy of the state of memory space attributes. GCD needs
// to know what the initial memory space attributes are. The CPU Arch. Protocol does not provide a
@@ -277,7 +283,7 @@ SyncCacheConfig (
);
// Update GCD with the last region if valid
- if (PageAttribute != INVALID_ENTRY) {
+ if ((PageAttribute != INVALID_ENTRY) && (EndAddressGcdRegion > BaseAddressGcdRegion)) {
SetGcdMemorySpaceAttributes (
MemorySpaceMap,
NumberOfDescriptors,
@@ -430,7 +436,10 @@ GetMemoryRegion (
UINTN EntryCount;
UINTN T0SZ;
- ASSERT ((BaseAddress != NULL) && (RegionLength != NULL) && (RegionAttributes != NULL));
+ if ((BaseAddress == NULL) || (RegionLength == NULL) || (RegionAttributes == NULL)) {
+ ASSERT ((BaseAddress != NULL) && (RegionLength != NULL) && (RegionAttributes != NULL));
+ return EFI_INVALID_PARAMETER;
+ }
TranslationTable = ArmGetTTBR0BaseAddress ();
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 268c0bf3f9ae..5a2f36d06086 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -217,7 +217,10 @@ SyncCacheConfigPage (
} else if (PageAttributes != NextPageAttributes) {
// Convert Section Attributes into GCD Attributes
Status = PageToGcdAttributes (NextPageAttributes, &GcdAttributes);
- ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ GcdAttributes = 0;
+ }
// update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK)
SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, *NextRegionBase, *NextRegionLength, GcdAttributes);
@@ -230,7 +233,10 @@ SyncCacheConfigPage (
} else if (NextPageAttributes != 0) {
// Convert Page Attributes into GCD Attributes
Status = PageToGcdAttributes (NextPageAttributes, &GcdAttributes);
- ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ ASSERT_EFI_ERROR (Status);
+ GcdAttributes = 0;
+ }
// update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK)
SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, *NextRegionBase, *NextRegionLength, GcdAttributes);
@@ -278,7 +284,12 @@ SyncCacheConfig (
//
MemorySpaceMap = NULL;
Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemorySpaceMap);
- ASSERT_EFI_ERROR (Status);
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "SyncCacheConfig - GetMemorySpaceMap() failed! Status: %r\n", Status));
+ ASSERT_EFI_ERROR (Status);
+ return Status;
+ }
// The GCD implementation maintains its own copy of the state of memory space attributes. GCD needs
// to know what the initial memory space attributes are. The CPU Arch. Protocol does not provide a
@@ -307,7 +318,12 @@ SyncCacheConfig (
} else if (SectionAttributes != NextSectionAttributes) {
// Convert Section Attributes into GCD Attributes
Status = SectionToGcdAttributes (NextSectionAttributes, &GcdAttributes);
- ASSERT_EFI_ERROR (Status);
+
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes() failed! Status: %r\n", Status));
+ ASSERT_EFI_ERROR (Status);
+ GcdAttributes = 0;
+ }
// update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK)
SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, NextRegionBase, NextRegionLength, GcdAttributes);
@@ -343,7 +359,11 @@ SyncCacheConfig (
if (NextSectionAttributes != 0) {
// Convert Section Attributes into GCD Attributes
Status = SectionToGcdAttributes (NextSectionAttributes, &GcdAttributes);
- ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes() failed! Status: %r\n", Status));
+ ASSERT_EFI_ERROR (Status);
+ GcdAttributes = 0;
+ }
// update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK)
SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, NextRegionBase, NextRegionLength, GcdAttributes);
@@ -360,7 +380,11 @@ SyncCacheConfig (
if (NextSectionAttributes != 0) {
// Convert Section Attributes into GCD Attributes
Status = SectionToGcdAttributes (NextSectionAttributes, &GcdAttributes);
- ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes() failed! Status: %r\n", Status));
+ ASSERT_EFI_ERROR (Status);
+ GcdAttributes = 0;
+ }
// update GCD with these changes (this will recurse into our own CpuSetMemoryAttributes below which is OK)
SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, NextRegionBase, NextRegionLength, GcdAttributes);
--
2.41.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/4] ArmPkg: Add Function Headers to MMU Logic
2023-06-29 16:17 [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency Taylor Beebe
` (2 preceding siblings ...)
2023-06-29 16:17 ` [PATCH 3/4] ArmPkg: Fix Unsafe ASSERTs in MMU Logic Taylor Beebe
@ 2023-06-29 16:17 ` Taylor Beebe
2023-07-03 14:17 ` [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency Ard Biesheuvel
4 siblings, 0 replies; 6+ messages in thread
From: Taylor Beebe @ 2023-06-29 16:17 UTC (permalink / raw)
To: devel; +Cc: Taylor Beebe, Leif Lindholm, Ard Biesheuvel, Taylor Beebe
From: Taylor Beebe <tabeebe@microsoft.com>
Much of the MMU logic was written without function headers. This patch
adds function headers where absent and updates function headers which
do not match the EDK2 standard.
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 92 ++++++++++++++++++++++++++++-
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 86 ++++++++++++++++++++++++---
2 files changed, 169 insertions(+), 9 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index d9d386dbed6b..e14eb47ce4c6 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -18,6 +18,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define MIN_T0SZ 16
#define BITS_PER_LEVEL 9
+/**
+ Parses T0SZ to determine the level and number of entries at the root
+ of the translation table.
+
+ @param T0SZ The T0SZ value to be parsed.
+ @param RootTableLevel The level of the root table.
+ @param RootTableEntryCount The number of entries in the root table.
+**/
STATIC
VOID
GetRootTranslationTableInfo (
@@ -30,6 +38,13 @@ GetRootTranslationTableInfo (
*RootTableEntryCount = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
}
+/**
+ Converts ARM translation table attributes to GCD attributes.
+
+ @param PageAttributes The translation table attributes to be converted.
+
+ @retval The analogous GCD attributes.
+**/
STATIC
UINT64
PageAttributeToGcdAttribute (
@@ -100,6 +115,14 @@ RegionAttributeToGcdAttribute (
return PageAttributeToGcdAttribute (PageAttributes);
}
+/**
+ Retrieves the attribute of the first page entry in the translation table.
+
+ @param[in] FirstLevelTableAddress The base address of the translation table.
+ @param[in] TableLevel The current level being traversed.
+
+ @retval The attributes of the first page entry found, or INVALID_ENTRY.
+**/
STATIC
UINT64
GetFirstPageAttribute (
@@ -126,6 +149,19 @@ GetFirstPageAttribute (
}
}
+/**
+ This function recursively traverses the translation table heirarchy to
+ synchronise the GCD with the translation table.
+
+ @param[in] TableAddress The address of the table being processed.
+ @param[in] EntryCount The number of entries in the current level of the table.
+ @param[in] TableLevel The current level of the memory table being processed.
+ @param[in] BaseAddress The starting address of the region.
+ @param[in, out] PrevEntryAttribute The attributes of the previous region.
+ @param[in, out] StartGcdRegion The start of the GCD region.
+
+ @retval The address at the end of the last region processed.
+**/
STATIC
UINT64
GetNextEntryAttribute (
@@ -220,6 +256,15 @@ GetNextEntryAttribute (
return BaseAddress + (EntryCount * TT_ADDRESS_AT_LEVEL (TableLevel));
}
+/**
+ Sync the GCD memory space attributes with the translation table.
+
+ @param[in] CpuProtocol The CPU architectural protocol instance.
+
+ @retval EFI_SUCCESS The GCD memory space attributes are synced with
+ the MMU page table.
+ @retval Others The return value of GetMemorySpaceMap().
+**/
EFI_STATUS
SyncCacheConfig (
IN EFI_CPU_ARCH_PROTOCOL *CpuProtocol
@@ -298,6 +343,13 @@ SyncCacheConfig (
return EFI_SUCCESS;
}
+/**
+ Convert EFI memory attributes to ARM translation table attributes.
+
+ @param[in] EfiAttributes EFI memory attributes.
+
+ @retval The analogous translation table attributes.
+**/
UINT64
EfiAttributeToArmAttribute (
IN UINT64 EfiAttributes
@@ -345,8 +397,25 @@ EfiAttributeToArmAttribute (
return ArmAttributes;
}
-// This function will recursively go down the page table to find the first block address linked to 'BaseAddress'.
-// And then the function will identify the size of the region that has the same page table attribute.
+/**
+ This function returns the attributes of the memory region containing the
+ specified address.
+
+ RegionLength and RegionAttributes are only valid if the result is EFI_SUCCESS.
+
+ @param[in] TranslationTable The translation table base address.
+ @param[in] TableLevel The level of the translation table.
+ @param[in] LastBlockEntry The last block address of the table level.
+ @param[in, out] BaseAddress The base address of the memory region.
+ @param[out] RegionLength The length of the memory region.
+ @param[out] RegionAttributes The attributes of the memory region.
+
+ @retval EFI_SUCCESS The attributes of the memory region were
+ returned successfully.
+ @retval EFI_NOT_FOUND The memory region was not found.
+ @retval EFI_NO_MAPPING The translation table entry associated with
+ BaseAddress is invalid.
+**/
EFI_STATUS
GetMemoryRegionRec (
IN UINT64 *TranslationTable,
@@ -423,6 +492,25 @@ GetMemoryRegionRec (
return EFI_NOT_FOUND;
}
+/**
+ Retrieves a memory region from a given base address.
+
+ This function retrieves a memory region starting from a given base address.
+
+ @param[in, out] BaseAddress The base address from which to retrieve
+ the memory region. On successful return, this is
+ updated to the end address of the retrieved region.
+ @param[out] RegionLength The length of the retrieved memory region.
+ @param[out] RegionAttributes The attributes of the retrieved memory region.
+
+ @retval EFI_STATUS Returns EFI_SUCCESS if the memory region is
+ retrieved successfully, or the status of the
+ recursive call to GetMemoryRegionRec.
+ @retval EFI_NOT_FOUND The memory region was not found.
+ @retval EFI_NO_MAPPING The translation table entry associated with
+ BaseAddress is invalid.
+ @retval EFI_INVALID_PARAMETER One of the input parameters was NULL.
+**/
EFI_STATUS
GetMemoryRegion (
IN OUT UINTN *BaseAddress,
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 5a2f36d06086..8c4de284e160 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -17,9 +17,12 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
Convert a set of ARM short descriptor section attributes into a mask
of EFI_MEMORY_xx constants.
- @param SectionAttributes The set of page attributes.
- @param GcdAttributes Pointer to the return value.
+ @param[in] SectionAttributes The set of page attributes.
+ @param[out] GcdAttributes Pointer to the return value.
+ @retval EFI_SUCCESS The attributes were converted successfully.
+ @retval EFI_UNSUPPORTED The section attributes did not have a
+ GCD transation.
**/
STATIC
EFI_STATUS
@@ -87,10 +90,11 @@ SectionToGcdAttributes (
Convert an arch specific set of page attributes into a mask
of EFI_MEMORY_xx constants.
- @param PageAttributes The set of page attributes.
-
- @retval The mask of EFI_MEMORY_xx constants.
+ @param[in] PageAttributes The set of page attributes.
+ @retval EFI_SUCCESS The attributes were converted successfully.
+ @retval EFI_UNSUPPORTED The section attributes did not have a
+ GCD transation.
**/
UINT64
RegionAttributeToGcdAttribute (
@@ -107,9 +111,11 @@ RegionAttributeToGcdAttribute (
Convert a set of ARM short descriptor page attributes into a mask
of EFI_MEMORY_xx constants.
- @param PageAttributes The set of page attributes.
- @param GcdAttributes Pointer to the return value.
+ @param[in] PageAttributes The set of page attributes.
+ @param[out] GcdAttributes Pointer to the return value.
+ @retval EFI_SUCCESS The attributes were converted successfully.
+ @retval EFI_UNSUPPORTED The page attributes did not have a GCD transation.
**/
STATIC
EFI_STATUS
@@ -173,6 +179,23 @@ PageToGcdAttributes (
return EFI_SUCCESS;
}
+/**
+ Synchronizes the GCD with the translation table for a specified page.
+
+ This function synchronizes cache configuration for a given page based on its section index
+ and the first level descriptor. It traverses the second level table entries of the page and
+ updates the GCD attributes accordingly for each entry.
+
+ @param[in] SectionIndex The index of the section where the page resides.
+ @param[in] FirstLevelDescriptor The first translation table level of the page.
+ @param[in] NumberOfDescriptors The number of descriptors in the GCD memory space map.
+ @param[in] MemorySpaceMap The GCD memory space descriptor.
+ @param[in, out] NextRegionBase The next region base address.
+ @param[in, out] NextRegionLength The next region length.
+ @param[in, out] NextSectionAttributes The next section attributes.
+
+ @retval EFI_STATUS Always return success
+**/
EFI_STATUS
SyncCacheConfigPage (
IN UINT32 SectionIndex,
@@ -258,6 +281,14 @@ SyncCacheConfigPage (
return EFI_SUCCESS;
}
+/**
+ Sync the GCD memory space attributes with the translation table.
+
+ @param[in] CpuProtocol The CPU architectural protocol instance.
+
+ @retval EFI_SUCCESS The GCD memory space attributes are synced with the MMU page table.
+ @retval Others The return value of GetMemorySpaceMap().
+**/
EFI_STATUS
SyncCacheConfig (
IN EFI_CPU_ARCH_PROTOCOL *CpuProtocol
@@ -395,6 +426,13 @@ SyncCacheConfig (
return EFI_SUCCESS;
}
+/**
+ Convert EFI memory attributes to ARM translation table attributes.
+
+ @param[in] EfiAttributes EFI memory attributes.
+
+ @retval The analogous translation table attributes.
+**/
UINT64
EfiAttributeToArmAttribute (
IN UINT64 EfiAttributes
@@ -448,6 +486,22 @@ EfiAttributeToArmAttribute (
return ArmAttributes;
}
+/**
+ This function finds the end of a memory region in a translation table. A
+ memory region is defined as a contiguous set of pages with the same attributes.
+
+ @param[in] PageTable The translation table to traverse.
+ @param[in] BaseAddress The address from which to start the search
+ @param[in] RegionAttributes The attributes of the start of the region.
+ @param[out] RegionLength The length of the region found.
+
+ @retval EFI_SUCCESS The region was found.
+ @retval EFI_NOT_FOUND The end of the region was not found.
+ @retval EFI_NO_MAPPING The region specified by BaseAddress is not mapped
+ in the input translation table.
+ @retval EFI_UNSUPPORTED Large pages are not supported.
+**/
+STATIC
EFI_STATUS
GetMemoryRegionPage (
IN UINT32 *PageTable,
@@ -496,6 +550,24 @@ GetMemoryRegionPage (
return Status;
}
+/**
+ Get the memory region that contains the specified address. A memory region is defined
+ as a contiguous set of pages with the same attributes.
+
+ RegionLength and RegionAttributes are only valid if EFI_SUCCESS is returned.
+
+ @param[in, out] BaseAddress On input, the address to search for.
+ On output, the base address of the region found.
+ @param[out] RegionLength The length of the region found.
+ @param[out] RegionAttributes The attributes of the region found.
+
+ @retval EFI_SUCCESS Region found
+ @retval EFI_NOT_FOUND Region not found
+ @retval EFI_UNSUPPORTED Large pages are unsupported
+ @retval EFI_NO_MAPPING The page specified by BaseAddress is unmapped
+ @retval EFI_INVALID_PARAMETER The BaseAddress exceeds the addressable range of
+ the translation table.
+**/
EFI_STATUS
GetMemoryRegion (
IN OUT UINTN *BaseAddress,
--
2.41.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency
2023-06-29 16:17 [PATCH 0/4] Update CpuDxe MMU Logic to Improve Consistency Taylor Beebe
` (3 preceding siblings ...)
2023-06-29 16:17 ` [PATCH 4/4] ArmPkg: Add Function Headers to " Taylor Beebe
@ 2023-07-03 14:17 ` Ard Biesheuvel
4 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2023-07-03 14:17 UTC (permalink / raw)
To: Taylor Beebe; +Cc: devel, Leif Lindholm, Ard Biesheuvel
On Thu, 29 Jun 2023 at 18:20, Taylor Beebe <t@taylorbeebe.com> wrote:
>
> This patch series:
>
> 1. Updates applies uncrustify to noncompliant files
> 2. Updates GetMemoryRegion() to handle the case where
> BaseAddress is an unmapped page, and update some other
> return values to be more consistent.
> 3. Adds some branching paths to what were previously only
> ASSERT statements to avoid dereferencing NULL and producing
> non-deterministic behavior when ASSERTs are disabled.
> 4. Adds function headers to the MMU logic documenting the
> behavior, parameters, and potetial return values.
>
> Taylor Beebe (4):
> ArmPkg: Apply Uncrustify to Non-Compliant Files
> ArmPkg: Update GetMemoryRegion() to Handle No mapping
> ArmPkg: Fix Unsafe ASSERTs in MMU Logic
> ArmPkg: Add Function Headers to MMU Logic
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>
Thanks a lot for cleaning this up.
For the series,
Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
I've submitted these to be merged,
^ permalink raw reply [flat|nested] 6+ messages in thread