[edk2-devel] [PATCH 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization

[edk2-devel] [PATCH 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization

[duntan]

This patch set is to fix potential issue in CpuPageTableLib and SMM page table initialization

The first patch "UefiCpuPkg: Reduce and optimize access to attribute" is to reduce and optimize access to
attribute in page table modification code in CpuPageTableLib. 

The patch "UefiCpuPkg: Add more Paging mode enumeration" and " UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity"
is to map SMRAM in 4K page granularity during SMM page table initialization(SmmInitPageTable).
This is to avoid the SMRAM paging-structure layout change(page split) when SMM ready to lock (PerformRemainingTasks).

Dun Tan (2):
  UefiCpuPkg: Add more Paging mode enumeration
  UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity

Zhou Jianfeng (1):
  UefiCpuPkg: Reduce and optimize access to attribute

 UefiCpuPkg/Include/Library/CpuPageTableLib.h         |   8 +++++++-
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c |  86 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c   | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
 3 files changed, 152 insertions(+), 58 deletions(-)

-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115115): https://edk2.groups.io/g/devel/message/115115
Mute This Topic: https://groups.io/mt/104176229/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[duntan]

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>
Cc: 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 (#115116): https://edk2.groups.io/g/devel/message/115116
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/3] UefiCpuPkg: Add more Paging mode enumeration

[duntan]

Add more Paging mode enumeration in CpuPageTableLib
to support forcibly mapping a range to 4k pages in
page table.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: 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/Include/Library/CpuPageTableLib.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 78aa83b8de..6225a1d8e9 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -46,11 +46,17 @@ typedef enum {
   // High byte in paging mode indicates the max levels of the page table.
   // Low byte in paging mode indicates the max level that can be a leaf entry.
   //
-  PagingPae = 0x0302,
+  PagingPae4KB = 0x0301,
+  PagingPae2MB = 0x0302,
+  PagingPae    = 0x0302,
 
+  Paging4Level4KB = 0x0401,
+  Paging4Level2MB = 0x0402,
   Paging4Level    = 0x0402,
   Paging4Level1GB = 0x0403,
 
+  Paging5Level4KB = 0x0501,
+  Paging5Level2MB = 0x0502,
   Paging5Level    = 0x0502,
   Paging5Level1GB = 0x0503,
 
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115117): https://edk2.groups.io/g/devel/message/115117
Mute This Topic: https://groups.io/mt/104176235/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity

[duntan]

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>
Cc: 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..0012d63674 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;
+  }
+
+  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.
+  //
+  ASSERT (mCpuHotPlugData.SmrrBase % SIZE_4KB == 0);
+  ASSERT (mCpuHotPlugData.SmrrSize % SIZE_4KB == 0);
+  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 (#115118): https://edk2.groups.io/g/devel/message/115118
Mute This Topic: https://groups.io/mt/104176237/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Monday, February 5, 2024 10:04 PM
> To: devel@edk2.groups.io
> Cc: Zhou, Jianfeng <jianfeng.zhou@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute
> 
> 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>
> Cc: 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 (#115130): https://edk2.groups.io/g/devel/message/115130
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg: Add more Paging mode enumeration

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@Intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Monday, February 5, 2024 10:04 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH 2/3] UefiCpuPkg: Add more Paging mode enumeration
> 
> Add more Paging mode enumeration in CpuPageTableLib
> to support forcibly mapping a range to 4k pages in
> page table.
> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: 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/Include/Library/CpuPageTableLib.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> index 78aa83b8de..6225a1d8e9 100644
> --- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> +++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> @@ -46,11 +46,17 @@ typedef enum {
>    // High byte in paging mode indicates the max levels of the page table.
>    // Low byte in paging mode indicates the max level that can be a leaf entry.
>    //
> -  PagingPae = 0x0302,
> +  PagingPae4KB = 0x0301,
> +  PagingPae2MB = 0x0302,
> +  PagingPae    = 0x0302,
> 
> +  Paging4Level4KB = 0x0401,
> +  Paging4Level2MB = 0x0402,
>    Paging4Level    = 0x0402,
>    Paging4Level1GB = 0x0403,
> 
> +  Paging5Level4KB = 0x0501,
> +  Paging5Level2MB = 0x0502,
>    Paging5Level    = 0x0502,
>    Paging5Level1GB = 0x0503,
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115131): https://edk2.groups.io/g/devel/message/115131
Mute This Topic: https://groups.io/mt/104176235/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity

[Ni, Ray]

> +
> +  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.
> +  //
> +  ASSERT (mCpuHotPlugData.SmrrBase % SIZE_4KB == 0);
> +  ASSERT (mCpuHotPlugData.SmrrSize % SIZE_4KB == 0);

Can you please move the above 2 assertions in front of the
first call of GenPageTable() because SmrrBase is used there?

With that, Reviewed-by: Ray Ni <ray.ni@intel.com>

> +  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 (#115132): https://edk2.groups.io/g/devel/message/115132
Mute This Topic: https://groups.io/mt/104176237/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/3] Fix potential issue in CpuPageTableLib and SMM page table initialization

[Ni, Ray]

More background: The patch series fix a SMM hang issue that's observed in reboot cycling test.

Thanks,
Ray
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Monday, February 5, 2024 10:04 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 0/3] Fix potential issue in CpuPageTableLib and
> SMM page table initialization
> 
> This patch set is to fix potential issue in CpuPageTableLib and SMM page table
> initialization
> 
> The first patch "UefiCpuPkg: Reduce and optimize access to attribute" is to
> reduce and optimize access to
> attribute in page table modification code in CpuPageTableLib.
> 
> The patch "UefiCpuPkg: Add more Paging mode enumeration" and "
> UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity"
> is to map SMRAM in 4K page granularity during SMM page table
> initialization(SmmInitPageTable).
> This is to avoid the SMRAM paging-structure layout change(page split) when
> SMM ready to lock (PerformRemainingTasks).
> 
> Dun Tan (2):
>   UefiCpuPkg: Add more Paging mode enumeration
>   UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity
> 
> Zhou Jianfeng (1):
>   UefiCpuPkg: Reduce and optimize access to attribute
> 
>  UefiCpuPkg/Include/Library/CpuPageTableLib.h         |   8 +++++++-
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c |  86
> +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> ------------------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c   | 116
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++------------------------
>  3 files changed, 152 insertions(+), 58 deletions(-)
> 
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115134): https://edk2.groups.io/g/devel/message/115134
Mute This Topic: https://groups.io/mt/104176229/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Laszlo Ersek]

