Re: [RFC PATCH v2 04/10] OvmfPkg/BaseMemcryptSevLib: Add SEV helper library

[Laszlo Ersek <lersek@redhat.com>]

On 03/21/17 22:13, Brijesh Singh wrote:
> Add Secure Encrypted Virtualization (SEV) helper library. The library provides
> the routines to set or clear memory encryption bit for a given memory region
> and functions to check whether SEV is enabled.

Please rewrap this commit message to 74 characters.

> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Include/Library/MemEncryptSevLib.h         |   69 +++++
>  .../BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf  |   46 +++
>  .../BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c   |  124 ++++++++
>  .../BaseMemEncryptSevLib/X64/MemEncryptSevLib.c    |  120 ++++++++
>  .../BaseMemEncryptSevLib/X64/VirtualMemory.c       |  304 ++++++++++++++++++++
>  .../BaseMemEncryptSevLib/X64/VirtualMemory.h       |  158 ++++++++++
>  OvmfPkg/OvmfPkgIa32.dsc                            |    1 
>  OvmfPkg/OvmfPkgIa32X64.dsc                         |    1 
>  OvmfPkg/OvmfPkgX64.dsc                             |    1 

Please format patches as described under

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23

In particular, the options

  --stat=1000                                  \
  --stat-graph-width=20                        \

will ensure that git does not use the "..." abbreviation in the
diffstat, while also keeping the plus/minus stats reasonably narrow.

>  9 files changed, 824 insertions(+)
>  create mode 100644 OvmfPkg/Include/Library/MemEncryptSevLib.h
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> 
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> new file mode 100644
> index 0000000..a9e9356
> --- /dev/null
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -0,0 +1,69 @@
> +/** @file
> +
> +  Define Secure Encrypted Virtualization (SEV) base library helper function
> +
> +  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at

Overlong line. No line should be wider than 79 chars.

> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __MEM_ENCRYPT_SEV_LIB_H_
> +#define __MEM_ENCRYPT_SEV_LIB_H_

If you start the guard macro name with two underscores, then please also
end it with two underscores.

> +
> +#include <Base.h>
> +
> +/**
> +  Returns a boolean to indicate whether SEV is enabled
> +
> +  @retval TRUE           When SEV is active
> +  @retval FALSE          When SEV is not enabled
> +  **/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevIsEnabled (
> +  VOID
> +  );

