Re: [edk2-devel] [RFC PATCH v3 04/43] MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page tables

["Ni, Ray" <ray.ni@intel.com>]

Tom,
When are GHCP pages are allocated? Can DxeIpl gets the address by reading the GHCB MSR?

Can the GHCB PCD be eliminated by updating all GHCB address consumer to read the GHCB MSR?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas
> Sent: Thursday, November 21, 2019 4:06 AM
> To: devel@edk2.groups.io
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.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] [RFC PATCH v3 04/43] MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page tables
> 
> 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.
> 
> 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       | 49 ++++++++++++++-----
>  5 files changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 98bc17fc9d1f..5e6b78e295e6 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -111,6 +111,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..32a81d1f3c21 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) {
> +    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,12 @@ Split2MPageTo4K (
>      //
>      // Fill in the Page Table entries
>      //
> -    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
> +    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K;
> +    if (!GhcbBase
> +        || (PhysicalAddress4K < GhcbBase)
> +        || (PhysicalAddress4K >= GhcbBase + GhcbSize)) {
> +      PageTableEntry->Uint64 |= AddressEncMask;
> +    }
>      PageTableEntry->Bits.ReadWrite = 1;
> 
>      if ((IsNullDetectionEnabled () && PhysicalAddress4K == 0) ||
> @@ -383,6 +402,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 +411,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 +436,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 +639,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 +648,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 +836,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 +867,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);
>              } else {
>                //
>                // Fill in the Page Directory entries
> --
> 2.17.1
> 
> 
>