Re: [edk2-devel] [PATCH v13 05/46] MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page tables

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Lendacky, Thomas
> Sent: Friday, July 31, 2020 2:43 AM
> To: devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Ard Biesheuvel
> <ard.biesheuvel@arm.com>; Dong, Eric <eric.dong@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>
> Subject: [edk2-devel] [PATCH v13 05/46] MdeModulePkg/DxeIplPeim:
> Support GHCB pages when creating page tables
> 
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> GHCB pages must be mapped as shared pages, so modify the process of
> creating identity mapped pagetable entries so that GHCB entries are created
> without the encryption bit set. The GHCB range consists of two pages per
> CPU, the first being the GHCB and the second being a per-CPU variable page.
> Only the GHCB page is mapped as shared.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf       |  2 +
>  .../Core/DxeIplPeim/X64/VirtualMemory.h       | 12 +++-
>  .../Core/DxeIplPeim/Ia32/DxeLoadFunc.c        |  4 +-
>  .../Core/DxeIplPeim/X64/DxeLoadFunc.c         | 11 +++-
>  .../Core/DxeIplPeim/X64/VirtualMemory.c       | 57 +++++++++++++++----
>  5 files changed, 70 insertions(+), 16 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 3f1702854660..19b8a4c8aefa 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -115,6 +115,8 @@ [Pcd.IA32,Pcd.X64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse5LevelPageTable                  ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbBase                            ##
> CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdGhcbSize                            ##
> CONSUMES
> 
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> index 2d0493f109e8..6b7c38a441d6 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> @@ -201,6 +201,8 @@ EnableExecuteDisableBit (
>    @param[in, out] PageEntry2M           Pointer to 2M page entry.
>    @param[in]      StackBase             Stack base address.
>    @param[in]      StackSize             Stack size.
> +  @param[in]      GhcbBase              GHCB page area base address.
> +  @param[in]      GhcbSize              GHCB page area size.
> 
>  **/
>  VOID
> @@ -208,7 +210,9 @@ Split2MPageTo4K (
>    IN EFI_PHYSICAL_ADDRESS               PhysicalAddress,
>    IN OUT UINT64                         *PageEntry2M,
>    IN EFI_PHYSICAL_ADDRESS               StackBase,
> -  IN UINTN                              StackSize
> +  IN UINTN                              StackSize,
> +  IN EFI_PHYSICAL_ADDRESS               GhcbBase,
> +  IN UINTN                              GhcbSize
>    );
> 
>  /**
> @@ -217,6 +221,8 @@ Split2MPageTo4K (
> 
>    @param[in] StackBase  Stack base address.
>    @param[in] StackSize  Stack size.
> +  @param[in] GhcbBase   GHCB page area base address.
> +  @param[in] GhcbSize   GHCB page area size.
> 
>    @return The address of 4 level page map.
> 
> @@ -224,7 +230,9 @@ Split2MPageTo4K (
>  UINTN
>  CreateIdentityMappingPageTables (
>    IN EFI_PHYSICAL_ADDRESS   StackBase,
> -  IN UINTN                  StackSize
> +  IN UINTN                  StackSize,
> +  IN EFI_PHYSICAL_ADDRESS   GhcbBase,
> +  IN UINTN                  GhcbkSize
>    );
> 
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> index 6e8ca824d469..284b34818ca7 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
> @@ -123,7 +123,7 @@ Create4GPageTablesIa32Pae (
>          //
>          // Need to split this 2M page that covers stack range.
>          //
> -        Split2MPageTo4K (PhysicalAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
> +        Split2MPageTo4K (PhysicalAddress, (UINT64 *)
> + PageDirectoryEntry, StackBase, StackSize, 0, 0);
>        } else {
>          //
>          // Fill in the Page Directory entries @@ -282,7 +282,7 @@
> HandOffToDxeCore (
>      //
>      // Create page table and save PageMapLevel4 to CR3
>      //
> -    PageTables = CreateIdentityMappingPageTables (BaseOfStack,
> STACK_SIZE);
> +    PageTables = CreateIdentityMappingPageTables (BaseOfStack,
> + STACK_SIZE, 0, 0);
> 
>      //
>      // End of PEI phase signal
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> index f465eb1d8ac4..156a477d8467 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
> @@ -35,6 +35,8 @@ HandOffToDxeCore (
>    UINT32                          Index;
>    EFI_VECTOR_HANDOFF_INFO         *VectorInfo;
>    EFI_PEI_VECTOR_HANDOFF_INFO_PPI *VectorHandoffInfoPpi;
> +  VOID                            *GhcbBase;
> +  UINTN                           GhcbSize;
> 
>    //
>    // Clear page 0 and mark it as allocated if NULL pointer detection is
> enabled.
> @@ -81,12 +83,19 @@ HandOffToDxeCore (
>    TopOfStack = (VOID *) ((UINTN) BaseOfStack + EFI_SIZE_TO_PAGES
> (STACK_SIZE) * EFI_PAGE_SIZE - CPU_STACK_ALIGNMENT);
>    TopOfStack = ALIGN_POINTER (TopOfStack, CPU_STACK_ALIGNMENT);
> 
> +  //
> +  // Get the address and size of the GHCB pages  //  GhcbBase = (VOID
> + *) PcdGet64 (PcdGhcbBase);  GhcbSize = PcdGet64 (PcdGhcbSize);
> +
>    PageTables = 0;
>    if (FeaturePcdGet (PcdDxeIplBuildPageTables)) {
>      //
>      // Create page table and save PageMapLevel4 to CR3
>      //
> -    PageTables = CreateIdentityMappingPageTables
> ((EFI_PHYSICAL_ADDRESS) (UINTN) BaseOfStack, STACK_SIZE);
> +    PageTables = CreateIdentityMappingPageTables
> ((EFI_PHYSICAL_ADDRESS) (UINTN) BaseOfStack, STACK_SIZE,
> +
> + (EFI_PHYSICAL_ADDRESS) (UINTN) GhcbBase, GhcbSize);
>    } else {
>      //
>      // Set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 516cf908bc88..6831946c54d3 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -181,6 +181,8 @@ EnableExecuteDisableBit (
>    @param Size         Size of the given physical memory.
>    @param StackBase    Base address of stack.
>    @param StackSize    Size of stack.
> +  @param GhcbBase     Base address of GHCB pages.
> +  @param GhcbSize     Size of GHCB area.
> 
>    @retval TRUE      Page table should be split.
>    @retval FALSE     Page table should not be split.
> @@ -190,7 +192,9 @@ ToSplitPageTable (
>    IN EFI_PHYSICAL_ADDRESS               Address,
>    IN UINTN                              Size,
>    IN EFI_PHYSICAL_ADDRESS               StackBase,
> -  IN UINTN                              StackSize
> +  IN UINTN                              StackSize,
> +  IN EFI_PHYSICAL_ADDRESS               GhcbBase,
> +  IN UINTN                              GhcbSize
>    )
>  {
>    if (IsNullDetectionEnabled () && Address == 0) { @@ -209,6 +213,12 @@
> ToSplitPageTable (
>      }
>    }
> 
> +  if (GhcbBase != 0) {
> +    if ((Address < GhcbBase + GhcbSize) && ((Address + Size) > GhcbBase)) {
> +      return TRUE;
> +    }
> +  }
> +
>    return FALSE;
>  }
>  /**
> @@ -322,6 +332,8 @@ AllocatePageTableMemory (
>    @param[in, out] PageEntry2M           Pointer to 2M page entry.
>    @param[in]      StackBase             Stack base address.
>    @param[in]      StackSize             Stack size.
> +  @param[in]      GhcbBase              GHCB page area base address.
> +  @param[in]      GhcbSize              GHCB page area size.
> 
>  **/
>  VOID
> @@ -329,7 +341,9 @@ Split2MPageTo4K (
>    IN EFI_PHYSICAL_ADDRESS               PhysicalAddress,
>    IN OUT UINT64                         *PageEntry2M,
>    IN EFI_PHYSICAL_ADDRESS               StackBase,
> -  IN UINTN                              StackSize
> +  IN UINTN                              StackSize,
> +  IN EFI_PHYSICAL_ADDRESS               GhcbBase,
> +  IN UINTN                              GhcbSize
>    )
>  {
>    EFI_PHYSICAL_ADDRESS                  PhysicalAddress4K;
> @@ -355,7 +369,20 @@ Split2MPageTo4K (
>      //
>      // Fill in the Page Table entries
>      //
> -    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
> +    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K;
> +
> +    //
> +    // 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.
> +    //
> +    if ((GhcbBase == 0)
> +        || (PhysicalAddress4K < GhcbBase)
> +        || (PhysicalAddress4K >= GhcbBase + GhcbSize)
> +        || (((PhysicalAddress4K - GhcbBase) & SIZE_4KB) != 0)) {
> +      PageTableEntry->Uint64 |= AddressEncMask;
> +    }
>      PageTableEntry->Bits.ReadWrite = 1;
> 
>      if ((IsNullDetectionEnabled () && PhysicalAddress4K == 0) || @@ -383,6
> +410,8 @@ Split2MPageTo4K (
>    @param[in, out] PageEntry1G           Pointer to 1G page entry.
>    @param[in]      StackBase             Stack base address.
>    @param[in]      StackSize             Stack size.
> +  @param[in]      GhcbBase              GHCB page area base address.
> +  @param[in]      GhcbSize              GHCB page area size.
> 
>  **/
>  VOID
> @@ -390,7 +419,9 @@ Split1GPageTo2M (
>    IN EFI_PHYSICAL_ADDRESS               PhysicalAddress,
>    IN OUT UINT64                         *PageEntry1G,
>    IN EFI_PHYSICAL_ADDRESS               StackBase,
> -  IN UINTN                              StackSize
> +  IN UINTN                              StackSize,
> +  IN EFI_PHYSICAL_ADDRESS               GhcbBase,
> +  IN UINTN                              GhcbSize
>    )
>  {
>    EFI_PHYSICAL_ADDRESS                  PhysicalAddress2M;
> @@ -413,11 +444,11 @@ Split1GPageTo2M (
> 
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M
> += SIZE_2MB) {
> -    if (ToSplitPageTable (PhysicalAddress2M, SIZE_2MB, StackBase, StackSize))
> {
> +    if (ToSplitPageTable (PhysicalAddress2M, SIZE_2MB, StackBase,
> + StackSize, GhcbBase, GhcbSize)) {
>        //
>        // Need to split this 2M page that covers NULL or stack range.
>        //
> -      Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
> +      Split2MPageTo4K (PhysicalAddress2M, (UINT64 *)
> + PageDirectoryEntry, StackBase, StackSize, GhcbBase, GhcbSize);
>      } else {
>        //
>        // Fill in the Page Directory entries @@ -616,6 +647,8 @@
> EnablePageTableProtection (
> 
>    @param[in] StackBase  Stack base address.
>    @param[in] StackSize  Stack size.
> +  @param[in] GhcbBase   GHCB base address.
> +  @param[in] GhcbSize   GHCB size.
> 
>    @return The address of 4 level page map.
> 
> @@ -623,7 +656,9 @@ EnablePageTableProtection (  UINTN
> CreateIdentityMappingPageTables (
>    IN EFI_PHYSICAL_ADDRESS   StackBase,
> -  IN UINTN                  StackSize
> +  IN UINTN                  StackSize,
> +  IN EFI_PHYSICAL_ADDRESS   GhcbBase,
> +  IN UINTN                  GhcbSize
>    )
>  {
>    UINT32                                        RegEax;
> @@ -809,8 +844,8 @@ CreateIdentityMappingPageTables (
>          PageDirectory1GEntry = (VOID *) PageDirectoryPointerEntry;
> 
>          for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectory1GEntry++, PageAddress
> += SIZE_1GB) {
> -          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize)) {
> -            Split1GPageTo2M (PageAddress, (UINT64 *) PageDirectory1GEntry,
> StackBase, StackSize);
> +          if (ToSplitPageTable (PageAddress, SIZE_1GB, StackBase, StackSize,
> GhcbBase, GhcbSize)) {
> +            Split1GPageTo2M (PageAddress, (UINT64 *)
> + PageDirectory1GEntry, StackBase, StackSize, GhcbBase, GhcbSize);
>            } else {
>              //
>              // Fill in the Page Directory entries @@ -840,11 +875,11 @@
> CreateIdentityMappingPageTables (
>            PageDirectoryPointerEntry->Bits.Present = 1;
> 
>            for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries <
> 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PageAddress +=
> SIZE_2MB) {
> -            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase, StackSize)) {
> +            if (ToSplitPageTable (PageAddress, SIZE_2MB, StackBase,
> + StackSize, GhcbBase, GhcbSize)) {
>                //
>                // Need to split this 2M page that covers NULL or stack range.
>                //
> -              Split2MPageTo4K (PageAddress, (UINT64 *) PageDirectoryEntry,
> StackBase, StackSize);
> +              Split2MPageTo4K (PageAddress, (UINT64 *)
> + PageDirectoryEntry, StackBase, StackSize, GhcbBase, GhcbSize);


Looks to me that the logic remains the same when PcdGhcbBase and PcdGhcbSize
are of their default values. Therefore:
Acked-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>              } else {
>                //
>                // Fill in the Page Directory entries
> --
> 2.27.0
> 
> 
>