* [edk2-devel] [Patch V2 1/3] UefiCpuPkg: Reduce and optimize access to attribute
2024-02-06 1:57 [edk2-devel] [Patch V2 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization duntan
@ 2024-02-06 1:57 ` duntan
2024-02-06 1:57 ` [edk2-devel] [Patch V2 2/3] UefiCpuPkg: Add more Paging mode enumeration duntan
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: duntan @ 2024-02-06 1:57 UTC (permalink / raw)
To: devel; +Cc: Zhou Jianfeng, Ray Ni, Laszlo Ersek, Rahul Kumar, Gerd Hoffmann
From: Zhou Jianfeng <jianfeng.zhou@intel.com>
This commit is to reduce and optimize access to
attribute in CpuPageTableLib.
Unreasonable writing to attribute of page table may
leads to expection.
The assembly code for C code Pnle->Bits.Present =
Attribute->Bits.Present looks like:
and dword [rcx], 0xfffffffe
and eax, 0x1
or [rcx], eax
In case Pnle->Bits.Present and Attribute->Bits.Present
is 1, Pnle->Bits.Present will be set to 0 for short
time(2 instructions) which is unexpected. If some other
core is accessing the page, it may leads to expection.
This change reduce and optimize access to attribute of
page table, attribute of page table is set only when it
need to be changed.
Signed-off-by: Zhou Jianfeng <jianfeng.zhou@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
1 file changed, 53 insertions(+), 33 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 36b2c4e6a3..ae4caf8dfe 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -26,52 +26,59 @@ PageTableLibSetPte4K (
IN IA32_MAP_ATTRIBUTE *Mask
)
{
+ IA32_PTE_4K LocalPte4K;
+
+ LocalPte4K.Uint64 = Pte4K->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
- Pte4K->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
+ LocalPte4K.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
}
if (Mask->Bits.Present) {
- Pte4K->Bits.Present = Attribute->Bits.Present;
+ LocalPte4K.Bits.Present = Attribute->Bits.Present;
}
if (Mask->Bits.ReadWrite) {
- Pte4K->Bits.ReadWrite = Attribute->Bits.ReadWrite;
+ LocalPte4K.Bits.ReadWrite = Attribute->Bits.ReadWrite;
}
if (Mask->Bits.UserSupervisor) {
- Pte4K->Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
+ LocalPte4K.Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
}
if (Mask->Bits.WriteThrough) {
- Pte4K->Bits.WriteThrough = Attribute->Bits.WriteThrough;
+ LocalPte4K.Bits.WriteThrough = Attribute->Bits.WriteThrough;
}
if (Mask->Bits.CacheDisabled) {
- Pte4K->Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
+ LocalPte4K.Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
}
if (Mask->Bits.Accessed) {
- Pte4K->Bits.Accessed = Attribute->Bits.Accessed;
+ LocalPte4K.Bits.Accessed = Attribute->Bits.Accessed;
}
if (Mask->Bits.Dirty) {
- Pte4K->Bits.Dirty = Attribute->Bits.Dirty;
+ LocalPte4K.Bits.Dirty = Attribute->Bits.Dirty;
}
if (Mask->Bits.Pat) {
- Pte4K->Bits.Pat = Attribute->Bits.Pat;
+ LocalPte4K.Bits.Pat = Attribute->Bits.Pat;
}
if (Mask->Bits.Global) {
- Pte4K->Bits.Global = Attribute->Bits.Global;
+ LocalPte4K.Bits.Global = Attribute->Bits.Global;
}
if (Mask->Bits.ProtectionKey) {
- Pte4K->Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
+ LocalPte4K.Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
}
if (Mask->Bits.Nx) {
- Pte4K->Bits.Nx = Attribute->Bits.Nx;
+ LocalPte4K.Bits.Nx = Attribute->Bits.Nx;
+ }
+
+ if (Pte4K->Uint64 != LocalPte4K.Uint64) {
+ Pte4K->Uint64 = LocalPte4K.Uint64;
}
}
@@ -93,54 +100,61 @@ PageTableLibSetPleB (
IN IA32_MAP_ATTRIBUTE *Mask
)
{
+ IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE LocalPleB;
+
+ LocalPleB.Uint64 = PleB->Uint64;
if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
- PleB->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
+ LocalPleB.Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
}
- PleB->Bits.MustBeOne = 1;
+ LocalPleB.Bits.MustBeOne = 1;
if (Mask->Bits.Present) {
- PleB->Bits.Present = Attribute->Bits.Present;
+ LocalPleB.Bits.Present = Attribute->Bits.Present;
}
if (Mask->Bits.ReadWrite) {
- PleB->Bits.ReadWrite = Attribute->Bits.ReadWrite;
+ LocalPleB.Bits.ReadWrite = Attribute->Bits.ReadWrite;
}
if (Mask->Bits.UserSupervisor) {
- PleB->Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
+ LocalPleB.Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
}
if (Mask->Bits.WriteThrough) {
- PleB->Bits.WriteThrough = Attribute->Bits.WriteThrough;
+ LocalPleB.Bits.WriteThrough = Attribute->Bits.WriteThrough;
}
if (Mask->Bits.CacheDisabled) {
- PleB->Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
+ LocalPleB.Bits.CacheDisabled = Attribute->Bits.CacheDisabled;
}
if (Mask->Bits.Accessed) {
- PleB->Bits.Accessed = Attribute->Bits.Accessed;
+ LocalPleB.Bits.Accessed = Attribute->Bits.Accessed;
}
if (Mask->Bits.Dirty) {
- PleB->Bits.Dirty = Attribute->Bits.Dirty;
+ LocalPleB.Bits.Dirty = Attribute->Bits.Dirty;
}
if (Mask->Bits.Pat) {
- PleB->Bits.Pat = Attribute->Bits.Pat;
+ LocalPleB.Bits.Pat = Attribute->Bits.Pat;
}
if (Mask->Bits.Global) {
- PleB->Bits.Global = Attribute->Bits.Global;
+ LocalPleB.Bits.Global = Attribute->Bits.Global;
}
if (Mask->Bits.ProtectionKey) {
- PleB->Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
+ LocalPleB.Bits.ProtectionKey = Attribute->Bits.ProtectionKey;
}
if (Mask->Bits.Nx) {
- PleB->Bits.Nx = Attribute->Bits.Nx;
+ LocalPleB.Bits.Nx = Attribute->Bits.Nx;
+ }
+
+ if (PleB->Uint64 != LocalPleB.Uint64) {
+ PleB->Uint64 = LocalPleB.Uint64;
}
}
@@ -186,24 +200,27 @@ PageTableLibSetPnle (
IN IA32_MAP_ATTRIBUTE *Mask
)
{
+ IA32_PAGE_NON_LEAF_ENTRY LocalPnle;
+
+ LocalPnle.Uint64 = Pnle->Uint64;
if (Mask->Bits.Present) {
- Pnle->Bits.Present = Attribute->Bits.Present;
+ LocalPnle.Bits.Present = Attribute->Bits.Present;
}
if (Mask->Bits.ReadWrite) {
- Pnle->Bits.ReadWrite = Attribute->Bits.ReadWrite;
+ LocalPnle.Bits.ReadWrite = Attribute->Bits.ReadWrite;
}
if (Mask->Bits.UserSupervisor) {
- Pnle->Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
+ LocalPnle.Bits.UserSupervisor = Attribute->Bits.UserSupervisor;
}
if (Mask->Bits.Nx) {
- Pnle->Bits.Nx = Attribute->Bits.Nx;
+ LocalPnle.Bits.Nx = Attribute->Bits.Nx;
}
- Pnle->Bits.Accessed = 0;
- Pnle->Bits.MustBeZero = 0;
+ LocalPnle.Bits.Accessed = 0;
+ LocalPnle.Bits.MustBeZero = 0;
//
// Set the attributes (WT, CD, A) to 0.
@@ -211,8 +228,11 @@ PageTableLibSetPnle (
// So, it implictly requires PAT[0] is Write Back.
// Create a new parameter if caller requires to use a different memory type for accessing page directories.
//
- Pnle->Bits.WriteThrough = 0;
- Pnle->Bits.CacheDisabled = 0;
+ LocalPnle.Bits.WriteThrough = 0;
+ LocalPnle.Bits.CacheDisabled = 0;
+ if (Pnle->Uint64 != LocalPnle.Uint64) {
+ Pnle->Uint64 = LocalPnle.Uint64;
+ }
}
/**
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115136): https://edk2.groups.io/g/devel/message/115136
Mute This Topic: https://groups.io/mt/104191039/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 7+ messages in thread* [edk2-devel] [Patch V2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity
2024-02-06 1:57 [edk2-devel] [Patch V2 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization duntan
2024-02-06 1:57 ` [edk2-devel] [Patch V2 1/3] UefiCpuPkg: Reduce and optimize access to attribute duntan
2024-02-06 1:57 ` [edk2-devel] [Patch V2 2/3] UefiCpuPkg: Add more Paging mode enumeration duntan
@ 2024-02-06 1:57 ` duntan
2024-02-06 2:27 ` [edk2-devel] [Patch V2 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization Ni, Ray
2024-02-06 4:52 ` Wu, Jiaxin
4 siblings, 0 replies; 7+ messages in thread
From: duntan @ 2024-02-06 1:57 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek, Rahul Kumar, Gerd Hoffmann
This patch is to map SMRAM in 4K page granularity
during SMM page table initialization(SmmInitPageTable)
so as to avoid the SMRAM paging-structure layout
change when SMI happens (PerformRemainingTasks).
The reason is to avoid the Paging-Structure change
impact to the multiple Processors. Refer SDM section
"4.10.4" & "4.10.5".
Currently, SMM BSP needs to update the SMRAM range
paging attribute in smm page table according to the
SmmMemoryAttributesTable when SMM ready to lock
happens. If the SMRAM range is not 4k mapped in page
table, the page table update process may split 1G/2M
paging entries to 4k ones.Meanwhile, all APs are still
running in SMI, which might access the affected
linear-address range between the time of modification
and the time of invalidation access. That will be
a potential problem leading exception happens.
Signed-off-by: Dun Tan <dun.tan@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
1 file changed, 92 insertions(+), 24 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 12f3c0b8e8..b8c356bfe8 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -1647,49 +1647,115 @@ EdkiiSmmClearMemoryAttributes (
}
/**
- Create page table based on input PagingMode and PhysicalAddressBits in smm.
-
- @param[in] PagingMode The paging mode.
- @param[in] PhysicalAddressBits The bits of physical address to map.
+ Create page table based on input PagingMode, LinearAddress and Length.
- @retval PageTable Address
+ @param[in, out] PageTable The pointer to the page table.
+ @param[in] PagingMode The paging mode.
+ @param[in] LinearAddress The start of the linear address range.
+ @param[in] Length The length of the linear address range.
**/
-UINTN
-GenSmmPageTable (
- IN PAGING_MODE PagingMode,
- IN UINT8 PhysicalAddressBits
+VOID
+GenPageTable (
+ IN OUT UINTN *PageTable,
+ IN PAGING_MODE PagingMode,
+ IN UINT64 LinearAddress,
+ IN UINT64 Length
)
{
+ RETURN_STATUS Status;
UINTN PageTableBufferSize;
- UINTN PageTable;
VOID *PageTableBuffer;
IA32_MAP_ATTRIBUTE MapAttribute;
IA32_MAP_ATTRIBUTE MapMask;
- RETURN_STATUS Status;
- UINTN GuardPage;
- UINTN Index;
- UINT64 Length;
- Length = LShiftU64 (1, PhysicalAddressBits);
- PageTable = 0;
- PageTableBufferSize = 0;
MapMask.Uint64 = MAX_UINT64;
- MapAttribute.Uint64 = mAddressEncMask;
+ MapAttribute.Uint64 = mAddressEncMask|LinearAddress;
MapAttribute.Bits.Present = 1;
MapAttribute.Bits.ReadWrite = 1;
MapAttribute.Bits.UserSupervisor = 1;
MapAttribute.Bits.Accessed = 1;
MapAttribute.Bits.Dirty = 1;
+ PageTableBufferSize = 0;
+
+ Status = PageTableMap (
+ PageTable,
+ PagingMode,
+ NULL,
+ &PageTableBufferSize,
+ LinearAddress,
+ Length,
+ &MapAttribute,
+ &MapMask,
+ NULL
+ );
+ if (Status == RETURN_BUFFER_TOO_SMALL) {
+ DEBUG ((DEBUG_INFO, "GenSMMPageTable: 0x%x bytes needed for initial SMM page table\n", PageTableBufferSize));
+ PageTableBuffer = AllocatePageTableMemory (EFI_SIZE_TO_PAGES (PageTableBufferSize));
+ ASSERT (PageTableBuffer != NULL);
+ Status = PageTableMap (
+ PageTable,
+ PagingMode,
+ PageTableBuffer,
+ &PageTableBufferSize,
+ LinearAddress,
+ Length,
+ &MapAttribute,
+ &MapMask,
+ NULL
+ );
+ }
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, 0, Length, &MapAttribute, &MapMask, NULL);
- ASSERT (Status == RETURN_BUFFER_TOO_SMALL);
- DEBUG ((DEBUG_INFO, "GenSMMPageTable: 0x%x bytes needed for initial SMM page table\n", PageTableBufferSize));
- PageTableBuffer = AllocatePageTableMemory (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- ASSERT (PageTableBuffer != NULL);
- Status = PageTableMap (&PageTable, PagingMode, PageTableBuffer, &PageTableBufferSize, 0, Length, &MapAttribute, &MapMask, NULL);
ASSERT (Status == RETURN_SUCCESS);
ASSERT (PageTableBufferSize == 0);
+}
+
+/**
+ Create page table based on input PagingMode and PhysicalAddressBits in smm.
+
+ @param[in] PagingMode The paging mode.
+ @param[in] PhysicalAddressBits The bits of physical address to map.
+
+ @retval PageTable Address
+
+**/
+UINTN
+GenSmmPageTable (
+ IN PAGING_MODE PagingMode,
+ IN UINT8 PhysicalAddressBits
+ )
+{
+ UINTN PageTable;
+ RETURN_STATUS Status;
+ UINTN GuardPage;
+ UINTN Index;
+ UINT64 Length;
+ PAGING_MODE SmramPagingMode;
+
+ PageTable = 0;
+ Length = LShiftU64 (1, PhysicalAddressBits);
+ ASSERT (Length > mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize);
+
+ if (sizeof (UINTN) == sizeof (UINT64)) {
+ SmramPagingMode = m5LevelPagingNeeded ? Paging5Level4KB : Paging4Level4KB;
+ } else {
+ SmramPagingMode = PagingPae4KB;
+ }
+
+ ASSERT (mCpuHotPlugData.SmrrBase % SIZE_4KB == 0);
+ ASSERT (mCpuHotPlugData.SmrrSize % SIZE_4KB == 0);
+ GenPageTable (&PageTable, PagingMode, 0, mCpuHotPlugData.SmrrBase);
+
+ //
+ // Map smram range in 4K page granularity to avoid subsequent page split when smm ready to lock.
+ // If BSP are splitting the 1G/2M paging entries to 512 2M/4K paging entries, and all APs are
+ // still running in SMI at the same time, which might access the affected linear-address range
+ // between the time of modification and the time of invalidation access. That will be a potential
+ // problem leading exception happen.
+ //
+ GenPageTable (&PageTable, SmramPagingMode, mCpuHotPlugData.SmrrBase, mCpuHotPlugData.SmrrSize);
+
+ GenPageTable (&PageTable, PagingMode, mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize, Length - mCpuHotPlugData.SmrrBase - mCpuHotPlugData.SmrrSize);
if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
//
@@ -1698,6 +1764,7 @@ GenSmmPageTable (
for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) {
GuardPage = mSmmStackArrayBase + EFI_PAGE_SIZE + Index * (mSmmStackSize + mSmmShadowStackSize);
Status = ConvertMemoryPageAttributes (PageTable, PagingMode, GuardPage, SIZE_4KB, EFI_MEMORY_RP, TRUE, NULL);
+ ASSERT (Status == RETURN_SUCCESS);
}
}
@@ -1706,6 +1773,7 @@ GenSmmPageTable (
// Mark [0, 4k] as non-present
//
Status = ConvertMemoryPageAttributes (PageTable, PagingMode, 0, SIZE_4KB, EFI_MEMORY_RP, TRUE, NULL);
+ ASSERT (Status == RETURN_SUCCESS);
}
return (UINTN)PageTable;
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115138): https://edk2.groups.io/g/devel/message/115138
Mute This Topic: https://groups.io/mt/104191041/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 7+ messages in thread