Would it make sense to call this library function in PlatformPei, rather
than add a separate SevIsEnabled() function to it (in patch #3)? The
implementations look nearly identical.

The client module type restrictions under LIBRARY_CLASS (below) look
fine for that; all of the permitted module types support writeable
global variables in OVMF.

> +
> +/**
> +  This function clears memory encryption bit for the memory region specified by BaseAddress and
> +  Number of pages from the current page table context.
> +
> +  @param[in]  BaseAddress           The physical address that is the start address of a memory region.
> +  @param[in]  NumberOfPages         The number of pages from start memory region.
> +
> +  @retval RETURN_SUCCESS            The attributes were cleared for the memory region.
> +  @retval RETURN_INVALID_PARAMETER  Number of pages is zero.
> +  @retval RETURN_UNSUPPORTED        Clearing memory encryption attribute is not supported
> +  **/

These lines as well are too wide.

> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevClearPageEncMask (
> +  IN PHYSICAL_ADDRESS         BaseAddress,
> +  IN UINT32                   NumberOfPages
> +  );
> +
> +/**
> +  This function sets memory encryption bit for the memory region specified by BaseAddress and
> +  Number of pages from the current page table context.
> +
> +  @param[in]  BaseAddress           The physical address that is the start address of a memory region.
> +  @param[in]  NumberOfPages         The number of pages from start memory region.
> +
> +  @retval RETURN_SUCCESS            The attributes were set for the memory region.
> +  @retval RETURN_INVALID_PARAMETER  Number of pages is zero.
> +  @retval RETURN_UNSUPPORTED        Clearing memory encryption attribute is not supported
> +  **/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevSetPageEncMask (
> +  IN PHYSICAL_ADDRESS         BaseAddress,
> +  IN UINT32                   NumberOfPages
> +  );
> +#endif // __MEM_ENCRYPT_SEV_LIB_H_
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
> new file mode 100644
> index 0000000..c23261f
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
> @@ -0,0 +1,46 @@
> +## @file

Please add one or two sentences under @file about the purpose of this
library instance.

> +#
> +# Copyright (c) 2017 Advanced Micro Devices. All rights reserved.<BR>
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD License
> +#  which accompanies this distribution. The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#
> +##

Please rewrap to 79 chars.

(I think I won't ask for that explicitly in any other locations; please
just ensure all new files conform to that.)

> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005

Please use "1.25" here, that's the most recent release of the INF spec
(to my knowledge).

> +  BASE_NAME                      = MemEncryptSevLib
> +  FILE_GUID                      = c1594631-3888-4be4-949f-9c630dbc842b
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = MemEncryptSevLib|PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +# VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[Sources.X64]
> +  X64/MemEncryptSevLib.c
> +  X64/VirtualMemory.c
> +
> +[Sources.IA32]
> +  Ia32/MemEncryptSevLib.c
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  MemoryAllocationLib
> +  CacheMaintenanceLib

Please sort the entries in each section alphabetically.

> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> new file mode 100644
> index 0000000..70fdd2e
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> @@ -0,0 +1,124 @@
> +/** @file
> +
> +  Secure Encrypted Virtualization (SEV) library helper function
> +
> +  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "Uefi.h"
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Register/Cpuid.h>
> +#include <Register/AmdSevMap.h>
> +#include <Library/MemEncryptSevLib.h>
> +
> +STATIC BOOLEAN SevStatus = FALSE;
> +STATIC BOOLEAN SevStatusChecked = FALSE;

These variables should be called mSevStatus and mSevStatusChecked.

> +
> +/**
> + 
> +  Returns a boolean to indicate whether SEV is enabled
> +
> +  @retval TRUE           When SEV is active
> +  @retval FALSE          When SEV is not enabled
> +  **/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevIsEnabled (
> +  VOID
> +  )
> +{
> +  UINT32 RegEax;
> +  MSR_SEV_STATUS_REGISTER Msr;
> +  CPUID_MEMORY_ENCRYPTION_INFO_EAX  Eax;

Please align the variable names.

> +
> +  //
> +  // If Status is already checked then return it
> +  //
> +  if (SevStatusChecked) {
> +    return SevStatus;
> +  }
> +
> +  //
> +  // Check if memory encryption leaf exist
> +  //
> +  AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> +  if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
> +    //
> +    // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
> +    //
> +    AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +
> +    if (Eax.Bits.SevBit) {
> +      //
> +      // Check MSR_0xC0010131 Bit 0 (Sev is Enabled)
> +      //
> +      Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
> +      if (Msr.Bits.SevBit) {

You forgot to set mSevStatusChecked to TRUE here.

> +        return TRUE;
> +      }
> +    }
> +  }
> +
> +  SevStatusChecked = TRUE;
> +
> +  return FALSE;
> +}
> +
> +/**
> +  This function clears memory encryption bit for the memory region specified by BaseAddress and
> +  Number of pages from the current page table context.
> +
> +  @param[in]  BaseAddress             The physical address that is the start address of a memory region.
> +  @param[in]  NumberOfPages           The number of pages from start memory region.
> +
> +  @retval RETURN_SUCCESS              The attributes were cleared for the memory region.
> +  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
> +  @retval RETURN_UNSUPPORTED          Setting the memory encyrption attribute is not supported
> +  **/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevClearPageEncMask (
> +  IN PHYSICAL_ADDRESS         BaseAddress,
> +  IN UINT32                   NumberOfPages
> +  )
> +{
> +  //
> +  // Memory encryption bit is not accessible in 32-bit mode
> +  //
> +  return RETURN_UNSUPPORTED;
> +}
> +
> +/**
> +  This function sets memory encryption bit for the memory region specified by BaseAddress and
> +  Number of pages from the current page table context.
> +
> +  @param[in]  BaseAddress             The physical address that is the start address of a memory region.
> +  @param[in]  NumberOfPages           The number of pages from start memory region.
> +
> +  @retval RETURN_SUCCESS              The attributes were cleared for the memory region.
> +  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
> +  @retval RETURN_UNSUPPORTED          Setting the memory encyrption attribute is not supported
> +  **/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevSetPageEncMask (
> +  IN PHYSICAL_ADDRESS         BaseAddress,
> +  IN UINT32                   NumberOfPages
> +  )
> +{
> +  //
> +  // Memory encryption bit is not accessible in 32-bit mode
> +  //
> +  return RETURN_UNSUPPORTED;
> +}
> +
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> new file mode 100644
> index 0000000..098acf2
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> @@ -0,0 +1,120 @@
> +/** @file
> +
> +  Secure Encrypted Virtualization (SEV) library helper function
> +
> +  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "Uefi.h"
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Register/Cpuid.h>
> +#include <Register/AmdSevMap.h>
> +#include <Library/MemEncryptSevLib.h>
> +
> +#include "VirtualMemory.h"
> +
> +STATIC BOOLEAN SevStatus = FALSE;
> +STATIC BOOLEAN SevStatusChecked = FALSE;
> +
> +/**
> +  Returns a boolean to indicate whether SEV is enabled
> +
> +  @retval TRUE           When SEV is active
> +  @retval FALSE          When SEV is not enabled
> +  **/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevIsEnabled (
> +  VOID
> +  )
> +{
> +  UINT32 RegEax;
> +  MSR_SEV_STATUS_REGISTER Msr;
> +  CPUID_MEMORY_ENCRYPTION_INFO_EAX  Eax;
> +
> +  //
> +  // If Status is already checked then return it
> +  //
> +  if (SevStatusChecked) {
> +    return SevStatus;
> +  }
> +
> +  //
> +  // Check if memory encryption leaf exist
> +  //
> +  AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> +  if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
> +    //
> +    // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
> +    //
> +    AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
> +
> +    if (Eax.Bits.SevBit) {
> +      //
> +      // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
> +      //
> +      Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
> +      if (Msr.Bits.SevBit) {
> +        return TRUE;
> +      }
> +    }
> +  }
> +
> +  SevStatusChecked = TRUE;
> +
> +  return FALSE;
> +}

