Re: [edk2-devel] [PATCH 08/12] OvmfPkg/MemEncryptSevLib: Make the MemEncryptSevLib available for SEC

["Laszlo Ersek" <lersek@redhat.com>]

On 12/15/20 21:51, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3108
> 
> In preparation for a new interface to be added to the MemEncryptSevLib
> library that will be used in SEC, create an SEC version of the library.
> 
> This requires the creation of SEC specific files.
> 
> Some of the current MemEncryptSevLib functions perform memory allocations
> which cannot be performed in SEC, so these interfaces will return an error
> during SEC. Also, the current MemEncryptSevLib library uses some static
> variables to optimize access to variables, which cannot be used in SEC.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  .../DxeBaseMemEncryptSevLib.inf               |   2 +-
>  .../PeiBaseMemEncryptSevLib.inf               |   2 +-
>  .../SecBaseMemEncryptSevLib.inf               |  54 ++++++++
>  .../SecMemEncryptSevLibInternal.c             | 130 ++++++++++++++++++
>  ...{VirtualMemory.c => PeiDxeVirtualMemory.c} |  12 +-
>  .../X64/SecVirtualMemory.c                    |  80 +++++++++++
>  6 files changed, 272 insertions(+), 8 deletions(-)
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
>  rename OvmfPkg/Library/BaseMemEncryptSevLib/X64/{VirtualMemory.c => PeiDxeVirtualMemory.c} (95%)
>  create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c

(1) /s/SecBase/Sec/ (in filenames and in filename references; the
BASE_NAME is OK)

