Re: [edk2-devel] [Patch V2 0/8] Create page table by CpuPageTableLib in DxeIpl

["duntan" <dun.tan@intel.com>]

Thank you a lot! I misunderstood the condition for remapping GHCB page in previous code. I'll fix the issue in next version patch. 

Thanks,
Dun

-----Original Message-----
From: Tom Lendacky <thomas.lendacky@amd.com> 
Sent: Tuesday, April 4, 2023 1:14 AM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [Patch V2 0/8] Create page table by CpuPageTableLib in DxeIpl

On 4/3/23 09:24, Tom Lendacky wrote:
> On 3/31/23 09:35, Tom Lendacky wrote:
>> On 3/31/23 04:41, Tan, Dun wrote:
>>> Hi Tom,
>>>
>>> Reccentlly I sent this patch set to change DxeIpl code to use 
>>> CpuPageTableLib to create page table. I have done some test on Intel 
>>> CPU to make sure that the page table created by DxeIpl before the 
>>> change is the same as the page table created by DxeIpl after the 
>>> change. But there was a remaining case that I didn't cover. The case 
>>> is that PcdPteMemoryEncryptionAddressOrMask, PcdGhcbBase and 
>>> PcdGhcbSize are not zero(when memory encryption is enabled on AMD 
>>> processors supporting the SEV feature).
>>> So could you please help do a test on AMD processor to make sure 
>>> that the SEV feature still works good with this pacth set?
>>
>> Yes, I can test it.
> 
> This is breaking the SEV-ES and SEV-SNP boots. I'll see if I can 
> figure out what or where the breakage is, but this patchset can't be merged as is.

The following change to the patch series allows SEV-ES and SEV-SNP guests to boot.

Thanks,
Tom


diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index a9edf4de32..a3f16c7cf9 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -416,6 +416,7 @@ CreateIdentityMappingPageTables (
    IA32_MAP_ATTRIBUTE                           MapAttribute;
    IA32_MAP_ATTRIBUTE                           MapMask;
    EFI_PHYSICAL_ADDRESS                         GhcbBase4K;
+  EFI_PHYSICAL_ADDRESS                         GhcbBaseEnd;
  
    //
    // Make sure AddressEncMask is contained to smallest supported address field @@ -504,15 +505,21 @@ CreateIdentityMappingPageTables (
      //
      // The GHCB range consists of two pages per CPU, the GHCB and a
      // per-CPU variable page. The GHCB page needs to be mapped as an
-    // unencrypted page while the per-CPU variable page needs to be
-    // mapped encrypted. These pages alternate in assignment.
+    // unencrypted page while the per-CPU variable page needs to remain
+    // mapped as an encrypted page.
+    //
+    // Loop through the GHCB range, remapping the GHCB page unencrypted
+    // and skipping over the per-CPU variable page.
      //
      ASSERT (Is64BitPageTable == TRUE);
-    GhcbBase4K                           = ALIGN_VALUE (GhcbBase, SIZE_4KB);
-    MapAttribute.Uint64                  = GhcbBase4K;
-    MapMask.Uint64                       = 0;
-    MapMask.Bits.PageTableBaseAddressLow = 1;
-    CreateOrUpdatePageTable (&PageTable, PagingMode, GhcbBase4K, SIZE_4KB, &MapAttribute, &MapMask);
+    GhcbBase4K  = ALIGN_VALUE (GhcbBase, SIZE_4KB);
+    GhcbBaseEnd = ALIGN_VALUE (GhcbBase + GhcbSize, SIZE_4KB);
+    for (; GhcbBase4K < GhcbBaseEnd; GhcbBase4K += (SIZE_4KB * 2)) {
+      MapAttribute.Uint64                  = GhcbBase4K;
+      MapMask.Uint64                       = 0;
+      MapMask.Bits.PageTableBaseAddressLow = 1;
+      CreateOrUpdatePageTable (&PageTable, PagingMode, GhcbBase4K, SIZE_4KB, &MapAttribute, &MapMask);
+    }
    }
  
    if (PcdGetBool (PcdSetNxForStack)) {

> 
> Thanks,
> Tom
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>> Thanks,
>>> Dun
>>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
>>> duntan
>>> Sent: Friday, March 31, 2023 5:34 PM
>>> To: devel@edk2.groups.io
>>> Subject: [edk2-devel] [Patch V2 0/8] Create page table by 
>>> CpuPageTableLib in DxeIpl
>>>
>>> In this V2 patch set:
>>> 1.Remove the unneeded patch for ArmVirtPkg 2.In this patch 'Create 
>>> page table by CpuPageTableLib', change the input parameter name from 
>>> Is32BitPageTable to Is64BitPageTable and add a line of 
>>> "MapAttribute.Bits.Present = 0" before set a range to non-present.
>>> 3.In this patch 'Refinement to the code to set PageTable as RO', add 
>>> a line of "MapAttribute.Bits.ReadWrite = 0" before set a range to ReadOnly.
>>>
>>> Dun Tan (8):
>>>    EmulatorPkg: Add CpuPageTableLib required by DxeIpl in DSC
>>>    IntelFsp2Pkg: Add CpuPageTableLib required by DxeIpl in DSC
>>>    MdeModulePkg: Add CpuPageTableLib required by DxeIpl in DSC
>>>    OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file
>>>    MdeModulePkg: Add UefiCpuPkg.dec to pass DependencyCheck
>>>    MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib
>>>    MdeModulePkg/DxeIpl: Remove duplicated code to enable NX
>>>    MdeModulePkg/DxeIpl: Refinement to the code to set PageTable as 
>>> RO
>>>
>>>   EmulatorPkg/EmulatorPkg.dsc                      |   3 ++-
>>>   IntelFsp2Pkg/Tools/Tests/QemuFspPkg.dsc          |   3 ++-
>>>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.h            |   3 ++-
>>>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |   4 +++-
>>>   MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  | 112
>>> ++++----------------------------------------------------------------
>>> ++++--------------------------------------------
>>>   MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |   5 +++--
>>>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 711
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>>>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 182
>>> ++++++++++----------------------------------------------------------
>>> ++++++++++----------------------------------------------------------
>>> ++++++++++--------------------------------------------------------
>>>   MdeModulePkg/MdeModulePkg.ci.yaml                |   5 +++--
>>>   MdeModulePkg/MdeModulePkg.dsc                    |   3 ++-
>>>   OvmfPkg/AmdSev/AmdSevX64.dsc                     |   2 +-
>>>   OvmfPkg/Bhyve/BhyveX64.dsc                       |   3 ++-
>>>   OvmfPkg/CloudHv/CloudHvX64.dsc                   |   2 +-
>>>   OvmfPkg/Microvm/MicrovmX64.dsc                   |   2 +-
>>>   OvmfPkg/OvmfPkgIa32.dsc                          |   3 ++-
>>>   OvmfPkg/OvmfPkgIa32X64.dsc                       |   2 +-
>>>   OvmfPkg/OvmfPkgX64.dsc                           |   2 +-
>>>   OvmfPkg/OvmfXen.dsc                              |   2 +-
>>>   18 files changed, 200 insertions(+), 849 deletions(-)
>>>
>>> --
>>> 2.31.1.windows.1
>>>
>>>
>>>
>>> 
>>>
>>>