[edk2-devel] [PATCH 08/12] OvmfPkg/MemEncryptSevLib: Make the MemEncryptSevLib available for SEC
Lendacky, Thomas
thomas.lendacky at amd.com
Tue Jan 5 15:38:59 UTC 2021
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 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.
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;
>>> +}
>>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69718): https://edk2.groups.io/g/devel/message/69718
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