[edk2-devel] [PATCH 08/12] OvmfPkg/MemEncryptSevLib: Make the MemEncryptSevLib available for SEC
Lendacky, Thomas
thomas.lendacky at amd.com
Tue Jan 5 14:34:05 UTC 2021
On 1/5/21 3:40 AM, Laszlo Ersek wrote:
> On 12/15/20 21:51, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky at amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&data=04%7C01%7Cthomas.lendacky%40amd.com%7C1440a9afd7f1450ba93d08d8b15e02a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637454364641627971%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=P52g0gS3SEhdgkF2qRY6l1J8%2FLjJm1DNR3LLlEmKSBk%3D&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 at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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;
>> +}
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69710): https://edk2.groups.io/g/devel/message/69710
Mute This Topic: https://groups.io/mt/78986172/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list