> 
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf
> index 2be6ca1fa737..390f2d60677f 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeBaseMemEncryptSevLib.inf
> @@ -33,7 +33,7 @@ [Sources.X64]
>    DxeMemEncryptSevLibInternal.c
>    MemEncryptSevLibInternal.c
>    X64/MemEncryptSevLib.c
> -  X64/VirtualMemory.c
> +  X64/PeiDxeVirtualMemory.c
>    X64/VirtualMemory.h
>  
>  [Sources.IA32]
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf
> index 7bdf8cb5210d..cb973fdeb868 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiBaseMemEncryptSevLib.inf
> @@ -33,7 +33,7 @@ [Sources.X64]
>    PeiMemEncryptSevLibInternal.c
>    MemEncryptSevLibInternal.c
>    X64/MemEncryptSevLib.c
> -  X64/VirtualMemory.c
> +  X64/PeiDxeVirtualMemory.c
>    X64/VirtualMemory.h
>  
>  [Sources.IA32]
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf b/OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf
> new file mode 100644
> index 000000000000..b26f739d69fd
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecBaseMemEncryptSevLib.inf
> @@ -0,0 +1,54 @@
> +## @file
> +#  Library provides the helper functions for SEV guest
> +#
> +# Copyright (c) 2020 Advanced Micro Devices. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.25
> +  BASE_NAME                      = SecMemEncryptSevLib
> +  FILE_GUID                      = 046388b4-430e-4e61-88f6-51ea21db2632
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = MemEncryptSevLib|SEC
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +# VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[Sources.X64]
> +  SecMemEncryptSevLibInternal.c
> +  MemEncryptSevLibInternal.c
> +  X64/MemEncryptSevLib.c
> +  X64/SecVirtualMemory.c
> +  X64/VirtualMemory.h
> +
> +[Sources.IA32]
> +  SecMemEncryptSevLibInternal.c
> +  MemEncryptSevLibInternal.c
> +  Ia32/MemEncryptSevLib.c
> +
> +[LibraryClasses]
> +  BaseLib
> +  CpuLib
> +  DebugLib
> +  PcdLib
> +
> +[FeaturePcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> +

(2) This PCD does not look useful for the new library instance (at least
at this stage).

> +[FixedPcd]
> +  gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> new file mode 100644
> index 000000000000..30d2ebe1d6e9
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> @@ -0,0 +1,130 @@
> +/** @file
> +
> +  Secure Encrypted Virtualization (SEV) library helper function
> +
> +  Copyright (c) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +#include <Library/PcdLib.h>
> +#include <Register/Amd/Cpuid.h>
> +#include <Register/Amd/Msr.h>
> +#include <Register/Cpuid.h>
> +#include <Uefi/UefiBaseType.h>
> +
> +/**
> +  Reads and sets the status of SEV features.
> +
> +  **/
> +STATIC
> +UINT32
> +EFIAPI
> +InternalMemEncryptSevStatus (
> +  VOID
> +  )
> +{
> +  UINT32                            RegEax;
> +  CPUID_MEMORY_ENCRYPTION_INFO_EAX  Eax;
> +  BOOLEAN                           ReadSevMsr;
> +  SEC_SEV_ES_WORK_AREA              *SevEsWorkArea;
> +
> +  ReadSevMsr = FALSE;
> +
> +  SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
> +  if (SevEsWorkArea != NULL && SevEsWorkArea->EncryptionMask != 0) {
> +    //
> +    // The MSR has been read before, so it is safe to read it again and avoid
> +    // having to validate the CPUID information.
> +    //
> +    ReadSevMsr = TRUE;
> +  } else {
> +    //
> +    // 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) {
> +        ReadSevMsr = TRUE;
> +      }
> +    }
> +  }
> +
> +  return ReadSevMsr ? AsmReadMsr32 (MSR_SEV_STATUS) : 0;
> +}
> +
> +/**
> +  Returns a boolean to indicate whether SEV-ES is enabled.
> +
> +  @retval TRUE           SEV-ES is enabled
> +  @retval FALSE          SEV-ES is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevEsIsEnabled (
> +  VOID
> +  )
> +{
> +  MSR_SEV_STATUS_REGISTER           Msr;
> +
> +  Msr.Uint32 = InternalMemEncryptSevStatus ();
> +
> +  return Msr.Bits.SevEsBit ? TRUE : FALSE;
> +}
> +
> +/**
> +  Returns a boolean to indicate whether SEV is enabled.
> +
> +  @retval TRUE           SEV is enabled
> +  @retval FALSE          SEV is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevIsEnabled (
> +  VOID
> +  )
> +{
> +  MSR_SEV_STATUS_REGISTER           Msr;
> +
> +  Msr.Uint32 = InternalMemEncryptSevStatus ();
> +
> +  return Msr.Bits.SevBit ? TRUE : FALSE;
> +}
> +
> +/**
> +  Returns the SEV encryption mask.
> +
> +  @return  The SEV pagtable encryption mask
> +**/
> +UINT64
> +EFIAPI
> +MemEncryptSevGetEncryptionMask (
> +  VOID
> +  )
> +{
> +  CPUID_MEMORY_ENCRYPTION_INFO_EBX  Ebx;
> +  SEC_SEV_ES_WORK_AREA              *SevEsWorkArea;
> +  UINT64                            EncryptionMask;
> +
> +  SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
> +  if (SevEsWorkArea != NULL) {
> +    EncryptionMask = SevEsWorkArea->EncryptionMask;
> +  } else {
> +    //
> +    // CPUID Fn8000_001F[EBX] Bit 0:5 (memory encryption bit position)
> +    //
> +    AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, NULL, &Ebx.Uint32, NULL, NULL);
> +    EncryptionMask = LShiftU64 (1, Ebx.Bits.PtePosBits);
> +  }
> +
> +  return EncryptionMask;
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> similarity index 95%
> rename from OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
> rename to OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index 6422bc53bd5d..3a5bab657bd7 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -192,7 +192,8 @@ Split2MPageTo4K (
>  {
>    PHYSICAL_ADDRESS                  PhysicalAddress4K;
>    UINTN                             IndexOfPageTableEntries;
> -  PAGE_TABLE_4K_ENTRY               *PageTableEntry, *PageTableEntry1;
> +  PAGE_TABLE_4K_ENTRY               *PageTableEntry;
> +  PAGE_TABLE_4K_ENTRY               *PageTableEntry1;
>    UINT64                            AddressEncMask;
>  
>    PageTableEntry = AllocatePageTableMemory(1);
> @@ -472,7 +473,7 @@ Split1GPageTo2M (
>  /**
>    Set or Clear the memory encryption bit
>  
> -  @param[in]      PagetablePoint        Page table entry pointer (PTE).
> +  @param[in, out] PageTablePointer      Page table entry pointer (PTE).
>    @param[in]      Mode                  Set or Clear encryption bit
>  
>  **/
> @@ -562,7 +563,6 @@ EnableReadOnlyPageWriteProtect (
>    @retval RETURN_UNSUPPORTED          Setting the memory encyrption attribute
>                                        is not supported
>  **/
> -
>  STATIC
>  RETURN_STATUS
>  EFIAPI
> @@ -635,7 +635,7 @@ SetMemoryEncDec (
>  
>    Status = EFI_SUCCESS;
>  
> -  while (Length)
> +  while (Length != 0)
>    {
>      //
>      // If Cr3BaseAddress is not specified then read the current CR3
> @@ -683,7 +683,7 @@ SetMemoryEncDec (
>        // Valid 1GB page
>        // If we have at least 1GB to go, we can just update this entry
>        //
> -      if (!(PhysicalAddress & (BIT30 - 1)) && Length >= BIT30) {
> +      if ((PhysicalAddress & (BIT30 - 1)) == 0 && Length >= BIT30) {
>          SetOrClearCBit(&PageDirectory1GEntry->Uint64, Mode);
>          DEBUG ((
>            DEBUG_VERBOSE,
> @@ -744,7 +744,7 @@ SetMemoryEncDec (
>          // Valid 2MB page
>          // If we have at least 2MB left to go, we can just update this entry
>          //
> -        if (!(PhysicalAddress & (BIT21-1)) && Length >= BIT21) {
> +        if ((PhysicalAddress & (BIT21-1)) == 0 && Length >= BIT21) {
>            SetOrClearCBit (&PageDirectory2MEntry->Uint64, Mode);
>            PhysicalAddress += BIT21;
>            Length -= BIT21;

(3) The style fixes in this file seem unrelated to the subject. Please
split them to a different patch.

(Were they motivated by ECC?)

Looks OK otherwise.

Thanks!
Laszlo

> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> new file mode 100644
> index 000000000000..5c337ea0b820
> --- /dev/null
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> @@ -0,0 +1,80 @@
> +/** @file
> +
> +  Virtual Memory Management Services to set or clear the memory encryption bit
> +
> +  Copyright (c) 2020, AMD Incorporated. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/CpuLib.h>
> +#include <Library/MemEncryptSevLib.h>
> +
> +#include "VirtualMemory.h"
> +
> +/**
> +  This function clears memory encryption bit for the memory region specified by
> +  PhysicalAddress and Length from the current page table context.
> +
> +  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
> +                                      current CR3)
> +  @param[in]  PhysicalAddress         The physical address that is the start
> +                                      address of a memory region.
> +  @param[in]  Length                  The length of memory region
> +  @param[in]  Flush                   Flush the caches before applying the
> +                                      encryption mask
> +
> +  @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 encyrption attribute
> +                                      is not supported
> +**/
> +RETURN_STATUS
> +EFIAPI
> +InternalMemEncryptSevSetMemoryDecrypted (
> +  IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
> +  IN  PHYSICAL_ADDRESS        PhysicalAddress,
> +  IN  UINTN                   Length,
> +  IN  BOOLEAN                 Flush
> +  )
> +{
> +  //
> +  // This function is not available during SEC.
> +  //
> +  return RETURN_UNSUPPORTED;
> +}
> +
> +/**
> +  This function sets memory encryption bit for the memory region specified by
> +  PhysicalAddress and Length from the current page table context.
> +
> +  @param[in]  Cr3BaseAddress          Cr3 Base Address (if zero then use
> +                                      current CR3)
> +  @param[in]  PhysicalAddress         The physical address that is the start
> +                                      address of a memory region.
> +  @param[in]  Length                  The length of memory region
> +  @param[in]  Flush                   Flush the caches before applying the
> +                                      encryption mask
> +
> +  @retval RETURN_SUCCESS              The attributes were set 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
> +InternalMemEncryptSevSetMemoryEncrypted (
> +  IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
> +  IN  PHYSICAL_ADDRESS        PhysicalAddress,
> +  IN  UINTN                   Length,
> +  IN  BOOLEAN                 Flush
> +  )
> +{
> +  //
> +  // This function is not available during SEC.
> +  //
> +  return RETURN_UNSUPPORTED;
> +}
>