[Patch V8 00/14] Subject: [Patch V8 00/14] Use CpuPageTableLib to create and update smm page table

[Patch V8 00/14] Subject: [Patch V8 00/14] Use CpuPageTableLib to create and update smm page table

[duntan]

In the V8 patch set:
In 'OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry', I refined the commit message and added comments in the code around the areas being changed to explain this code change.

Only resend the changed patch in OvmfPkg. The patch set has been reviewed-by

Dun Tan (14):
  OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry
  MdeModulePkg: Remove other attribute protection in UnsetGuardPage
  UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.
  UefiCpuPkg: Add DEBUG_CODE for special case when clear RP
  UefiCpuPkg/PiSmmCpuDxeSmm: Avoid setting non-present range to RO/NX
  UefiCpuPkg/PiSmmCpuDxeSmm: Add 2 function to disable/enable CR0.WP
  UefiCpuPkg/PiSmmCpuDxeSmm: Clear CR0.WP before modify page table
  UefiCpuPkg: Extern mSmmShadowStackSize in PiSmmCpuDxeSmm.h
  UefiCpuPkg: Add GenSmmPageTable() to create smm page table
  UefiCpuPkg: Use GenSmmPageTable() to create Smm S3 page table
  UefiCpuPkg: Sort mSmmCpuSmramRanges in FindSmramInfo
  UefiCpuPkg: Sort mProtectionMemRange when ReadyToLock
  UefiCpuPkg: Refinement to smm runtime InitPaging() code
  UefiCpuPkg/PiSmmCpuDxeSmm: Remove unnecessary function

 MdeModulePkg/Core/PiSmmCore/HeapGuard.c                        |  16 +++++++++++++++-
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c |  23 +++++++++++++++++++----
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c                       |   5 +++--
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c                  |   3 +--
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c                |   2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c                          | 132 ------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c                     |  40 ++++++++++++++++++++++++++++++++++++++--
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h                     | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf                   |   1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c             | 793 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c                         | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c                        | 229 ++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c                   |   3 +--
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c                 |  20 ++++----------------
 14 files changed, 696 insertions(+), 1014 deletions(-)

-- 
2.31.1.windows.1

[Patch V8 01/14] OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry

[duntan]

Remove code that sets AddressEncMask for non-leaf entries when
modifing smm page table by MemEncryptSevLib. In FvbServicesSmm
driver, it calls MemEncryptSevClearMmioPageEncMask to clear
AddressEncMask bit in page table for a specific range. In AMD
SEV feature, this AddressEncMask bit in page table is used to
indicate if the memory is guest private memory or shared memory.
But all memory accessed by the hardware page table walker is
treated as encrypted, regardless of whether the encryption bit
is present. So remove the code to set the EncMask bit for smm
non-leaf entries doesn't impact AMD SEV feature.

