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

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

On 1/5/21 8:34 AM, Tom Lendacky wrote:
> 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.

Removing it does cause an error.

If we really don't want to include this PCD, I can create SEC and PEI/DXE
specific versions of the MemEncryptSevLibInternal.c file and just return
RETURN_UNSUPPORTED for the SEC version of
MemEncryptSevLocateInitialSmramSaveStateMapPages().

Alternatively, I can just remove the MemEncryptSevLibInternal.c file from
the build of the SEC library. This should be ok during SEC because there
are no calls to MemEncryptSevLocateInitialSmramSaveStateMapPages(). If,
for some reason a call is added later, then the build will fail, but it
should be obvious why it failed.

Or I can just leave the FeaturePcd section in the SEC inf file.

Thoughts?

Thanks,
Tom

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