In fact, I suggest that:

(a) you introduce two new files, called

MemEncryptSevLibInternal.h
MemEncryptSevLibInternal.c

(b) add the .c file to both [Sources.X64] and [Sources.IA32],

(c) rename MemEncryptSevIsEnabled() to InternalMemEncryptSevIsEnabled,
and declare it in the new header file,

(d) move MemEncryptSevIsEnabled, SevStatus and SevStatusChecked to the
new .c file.


> +
> +/**
> + 
> +  This function clears memory encryption bit for the memory region specified by BaseAddress and
> +  Number of pages from the current page table context.
> +
> +  @param[in]  BaseAddress             The physical address that is the start address of a memory region.
> +  @param[in]  NumberOfPages           The number of pages from start memory region.
> +
> +  @retval RETURN_SUCCESS              The attributes were cleared for the memory region.
> +  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
> +  @retval RETURN_UNSUPPORTED          Clearing the memory encryption attribute is not supported
> +  **/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevClearPageEncMask (
> +  IN PHYSICAL_ADDRESS         BaseAddress,
> +  IN UINT32                   NumPages
> +  )
> +{
> +  return Set_Memory_Decrypted (BaseAddress, NumPages * EFI_PAGE_SIZE);
> +}

"Set_Memory_Decrypted" is not idiomatic edk2, please call it
SetMemoryDecrypted,

Also, the second argument should be written as

  EFI_PAGES_TO_SIZE (NumPages)

... However, for that, you should please switch the type of "NumPages"
to UINTN. Is that functionally okay?


> +
> +/**
> + 
> +  This function clears memory encryption bit for the memory region specified by BaseAddress and
> +  Number of pages from the current page table context.
> +
> +  @param[in]  BaseAddress             The physical address that is the start address of a memory region.
> +  @param[in]  NumberOfPages           The number of pages from start memory region.
> +
> +  @retval RETURN_SUCCESS              The attributes were cleared for the memory region.
> +  @retval RETURN_INVALID_PARAMETER    Number of pages is zero.
> +  @retval RETURN_UNSUPPORTED          Clearing the memory encryption attribute is not supported
> +  **/
> +RETURN_STATUS
> +EFIAPI
> +MemEncryptSevSetPageEncMask (
> +  IN PHYSICAL_ADDRESS         BaseAddress,
> +  IN UINT32                   NumPages
> +  )
> +{
> +  return Set_Memory_Encrypted (BaseAddress, NumPages * EFI_PAGE_SIZE);
> +}

Same comments here.

> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
> new file mode 100644
> index 0000000..7acebf3
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
> @@ -0,0 +1,304 @@
> +/** @file
> + 
> +  Virtual Memory Management Services to set or clear the memory encryption bit
> +
> +  References:
> +    1) IA-32 Intel(R) Architecture Software Developer's Manual Volume 1:Basic Architecture, Intel
> +    2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel
> +    3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel
> +
> +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> +
> +This program and the accompanying materials
> +are licensed and made available under the terms and conditions of the BSD License
> +which accompanies this distribution.  The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include "VirtualMemory.h"
> +
> +#include <Register/AmdSevMap.h>

Please put the <> includes first, and the "" includes second.

> +
> +STATIC UINT64 AddressEncMask;

Should be called "mAddressEncMask".

> +
> +typedef enum {
> +   SetCBit,
> +   ClearCBit
> +} MAP_RANGE_MODE;
> +
> +/**
> +  Split 2M page to 4K.
> +
> +  @param[in]      PhysicalAddress       Start physical address the 2M page covered.
> +  @param[in, out] PageEntry2M           Pointer to 2M page entry.
> +  @param[in]      StackBase             Stack base address.
> +  @param[in]      StackSize             Stack size.
> +
> +**/
> +STATIC
> +VOID
> +Split2MPageTo4K (
> +  IN EFI_PHYSICAL_ADDRESS               PhysicalAddress,

Your use of PHYSICAL_ADDRESS <-> EFI_PHYSICAL_ADDRESS is inconsistent.
Since this is a BASE type library, I suggest to stick with
PHYSICAL_ADDRESS. Can you please check all occurrences of
EFI_PHYSICAL_ADDRESS?

> +  IN OUT UINT64                         *PageEntry2M,

IN and OUT columns should be visually separated across all parameters.

> +  IN EFI_PHYSICAL_ADDRESS               StackBase,
> +  IN UINTN                              StackSize
> +  )
> +{
> +  EFI_PHYSICAL_ADDRESS                  PhysicalAddress4K;
> +  UINTN                                 IndexOfPageTableEntries;
> +  PAGE_TABLE_4K_ENTRY                   *PageTableEntry, *PageTableEntry1;
> +
> +  PageTableEntry = AllocatePages(1);
> +
> +  PageTableEntry1 = PageTableEntry;
> +
> +  ASSERT (PageTableEntry != NULL);
> +  ASSERT (*PageEntry2M & AddressEncMask);
> +
> +  PhysicalAddress4K = PhysicalAddress;
> +  for (IndexOfPageTableEntries = 0; IndexOfPageTableEntries < 512; IndexOfPageTableEntries++, PageTableEntry++, PhysicalAddress4K += SIZE_4KB) {
> +    //
> +    // Fill in the Page Table entries
> +    //
> +    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | AddressEncMask;
> +    PageTableEntry->Bits.ReadWrite = 1;
> +    PageTableEntry->Bits.Present = 1;
> +    if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase + StackSize)) {
> +      //
> +      // Set Nx bit for stack.
> +      //
> +      PageTableEntry->Bits.Nx = 1;
> +    }
> +  }
> +
> +  //
> +  // Fill in 2M page entry.
> +  //
> +  *PageEntry2M = (UINT64) (UINTN) PageTableEntry1 | IA32_PG_P | IA32_PG_RW | AddressEncMask;
> +}

Side question: is this function copied from some other place in edk2? If
so, I suggest to add a comment (in the code or in the commit message)
where it comes from.

Also, shouldn't you call CpuFlushTlb() somewhere, when messing with page
tables? (Hm, maybe not; a given virtual address will continue to map to
the same physical address.)

