* [PATCH V2 0/3] Add New Memory Attributes
@ 2020-06-23 21:55 Oleksiy Yakovlev
2020-06-23 21:55 ` [PATCH V2 1/3] MdePkg: " Oleksiy Yakovlev
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Oleksiy Yakovlev @ 2020-06-23 21:55 UTC (permalink / raw)
To: devel
Cc: liming.gao, michael.d.kinney, dandan.bi, ray.ni, lersek,
rahul1.kumar, Felixp, oleksiyy
This series of patches add usage of new memory
attributes EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO,
introduced in UEFI2.8 (mantis 1919 and 1872).
First patch fix typos in description and introduce two
bitmasks for all memory type attributes.
Second and third patches get rid of multiple memory attributes
bitmasks definitions trough multiple files and headers,
and replace them with new common definitions from MdePkg.
Oleksiy Yakovlev (3):
MdePkg: Add New Memory Attributes
MdeModulePkg: Add New Memory Attributes
UefiCpuPkg: Add New Memory Attributes
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 11 ++---------
MdeModulePkg/Core/Dxe/Mem/Page.c | 9 +++------
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 7 ++-----
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 10 ++--------
MdePkg/Include/Uefi/UefiSpec.h | 10 ++++++++--
UefiCpuPkg/CpuDxe/CpuDxe.c | 11 ++++-------
UefiCpuPkg/CpuDxe/CpuDxe.h | 12 ------------
UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +-
9 files changed, 25 insertions(+), 53 deletions(-)
--
2.9.0.windows.1
Please consider the environment before printing this email.
The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2 1/3] MdePkg: Add New Memory Attributes
2020-06-23 21:55 [PATCH V2 0/3] Add New Memory Attributes Oleksiy Yakovlev
@ 2020-06-23 21:55 ` Oleksiy Yakovlev
2020-06-24 2:33 ` [edk2-devel] " Zhiguang Liu
2020-06-24 9:27 ` Laszlo Ersek
2020-06-23 21:55 ` [PATCH V2 2/3] MdeModulePkg: " Oleksiy Yakovlev
2020-06-23 21:55 ` [PATCH V2 3/3] UefiCpuPkg: " Oleksiy Yakovlev
2 siblings, 2 replies; 12+ messages in thread
From: Oleksiy Yakovlev @ 2020-06-23 21:55 UTC (permalink / raw)
To: devel
Cc: liming.gao, michael.d.kinney, dandan.bi, ray.ni, lersek,
rahul1.kumar, Felixp, oleksiyy
Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
attributes introduced in UEFI 2.8.
(UEFI 2.8, mantis 1919 and 1872).
Fix typos in EFI_MEMORY_CPU_CRYPTO description.
Add attributes bitmasks, grouped by type.
Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
---
MdePkg/Include/Uefi/UefiSpec.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
index 558e1bc..05b82e0 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -96,9 +96,9 @@ typedef enum {
#define EFI_MEMORY_SP 0x0000000000040000ULL
//
// If this flag is set, the memory region is capable of being
-// protected with the CPU?s memory cryptographic
+// protected with the CPU's memory cryptographic
// capabilities. If this flag is clear, the memory region is not
-// capable of being protected with the CPU?s memory
+// capable of being protected with the CPU's memory
// cryptographic capabilities or the CPU does not support CPU
// memory cryptographic capabilities.
//
@@ -109,6 +109,12 @@ typedef enum {
//
#define EFI_MEMORY_RUNTIME 0x8000000000000000ULL
+//
+// Attributes bitmasks, grouped by type
+//
+#define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
+#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
+
///
/// Memory descriptor version number.
///
--
2.9.0.windows.1
Please consider the environment before printing this email.
The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH V2 2/3] MdeModulePkg: Add New Memory Attributes
2020-06-23 21:55 [PATCH V2 0/3] Add New Memory Attributes Oleksiy Yakovlev
2020-06-23 21:55 ` [PATCH V2 1/3] MdePkg: " Oleksiy Yakovlev
@ 2020-06-23 21:55 ` Oleksiy Yakovlev
2020-06-24 5:32 ` Dandan Bi
2020-06-24 9:27 ` Laszlo Ersek
2020-06-23 21:55 ` [PATCH V2 3/3] UefiCpuPkg: " Oleksiy Yakovlev
2 siblings, 2 replies; 12+ messages in thread
From: Oleksiy Yakovlev @ 2020-06-23 21:55 UTC (permalink / raw)
To: devel
Cc: liming.gao, michael.d.kinney, dandan.bi, ray.ni, lersek,
rahul1.kumar, Felixp, oleksiyy
Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
attributes introduced in UEFI 2.8.
(UEFI 2.8, mantis 1919 and 1872).
Use attributes bitmasks, defined in MdePkg.
Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 11 ++---------
MdeModulePkg/Core/Dxe/Mem/Page.c | 9 +++------
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 7 ++-----
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 10 ++--------
4 files changed, 9 insertions(+), 28 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 74f3b1b..2d8c076 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -35,13 +35,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define PRESENT_MEMORY_ATTRIBUTES (EFI_RESOURCE_ATTRIBUTE_PRESENT)
-#define EXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_UC | EFI_MEMORY_WC | \
- EFI_MEMORY_WT | EFI_MEMORY_WB | \
- EFI_MEMORY_WP | EFI_MEMORY_UCE)
-
-#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | \
- EFI_MEMORY_RO)
-
//
// Module Variables
//
@@ -665,7 +658,7 @@ ConverToCpuArchAttributes (
{
UINT64 CpuArchAttributes;
- CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
+ CpuArchAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
CpuArchAttributes |= EFI_MEMORY_UC;
@@ -951,7 +944,7 @@ CoreConvertSpace (
// Keep original CPU arch attributes when caller just calls
// SetMemorySpaceAttributes() with none CPU arch attributes (for example, RUNTIME).
//
- Attributes |= (Entry->Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES | NONEXCLUSIVE_MEMORY_ATTRIBUTES));
+ Attributes |= (Entry->Attributes & (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
}
Entry->Attributes = Attributes;
break;
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 1f0e3d9..2c2c9cd 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1857,8 +1857,7 @@ CoreGetMemoryMap (
MemoryMap->VirtualStart = 0;
MemoryMap->NumberOfPages = RShiftU64 ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1), EFI_PAGE_SHIFT);
MemoryMap->Attribute = (MergeGcdMapEntry.Attributes & ~EFI_MEMORY_PORT_IO) |
- (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP | EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
- EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB));
+ (MergeGcdMapEntry.Capabilities & (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
if (MergeGcdMapEntry.GcdMemoryType == EfiGcdMemoryTypeReserved) {
MemoryMap->Type = EfiReservedMemoryType;
@@ -1892,8 +1891,7 @@ CoreGetMemoryMap (
MemoryMap->VirtualStart = 0;
MemoryMap->NumberOfPages = RShiftU64 ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1), EFI_PAGE_SHIFT);
MemoryMap->Attribute = MergeGcdMapEntry.Attributes | EFI_MEMORY_NV |
- (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP | EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
- EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB));
+ (MergeGcdMapEntry.Capabilities & (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
MemoryMap->Type = EfiPersistentMemory;
//
@@ -1935,8 +1933,7 @@ CoreGetMemoryMap (
MemoryMapEnd = MemoryMap;
MemoryMap = MemoryMapStart;
while (MemoryMap < MemoryMapEnd) {
- MemoryMap->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
- EFI_MEMORY_XP);
+ MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK;
MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
}
MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 92a442f..7d1daf0 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -42,9 +42,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "DxeMain.h"
#include "Mem/HeapGuard.h"
-#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
-#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
-
//
// Image type definitions
//
@@ -221,7 +218,7 @@ SetUefiImageMemoryAttributes (
Status = CoreGetMemorySpaceDescriptor(BaseAddress, &Descriptor);
ASSERT_EFI_ERROR(Status);
- FinalAttributes = (Descriptor.Attributes & CACHE_ATTRIBUTE_MASK) | (Attributes & MEMORY_ATTRIBUTE_MASK);
+ FinalAttributes = (Descriptor.Attributes & EFI_CACHE_ATTRIBUTE_MASK) | (Attributes & EFI_MEMORY_ATTRIBUTE_MASK);
DEBUG ((DEBUG_INFO, "SetUefiImageMemoryAttributes - 0x%016lx - 0x%016lx (0x%016lx)\n", BaseAddress, Length, FinalAttributes));
@@ -924,7 +921,7 @@ InitializeDxeNxMemoryProtectionPolicy (
(EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)) {
Attributes = GetPermissionAttributeForMemoryType (EfiConventionalMemory) |
- (Entry->Attributes & CACHE_ATTRIBUTE_MASK);
+ (Entry->Attributes & EFI_CACHE_ATTRIBUTE_MASK);
DEBUG ((DEBUG_INFO,
"Untested GCD memory space region: - 0x%016lx - 0x%016lx (0x%016lx)\n",
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 0385f1d..599a0cd 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -39,12 +39,6 @@
#define SMRAM_CAPABILITIES (EFI_MEMORY_WB | EFI_MEMORY_UC)
-#define MEMORY_CACHE_ATTRIBUTES (EFI_MEMORY_UC | EFI_MEMORY_WC | \
- EFI_MEMORY_WT | EFI_MEMORY_WB | \
- EFI_MEMORY_WP | EFI_MEMORY_UCE)
-
-#define MEMORY_PAGE_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | EFI_MEMORY_RO)
-
//
// Function prototypes from produced protocols
//
@@ -1710,7 +1704,7 @@ SmmIplEntry (
CpuArch = NULL;
Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&CpuArch);
if (!EFI_ERROR (Status)) {
- MemDesc.Attributes &= ~(MEMORY_CACHE_ATTRIBUTES | MEMORY_PAGE_ATTRIBUTES);
+ MemDesc.Attributes &= ~(EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK);
MemDesc.Attributes |= EFI_MEMORY_WB;
Status = gDS->SetMemorySpaceAttributes (
mSmramCacheBase,
@@ -1727,7 +1721,7 @@ SmmIplEntry (
&MemDesc
);
DEBUG ((DEBUG_INFO, "SMRAM attributes: %016lx\n", MemDesc.Attributes));
- ASSERT ((MemDesc.Attributes & MEMORY_PAGE_ATTRIBUTES) == 0);
+ ASSERT ((MemDesc.Attributes & EFI_MEMORY_ATTRIBUTE_MASK) == 0);
);
}
//
--
2.9.0.windows.1
Please consider the environment before printing this email.
The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes
2020-06-23 21:55 [PATCH V2 0/3] Add New Memory Attributes Oleksiy Yakovlev
2020-06-23 21:55 ` [PATCH V2 1/3] MdePkg: " Oleksiy Yakovlev
2020-06-23 21:55 ` [PATCH V2 2/3] MdeModulePkg: " Oleksiy Yakovlev
@ 2020-06-23 21:55 ` Oleksiy Yakovlev
2020-06-24 9:42 ` Laszlo Ersek
2 siblings, 1 reply; 12+ messages in thread
From: Oleksiy Yakovlev @ 2020-06-23 21:55 UTC (permalink / raw)
To: devel
Cc: liming.gao, michael.d.kinney, dandan.bi, ray.ni, lersek,
rahul1.kumar, Felixp, oleksiyy
Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
attributes introduced in UEFI 2.8.
(UEFI 2.8, mantis 1919 and 1872).
Use attributes bitmasks, defined in MdePkg.
Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
---
UefiCpuPkg/CpuDxe/CpuDxe.c | 11 ++++-------
UefiCpuPkg/CpuDxe/CpuDxe.h | 12 ------------
UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +-
4 files changed, 8 insertions(+), 23 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index a571fc3..52cc26e 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -10,9 +10,6 @@
#include "CpuMp.h"
#include "CpuPageTable.h"
-#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
-#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
-
//
// Global Variables
//
@@ -417,8 +414,8 @@ CpuSetMemoryAttributes (
return EFI_SUCCESS;
}
- CacheAttributes = Attributes & CACHE_ATTRIBUTE_MASK;
- MemoryAttributes = Attributes & MEMORY_ATTRIBUTE_MASK;
+ CacheAttributes = Attributes & EFI_CACHE_ATTRIBUTE_MASK;
+ MemoryAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
if (Attributes != (CacheAttributes | MemoryAttributes)) {
return EFI_INVALID_PARAMETER;
@@ -677,7 +674,7 @@ SetGcdMemorySpaceAttributes (
gDS->SetMemorySpaceAttributes (
RegionStart,
RegionLength,
- (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
+ (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
);
}
@@ -754,7 +751,7 @@ RefreshMemoryAttributesFromMtrr (
gDS->SetMemorySpaceAttributes (
MemorySpaceMap[Index].BaseAddress,
MemorySpaceMap[Index].Length,
- (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) |
+ (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) |
(MemorySpaceMap[Index].Capabilities & DefaultAttributes)
);
}
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index 9299eaa..9771ec8 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -39,18 +39,6 @@
#include <Guid/IdleLoopEvent.h>
#include <Guid/VectorHandoffTable.h>
-#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | \
- EFI_MEMORY_WC | \
- EFI_MEMORY_WT | \
- EFI_MEMORY_WB | \
- EFI_MEMORY_UCE \
- )
-
-#define EFI_MEMORY_PAGETYPE_MASK (EFI_MEMORY_RP | \
- EFI_MEMORY_XP | \
- EFI_MEMORY_RO \
- )
-
#define HEAP_GUARD_NONSTOP_MODE \
((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 0a02cb3..06ee1b8 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -717,7 +717,7 @@ ConvertMemoryPageAttributes (
return RETURN_INVALID_PARAMETER;
}
- if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) {
+ if ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) != 0) {
DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", Attributes));
return EFI_UNSUPPORTED;
}
@@ -1018,9 +1018,9 @@ RefreshGcdMemoryAttributesFromPaging (
Length = MIN (PageLength, MemorySpaceLength);
if (Attributes != (MemorySpaceMap[Index].Attributes &
- EFI_MEMORY_PAGETYPE_MASK)) {
+ EFI_MEMORY_ATTRIBUTE_MASK)) {
NewAttributes = (MemorySpaceMap[Index].Attributes &
- ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
+ ~EFI_MEMORY_ATTRIBUTE_MASK) | Attributes;
Status = gDS->SetMemorySpaceAttributes (
BaseAddress,
Length,
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 9c5a92a..ebfc46a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -435,7 +435,7 @@ ConvertMemoryPageAttributes (
EFI_PHYSICAL_ADDRESS MaximumSupportMemAddress;
ASSERT (Attributes != 0);
- ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
+ ASSERT ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) == 0);
ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
ASSERT ((Length & (SIZE_4KB - 1)) == 0);
--
2.9.0.windows.1
Please consider the environment before printing this email.
The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/3] MdePkg: Add New Memory Attributes
2020-06-23 21:55 ` [PATCH V2 1/3] MdePkg: " Oleksiy Yakovlev
@ 2020-06-24 2:33 ` Zhiguang Liu
2020-06-24 9:27 ` Laszlo Ersek
1 sibling, 0 replies; 12+ messages in thread
From: Zhiguang Liu @ 2020-06-24 2:33 UTC (permalink / raw)
To: devel@edk2.groups.io, oleksiyy@ami.com
Cc: Gao, Liming, Kinney, Michael D, Bi, Dandan, Ni, Ray,
lersek@redhat.com, Kumar, Rahul1, Felixp@ami.com
Reviewed-by: Zhiguang Liu <zhiguang.liu@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Oleksiy
> Yakovlev
> Sent: Wednesday, June 24, 2020 5:56 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Ni, Ray
> <ray.ni@intel.com>; lersek@redhat.com; Kumar, Rahul1
> <rahul1.kumar@intel.com>; Felixp@ami.com; oleksiyy@ami.com
> Subject: [edk2-devel] [PATCH V2 1/3] MdePkg: Add New Memory Attributes
>
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
> attributes introduced in UEFI 2.8.
> (UEFI 2.8, mantis 1919 and 1872).
> Fix typos in EFI_MEMORY_CPU_CRYPTO description.
> Add attributes bitmasks, grouped by type.
>
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
> MdePkg/Include/Uefi/UefiSpec.h | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/MdePkg/Include/Uefi/UefiSpec.h
> b/MdePkg/Include/Uefi/UefiSpec.h
> index 558e1bc..05b82e0 100644
> --- a/MdePkg/Include/Uefi/UefiSpec.h
> +++ b/MdePkg/Include/Uefi/UefiSpec.h
> @@ -96,9 +96,9 @@ typedef enum {
> #define EFI_MEMORY_SP 0x0000000000040000ULL
> //
> // If this flag is set, the memory region is capable of being
> -// protected with the CPU?s memory cryptographic
> +// protected with the CPU's memory cryptographic
> // capabilities. If this flag is clear, the memory region is not
> -// capable of being protected with the CPU?s memory
> +// capable of being protected with the CPU's memory
> // cryptographic capabilities or the CPU does not support CPU
> // memory cryptographic capabilities.
> //
> @@ -109,6 +109,12 @@ typedef enum {
> //
> #define EFI_MEMORY_RUNTIME 0x8000000000000000ULL
>
> +//
> +// Attributes bitmasks, grouped by type
> +//
> +#define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC |
> EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB |
> EFI_MEMORY_UCE | EFI_MEMORY_WP)
> +#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP |
> EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP |
> EFI_MEMORY_CPU_CRYPTO)
> +
> ///
> /// Memory descriptor version number.
> ///
> --
> 2.9.0.windows.1
>
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 2/3] MdeModulePkg: Add New Memory Attributes
2020-06-23 21:55 ` [PATCH V2 2/3] MdeModulePkg: " Oleksiy Yakovlev
@ 2020-06-24 5:32 ` Dandan Bi
2020-06-24 9:27 ` Laszlo Ersek
1 sibling, 0 replies; 12+ messages in thread
From: Dandan Bi @ 2020-06-24 5:32 UTC (permalink / raw)
To: Oleksiy Yakovlev, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Ni, Ray, lersek@redhat.com,
Kumar, Rahul1, Felixp@ami.com
Reviewed-by: Dandan Bi <dandan.bi@intel.com>
Thanks,
Dandan
> -----Original Message-----
> From: Oleksiy Yakovlev <oleksiyy@ami.com>
> Sent: Wednesday, June 24, 2020 5:56 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Ni, Ray
> <ray.ni@intel.com>; lersek@redhat.com; Kumar, Rahul1
> <rahul1.kumar@intel.com>; Felixp@ami.com; oleksiyy@ami.com
> Subject: [PATCH V2 2/3] MdeModulePkg: Add New Memory Attributes
>
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO attributes
> introduced in UEFI 2.8.
> (UEFI 2.8, mantis 1919 and 1872).
> Use attributes bitmasks, defined in MdePkg.
>
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
> MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 11 ++---------
> MdeModulePkg/Core/Dxe/Mem/Page.c | 9 +++------
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 7 ++-----
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 10 ++--------
> 4 files changed, 9 insertions(+), 28 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 74f3b1b..2d8c076 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -35,13 +35,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #define PRESENT_MEMORY_ATTRIBUTES
> (EFI_RESOURCE_ATTRIBUTE_PRESENT)
>
> -#define EXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_UC |
> EFI_MEMORY_WC | \
> - EFI_MEMORY_WT | EFI_MEMORY_WB | \
> - EFI_MEMORY_WP | EFI_MEMORY_UCE)
> -
> -#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP |
> EFI_MEMORY_RP | \
> - EFI_MEMORY_RO)
> -
> //
> // Module Variables
> //
> @@ -665,7 +658,7 @@ ConverToCpuArchAttributes ( {
> UINT64 CpuArchAttributes;
>
> - CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
> + CpuArchAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
>
> if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
> CpuArchAttributes |= EFI_MEMORY_UC; @@ -951,7 +944,7 @@
> CoreConvertSpace (
> // Keep original CPU arch attributes when caller just calls
> // SetMemorySpaceAttributes() with none CPU arch attributes (for
> example, RUNTIME).
> //
> - Attributes |= (Entry->Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> NONEXCLUSIVE_MEMORY_ATTRIBUTES));
> + Attributes |= (Entry->Attributes & (EFI_CACHE_ATTRIBUTE_MASK |
> + EFI_MEMORY_ATTRIBUTE_MASK));
> }
> Entry->Attributes = Attributes;
> break;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 1f0e3d9..2c2c9cd 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1857,8 +1857,7 @@ CoreGetMemoryMap (
> MemoryMap->VirtualStart = 0;
> MemoryMap->NumberOfPages = RShiftU64
> ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1),
> EFI_PAGE_SHIFT);
> MemoryMap->Attribute = (MergeGcdMapEntry.Attributes &
> ~EFI_MEMORY_PORT_IO) |
> - (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP |
> EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
> - EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC
> | EFI_MEMORY_WT | EFI_MEMORY_WB));
> + (MergeGcdMapEntry.Capabilities &
> + (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
>
> if (MergeGcdMapEntry.GcdMemoryType ==
> EfiGcdMemoryTypeReserved) {
> MemoryMap->Type = EfiReservedMemoryType; @@ -1892,8 +1891,7
> @@ CoreGetMemoryMap (
> MemoryMap->VirtualStart = 0;
> MemoryMap->NumberOfPages = RShiftU64
> ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1),
> EFI_PAGE_SHIFT);
> MemoryMap->Attribute = MergeGcdMapEntry.Attributes |
> EFI_MEMORY_NV |
> - (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP |
> EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
> - EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC
> | EFI_MEMORY_WT | EFI_MEMORY_WB));
> + (MergeGcdMapEntry.Capabilities &
> + (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
> MemoryMap->Type = EfiPersistentMemory;
>
> //
> @@ -1935,8 +1933,7 @@ CoreGetMemoryMap (
> MemoryMapEnd = MemoryMap;
> MemoryMap = MemoryMapStart;
> while (MemoryMap < MemoryMapEnd) {
> - MemoryMap->Attribute &= ~(UINT64)(EFI_MEMORY_RP |
> EFI_MEMORY_RO |
> - EFI_MEMORY_XP);
> + MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK;
> MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
> }
> MergeMemoryMap (MemoryMapStart, &BufferSize, Size); diff --git
> a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 92a442f..7d1daf0 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -42,9 +42,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include
> "DxeMain.h"
> #include "Mem/HeapGuard.h"
>
> -#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC
> | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE |
> EFI_MEMORY_WP)
> -#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP |
> EFI_MEMORY_XP | EFI_MEMORY_RO)
> -
> //
> // Image type definitions
> //
> @@ -221,7 +218,7 @@ SetUefiImageMemoryAttributes (
> Status = CoreGetMemorySpaceDescriptor(BaseAddress, &Descriptor);
> ASSERT_EFI_ERROR(Status);
>
> - FinalAttributes = (Descriptor.Attributes & CACHE_ATTRIBUTE_MASK) |
> (Attributes & MEMORY_ATTRIBUTE_MASK);
> + FinalAttributes = (Descriptor.Attributes & EFI_CACHE_ATTRIBUTE_MASK)
> + | (Attributes & EFI_MEMORY_ATTRIBUTE_MASK);
>
> DEBUG ((DEBUG_INFO, "SetUefiImageMemoryAttributes - 0x%016lx -
> 0x%016lx (0x%016lx)\n", BaseAddress, Length, FinalAttributes));
>
> @@ -924,7 +921,7 @@ InitializeDxeNxMemoryProtectionPolicy (
> (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)) {
>
> Attributes = GetPermissionAttributeForMemoryType
> (EfiConventionalMemory) |
> - (Entry->Attributes & CACHE_ATTRIBUTE_MASK);
> + (Entry->Attributes & EFI_CACHE_ATTRIBUTE_MASK);
>
> DEBUG ((DEBUG_INFO,
> "Untested GCD memory space region: - 0x%016lx - 0x%016lx
> (0x%016lx)\n", diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 0385f1d..599a0cd 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -39,12 +39,6 @@
>
> #define SMRAM_CAPABILITIES (EFI_MEMORY_WB | EFI_MEMORY_UC)
>
> -#define MEMORY_CACHE_ATTRIBUTES (EFI_MEMORY_UC |
> EFI_MEMORY_WC | \
> - EFI_MEMORY_WT | EFI_MEMORY_WB | \
> - EFI_MEMORY_WP | EFI_MEMORY_UCE)
> -
> -#define MEMORY_PAGE_ATTRIBUTES (EFI_MEMORY_XP |
> EFI_MEMORY_RP | EFI_MEMORY_RO)
> -
> //
> // Function prototypes from produced protocols // @@ -1710,7 +1704,7 @@
> SmmIplEntry (
> CpuArch = NULL;
> Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID
> **)&CpuArch);
> if (!EFI_ERROR (Status)) {
> - MemDesc.Attributes &= ~(MEMORY_CACHE_ATTRIBUTES |
> MEMORY_PAGE_ATTRIBUTES);
> + MemDesc.Attributes &= ~(EFI_CACHE_ATTRIBUTE_MASK |
> + EFI_MEMORY_ATTRIBUTE_MASK);
> MemDesc.Attributes |= EFI_MEMORY_WB;
> Status = gDS->SetMemorySpaceAttributes (
> mSmramCacheBase,
> @@ -1727,7 +1721,7 @@ SmmIplEntry (
> &MemDesc
> );
> DEBUG ((DEBUG_INFO, "SMRAM attributes: %016lx\n",
> MemDesc.Attributes));
> - ASSERT ((MemDesc.Attributes & MEMORY_PAGE_ATTRIBUTES) == 0);
> + ASSERT ((MemDesc.Attributes & EFI_MEMORY_ATTRIBUTE_MASK) ==
> 0);
> );
> }
> //
> --
> 2.9.0.windows.1
>
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI). This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited. Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 1/3] MdePkg: Add New Memory Attributes
2020-06-23 21:55 ` [PATCH V2 1/3] MdePkg: " Oleksiy Yakovlev
2020-06-24 2:33 ` [edk2-devel] " Zhiguang Liu
@ 2020-06-24 9:27 ` Laszlo Ersek
2020-06-24 14:24 ` Liming Gao
1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2020-06-24 9:27 UTC (permalink / raw)
To: Oleksiy Yakovlev, devel
Cc: liming.gao, michael.d.kinney, dandan.bi, ray.ni, rahul1.kumar,
Felixp
On 06/23/20 23:55, Oleksiy Yakovlev wrote:
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
> attributes introduced in UEFI 2.8.
> (UEFI 2.8, mantis 1919 and 1872).
> Fix typos in EFI_MEMORY_CPU_CRYPTO description.
> Add attributes bitmasks, grouped by type.
>
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
> MdePkg/Include/Uefi/UefiSpec.h | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
> index 558e1bc..05b82e0 100644
> --- a/MdePkg/Include/Uefi/UefiSpec.h
> +++ b/MdePkg/Include/Uefi/UefiSpec.h
> @@ -96,9 +96,9 @@ typedef enum {
> #define EFI_MEMORY_SP 0x0000000000040000ULL
> //
> // If this flag is set, the memory region is capable of being
> -// protected with the CPU?s memory cryptographic
> +// protected with the CPU's memory cryptographic
> // capabilities. If this flag is clear, the memory region is not
> -// capable of being protected with the CPU?s memory
> +// capable of being protected with the CPU's memory
> // cryptographic capabilities or the CPU does not support CPU
> // memory cryptographic capabilities.
> //
> @@ -109,6 +109,12 @@ typedef enum {
> //
> #define EFI_MEMORY_RUNTIME 0x8000000000000000ULL
>
> +//
> +// Attributes bitmasks, grouped by type
> +//
> +#define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
> +#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
> +
> ///
> /// Memory descriptor version number.
> ///
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 2/3] MdeModulePkg: Add New Memory Attributes
2020-06-23 21:55 ` [PATCH V2 2/3] MdeModulePkg: " Oleksiy Yakovlev
2020-06-24 5:32 ` Dandan Bi
@ 2020-06-24 9:27 ` Laszlo Ersek
1 sibling, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2020-06-24 9:27 UTC (permalink / raw)
To: Oleksiy Yakovlev, devel
Cc: liming.gao, michael.d.kinney, dandan.bi, ray.ni, rahul1.kumar,
Felixp
On 06/23/20 23:55, Oleksiy Yakovlev wrote:
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
> attributes introduced in UEFI 2.8.
> (UEFI 2.8, mantis 1919 and 1872).
> Use attributes bitmasks, defined in MdePkg.
>
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
> MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 11 ++---------
> MdeModulePkg/Core/Dxe/Mem/Page.c | 9 +++------
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 7 ++-----
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 10 ++--------
> 4 files changed, 9 insertions(+), 28 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> index 74f3b1b..2d8c076 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -35,13 +35,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #define PRESENT_MEMORY_ATTRIBUTES (EFI_RESOURCE_ATTRIBUTE_PRESENT)
>
> -#define EXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_UC | EFI_MEMORY_WC | \
> - EFI_MEMORY_WT | EFI_MEMORY_WB | \
> - EFI_MEMORY_WP | EFI_MEMORY_UCE)
> -
> -#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | \
> - EFI_MEMORY_RO)
> -
> //
> // Module Variables
> //
> @@ -665,7 +658,7 @@ ConverToCpuArchAttributes (
> {
> UINT64 CpuArchAttributes;
>
> - CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
> + CpuArchAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
>
> if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
> CpuArchAttributes |= EFI_MEMORY_UC;
> @@ -951,7 +944,7 @@ CoreConvertSpace (
> // Keep original CPU arch attributes when caller just calls
> // SetMemorySpaceAttributes() with none CPU arch attributes (for example, RUNTIME).
> //
> - Attributes |= (Entry->Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES | NONEXCLUSIVE_MEMORY_ATTRIBUTES));
> + Attributes |= (Entry->Attributes & (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
> }
> Entry->Attributes = Attributes;
> break;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 1f0e3d9..2c2c9cd 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1857,8 +1857,7 @@ CoreGetMemoryMap (
> MemoryMap->VirtualStart = 0;
> MemoryMap->NumberOfPages = RShiftU64 ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1), EFI_PAGE_SHIFT);
> MemoryMap->Attribute = (MergeGcdMapEntry.Attributes & ~EFI_MEMORY_PORT_IO) |
> - (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP | EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
> - EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB));
> + (MergeGcdMapEntry.Capabilities & (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
>
> if (MergeGcdMapEntry.GcdMemoryType == EfiGcdMemoryTypeReserved) {
> MemoryMap->Type = EfiReservedMemoryType;
> @@ -1892,8 +1891,7 @@ CoreGetMemoryMap (
> MemoryMap->VirtualStart = 0;
> MemoryMap->NumberOfPages = RShiftU64 ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1), EFI_PAGE_SHIFT);
> MemoryMap->Attribute = MergeGcdMapEntry.Attributes | EFI_MEMORY_NV |
> - (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP | EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
> - EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB));
> + (MergeGcdMapEntry.Capabilities & (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
> MemoryMap->Type = EfiPersistentMemory;
>
> //
> @@ -1935,8 +1933,7 @@ CoreGetMemoryMap (
> MemoryMapEnd = MemoryMap;
> MemoryMap = MemoryMapStart;
> while (MemoryMap < MemoryMapEnd) {
> - MemoryMap->Attribute &= ~(UINT64)(EFI_MEMORY_RP | EFI_MEMORY_RO |
> - EFI_MEMORY_XP);
> + MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK;
> MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
> }
> MergeMemoryMap (MemoryMapStart, &BufferSize, Size);
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 92a442f..7d1daf0 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -42,9 +42,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include "DxeMain.h"
> #include "Mem/HeapGuard.h"
>
> -#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
> -#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
> -
> //
> // Image type definitions
> //
> @@ -221,7 +218,7 @@ SetUefiImageMemoryAttributes (
> Status = CoreGetMemorySpaceDescriptor(BaseAddress, &Descriptor);
> ASSERT_EFI_ERROR(Status);
>
> - FinalAttributes = (Descriptor.Attributes & CACHE_ATTRIBUTE_MASK) | (Attributes & MEMORY_ATTRIBUTE_MASK);
> + FinalAttributes = (Descriptor.Attributes & EFI_CACHE_ATTRIBUTE_MASK) | (Attributes & EFI_MEMORY_ATTRIBUTE_MASK);
>
> DEBUG ((DEBUG_INFO, "SetUefiImageMemoryAttributes - 0x%016lx - 0x%016lx (0x%016lx)\n", BaseAddress, Length, FinalAttributes));
>
> @@ -924,7 +921,7 @@ InitializeDxeNxMemoryProtectionPolicy (
> (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)) {
>
> Attributes = GetPermissionAttributeForMemoryType (EfiConventionalMemory) |
> - (Entry->Attributes & CACHE_ATTRIBUTE_MASK);
> + (Entry->Attributes & EFI_CACHE_ATTRIBUTE_MASK);
>
> DEBUG ((DEBUG_INFO,
> "Untested GCD memory space region: - 0x%016lx - 0x%016lx (0x%016lx)\n",
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 0385f1d..599a0cd 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -39,12 +39,6 @@
>
> #define SMRAM_CAPABILITIES (EFI_MEMORY_WB | EFI_MEMORY_UC)
>
> -#define MEMORY_CACHE_ATTRIBUTES (EFI_MEMORY_UC | EFI_MEMORY_WC | \
> - EFI_MEMORY_WT | EFI_MEMORY_WB | \
> - EFI_MEMORY_WP | EFI_MEMORY_UCE)
> -
> -#define MEMORY_PAGE_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | EFI_MEMORY_RO)
> -
> //
> // Function prototypes from produced protocols
> //
> @@ -1710,7 +1704,7 @@ SmmIplEntry (
> CpuArch = NULL;
> Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&CpuArch);
> if (!EFI_ERROR (Status)) {
> - MemDesc.Attributes &= ~(MEMORY_CACHE_ATTRIBUTES | MEMORY_PAGE_ATTRIBUTES);
> + MemDesc.Attributes &= ~(EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK);
> MemDesc.Attributes |= EFI_MEMORY_WB;
> Status = gDS->SetMemorySpaceAttributes (
> mSmramCacheBase,
> @@ -1727,7 +1721,7 @@ SmmIplEntry (
> &MemDesc
> );
> DEBUG ((DEBUG_INFO, "SMRAM attributes: %016lx\n", MemDesc.Attributes));
> - ASSERT ((MemDesc.Attributes & MEMORY_PAGE_ATTRIBUTES) == 0);
> + ASSERT ((MemDesc.Attributes & EFI_MEMORY_ATTRIBUTE_MASK) == 0);
> );
> }
> //
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes
2020-06-23 21:55 ` [PATCH V2 3/3] UefiCpuPkg: " Oleksiy Yakovlev
@ 2020-06-24 9:42 ` Laszlo Ersek
2020-06-30 21:11 ` Oleksiy Yakovlev
0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2020-06-24 9:42 UTC (permalink / raw)
To: Oleksiy Yakovlev, devel
Cc: liming.gao, michael.d.kinney, dandan.bi, ray.ni, rahul1.kumar,
Felixp, Eric Dong
On 06/23/20 23:55, Oleksiy Yakovlev wrote:
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
> attributes introduced in UEFI 2.8.
> (UEFI 2.8, mantis 1919 and 1872).
> Use attributes bitmasks, defined in MdePkg.
>
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
> UefiCpuPkg/CpuDxe/CpuDxe.c | 11 ++++-------
> UefiCpuPkg/CpuDxe/CpuDxe.h | 12 ------------
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +-
> 4 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index a571fc3..52cc26e 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -10,9 +10,6 @@
> #include "CpuMp.h"
> #include "CpuPageTable.h"
>
> -#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
> -#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
> -
> //
> // Global Variables
> //
> @@ -417,8 +414,8 @@ CpuSetMemoryAttributes (
> return EFI_SUCCESS;
> }
>
> - CacheAttributes = Attributes & CACHE_ATTRIBUTE_MASK;
> - MemoryAttributes = Attributes & MEMORY_ATTRIBUTE_MASK;
> + CacheAttributes = Attributes & EFI_CACHE_ATTRIBUTE_MASK;
> + MemoryAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
>
> if (Attributes != (CacheAttributes | MemoryAttributes)) {
> return EFI_INVALID_PARAMETER;
OK.
> @@ -677,7 +674,7 @@ SetGcdMemorySpaceAttributes (
> gDS->SetMemorySpaceAttributes (
> RegionStart,
> RegionLength,
> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
> );
> }
>
> @@ -754,7 +751,7 @@ RefreshMemoryAttributesFromMtrr (
> gDS->SetMemorySpaceAttributes (
> MemorySpaceMap[Index].BaseAddress,
> MemorySpaceMap[Index].Length,
> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) |
> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) |
> (MemorySpaceMap[Index].Capabilities & DefaultAttributes)
> );
> }
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 9299eaa..9771ec8 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -39,18 +39,6 @@
> #include <Guid/IdleLoopEvent.h>
> #include <Guid/VectorHandoffTable.h>
>
> -#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | \
> - EFI_MEMORY_WC | \
> - EFI_MEMORY_WT | \
> - EFI_MEMORY_WB | \
> - EFI_MEMORY_UCE \
> - )
> -
> -#define EFI_MEMORY_PAGETYPE_MASK (EFI_MEMORY_RP | \
> - EFI_MEMORY_XP | \
> - EFI_MEMORY_RO \
> - )
> -
> #define HEAP_GUARD_NONSTOP_MODE \
> ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
>
(1) These changes have an additional effect. EFI_MEMORY_CACHETYPE_MASK
does not include EFI_MEMORY_WP, but EFI_CACHE_ATTRIBUTE_MASK does.
(1a) If that change is intentional, then this patch can remain as it is,
but we need an extra patch prepended (i.e., inserted between v2 patches
#2 and #3), for adding EFI_MEMORY_WP to EFI_MEMORY_CACHETYPE_MASK first.
(1b) If the EFI_MEMORY_WP change is not intended (i.e., it is an
oversight in this patch), then in every spot where we replace
EFI_MEMORY_CACHETYPE_MASK with EFI_CACHE_ATTRIBUTE_MASK, we need to
account for EFI_MEMORY_WP separately.
... After reading up on EFI_MEMORY_WP in the UEFI spec, I think it's
(1a) -- meaning that, this patch is correct, in itself. But, we should
still not hide the EFI_MEMORY_WP bugfix, for EFI_MEMORY_CACHETYPE_MASK,
in this patch.
So please insert a new patch just before this one, that does nothing
other than include EFI_MEMORY_WP in EFI_MEMORY_CACHETYPE_MASK.
The rest of the patch looks OK to me. Therefore, for this patch (in itself):
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Eric, Ray, Rahul: correct me if I'm wrong, please.
Thanks,
Laszlo
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index 0a02cb3..06ee1b8 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -717,7 +717,7 @@ ConvertMemoryPageAttributes (
> return RETURN_INVALID_PARAMETER;
> }
>
> - if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) {
> + if ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) != 0) {
> DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", Attributes));
> return EFI_UNSUPPORTED;
> }
> @@ -1018,9 +1018,9 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (Attributes != (MemorySpaceMap[Index].Attributes &
> - EFI_MEMORY_PAGETYPE_MASK)) {
> + EFI_MEMORY_ATTRIBUTE_MASK)) {
> NewAttributes = (MemorySpaceMap[Index].Attributes &
> - ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
> + ~EFI_MEMORY_ATTRIBUTE_MASK) | Attributes;
> Status = gDS->SetMemorySpaceAttributes (
> BaseAddress,
> Length,
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 9c5a92a..ebfc46a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -435,7 +435,7 @@ ConvertMemoryPageAttributes (
> EFI_PHYSICAL_ADDRESS MaximumSupportMemAddress;
>
> ASSERT (Attributes != 0);
> - ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
> + ASSERT ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) == 0);
>
> ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
> ASSERT ((Length & (SIZE_4KB - 1)) == 0);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 1/3] MdePkg: Add New Memory Attributes
2020-06-24 9:27 ` Laszlo Ersek
@ 2020-06-24 14:24 ` Liming Gao
0 siblings, 0 replies; 12+ messages in thread
From: Liming Gao @ 2020-06-24 14:24 UTC (permalink / raw)
To: Laszlo Ersek, Oleksiy Yakovlev, devel@edk2.groups.io
Cc: Kinney, Michael D, Bi, Dandan, Ni, Ray, Kumar, Rahul1,
Felixp@ami.com
Reviewed-by: Liming Gao <liming.gao@intel.com>
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, June 24, 2020 5:27 PM
> To: Oleksiy Yakovlev <oleksiyy@ami.com>; devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Felixp@ami.com
> Subject: Re: [PATCH V2 1/3] MdePkg: Add New Memory Attributes
>
> On 06/23/20 23:55, Oleksiy Yakovlev wrote:
> > Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
> > attributes introduced in UEFI 2.8.
> > (UEFI 2.8, mantis 1919 and 1872).
> > Fix typos in EFI_MEMORY_CPU_CRYPTO description.
> > Add attributes bitmasks, grouped by type.
> >
> > Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> > ---
> > MdePkg/Include/Uefi/UefiSpec.h | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h
> > index 558e1bc..05b82e0 100644
> > --- a/MdePkg/Include/Uefi/UefiSpec.h
> > +++ b/MdePkg/Include/Uefi/UefiSpec.h
> > @@ -96,9 +96,9 @@ typedef enum {
> > #define EFI_MEMORY_SP 0x0000000000040000ULL
> > //
> > // If this flag is set, the memory region is capable of being
> > -// protected with the CPU?s memory cryptographic
> > +// protected with the CPU's memory cryptographic
> > // capabilities. If this flag is clear, the memory region is not
> > -// capable of being protected with the CPU?s memory
> > +// capable of being protected with the CPU's memory
> > // cryptographic capabilities or the CPU does not support CPU
> > // memory cryptographic capabilities.
> > //
> > @@ -109,6 +109,12 @@ typedef enum {
> > //
> > #define EFI_MEMORY_RUNTIME 0x8000000000000000ULL
> >
> > +//
> > +// Attributes bitmasks, grouped by type
> > +//
> > +#define EFI_CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB |
> EFI_MEMORY_UCE | EFI_MEMORY_WP)
> > +#define EFI_MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP |
> EFI_MEMORY_CPU_CRYPTO)
> > +
> > ///
> > /// Memory descriptor version number.
> > ///
> >
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes
2020-06-24 9:42 ` Laszlo Ersek
@ 2020-06-30 21:11 ` Oleksiy Yakovlev
2020-07-02 10:44 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Oleksiy Yakovlev @ 2020-06-30 21:11 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: liming.gao@intel.com, michael.d.kinney@intel.com,
dandan.bi@intel.com, ray.ni@intel.com, rahul1.kumar@intel.com,
Felix Polyudov, Eric Dong
Hi Laszlo.
I think WP should be included also. Spec says that WP "typically used as a cacheability attribute today".
Do you want me to submit just additional patch for CpuDxe.h, or resubmit the whole series adding this inclusion of WP to EFI_MEMORY_CACHETYPE_MASK in CpuDxe.h?
Regards, Oleksiy.
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Wednesday, June 24, 2020 5:42 AM
To: Oleksiy Yakovlev; devel@edk2.groups.io
Cc: liming.gao@intel.com; michael.d.kinney@intel.com; dandan.bi@intel.com; ray.ni@intel.com; rahul1.kumar@intel.com; Felix Polyudov; Eric Dong
Subject: Re: [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes
On 06/23/20 23:55, Oleksiy Yakovlev wrote:
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
> attributes introduced in UEFI 2.8.
> (UEFI 2.8, mantis 1919 and 1872).
> Use attributes bitmasks, defined in MdePkg.
>
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
> UefiCpuPkg/CpuDxe/CpuDxe.c | 11 ++++-------
> UefiCpuPkg/CpuDxe/CpuDxe.h | 12 ------------
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++---
> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +-
> 4 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index a571fc3..52cc26e 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -10,9 +10,6 @@
> #include "CpuMp.h"
> #include "CpuPageTable.h"
>
> -#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
> -#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
> -
> //
> // Global Variables
> //
> @@ -417,8 +414,8 @@ CpuSetMemoryAttributes (
> return EFI_SUCCESS;
> }
>
> - CacheAttributes = Attributes & CACHE_ATTRIBUTE_MASK;
> - MemoryAttributes = Attributes & MEMORY_ATTRIBUTE_MASK;
> + CacheAttributes = Attributes & EFI_CACHE_ATTRIBUTE_MASK;
> + MemoryAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
>
> if (Attributes != (CacheAttributes | MemoryAttributes)) {
> return EFI_INVALID_PARAMETER;
OK.
> @@ -677,7 +674,7 @@ SetGcdMemorySpaceAttributes (
> gDS->SetMemorySpaceAttributes (
> RegionStart,
> RegionLength,
> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
> );
> }
>
> @@ -754,7 +751,7 @@ RefreshMemoryAttributesFromMtrr (
> gDS->SetMemorySpaceAttributes (
> MemorySpaceMap[Index].BaseAddress,
> MemorySpaceMap[Index].Length,
> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) |
> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) |
> (MemorySpaceMap[Index].Capabilities & DefaultAttributes)
> );
> }
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 9299eaa..9771ec8 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -39,18 +39,6 @@
> #include <Guid/IdleLoopEvent.h>
> #include <Guid/VectorHandoffTable.h>
>
> -#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | \
> - EFI_MEMORY_WC | \
> - EFI_MEMORY_WT | \
> - EFI_MEMORY_WB | \
> - EFI_MEMORY_UCE \
> - )
> -
> -#define EFI_MEMORY_PAGETYPE_MASK (EFI_MEMORY_RP | \
> - EFI_MEMORY_XP | \
> - EFI_MEMORY_RO \
> - )
> -
> #define HEAP_GUARD_NONSTOP_MODE \
> ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
>
(1) These changes have an additional effect. EFI_MEMORY_CACHETYPE_MASK
does not include EFI_MEMORY_WP, but EFI_CACHE_ATTRIBUTE_MASK does.
(1a) If that change is intentional, then this patch can remain as it is,
but we need an extra patch prepended (i.e., inserted between v2 patches
#2 and #3), for adding EFI_MEMORY_WP to EFI_MEMORY_CACHETYPE_MASK first.
(1b) If the EFI_MEMORY_WP change is not intended (i.e., it is an
oversight in this patch), then in every spot where we replace
EFI_MEMORY_CACHETYPE_MASK with EFI_CACHE_ATTRIBUTE_MASK, we need to
account for EFI_MEMORY_WP separately.
... After reading up on EFI_MEMORY_WP in the UEFI spec, I think it's
(1a) -- meaning that, this patch is correct, in itself. But, we should
still not hide the EFI_MEMORY_WP bugfix, for EFI_MEMORY_CACHETYPE_MASK,
in this patch.
So please insert a new patch just before this one, that does nothing
other than include EFI_MEMORY_WP in EFI_MEMORY_CACHETYPE_MASK.
The rest of the patch looks OK to me. Therefore, for this patch (in itself):
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Eric, Ray, Rahul: correct me if I'm wrong, please.
Thanks,
Laszlo
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index 0a02cb3..06ee1b8 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -717,7 +717,7 @@ ConvertMemoryPageAttributes (
> return RETURN_INVALID_PARAMETER;
> }
>
> - if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) {
> + if ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) != 0) {
> DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", Attributes));
> return EFI_UNSUPPORTED;
> }
> @@ -1018,9 +1018,9 @@ RefreshGcdMemoryAttributesFromPaging (
>
> Length = MIN (PageLength, MemorySpaceLength);
> if (Attributes != (MemorySpaceMap[Index].Attributes &
> - EFI_MEMORY_PAGETYPE_MASK)) {
> + EFI_MEMORY_ATTRIBUTE_MASK)) {
> NewAttributes = (MemorySpaceMap[Index].Attributes &
> - ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
> + ~EFI_MEMORY_ATTRIBUTE_MASK) | Attributes;
> Status = gDS->SetMemorySpaceAttributes (
> BaseAddress,
> Length,
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 9c5a92a..ebfc46a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -435,7 +435,7 @@ ConvertMemoryPageAttributes (
> EFI_PHYSICAL_ADDRESS MaximumSupportMemAddress;
>
> ASSERT (Attributes != 0);
> - ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
> + ASSERT ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) == 0);
>
> ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
> ASSERT ((Length & (SIZE_4KB - 1)) == 0);
>
Please consider the environment before printing this email.
The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes
2020-06-30 21:11 ` Oleksiy Yakovlev
@ 2020-07-02 10:44 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2020-07-02 10:44 UTC (permalink / raw)
To: Oleksiy Yakovlev, devel@edk2.groups.io
Cc: liming.gao@intel.com, michael.d.kinney@intel.com,
dandan.bi@intel.com, ray.ni@intel.com, rahul1.kumar@intel.com,
Felix Polyudov, Eric Dong
Hi Oleksiy,
On 06/30/20 23:11, Oleksiy Yakovlev wrote:
> Hi Laszlo.
>
> I think WP should be included also. Spec says that WP "typically used as a cacheability attribute today".
>
> Do you want me to submit just additional patch for CpuDxe.h, or resubmit the whole series adding this inclusion of WP to EFI_MEMORY_CACHETYPE_MASK in CpuDxe.h?
I'd suggest posting a v3 patch set, with 4 patches in total:
- v4 1/4: equals v3 1/3
- v4 2/4: equals v3 2/3
- v4 3/4: new patch (add WP to EFI_MEMORY_CACHETYPE_MASK)
- v4 4/4: v3 3/3, updated
The reason for my suggestion is that, once you add EFI_MEMORY_WP to
EFI_MEMORY_CACHETYPE_MASK, the present patch will no longer apply
verbatim (it will have to be rebased). Namely, the hunk that removes the
EFI_MEMORY_CACHETYPE_MASK #define needs to be updated for EFI_MEMORY_WP.
When posting v4, please pick up the feedback tags you received during
the v3 review. (The small update for the final patch in the series does
not invalidate my R-b.)
Thank you!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, June 24, 2020 5:42 AM
> To: Oleksiy Yakovlev; devel@edk2.groups.io
> Cc: liming.gao@intel.com; michael.d.kinney@intel.com; dandan.bi@intel.com; ray.ni@intel.com; rahul1.kumar@intel.com; Felix Polyudov; Eric Dong
> Subject: Re: [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes
>
> On 06/23/20 23:55, Oleksiy Yakovlev wrote:
>> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
>> attributes introduced in UEFI 2.8.
>> (UEFI 2.8, mantis 1919 and 1872).
>> Use attributes bitmasks, defined in MdePkg.
>>
>> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
>> ---
>> UefiCpuPkg/CpuDxe/CpuDxe.c | 11 ++++-------
>> UefiCpuPkg/CpuDxe/CpuDxe.h | 12 ------------
>> UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++---
>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +-
>> 4 files changed, 8 insertions(+), 23 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
>> index a571fc3..52cc26e 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
>> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
>> @@ -10,9 +10,6 @@
>> #include "CpuMp.h"
>> #include "CpuPageTable.h"
>>
>> -#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)
>> -#define MEMORY_ATTRIBUTE_MASK (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
>> -
>> //
>> // Global Variables
>> //
>> @@ -417,8 +414,8 @@ CpuSetMemoryAttributes (
>> return EFI_SUCCESS;
>> }
>>
>> - CacheAttributes = Attributes & CACHE_ATTRIBUTE_MASK;
>> - MemoryAttributes = Attributes & MEMORY_ATTRIBUTE_MASK;
>> + CacheAttributes = Attributes & EFI_CACHE_ATTRIBUTE_MASK;
>> + MemoryAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
>>
>> if (Attributes != (CacheAttributes | MemoryAttributes)) {
>> return EFI_INVALID_PARAMETER;
>
> OK.
>
>> @@ -677,7 +674,7 @@ SetGcdMemorySpaceAttributes (
>> gDS->SetMemorySpaceAttributes (
>> RegionStart,
>> RegionLength,
>> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
>> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | (MemorySpaceMap[Index].Capabilities & Attributes)
>> );
>> }
>>
>> @@ -754,7 +751,7 @@ RefreshMemoryAttributesFromMtrr (
>> gDS->SetMemorySpaceAttributes (
>> MemorySpaceMap[Index].BaseAddress,
>> MemorySpaceMap[Index].Length,
>> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) |
>> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) |
>> (MemorySpaceMap[Index].Capabilities & DefaultAttributes)
>> );
>> }
>> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
>> index 9299eaa..9771ec8 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
>> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
>> @@ -39,18 +39,6 @@
>> #include <Guid/IdleLoopEvent.h>
>> #include <Guid/VectorHandoffTable.h>
>>
>> -#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | \
>> - EFI_MEMORY_WC | \
>> - EFI_MEMORY_WT | \
>> - EFI_MEMORY_WB | \
>> - EFI_MEMORY_UCE \
>> - )
>> -
>> -#define EFI_MEMORY_PAGETYPE_MASK (EFI_MEMORY_RP | \
>> - EFI_MEMORY_XP | \
>> - EFI_MEMORY_RO \
>> - )
>> -
>> #define HEAP_GUARD_NONSTOP_MODE \
>> ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
>>
>
> (1) These changes have an additional effect. EFI_MEMORY_CACHETYPE_MASK
> does not include EFI_MEMORY_WP, but EFI_CACHE_ATTRIBUTE_MASK does.
>
> (1a) If that change is intentional, then this patch can remain as it is,
> but we need an extra patch prepended (i.e., inserted between v2 patches
> #2 and #3), for adding EFI_MEMORY_WP to EFI_MEMORY_CACHETYPE_MASK first.
>
> (1b) If the EFI_MEMORY_WP change is not intended (i.e., it is an
> oversight in this patch), then in every spot where we replace
> EFI_MEMORY_CACHETYPE_MASK with EFI_CACHE_ATTRIBUTE_MASK, we need to
> account for EFI_MEMORY_WP separately.
>
> ... After reading up on EFI_MEMORY_WP in the UEFI spec, I think it's
> (1a) -- meaning that, this patch is correct, in itself. But, we should
> still not hide the EFI_MEMORY_WP bugfix, for EFI_MEMORY_CACHETYPE_MASK,
> in this patch.
>
> So please insert a new patch just before this one, that does nothing
> other than include EFI_MEMORY_WP in EFI_MEMORY_CACHETYPE_MASK.
>
> The rest of the patch looks OK to me. Therefore, for this patch (in itself):
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Eric, Ray, Rahul: correct me if I'm wrong, please.
>
> Thanks,
> Laszlo
>
>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> index 0a02cb3..06ee1b8 100644
>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>> @@ -717,7 +717,7 @@ ConvertMemoryPageAttributes (
>> return RETURN_INVALID_PARAMETER;
>> }
>>
>> - if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) {
>> + if ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) != 0) {
>> DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", Attributes));
>> return EFI_UNSUPPORTED;
>> }
>> @@ -1018,9 +1018,9 @@ RefreshGcdMemoryAttributesFromPaging (
>>
>> Length = MIN (PageLength, MemorySpaceLength);
>> if (Attributes != (MemorySpaceMap[Index].Attributes &
>> - EFI_MEMORY_PAGETYPE_MASK)) {
>> + EFI_MEMORY_ATTRIBUTE_MASK)) {
>> NewAttributes = (MemorySpaceMap[Index].Attributes &
>> - ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
>> + ~EFI_MEMORY_ATTRIBUTE_MASK) | Attributes;
>> Status = gDS->SetMemorySpaceAttributes (
>> BaseAddress,
>> Length,
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> index 9c5a92a..ebfc46a 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
>> @@ -435,7 +435,7 @@ ConvertMemoryPageAttributes (
>> EFI_PHYSICAL_ADDRESS MaximumSupportMemAddress;
>>
>> ASSERT (Attributes != 0);
>> - ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
>> + ASSERT ((Attributes & ~EFI_MEMORY_ATTRIBUTE_MASK) == 0);
>>
>> ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
>> ASSERT ((Length & (SIZE_4KB - 1)) == 0);
>>
>
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-02 10:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-23 21:55 [PATCH V2 0/3] Add New Memory Attributes Oleksiy Yakovlev
2020-06-23 21:55 ` [PATCH V2 1/3] MdePkg: " Oleksiy Yakovlev
2020-06-24 2:33 ` [edk2-devel] " Zhiguang Liu
2020-06-24 9:27 ` Laszlo Ersek
2020-06-24 14:24 ` Liming Gao
2020-06-23 21:55 ` [PATCH V2 2/3] MdeModulePkg: " Oleksiy Yakovlev
2020-06-24 5:32 ` Dandan Bi
2020-06-24 9:27 ` Laszlo Ersek
2020-06-23 21:55 ` [PATCH V2 3/3] UefiCpuPkg: " Oleksiy Yakovlev
2020-06-24 9:42 ` Laszlo Ersek
2020-06-30 21:11 ` Oleksiy Yakovlev
2020-07-02 10:44 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox