Re: [RFC PATCH 16/19] OvmfPkg/MemEncryptSevLib: Add support to validate > 4GB memory in PEI phase

["Yao, Jiewen" <jiewen.yao@intel.com>]

I recommend to use SIZE_1GB to indicate size, instead of BIT30, for readability.

===========================
    if (Length <= BIT30) {
      Length = 0;
    } else {
      Length -= BIT30;
    }

    PhysicalAddress += BIT30;
===========================

Also 
===========================
    if (Length <= BIT30) {
      Length = 0;
===========================
Can be:
===========================
    if (Length <= BIT30) {
      break;
===========================

Thank you

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Wednesday, March 24, 2021 11:32 PM
> To: devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; James Bottomley
> <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: [RFC PATCH 16/19] OvmfPkg/MemEncryptSevLib: Add support to
> validate > 4GB memory in PEI phase
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> The initial page built during the SEC phase is used by the
> MemEncryptSevSnpValidateSystemRam() for the system RAM validation. The
> page validation process requires using the PVALIDATE instruction;  the
> instruction accepts a virtual address of the memory region that needs
> to be validated. If hardware encounters a page table walk failure
> (due to page-not-present) then it raises #GP.
> 
> The initial page table built in SEC phase address up to 4GB. Add an
> internal function to extend the page table to cover > 4GB. The function
> builds 1GB entries in the page table for access > 4GB. This will provide
> the support to call PVALIDATE instruction for the virtual address >
> 4GB in PEI phase.
> 
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c     | 115
> ++++++++++++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c |
> 16 +++
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h           |  19
> ++++
>  3 files changed, 150 insertions(+)
> 
> diff --git
> a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index d3455e812b..33d9bafe9f 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -536,6 +536,121 @@ EnableReadOnlyPageWriteProtect (
>    AsmWriteCr0 (AsmReadCr0() | BIT16);
>  }
> 
> +RETURN_STATUS
> +EFIAPI
> +InternalMemEncryptSevCreateIdentityMap1G (
> +  IN    PHYSICAL_ADDRESS      Cr3BaseAddress,
> +  IN    PHYSICAL_ADDRESS      PhysicalAddress,
> +  IN    UINTN                 Length
> +  )
> +{
> +  PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
> +  PAGE_TABLE_1G_ENTRY            *PageDirectory1GEntry;
> +  UINT64                         PgTableMask;
> +  UINT64                         AddressEncMask;
> +  BOOLEAN                        IsWpEnabled;
> +  RETURN_STATUS                  Status;
> +
> +  //
> +  // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings.
> +  //
> +  PageMapLevel4Entry = NULL;
> +
> +  DEBUG ((
> +    DEBUG_VERBOSE,
> +    "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx\n",
> +    gEfiCallerBaseName,
> +    __FUNCTION__,
> +    Cr3BaseAddress,
> +    PhysicalAddress,
> +    (UINT64)Length
> +    ));
> +
> +  if (Length == 0) {
> +    return RETURN_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Check if we have a valid memory encryption mask
> +  //
> +  AddressEncMask = InternalGetMemEncryptionAddressMask ();
> +  if (!AddressEncMask) {
> +    return RETURN_ACCESS_DENIED;
> +  }
> +
> +  PgTableMask = AddressEncMask | EFI_PAGE_MASK;
> +
> +
> +  //
> +  // Make sure that the page table is changeable.
> +  //
> +  IsWpEnabled = IsReadOnlyPageWriteProtected ();
> +  if (IsWpEnabled) {
> +    DisableReadOnlyPageWriteProtect ();
> +  }
> +
> +  Status = EFI_SUCCESS;
> +
> +  while (Length)
> +  {
> +    //
> +    // If Cr3BaseAddress is not specified then read the current CR3
> +    //
> +    if (Cr3BaseAddress == 0) {
> +      Cr3BaseAddress = AsmReadCr3();
> +    }
> +
> +    PageMapLevel4Entry = (VOID*) (Cr3BaseAddress & ~PgTableMask);
> +    PageMapLevel4Entry += PML4_OFFSET(PhysicalAddress);
> +    if (!PageMapLevel4Entry->Bits.Present) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a:%a: bad PML4 for Physical=0x%Lx\n",
> +        gEfiCallerBaseName,
> +        __FUNCTION__,
> +        PhysicalAddress
> +        ));
> +      Status = RETURN_NO_MAPPING;
> +      goto Done;
> +    }
> +
> +    PageDirectory1GEntry = (VOID *)(
> +                             (PageMapLevel4Entry->Bits.PageTableBaseAddress <<
> +                              12) & ~PgTableMask
> +                             );
> +    PageDirectory1GEntry += PDP_OFFSET(PhysicalAddress);
> +    if (!PageDirectory1GEntry->Bits.Present) {
> +      PageDirectory1GEntry->Bits.Present = 1;
> +      PageDirectory1GEntry->Bits.MustBe1 = 1;
> +      PageDirectory1GEntry->Bits.MustBeZero = 0;
> +      PageDirectory1GEntry->Bits.ReadWrite = 1;
> +      PageDirectory1GEntry->Uint64 |= (UINT64)PhysicalAddress |
> AddressEncMask;
> +    }
> +
> +    if (Length <= BIT30) {
> +      Length = 0;
> +    } else {
> +      Length -= BIT30;
> +    }
> +
> +    PhysicalAddress += BIT30;
> +  }
> +
> +  //
> +  // Flush TLB
> +  //
> +  CpuFlushTlb();
> +
> +Done:
> +  //
> +  // Restore page table write protection, if any.
> +  //
> +  if (IsWpEnabled) {
> +    EnableReadOnlyPageWriteProtect ();
> +  }
> +
> +  return Status;
> +}
> 
>  /**
>    This function either sets or clears memory encryption bit for the memory
> diff --git
> a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> index ce8a05bb1f..41bf301efe 100644
> ---
> a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> +++
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiSnpSystemRamValidate.c
> @@ -16,6 +16,7 @@
> 
>  #include "../SnpPageStateChange.h"
>  #include "SnpPageStateTrack.h"
> +#include "VirtualMemory.h"
> 
>  STATIC SNP_VALIDATED_RANGE     *mRootNode;
> 
> @@ -62,9 +63,24 @@ SevSnpValidateSystemRam (
>  {
>    UINTN                   EndAddress;
>    SNP_VALIDATED_RANGE     *Range;
> +  EFI_STATUS              Status;
> 
>    EndAddress = BaseAddress + EFI_PAGES_TO_SIZE (NumPages);
> 
> +  //
> +  // The page table used in PEI can address up to 4GB memory. If we are asked
> to validate
> +  // a range above the 4GB, then create an identity mapping so that the
> PVALIDATE instruction
> +  // can execute correctly. If the page table entry is not present then PVALIDATE
> will
> +  // cause the #GP.
> +  //
> +  if (BaseAddress >= SIZE_4GB) {
> +    Status = InternalMemEncryptSevCreateIdentityMap1G (0, BaseAddress,
> +                  EFI_PAGES_TO_SIZE (NumPages));
> +    if (EFI_ERROR (Status)) {
> +      ASSERT (FALSE);
> +    }
> +  }
> +
>    //
>    // If the Root is NULL then its the first call. Lets initialize the List before
>    // we process the request.
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> index 996f94f07e..829dc96a1d 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> @@ -267,4 +267,23 @@ InternalMemEncryptSevGetAddressRangeState (
>    IN UINTN                    Length
>    );
> 
> +/**
> +  Create 1GB identity mapping for the specified virtual address range.
> +
> +  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
> +                                      current CR3)
> +  @param[in]  VirtualAddress          Virtual address to check
> +  @param[in]  Length                  Length of virtual address range
> +
> +  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +InternalMemEncryptSevCreateIdentityMap1G (
> +  IN    PHYSICAL_ADDRESS      Cr3BaseAddress,
> +  IN    PHYSICAL_ADDRESS      PhysicalAddress,
> +  IN    UINTN                 Length
> +  );
> +
>  #endif
> --
> 2.17.1