> +
> +/**
> +  Split 1G page to 2M.
> +
> +  @param[in]      PhysicalAddress       Start physical address the 1G page covered.
> +  @param[in, out] PageEntry1G           Pointer to 1G page entry.
> +  @param[in]      StackBase             Stack base address.
> +  @param[in]      StackSize             Stack size.
> +
> +**/
> +STATIC
> +VOID
> +Split1GPageTo2M (
> +  IN EFI_PHYSICAL_ADDRESS               PhysicalAddress,
> +  IN OUT UINT64                         *PageEntry1G,
> +  IN EFI_PHYSICAL_ADDRESS               StackBase,
> +  IN UINTN                              StackSize
> +  )
> +{
> +  EFI_PHYSICAL_ADDRESS                  PhysicalAddress2M;
> +  UINTN                                 IndexOfPageDirectoryEntries;
> +  PAGE_TABLE_ENTRY                      *PageDirectoryEntry;
> +
> +  PageDirectoryEntry = AllocatePages(1);
> +
> +  ASSERT (PageDirectoryEntry != NULL);
> +  ASSERT (*PageEntry1G & AddressEncMask);
> +  //
> +  // Fill in 1G page entry.
> +  //
> +  *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | IA32_PG_P | IA32_PG_RW | AddressEncMask;
> +
> +  PhysicalAddress2M = PhysicalAddress;
> +  for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512; IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M += SIZE_2MB) {
> +    if ((PhysicalAddress2M < StackBase + StackSize) && ((PhysicalAddress2M + SIZE_2MB) > StackBase)) {
> +      //
> +      // Need to split this 2M page that covers stack range.
> +      //
> +      Split2MPageTo4K (PhysicalAddress2M, (UINT64 *) PageDirectoryEntry, StackBase, StackSize);
> +    } else {
> +      //
> +      // Fill in the Page Directory entries
> +      //
> +      PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M | AddressEncMask;
> +      PageDirectoryEntry->Bits.ReadWrite = 1;
> +      PageDirectoryEntry->Bits.Present = 1;
> +      PageDirectoryEntry->Bits.MustBe1 = 1;
> +    }
> +  }
> +}
> +
> +
> +STATIC VOID
> +SetOrClearCBit(
> +  IN UINT64*  PageTablePointer,
> +  IN MAP_RANGE_MODE Mode

Non-idiomatic formatting, plus "IN" is actually incorrect for
PageTablePointer, as it is both read and written. This is what you need:

  IN OUT UINT64         *PageTablePointer,
  IN     MAP_RANGE_MODE Mode

> +  )
> +{
> +  if (Mode == SetCBit) {
> +    *PageTablePointer |= AddressEncMask;
> +  } else {
> +    *PageTablePointer &= ~AddressEncMask;
> +  }
> +
> +}
> +
> +STATIC
> +UINT64
> +GetMemEncryptionAddressMask (
> +  VOID
> +  )
> +{
> +  UINT64 MeMask;
> +  CPUID_MEMORY_ENCRYPTION_INFO_EBX  Ebx;

variable name alignment

> +
> +  //
> +  // CPUID Fn8000_001F[EBX] Bit 0:5 (memory encryption bit position)
> +  //
> +  AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, NULL, &Ebx.Uint32, NULL, NULL);
> +  MeMask = LShiftU64 (1, Ebx.Bits.PtePosBits);
> +
> +  return MeMask & PAGING_1G_ADDRESS_MASK_64;
> +}

Can you perhaps export this function and call it from PlatformPei as
well? Just asking.

> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Set_Memory_Enc_Dec (

Please use camel case without underscores.

Also, this function looks like the "workhorse" of the library. Can you
add a leading comment block to it?

> +  IN EFI_PHYSICAL_ADDRESS     PhysicalAddress,
> +  IN UINT64                   Length,
> +  IN MAP_RANGE_MODE           Mode
> +  )
> +{
> +  PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry;
> +  PAGE_MAP_AND_DIRECTORY_POINTER *PageUpperDirectoryPointerEntry;
> +  PAGE_MAP_AND_DIRECTORY_POINTER *PageDirectoryPointerEntry;
> +  PAGE_TABLE_1G_ENTRY            *PageDirectory1GEntry;
> +  PAGE_TABLE_ENTRY               *PageDirectory2MEntry;
> +  PAGE_TABLE_4K_ENTRY            *PageTableEntry;
> +  UINT64                         PgTableMask;
> +
> +  AddressEncMask = GetMemEncryptionAddressMask ();

You unconditionally rewrite the static variable here, and then you rely
on it in the other helper functions.

What do you think about adding some internal caching to
GetMemEncryptionAddressMask(), and then calling
GetMemEncryptionAddressMask() explicitly everywhere the mask is needed?

> +
> +  if (!AddressEncMask) {
> +    return RETURN_ACCESS_DENIED;
> +  }
> +
> +  PgTableMask = AddressEncMask | EFI_PAGE_MASK;
> +
> +  DEBUG ((EFI_D_VERBOSE, "Set memory range 0x%#Lx+0x%x (%s)\n", PhysicalAddress, Length,
> +			  Mode == ClearCBit ? "unencrypted" : "encrypted"));

Please use DEBUG_VERBOSE.

Also, %x is not right for printing Length (UINT64).

> +
> +  if (Length == 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // We are going to change the memory encryption attribute from C=0 -> C=1 or vice versa
> +  // Flush the caches to ensure that data is written into memory with correct C-bit
> +  //
> +  WriteBackInvalidateDataCacheRange((VOID*) PhysicalAddress, Length);

Please convert pointers to (UINTN) first.

> +
> +  while (Length)
> +  {
> +
> +    PageMapLevel4Entry = (VOID*) (AsmReadCr3() & ~PgTableMask);
> +    PageMapLevel4Entry += PML4_OFFSET(PhysicalAddress);
> +    if (!PageMapLevel4Entry->Bits.Present) {
> +      DEBUG((DEBUG_WARN, "ERROR bad PML4 for %lx\n", PhysicalAddress));

missing space after DEBUG

> +      return EFI_NO_MAPPING;
> +    }
> +
> +    PageDirectory1GEntry = (VOID*) (PageMapLevel4Entry->Bits.PageTableBaseAddress<<12 & ~PgTableMask);

missing spaces around <<

also, to clarify the binding precedence of << over &, please use
parentheses.

> +    PageDirectory1GEntry += PDP_OFFSET(PhysicalAddress);
> +    if (!PageDirectory1GEntry->Bits.Present) {
> +       DEBUG((DEBUG_WARN, "ERROR bad PDPE for %lx\n", PhysicalAddress));

missing space after DEBUG... please check the rest of the occurrences

> +       return EFI_NO_MAPPING;
> +    }
> +
> +    // If the MustBe1 bit is not 1, it's not actually a 1GB entry

Comment style is

//
// comment
//

please check the rest

> +    if (PageDirectory1GEntry->Bits.MustBe1) {
> +      // Valid 1GB page
> +      // If we have at least 1GB to go, we can just update this entry
> +      if (!(PhysicalAddress & ((1<<30) - 1)) && Length >= (1<<30)) {

For constants like the above, please consider using the macro BIT30

> +        SetOrClearCBit(&PageDirectory1GEntry->Uint64, Mode);

missing space after SetOrClearCBit

> +        DEBUG((DEBUG_VERBOSE, "Updated 1GB entry for %lx\n", PhysicalAddress));
> +        PhysicalAddress += 1<<30;
> +        Length -= 1<<30;
> +      } else {
> +        // We must split the page
> +        DEBUG((DEBUG_VERBOSE, "Spliting 1GB page\n"));
> +        Split1GPageTo2M(((UINT64)PageDirectory1GEntry->Bits.PageTableBaseAddress)<<30, (UINT64*) PageDirectory1GEntry, 0, 0);
> +        continue;
> +      }
> +    } else {
> +      // Actually a PDP
> +      PageUpperDirectoryPointerEntry = (PAGE_MAP_AND_DIRECTORY_POINTER*) PageDirectory1GEntry;
> +      PageDirectory2MEntry = (VOID*) (PageUpperDirectoryPointerEntry->Bits.PageTableBaseAddress<<12 & ~PgTableMask);
> +      PageDirectory2MEntry += PDE_OFFSET(PhysicalAddress);
> +      if (!PageDirectory2MEntry->Bits.Present) {
> +        DEBUG((DEBUG_WARN, "ERROR bad PDE for %lx\n", PhysicalAddress));
> +        return EFI_NO_MAPPING;
> +      }
> +      // If the MustBe1 bit is not a 1, it's not a 2MB entry
> +      if (PageDirectory2MEntry->Bits.MustBe1) {
> +        // Valid 2MB page
> +        // If we have at least 2MB left to go, we can just update this entry
> +        if (!(PhysicalAddress & ((1<<21)-1)) && Length >= (1<<21)) {
> +          SetOrClearCBit(&PageDirectory2MEntry->Uint64, Mode);
> +          DEBUG((DEBUG_VERBOSE, "Updated 2MB entry for %lx\n", PhysicalAddress));
> +          PhysicalAddress += 1<<21;
> +          Length -= 1<<21;
> +        } else {
> +          // We must split up this page into 4K pages
> +          DEBUG((DEBUG_VERBOSE, "Spliting 2MB page at %lx\n", PhysicalAddress));
> +          Split2MPageTo4K(((UINT64)PageDirectory2MEntry->Bits.PageTableBaseAddress) << 21, (UINT64*) PageDirectory2MEntry, 0, 0);
> +          continue;
> +        }
> +      } else {
> +        PageDirectoryPointerEntry = (PAGE_MAP_AND_DIRECTORY_POINTER*) PageDirectory2MEntry;
> +        PageTableEntry = (VOID*) (PageDirectoryPointerEntry->Bits.PageTableBaseAddress<<12 & ~PgTableMask);
> +        PageTableEntry += PTE_OFFSET(PhysicalAddress);
> +        if (!PageTableEntry->Bits.Present) {
> +          DEBUG((DEBUG_WARN, "ERROR bad PTE for %lx\n", PhysicalAddress));
> +          return EFI_NO_MAPPING;
> +        }
> +        SetOrClearCBit(&PageTableEntry->Uint64, Mode);
> +        DEBUG((DEBUG_VERBOSE, "Updated 4KB entry for %lx\n", PhysicalAddress));
> +        PhysicalAddress += EFI_PAGE_SIZE;
> +        Length -= EFI_PAGE_SIZE;
> +      }
> +    }
> +  }

So, I didn't do any functional validation for the above, just coding style.

> +
> +  //
> +  // Flush TLB
> +  //
> +  AsmWriteCr3(AsmReadCr3());

Please just call CpuFlushTlb().

> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +Set_Memory_Decrypted (
> +  IN  EFI_PHYSICAL_ADDRESS     PhysicalAddress,
> +  IN  UINT64                   Length
> +  )
> +{
> +  return Set_Memory_Enc_Dec (PhysicalAddress, Length, ClearCBit);
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +Set_Memory_Encrypted (
> +  IN  EFI_PHYSICAL_ADDRESS     PhysicalAddress,
> +  IN  UINT64                   Length
> +  )
> +{
> +  return Set_Memory_Enc_Dec (PhysicalAddress, Length, SetCBit);
> +}
> +
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h

Can we upstream the contents of this file to MdePkg somehow? My first
idea would be Include/IndustryStandard.

However, the following types are already in
"MdePkg/Include/Library/BaseLib.h":

///
/// IA32 and x64 Specific Functions.
/// Byte packed structure for 16-bit Real Mode EFLAGS.
///

IA32_FLAGS16, IA32_EFLAGS32, IA32_CR0, IA32_CR4,
IA32_SEGMENT_DESCRIPTOR, IA32_DESCRIPTOR, IA32_IDT_GATE_DESCRIPTOR, ...
and a bunch of others.

I'm asking because now we have:

BaseTools/Source/C/GenPage/VirtualMemory.h
DuetPkg/DxeIpl/Ia32/VirtualMemory.h
DuetPkg/DxeIpl/X64/VirtualMemory.h
MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h

> new file mode 100644
> index 0000000..a556211
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> @@ -0,0 +1,158 @@
> +/** @file
> + 
> +  Virtual Memory Management Services to set or clear the memory encryption bit
> +
> +  References:
> +    1) IA-32 Intel(R) Architecture Software Developer's Manual Volume 1:Basic Architecture, Intel
> +    2) IA-32 Intel(R) Architecture Software Developer's Manual Volume 2:Instruction Set Reference, Intel
> +    3) IA-32 Intel(R) Architecture Software Developer's Manual Volume 3:System Programmer's Guide, Intel
> +
> +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> +
> +This program and the accompanying materials
> +are licensed and made available under the terms and conditions of the BSD License
> +which accompanies this distribution.  The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef __VIRTUAL_MEMORY__
> +#define __VIRTUAL_MEMORY__
> +
> +#include <Uefi.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +//#include <Library/UefiBootServicesTableLib.h>
> +//#include <Library/UefiRuntimeServicesTableLib.h>
> +
> +#include <Library/CacheMaintenanceLib.h>
> +#define SYS_CODE64_SEL 0x38
> +
> +#pragma pack(1)
> +
> +//
> +// Page-Map Level-4 Offset (PML4) and
> +// Page-Directory-Pointer Offset (PDPE) entries 4K & 2MB
> +//
> +
> +typedef union {
> +  struct {
> +    UINT64  Present:1;                // 0 = Not present in memory, 1 = Present in memory
> +    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> +    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> +    UINT64  WriteThrough:1;           // 0 = Write-Back caching, 1=Write-Through caching
> +    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> +    UINT64  Accessed:1;               // 0 = Not accessed, 1 = Accessed (set by CPU)
> +    UINT64  Reserved:1;               // Reserved
> +    UINT64  MustBeZero:2;             // Must Be Zero
> +    UINT64  Available:3;              // Available for use by system software
> +    UINT64  PageTableBaseAddress:40;  // Page Table Base Address
> +    UINT64  AvabilableHigh:11;        // Available for use by system software
> +    UINT64  Nx:1;                     // No Execute bit
> +  } Bits;
> +  UINT64    Uint64;
> +} PAGE_MAP_AND_DIRECTORY_POINTER;
> +
> +//
> +// Page Table Entry 4KB
> +//
> +typedef union {
> +  struct {
> +    UINT64  Present:1;                // 0 = Not present in memory, 1 = Present in memory
> +    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> +    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> +    UINT64  WriteThrough:1;           // 0 = Write-Back caching, 1=Write-Through caching
> +    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> +    UINT64  Accessed:1;               // 0 = Not accessed, 1 = Accessed (set by CPU)
> +    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by processor on access to page
> +    UINT64  PAT:1;                    //
> +    UINT64  Global:1;                 // 0 = Not global page, 1 = global page TLB not cleared on CR3 write
> +    UINT64  Available:3;              // Available for use by system software
> +    UINT64  PageTableBaseAddress:40;  // Page Table Base Address
> +    UINT64  AvabilableHigh:11;        // Available for use by system software
> +    UINT64  Nx:1;                     // 0 = Execute Code, 1 = No Code Execution
> +  } Bits;
> +  UINT64    Uint64;
> +} PAGE_TABLE_4K_ENTRY;
> +
> +//
> +// Page Table Entry 2MB
> +//
> +typedef union {
> +  struct {
> +    UINT64  Present:1;                // 0 = Not present in memory, 1 = Present in memory
> +    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> +    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> +    UINT64  WriteThrough:1;           // 0 = Write-Back caching, 1=Write-Through caching
> +    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> +    UINT64  Accessed:1;               // 0 = Not accessed, 1 = Accessed (set by CPU)
> +    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by processor on access to page
> +    UINT64  MustBe1:1;                // Must be 1 
> +    UINT64  Global:1;                 // 0 = Not global page, 1 = global page TLB not cleared on CR3 write
> +    UINT64  Available:3;              // Available for use by system software
> +    UINT64  PAT:1;                    //
> +    UINT64  MustBeZero:8;             // Must be zero;
> +    UINT64  PageTableBaseAddress:31;  // Page Table Base Address
> +    UINT64  AvabilableHigh:11;        // Available for use by system software
> +    UINT64  Nx:1;                     // 0 = Execute Code, 1 = No Code Execution
> +  } Bits;
> +  UINT64    Uint64;
> +} PAGE_TABLE_ENTRY;
> +
> +//
> +// Page Table Entry 1GB
> +//
> +typedef union {
> +  struct {
> +    UINT64  Present:1;                // 0 = Not present in memory, 1 = Present in memory
> +    UINT64  ReadWrite:1;              // 0 = Read-Only, 1= Read/Write
> +    UINT64  UserSupervisor:1;         // 0 = Supervisor, 1=User
> +    UINT64  WriteThrough:1;           // 0 = Write-Back caching, 1=Write-Through caching
> +    UINT64  CacheDisabled:1;          // 0 = Cached, 1=Non-Cached
> +    UINT64  Accessed:1;               // 0 = Not accessed, 1 = Accessed (set by CPU)
> +    UINT64  Dirty:1;                  // 0 = Not Dirty, 1 = written by processor on access to page
> +    UINT64  MustBe1:1;                // Must be 1 
> +    UINT64  Global:1;                 // 0 = Not global page, 1 = global page TLB not cleared on CR3 write
> +    UINT64  Available:3;              // Available for use by system software
> +    UINT64  PAT:1;                    //
> +    UINT64  MustBeZero:17;            // Must be zero;
> +    UINT64  PageTableBaseAddress:22;  // Page Table Base Address
> +    UINT64  AvabilableHigh:11;        // Available for use by system software
> +    UINT64  Nx:1;                     // 0 = Execute Code, 1 = No Code Execution
> +  } Bits;
> +  UINT64    Uint64;
> +} PAGE_TABLE_1G_ENTRY;
> +
> +#pragma pack()
> +
> +#define IA32_PG_P                   BIT0
> +#define IA32_PG_RW                  BIT1
> +
> +#define PAGETABLE_ENTRY_MASK        ((1UL << 9) - 1) 
> +#define PML4_OFFSET(x)              ( (x >> 39) & PAGETABLE_ENTRY_MASK) 
> +#define PDP_OFFSET(x)               ( (x >> 30) & PAGETABLE_ENTRY_MASK) 
> +#define PDE_OFFSET(x)               ( (x >> 21) & PAGETABLE_ENTRY_MASK) 
> +#define PTE_OFFSET(x)               ( (x >> 12) & PAGETABLE_ENTRY_MASK) 
> +#define PAGING_1G_ADDRESS_MASK_64   0x000FFFFFC0000000ull
> +
> +EFI_STATUS
> +EFIAPI
> +Set_Memory_Decrypted (
> +  IN  EFI_PHYSICAL_ADDRESS     PhysicalAddress,
> +  IN  UINT64                   Length
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +Set_Memory_Encrypted (
> +  IN  EFI_PHYSICAL_ADDRESS     PhysicalAddress,
> +  IN  UINT64                   Length
> +  );
> +
> +#endif
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 769251d..c2821b7 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -126,6 +126,7 @@
>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> +  MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>  !if $(SMM_REQUIRE) == FALSE
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 3874c35..1dd8064 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -131,6 +131,7 @@
>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> +  MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>  !if $(SMM_REQUIRE) == FALSE
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>  !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index fe7f086..06bee32 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -131,6 +131,7 @@
>    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
>    VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
>    LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf
> +  MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf
>  !if $(SMM_REQUIRE) == FALSE
>    LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>  !endif
> 

Thanks
Laszlo