The reason encryption mask should not be set for non-leaf
entries is because CpuPageTableLib doesn't consume encryption
mask PCD. In PiSmmCpuDxeSmm module, it will use CpuPageTableLib
to modify smm page table in next patch. The encryption mask is
overlapped with the PageTableBaseAddress field of non-leaf page
table entries. If the encryption mask is set for smm non-leaf
page table entries, issue happens when CpuPageTableLib code
use the non-leaf entry PageTableBaseAddress field with the
encryption mask set to find the next level page table.

Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
index cf2441b551..dee3fb8914 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
@@ -232,8 +232,14 @@ Split2MPageTo4K (
   //
   // Fill in 2M page entry.
   //
+  // AddressEncMask is not set for non-leaf entries since CpuPageTableLib doesn't consume
+  // encryption mask PCD. The encryption mask is overlapped with the PageTableBaseAddress
+  // field of non-leaf page table entries. If encryption mask is set for non-leaf entries,
+  // issue happens when CpuPageTableLib code use the non-leaf entry PageTableBaseAddress
+  // field with the encryption mask set to find the next level page table.
+  //
   *PageEntry2M = ((UINT64)(UINTN)PageTableEntry1 |
-                  IA32_PG_P | IA32_PG_RW | AddressEncMask);
+                  IA32_PG_P | IA32_PG_RW);
 }
 
 /**
@@ -352,7 +358,10 @@ SetPageTablePoolReadOnly (
         PhysicalAddress += LevelSize[Level - 1];
       }
 
-      PageTable[Index] = (UINT64)(UINTN)NewPageTable | AddressEncMask |
+      //
+      // AddressEncMask is not set for non-leaf entries because of the way CpuPageTableLib works
+      //
+      PageTable[Index] = (UINT64)(UINTN)NewPageTable |
                          IA32_PG_P | IA32_PG_RW;
       PageTable = NewPageTable;
     }
@@ -439,8 +448,10 @@ Split1GPageTo2M (
   //
   // Fill in 1G page entry.
   //
+  // AddressEncMask is not set for non-leaf entries because of the way CpuPageTableLib works
+  //
   *PageEntry1G = ((UINT64)(UINTN)PageDirectoryEntry |
-                  IA32_PG_P | IA32_PG_RW | AddressEncMask);
+                  IA32_PG_P | IA32_PG_RW);
 
   PhysicalAddress2M = PhysicalAddress;
   for (IndexOfPageDirectoryEntries = 0;
@@ -616,7 +627,11 @@ InternalMemEncryptSevCreateIdentityMap1G (
       }
 
       SetMem (NewPageTable, EFI_PAGE_SIZE, 0);
-      PageMapLevel4Entry->Uint64          = (UINT64)(UINTN)NewPageTable | AddressEncMask;
+
+      //
+      // AddressEncMask is not set for non-leaf entries because of the way CpuPageTableLib works
+      //
+      PageMapLevel4Entry->Uint64          = (UINT64)(UINTN)NewPageTable;
       PageMapLevel4Entry->Bits.MustBeZero = 0;
       PageMapLevel4Entry->Bits.ReadWrite  = 1;
       PageMapLevel4Entry->Bits.Present    = 1;
-- 
2.31.1.windows.1

Re: [edk2-devel] [Patch V8 00/14] Subject: [Patch V8 00/14] Use CpuPageTableLib to create and update smm page table

[Ard Biesheuvel]

On Thu, 29 Jun 2023 at 10:09, duntan <dun.tan@intel.com> wrote:
>
> In the V8 patch set:
> In 'OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry', I refined the commit message and added comments in the code around the areas being changed to explain this code change.
>
> Only resend the changed patch in OvmfPkg. The patch set has been reviewed-by
>
> Dun Tan (14):
>   OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry
>   MdeModulePkg: Remove other attribute protection in UnsetGuardPage


>   UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.

This patch breaks SMM on IA32.

!!!! IA32 Exception Type - 0E(#PF - Page-Fault)  CPU Apic ID - 00000000 !!!!
ExceptionData - 00000008  I:0 R:1 U:0 W:0 P:0 PK:0 SS:0 SGX:0
EIP  - 07FF97A6, CS  - 00000008, EFLAGS - 00000046
EAX  - 07FF2400, ECX - 07FC5140, EDX - 06AD7120, EBX - FFFFFFFF
ESP  - 07FCCDB4, EBP - 07FCCF4C, ESI - 00000000, EDI - 00000000
DS   - 00000020, ES  - 00000020, FS  - 00000020, GS  - 00000020, SS - 00000020
CR0  - 8001003B, CR2 - 06AD713C, CR3 - 07FA5000, CR4 - 00000668
DR0  - 00000000, DR1 - 00000000, DR2 - 00000000, DR3 - 00000000
DR6  - FFFF0FF0, DR7 - 00000400
GDTR - 07FC3000 0000004F, IDTR - 07FC6000 000000FF
LDTR - 00000000, TR - 00000040
FXSAVE_STATE - 07FC7D60
qemu: terminating on signal 2

This appears to be a result from the following code in
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c:SmmInitPageTable()

@@ -31,7 +31,7 @@ SmmInitPageTable (
   InitializeSpinLock (mPFLock);

   mPhysicalAddressBits = 32;
   mPagingMode          = PagingPae;

which seems to be the wrong paging mode. However, 'Paging32bit' is not
actually supported by the library so changing it results in an
ASSERT():

Patch page table start ...

ASSERT_RETURN_ERROR (Status = Unsupported)
ASSERT [PiSmmCpuDxeSmm]
/home/ardb/build/edk2/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c(537):
!(((INTN)(RETURN_STATUS)(Status)) < 0)


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

Re: [edk2-devel] [Patch V8 00/14] Subject: [Patch V8 00/14] Use CpuPageTableLib to create and update smm page table

[duntan]

Hi Ard,

Could you send me your build and boot command? 

I think the paging mode for IA32 smm should be PagingPae instead of 'Paging32bit'. Also in previous code logic before my patch PagingPae is created for IA32 smm.

Thanks,
Dun

-----Original Message-----
From: Ard Biesheuvel <ardb@kernel.org> 
Sent: Thursday, September 21, 2023 5:06 PM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [Patch V8 00/14] Subject: [Patch V8 00/14] Use CpuPageTableLib to create and update smm page table

On Thu, 29 Jun 2023 at 10:09, duntan <dun.tan@intel.com> wrote:
>
> In the V8 patch set:
> In 'OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry', I refined the commit message and added comments in the code around the areas being changed to explain this code change.
>
> Only resend the changed patch in OvmfPkg. The patch set has been 
> reviewed-by
>
> Dun Tan (14):
>   OvmfPkg:Remove code that apply AddressEncMask to non-leaf entry
>   MdeModulePkg: Remove other attribute protection in UnsetGuardPage


>   UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.

This patch breaks SMM on IA32.

!!!! IA32 Exception Type - 0E(#PF - Page-Fault)  CPU Apic ID - 00000000 !!!!
ExceptionData - 00000008  I:0 R:1 U:0 W:0 P:0 PK:0 SS:0 SGX:0 EIP  - 07FF97A6, CS  - 00000008, EFLAGS - 00000046 EAX  - 07FF2400, ECX - 07FC5140, EDX - 06AD7120, EBX - FFFFFFFF ESP  - 07FCCDB4, EBP - 07FCCF4C, ESI - 00000000, EDI - 00000000
DS   - 00000020, ES  - 00000020, FS  - 00000020, GS  - 00000020, SS - 00000020
CR0  - 8001003B, CR2 - 06AD713C, CR3 - 07FA5000, CR4 - 00000668
DR0  - 00000000, DR1 - 00000000, DR2 - 00000000, DR3 - 00000000
DR6  - FFFF0FF0, DR7 - 00000400
GDTR - 07FC3000 0000004F, IDTR - 07FC6000 000000FF LDTR - 00000000, TR - 00000040 FXSAVE_STATE - 07FC7D60
qemu: terminating on signal 2

This appears to be a result from the following code in
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c:SmmInitPageTable()

@@ -31,7 +31,7 @@ SmmInitPageTable (
   InitializeSpinLock (mPFLock);

   mPhysicalAddressBits = 32;
   mPagingMode          = PagingPae;

which seems to be the wrong paging mode. However, 'Paging32bit' is not actually supported by the library so changing it results in an
ASSERT():

Patch page table start ...

ASSERT_RETURN_ERROR (Status = Unsupported) ASSERT [PiSmmCpuDxeSmm]
/home/ardb/build/edk2/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c(537):
!(((INTN)(RETURN_STATUS)(Status)) < 0)


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

Re: [edk2-devel] [Patch V8 00/14] Subject: [Patch V8 00/14] Use CpuPageTableLib to create and update smm page table

[Ard Biesheuvel]

On Thu, 21 Sept 2023 at 10:10, Tan, Dun <dun.tan@intel.com> wrote:
>
> Hi Ard,
>
> Could you send me your build and boot command?
>
> I think the paging mode for IA32 smm should be PagingPae instead of 'Paging32bit'. Also in previous code logic before my patch PagingPae is created for IA32 smm.
>


Build like this

build -p OvmfPkg/OvmfPkgIa32.dsc -b DEBUG -a IA32 -t GCC5 \
 -D SMM_REQUIRE -D SECURE_BOOT_ENABLE -D DEBUG_ON_SERIAL_PORT

and boot like this

qemu-system-x86_64 -M q35,smm=on -serial stdio \
-drive if=pflash,file=Build/OvmfIa32/DEBUG_GCC5/FV/OVMF_CODE.fd \
-drive if=pflash,file=Build/OvmfIa32/DEBUG_GCC5/FV/OVMF_VARS.fd


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