On 2/5/24 15:03, duntan wrote:
> 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.

This patch does nothing to eliminate the actual race condition, it only
shrinks the window of potential corruption.

The PTEs continue to be overwritten without any kind of synchronization
with the other processors.

Feel free to merge this with Ray's R-b.

Laszlo

> 
> Signed-off-by: Zhou Jianfeng <jianfeng.zhou@intel.com>
> Cc: 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;
> +  }
>  }
>  
>  /**



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115162): https://edk2.groups.io/g/devel/message/115162
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 3/3] UefiCpuPkg/PiSmmCpuDxeSmm:Map SMRAM in 4K page granularity

[Laszlo Ersek]

On 2/5/24 15:03, Dun Tan wrote:
> 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.

Is this originally a bug from commit 717fb60443fb
("UefiCpuPkg/PiSmmCpuDxeSmm: Add paging protection.", 2016-11-17)?

Laszlo

> 
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: 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..0012d63674 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;
> +  }
> +
> +  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.
> +  //
> +  ASSERT (mCpuHotPlugData.SmrrBase % SIZE_4KB == 0);
> +  ASSERT (mCpuHotPlugData.SmrrSize % SIZE_4KB == 0);
> +  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;



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115163): https://edk2.groups.io/g/devel/message/115163
Mute This Topic: https://groups.io/mt/104176237/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Ni, Ray]

Laszlo,
You are right.
It only fixes the issue when the CPU changes the page table it's using (UP page table issue).

But it does not fix the MP page table issue when BSP changes the page table that AP is using.

The MP page table issue is an interesting one.
Right now we only can reduce the rate but cannot guarantee it never happens.


Thanks,
Ray
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, February 6, 2024 9:33 PM
> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> Cc: Zhou, Jianfeng <jianfeng.zhou@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize
> access to attribute
> 
> On 2/5/24 15:03, duntan wrote:
> > 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.
> 
> This patch does nothing to eliminate the actual race condition, it only
> shrinks the window of potential corruption.
> 
> The PTEs continue to be overwritten without any kind of synchronization
> with the other processors.
> 
> Feel free to merge this with Ray's R-b.
> 
> Laszlo
> 
> >
> > Signed-off-by: Zhou Jianfeng <jianfeng.zhou@intel.com>
> > Cc: 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;
> > +  }
> >  }
> >
> >  /**



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115172): https://edk2.groups.io/g/devel/message/115172
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Pedro Falcato]

On Tue, Feb 6, 2024 at 1:32 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 2/5/24 15:03, duntan wrote:
> > 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.
>
> This patch does nothing to eliminate the actual race condition, it only
> shrinks the window of potential corruption.

FWIW, it's still not entirely correct: the compiler can tear the Uint64 store.
You'd need something like WRITE_ONCE (which in Linux essentially does
*(volatile Type *) ptr = val;)

> The PTEs continue to be overwritten without any kind of synchronization
> with the other processors.

I don't think we should be messing with page tables while APs are up.
That will require a whole infrastructure to do TLB shootdowns.

Zhou, Ray, what exactly is racing here?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115187): https://edk2.groups.io/g/devel/message/115187
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Zhou, Jianfeng]

Hi Laszlo, Pedro,

Clarify one thing, this change is not for racing introduced by MP reading/writing to the same page table at the same time, but for unexpected behavior introduced by compiler.
As my understanding,  MP reading/writing to the same page table at the same time is not recommended, perhaps, it is not allowed.

For bit operation code, such as Pnle->Bits.Present = Attribute->Bits.Present, we might think it is atomic assignment, while not. The assembly code looks like:
    and dword [rcx], 0xfffffffe
    and eax, 0x1
    or [rcx], eax
In case Pnle->Bits.Present = 1,  Attribute->Bits.Present = 1,  we might think it is harmless, as the value not changed.  While actually,
    and dword [rcx], 0xfffffffe  // the present bit set to 0 ---- this is unexpected !!!!! we don’t want the present bit set to 0!
    and eax, 0x1
    or [rcx], eax             // the present bit set to right value 1

Let's consider such a MP scenario: 
1) one processor executing instruction "and dword [rcx], 0xfffffffe"
2) other processors happened to access the memory mapped by Pnle, it may lead to exception.

We hit this case recently.  Several engineers pay days for test, root case and verification:  the reproducibility rate is low and not reproduced on every system.

We can fix it by other solution, while we decided to upstream this change for:
1) the change is harmless
2) It is a defect
3) It hard to debug and root cause
4) We don't want other engineers to spend a lot of time dealing with this kind of problem.


Thanks & Regards,
Zhou Jianfeng

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Wednesday, February 7, 2024 1:35 AM
To: devel@edk2.groups.io; lersek@redhat.com
Cc: Tan, Dun <dun.tan@intel.com>; Zhou, Jianfeng <jianfeng.zhou@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

On Tue, Feb 6, 2024 at 1:32 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 2/5/24 15:03, duntan wrote:
> > 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.
>
> This patch does nothing to eliminate the actual race condition, it 
> only shrinks the window of potential corruption.

FWIW, it's still not entirely correct: the compiler can tear the Uint64 store.
You'd need something like WRITE_ONCE (which in Linux essentially does *(volatile Type *) ptr = val;)

> The PTEs continue to be overwritten without any kind of 
> synchronization with the other processors.

I don't think we should be messing with page tables while APs are up.
That will require a whole infrastructure to do TLB shootdowns.

Zhou, Ray, what exactly is racing here?

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115194): https://edk2.groups.io/g/devel/message/115194
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Pedro Falcato]

On Wed, Feb 7, 2024 at 12:47 AM Zhou, Jianfeng <jianfeng.zhou@intel.com> wrote:
>
> Hi Laszlo, Pedro,
>
> Clarify one thing, this change is not for racing introduced by MP reading/writing to the same page table at the same time, but for unexpected behavior introduced by compiler.
> As my understanding,  MP reading/writing to the same page table at the same time is not recommended, perhaps, it is not allowed.

AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code.

>
> For bit operation code, such as Pnle->Bits.Present = Attribute->Bits.Present, we might think it is atomic assignment, while not. The assembly code looks like:
>     and dword [rcx], 0xfffffffe
>     and eax, 0x1
>     or [rcx], eax
> In case Pnle->Bits.Present = 1,  Attribute->Bits.Present = 1,  we might think it is harmless, as the value not changed.  While actually,
>     and dword [rcx], 0xfffffffe  // the present bit set to 0 ---- this is unexpected !!!!! we don’t want the present bit set to 0!
>     and eax, 0x1
>     or [rcx], eax             // the present bit set to right value 1
>
> Let's consider such a MP scenario:
> 1) one processor executing instruction "and dword [rcx], 0xfffffffe"
> 2) other processors happened to access the memory mapped by Pnle, it may lead to exception.

I understand your original problem, but:

1) Your fix is not correct. The compiler can tear your store, you need
to use a volatile store for this.
2) What kind of page table manipulation is happening while APs are
running code, and does this mean you need a TLB shootdown mechanism?

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115207): https://edk2.groups.io/g/devel/message/115207
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Zhou, Jianfeng]

[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]

Hi Pedro,

>>  AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code.
[Zhou] The scenario I mentioned/The case we hit is during BIOS boot, not software after EndOfBootService.

>>  1) Your fix is not correct. The compiler can tear your store, you need to use a volatile store for this.
[Zhou] Assembly code of function PageTableLibSetPnle attached. The code is expected. 
      Can't get "need to use a volatile store for this",  would you please share more detail about it?

>>  2) What kind of page table manipulation is happening while APs are running code, and does this mean you need a TLB shootdown mechanism?
[Zhou]  a) happened while split 2M Page to 4K to make full use of memory: it is long story.  
         Anyway, the bit operation code is unexpected, and my change is harmless.
       b) my understanding, TLB shootdown mechanism not suitable for this case. It's too late to "shootdown".  AP might exception before "shootdown"

One more extreme scenario, the code " Pnle->Bits.Present = Attribute->Bits.Present " happened to be mapped by Pnle, it will lead to exception even in one processor case.  Just mentioning, please ignore this scenario as it is hard to verify.

Thanks & Regards,
Zhou Jianfeng

-----Original Message-----
From: Pedro Falcato <pedro.falcato@gmail.com> 
Sent: Wednesday, February 7, 2024 9:05 AM
To: Zhou, Jianfeng <jianfeng.zhou@intel.com>
Cc: devel@edk2.groups.io; lersek@redhat.com; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

On Wed, Feb 7, 2024 at 12:47 AM Zhou, Jianfeng <jianfeng.zhou@intel.com> wrote:
>
> Hi Laszlo, Pedro,
>
> Clarify one thing, this change is not for racing introduced by MP reading/writing to the same page table at the same time, but for unexpected behavior introduced by compiler.
> As my understanding,  MP reading/writing to the same page table at the same time is not recommended, perhaps, it is not allowed.

AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code.

>
> For bit operation code, such as Pnle->Bits.Present = Attribute->Bits.Present, we might think it is atomic assignment, while not. The assembly code looks like:
>     and dword [rcx], 0xfffffffe
>     and eax, 0x1
>     or [rcx], eax
> In case Pnle->Bits.Present = 1,  Attribute->Bits.Present = 1,  we might think it is harmless, as the value not changed.  While actually,
>     and dword [rcx], 0xfffffffe  // the present bit set to 0 ---- this is unexpected !!!!! we don’t want the present bit set to 0!
>     and eax, 0x1
>     or [rcx], eax             // the present bit set to right value 1
>
> Let's consider such a MP scenario:
> 1) one processor executing instruction "and dword [rcx], 0xfffffffe"
> 2) other processors happened to access the memory mapped by Pnle, it may lead to exception.

I understand your original problem, but:

1) Your fix is not correct. The compiler can tear your store, you need to use a volatile store for this.
2) What kind of page table manipulation is happening while APs are running code, and does this mean you need a TLB shootdown mechanism?

--
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115257): https://edk2.groups.io/g/devel/message/115257
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: PageTableLibSetPnle.s --]
[-- Type: application/octet-stream, Size: 1207 bytes --]

0x7FD42994 PageTableLibSetPnle:         test byte [r8], 0x1
0x7FD42998    mov r10, [rcx]
0x7FD4299B    mov [rsp+0x8], r10
0x7FD429A0    mov r9d, r10d
0x7FD429A3    jz 0x7fd429b0
0x7FD429A5    mov eax, [rdx]
0x7FD429A7    xor eax, r10d
0x7FD429AA    and eax, 0x1
0x7FD429AD    xor r9d, eax
0x7FD429B0    test byte [r8], 0x2
0x7FD429B4    jz 0x7fd429c1
0x7FD429B6    mov eax, [rdx]
0x7FD429B8    xor eax, r9d
0x7FD429BB    and eax, 0x2
0x7FD429BE    xor r9d, eax
0x7FD429C1    test byte [r8], 0x4
0x7FD429C5    jz 0x7fd429d2
0x7FD429C7    mov eax, [rdx]
0x7FD429C9    xor eax, r9d
0x7FD429CC    and eax, 0x4
0x7FD429CF    xor r9d, eax
0x7FD429D2    cmp dword [r8+0x4], 0x80000000
0x7FD429DA    jb 0x7fd429ee
0x7FD429DC    mov eax, [rdx+0x4]
0x7FD429DF    xor eax, [rsp+0xc]
0x7FD429E3    btr eax, 0x1f
0x7FD429E7    xor eax, [rdx+0x4]
0x7FD429EA    mov [rsp+0xc], eax
0x7FD429EE    and r9d, 0xffffff47
0x7FD429F5    mov [rsp+0x8], r9d   ###
0x7FD429FA    mov rax, [rsp+0x8]   ###
0x7FD429FF    cmp r10, rax         ### if (Pnle->Uint64 != LocalPnle.Uint64)
0x7FD42A02    jz 0x7fd42a07
0x7FD42A04    mov [rcx], rax       ### Pnle->Uint64 = LocalPnle.Uint64
0x7FD42A07    ret

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Pedro Falcato]

On Wed, Feb 7, 2024 at 1:57 AM Zhou, Jianfeng <jianfeng.zhou@intel.com> wrote:
>
> Hi Pedro,
>
> >>  AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code.
> [Zhou] The scenario I mentioned/The case we hit is during BIOS boot, not software after EndOfBootService.

I know, but the PI spec mentions (in the EFI_MP_SERVICES section) that
you need to be careful and can't call EFI code willy-nilly, which is
why (most) EFI code does not need to handle concurrency and locking.

>
> >>  1) Your fix is not correct. The compiler can tear your store, you need to use a volatile store for this.
> [Zhou] Assembly code of function PageTableLibSetPnle attached. The code is expected.
>       Can't get "need to use a volatile store for this",  would you please share more detail about it?

The compiler (and this is defined by the C spec) does not know that
your write has side effects. As such, it can re-order it, delete it,
split it in half, etc.
Say, for example:

UINT64 gGlobal;
UINT64 gGlobal2;

VOID
Foo (VOID)
{
    gGlobal = 10;
    gGlobal2 = 20;
}

The compiler can legally generate this (I hope i get the intel syntax
right, i usually write AT&T :)):
mov qword ptr [gGlobal2], 20
mov qword ptr [gGlobal], 10

or

mov dword ptr [gGlobal2], 20
mov dword ptr [gGlobal2 + 4], 0
mov dword ptr [gGlobal], 10
mov dword ptr [gGlobal + 4], 0

or any other variant, because "there are no side effects".
With volatile, the compiler knows there are side-effects so the
optimization it can do is quite limited.

>
> >>  2) What kind of page table manipulation is happening while APs are running code, and does this mean you need a TLB shootdown mechanism?
> [Zhou]  a) happened while split 2M Page to 4K to make full use of memory: it is long story.
>          Anyway, the bit operation code is unexpected, and my change is harmless.
>        b) my understanding, TLB shootdown mechanism not suitable for this case. It's too late to "shootdown".  AP might exception before "shootdown"

This is a parallel concern to this patch.
If you're running other APs and changing page table entries you *need*
to shoot their TLB entries down, for correctness. Or at least make
sure that when an AP goes to execute code, the TLB is completely
synchronized (by, say, a CR3 reload in every AP right after a wakeup).

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115241): https://edk2.groups.io/g/devel/message/115241
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Laszlo Ersek]

On 2/7/24 01:47, Zhou, Jianfeng wrote:
> Hi Laszlo, Pedro,
> 
> Clarify one thing, this change is not for racing introduced by MP reading/writing to the same page table at the same time, but for unexpected behavior introduced by compiler.

I thought that patch#1 was related to MP paging because:

- patch#3 *was* indeed related to MP paging, and these patches are,
well, part of the same series;

- more importantly, the commit message on patch#1 explicitly says, "If
*some other core* is accessing the page, it may leads to expection" --
emphasis mine.

(The first patch also uses the word "optimize", which I find very
misleading.)

Furthermore, the logic change in the first patch targets
CpuPageTableLib. As far as I can tell, the logic change bubbles up to
the public PageTableMap() API. I don't know if that API is designed for
just UP usage (a processor can use it only for manipulating its own page
tables), or for MP usage. Given the larger context of patch#1, I assumed
there was an MP context to patch#1 as well. (And, again, the commit
message explicitly references other cores.)

> As my understanding,  MP reading/writing to the same page table at the same time is not recommended, perhaps, it is not allowed.
> 
> For bit operation code, such as Pnle->Bits.Present = Attribute->Bits.Present, we might think it is atomic assignment, while not. The assembly code looks like:
>     and dword [rcx], 0xfffffffe
>     and eax, 0x1
>     or [rcx], eax
> In case Pnle->Bits.Present = 1,  Attribute->Bits.Present = 1,  we might think it is harmless, as the value not changed.  While actually,
>     and dword [rcx], 0xfffffffe  // the present bit set to 0 ---- this is unexpected !!!!! we don’t want the present bit set to 0!
>     and eax, 0x1
>     or [rcx], eax             // the present bit set to right value 1
> 
> Let's consider such a MP scenario: 
> 1) one processor executing instruction "and dword [rcx], 0xfffffffe"
> 2) other processors happened to access the memory mapped by Pnle, it may lead to exception.
> 
> We hit this case recently.  Several engineers pay days for test, root case and verification:  the reproducibility rate is low and not reproduced on every system.

OK, so it *does* happen in an MP context, too.

> 
> We can fix it by other solution, while we decided to upstream this change for:
> 1) the change is harmless
> 2) It is a defect
> 3) It hard to debug and root cause
> 4) We don't want other engineers to spend a lot of time dealing with this kind of problem.

This is all very nice; I can accept that the present code change may be
a useful practical improvement. My issue is that the commit message does
not represent either the problem faithfully, or the scope / intent of
the code changes.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115249): https://edk2.groups.io/g/devel/message/115249
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Laszlo Ersek]

On 2/7/24 02:05, Pedro Falcato wrote:
> On Wed, Feb 7, 2024 at 12:47 AM Zhou, Jianfeng <jianfeng.zhou@intel.com> wrote:
>>
>> Hi Laszlo, Pedro,
>>
>> Clarify one thing, this change is not for racing introduced by MP reading/writing to the same page table at the same time, but for unexpected behavior introduced by compiler.
>> As my understanding,  MP reading/writing to the same page table at the same time is not recommended, perhaps, it is not allowed.
> 
> AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code.

It need not be about two processors manipulating the same page table; it
suffices (for the problem) if one processor is massaging the page table
while another processor is accesing the affected virtual address range.
The AP may be running custom code that does not touch UEFI / PI services
at all (computing something in preallocated memory etc).

> 
>>
>> For bit operation code, such as Pnle->Bits.Present = Attribute->Bits.Present, we might think it is atomic assignment, while not. The assembly code looks like:
>>     and dword [rcx], 0xfffffffe
>>     and eax, 0x1
>>     or [rcx], eax
>> In case Pnle->Bits.Present = 1,  Attribute->Bits.Present = 1,  we might think it is harmless, as the value not changed.  While actually,
>>     and dword [rcx], 0xfffffffe  // the present bit set to 0 ---- this is unexpected !!!!! we don’t want the present bit set to 0!
>>     and eax, 0x1
>>     or [rcx], eax             // the present bit set to right value 1
>>
>> Let's consider such a MP scenario:
>> 1) one processor executing instruction "and dword [rcx], 0xfffffffe"
>> 2) other processors happened to access the memory mapped by Pnle, it may lead to exception.
> 
> I understand your original problem, but:
> 
> 1) Your fix is not correct. The compiler can tear your store, you need
> to use a volatile store for this.

(Side comment: I don't disagree that "volatile" may be, and mostly is,
sufficient for preventing tearing, but I'd like to note that the C99
spec itself does not seem to require that behavior of volatile. Section
"5.1.2.3 Program execution", paragraph 5 says, "The least requirements
on a conforming implementation are: — At sequence points, volatile
objects are stable in the sense that previous accesses are
complete and subsequent accesses have not yet occurred. [...]". I think
Linux is very correct to use a dedicated macro WRITE_ONCE, and if that
*happens* to expand to a volatile access on a particular platform &
compiler, that's great, but an implementation detail.

In edk2, I would like to see something stronger than just volatile; at
least semantically. For example, we have MmioWrite64() from IoLib, we
have EFI_CPU_IO2_PROTOCOL.Mem.Write() etc. If they are implemented with
plain "volatile" in the background, that may be fine in practice. It's
just always difficult to tell from the code and the patches whether an
access is *supposed* to be "atomic" or not!)

> 2) What kind of page table manipulation is happening while APs are
> running code, and does this mean you need a TLB shootdown mechanism?
> 

Yes, ideally, patch#1's commit message should include a stack trace for
CPU A and another for CPU B. I reckon those may be difficult to collect
*precisely*, but some natural language explanation would help.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115250): https://edk2.groups.io/g/devel/message/115250
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Laszlo Ersek]

On 2/7/24 02:57, Zhou, Jianfeng wrote:
> Hi Pedro,
> 
>>>  AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code.
> [Zhou] The scenario I mentioned/The case we hit is during BIOS boot, not software after EndOfBootService.
> 
>>>  1) Your fix is not correct. The compiler can tear your store, you need to use a volatile store for this.
> [Zhou] Assembly code of function PageTableLibSetPnle attached. The code is expected. 
>       Can't get "need to use a volatile store for this",  would you please share more detail about it?

With the patch in place, the compiler *happens* to generate code that
uses a single instruction. That's nice, but how stable is that?

IIUC, Pedro's point is that the "Pte4K" parameter of
PageTableLibSetPte4K() should point to a volatile IA32_PTE_4K object,
because that would *guarantee* that the compiler *always* generates a
single instruction for the final assignment.

(Now, I'm not sure about that, i.e. that even volatile is strong enough,
but that's a different topic.)

--*--

BTW, with this patch in place (as commit 30a25f277821, "UefiCpuPkg:
Reduce and optimize access to attribute", 2024-02-06), we have:

VOID
PageTableLibSetPte4K (
  IN IA32_PTE_4K         *Pte4K,
  IN UINT64              Offset,
  IN IA32_MAP_ATTRIBUTE  *Attribute,
  IN IA32_MAP_ATTRIBUTE  *Mask
  )
{
  IA32_PTE_4K  LocalPte4K;

  LocalPte4K.Uint64 = Pte4K->Uint64;
...
  if (Pte4K->Uint64 != LocalPte4K.Uint64) {
    Pte4K->Uint64 = LocalPte4K.Uint64;
  }
}

This means that the "Pte4K" parameter should be marked "IN OUT", not
just "IN". (Independently of whether we also qualify (*Pte4K) as volatile.)

Of course, it's not a bug in the patch, it's a (documentation) bug in
the pre-patch code. Can you perhaps submit a patch to fix that?

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115251): https://edk2.groups.io/g/devel/message/115251
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

[Zhou, Jianfeng]

Sorry for misleading information in the commit message.
1) regarding the compiler tear down, I have reservations.  Anyway, adding volatile is harmless,  will submit patch for it
2) regarding "IN OUT" for parameter,  will add "OUT" for the parameter in the patch

Thanks & Regards,
Zhou Jianfeng

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: Thursday, February 8, 2024 4:42 AM
To: Zhou, Jianfeng <jianfeng.zhou@intel.com>; Pedro Falcato <pedro.falcato@gmail.com>
Cc: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Reduce and optimize access to attribute

On 2/7/24 02:57, Zhou, Jianfeng wrote:
> Hi Pedro,
> 
>>>  AFAIK, it's not allowed as IIRC APs cannot run arbitrary EFI boot services code.
> [Zhou] The scenario I mentioned/The case we hit is during BIOS boot, not software after EndOfBootService.
> 
>>>  1) Your fix is not correct. The compiler can tear your store, you need to use a volatile store for this.
> [Zhou] Assembly code of function PageTableLibSetPnle attached. The code is expected. 
>       Can't get "need to use a volatile store for this",  would you please share more detail about it?

With the patch in place, the compiler *happens* to generate code that uses a single instruction. That's nice, but how stable is that?

IIUC, Pedro's point is that the "Pte4K" parameter of
PageTableLibSetPte4K() should point to a volatile IA32_PTE_4K object, because that would *guarantee* that the compiler *always* generates a single instruction for the final assignment.

(Now, I'm not sure about that, i.e. that even volatile is strong enough, but that's a different topic.)

--*--

BTW, with this patch in place (as commit 30a25f277821, "UefiCpuPkg:
Reduce and optimize access to attribute", 2024-02-06), we have:

VOID
PageTableLibSetPte4K (
  IN IA32_PTE_4K         *Pte4K,
  IN UINT64              Offset,
  IN IA32_MAP_ATTRIBUTE  *Attribute,
  IN IA32_MAP_ATTRIBUTE  *Mask
  )
{
  IA32_PTE_4K  LocalPte4K;

  LocalPte4K.Uint64 = Pte4K->Uint64;
...
  if (Pte4K->Uint64 != LocalPte4K.Uint64) {
    Pte4K->Uint64 = LocalPte4K.Uint64;
  }
}

This means that the "Pte4K" parameter should be marked "IN OUT", not just "IN". (Independently of whether we also qualify (*Pte4K) as volatile.)

Of course, it's not a bug in the patch, it's a (documentation) bug in the pre-patch code. Can you perhaps submit a patch to fix that?

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115297): https://edk2.groups.io/g/devel/message/115297
Mute This Topic: https://groups.io/mt/104176232/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-