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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 1/5/21 3:40 AM, Laszlo Ersek wrote:
> On 12/15/20 21:51, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C1440a9afd7f1450ba93d08d8b15e02a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637454364641627971%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=P52g0gS3SEhdgkF2qRY6l1J8%2FLjJm1DNR3LLlEmKSBk%3D&amp;reserved=0
>>
>> 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)

Yup, I'll fix that.

> 
>>
>> 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).

The PCD is used in MemEncryptSevLocateInitialSmramSaveStateMapPages() in
the MemEncryptSevLibInternal.c file, which is part of the library. Because
of that, I assumed that it needed to be added even though the function
that uses it isn't called during SEC.

I'll remove it.

> 
>> +[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?)

Yes, IIRC (I need to swap everything back in after the holidays :)), the
test pull request was failing until I made these changes. I'll split these
changes out as a pre-patch to this patch.

Thanks,
Tom

> 
> 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;
>> +}
>>
>